Merge pull request #65719 from Cynerva/gkk/upgrade-resources

Automatic merge from submit-queue (batch tested with PRs 65719, 65764). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

juju: Fix upgrade actions not working with resources

**What this PR does / why we need it**:

This fixes an issue with the kubernetes-master and kubernetes-worker charms, where running the `upgrade` action does not actually perform an upgrade when snaps are attached as resources.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:

https://github.com/juju-solutions/bundle-canonical-kubernetes/issues/528

**Special notes for your reviewer**:

The underlying issue is that both layer-snap and the kubernetes layers are using `any_file_changed` to look for changes in the resources. This PR fixes it by removing the use of `any_file_changed` in the top-level layers, and implementing our own code for it instead.

**Release note**:

```release-note
NONE
```

cluster/juju/layers/kubernetes-master/reactive/kubernetes_master.py

@@ -15,6 +15,7 @@
 # limitations under the License.
 
 import base64
+import hashlib
 import os
 import re
 import random
@@ -64,8 +65,11 @@ from charmhelpers.contrib.charmsupport import nrpe
 nrpe.Check.shortname_re = '[\.A-Za-z0-9-_]+$'
 
 gcp_creds_env_key = 'GOOGLE_APPLICATION_CREDENTIALS'
+snap_resources = ['kubectl', 'kube-apiserver', 'kube-controller-manager',
+                  'kube-scheduler', 'cdk-addons']
 
 os.environ['PATH'] += os.pathsep + os.path.join(os.sep, 'snap', 'bin')
+db = unitdata.kv()
 
 
 def set_upgrade_needed(forced=False):
@@ -86,7 +90,6 @@ def channel_changed():
 
 def service_cidr():
     ''' Return the charm's service-cidr config '''
-    db = unitdata.kv()
     frozen_cidr = db.get('kubernetes-master.service-cidr')
     return frozen_cidr or hookenv.config('service-cidr')
 
@@ -94,7 +97,6 @@ def service_cidr():
 def freeze_service_cidr():
     ''' Freeze the service CIDR. Once the apiserver has started, we can no
     longer safely change this value. '''
-    db = unitdata.kv()
     db.set('kubernetes-master.service-cidr', service_cidr())
 
 
@@ -107,10 +109,8 @@ def check_for_upgrade_needed():
     add_rbac_roles()
     set_state('reconfigure.authentication.setup')
     remove_state('authentication.setup')
-    changed = snap_resources_changed()
-    if changed == 'yes':
-        set_upgrade_needed()
-    elif changed == 'unknown':
+
+    if not db.get('snap.resources.fingerprint.initialised'):
         # We are here on an upgrade from non-rolling master
         # Since this upgrade might also include resource updates eg
         # juju upgrade-charm kubernetes-master --resource kube-any=my.snap
@@ -118,6 +118,9 @@ def check_for_upgrade_needed():
         # Forcibly means we do not prompt the user to call the upgrade action.
         set_upgrade_needed(forced=True)
 
+    migrate_resource_checksums()
+    check_resources_for_upgrade_needed()
+
     # Set the auto storage backend to etcd2.
     auto_storage_backend = leader_get('auto_storage_backend')
     is_leader = is_state('leadership.is_leader')
@@ -125,27 +128,56 @@ def check_for_upgrade_needed():
         leader_set(auto_storage_backend='etcd2')
 
 
-def snap_resources_changed():
-    '''
-    Check if the snapped resources have changed. The first time this method is
-    called will report "unknown".
+def get_resource_checksum_db_key(resource):
+    ''' Convert a resource name to a resource checksum database key. '''
+    return 'kubernetes-master.resource-checksums.' + resource
 
-    Returns: "yes" in case a snap resource file has changed,
-             "no" in case a snap resources are the same as last call,
-             "unknown" if it is the first time this method is called
 
-    '''
-    db = unitdata.kv()
-    resources = ['kubectl', 'kube-apiserver', 'kube-controller-manager',
-                 'kube-scheduler', 'cdk-addons']
-    paths = [hookenv.resource_get(resource) for resource in resources]
-    if db.get('snap.resources.fingerprint.initialised'):
-        result = 'yes' if any_file_changed(paths) else 'no'
-        return result
+def calculate_resource_checksum(resource):
+    ''' Calculate a checksum for a resource '''
+    md5 = hashlib.md5()
+    path = hookenv.resource_get(resource)
+    if path:
+        with open(path, 'rb') as f:
+            data = f.read()
+        md5.update(data)
+    return md5.hexdigest()
+
+
+def migrate_resource_checksums():
+    ''' Migrate resource checksums from the old schema to the new one '''
+    for resource in snap_resources:
+        new_key = get_resource_checksum_db_key(resource)
+        if not db.get(new_key):
+            path = hookenv.resource_get(resource)
+            if path:
+                # old key from charms.reactive.helpers.any_file_changed
+                old_key = 'reactive.files_changed.' + path
+                old_checksum = db.get(old_key)
+                db.set(new_key, old_checksum)
             else:
-        db.set('snap.resources.fingerprint.initialised', True)
-        any_file_changed(paths)
-        return 'unknown'
+                # No resource is attached. Previously, this meant no checksum
+                # would be calculated and stored. But now we calculate it as if
+                # it is a 0-byte resource, so let's go ahead and do that.
+                zero_checksum = hashlib.md5().hexdigest()
+                db.set(new_key, zero_checksum)
+
+
+def check_resources_for_upgrade_needed():
+    hookenv.status_set('maintenance', 'Checking resources')
+    for resource in snap_resources:
+        key = get_resource_checksum_db_key(resource)
+        old_checksum = db.get(key)
+        new_checksum = calculate_resource_checksum(resource)
+        if new_checksum != old_checksum:
+            set_upgrade_needed()
+
+
+def calculate_and_store_resource_checksums():
+    for resource in snap_resources:
+        key = get_resource_checksum_db_key(resource)
+        checksum = calculate_resource_checksum(resource)
+        db.set(key, checksum)
 
 
 def add_rbac_roles():
@@ -253,7 +285,8 @@ def install_snaps():
     snap.install('kube-scheduler', channel=channel)
     hookenv.status_set('maintenance', 'Installing cdk-addons snap')
     snap.install('cdk-addons', channel=channel)
-    snap_resources_changed()
+    calculate_and_store_resource_checksums()
+    db.set('snap.resources.fingerprint.initialised', True)
     set_state('kubernetes-master.snaps.installed')
     remove_state('kubernetes-master.components.started')
 
@@ -1123,8 +1156,6 @@ def parse_extra_args(config_key):
 
 
 def configure_kubernetes_service(service, base_args, extra_args_key):
-    db = unitdata.kv()
-
     prev_args_key = 'kubernetes-master.prev_args.' + service
     prev_args = db.get(prev_args_key) or {}
 
@@ -1412,7 +1443,6 @@ def set_token(password, save_salt):
 
     param: password - the password to be stored
     param: save_salt - the key to store the value of the token.'''
-    db = unitdata.kv()
     db.set(save_salt, password)
     return db.get(save_salt)
 

cluster/juju/layers/kubernetes-worker/reactive/kubernetes_worker.py

@@ -14,6 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import hashlib
 import json
 import os
 import random
@@ -36,7 +37,7 @@ from charms.reactive import when, when_any, when_not, when_none
 
 from charms.kubernetes.common import get_version
 
-from charms.reactive.helpers import data_changed, any_file_changed
+from charms.reactive.helpers import data_changed
 from charms.templating.jinja2 import render
 
 from charmhelpers.core import hookenv, unitdata
@@ -52,6 +53,7 @@ kubeconfig_path = '/root/cdk/kubeconfig'
 kubeproxyconfig_path = '/root/cdk/kubeproxyconfig'
 kubeclientconfig_path = '/root/.kube/config'
 gcp_creds_env_key = 'GOOGLE_APPLICATION_CREDENTIALS'
+snap_resources = ['kubectl', 'kubelet', 'kube-proxy']
 
 os.environ['PATH'] += os.pathsep + os.path.join(os.sep, 'snap', 'bin')
 db = unitdata.kv()
@@ -64,6 +66,7 @@ def upgrade_charm():
     hookenv.atexit(remove_state, 'config.changed.install_from_upstream')
 
     cleanup_pre_snap_services()
+    migrate_resource_checksums()
     check_resources_for_upgrade_needed()
 
     # Remove the RC for nginx ingress if it exists
@@ -88,14 +91,58 @@ def upgrade_charm():
     set_state('kubernetes-worker.restart-needed')
 
 
+def get_resource_checksum_db_key(resource):
+    ''' Convert a resource name to a resource checksum database key. '''
+    return 'kubernetes-worker.resource-checksums.' + resource
+
+
+def calculate_resource_checksum(resource):
+    ''' Calculate a checksum for a resource '''
+    md5 = hashlib.md5()
+    path = hookenv.resource_get(resource)
+    if path:
+        with open(path, 'rb') as f:
+            data = f.read()
+        md5.update(data)
+    return md5.hexdigest()
+
+
+def migrate_resource_checksums():
+    ''' Migrate resource checksums from the old schema to the new one '''
+    for resource in snap_resources:
+        new_key = get_resource_checksum_db_key(resource)
+        if not db.get(new_key):
+            path = hookenv.resource_get(resource)
+            if path:
+                # old key from charms.reactive.helpers.any_file_changed
+                old_key = 'reactive.files_changed.' + path
+                old_checksum = db.get(old_key)
+                db.set(new_key, old_checksum)
+            else:
+                # No resource is attached. Previously, this meant no checksum
+                # would be calculated and stored. But now we calculate it as if
+                # it is a 0-byte resource, so let's go ahead and do that.
+                zero_checksum = hashlib.md5().hexdigest()
+                db.set(new_key, zero_checksum)
+
+
 def check_resources_for_upgrade_needed():
     hookenv.status_set('maintenance', 'Checking resources')
-    resources = ['kubectl', 'kubelet', 'kube-proxy']
-    paths = [hookenv.resource_get(resource) for resource in resources]
-    if any_file_changed(paths):
+    for resource in snap_resources:
+        key = get_resource_checksum_db_key(resource)
+        old_checksum = db.get(key)
+        new_checksum = calculate_resource_checksum(resource)
+        if new_checksum != old_checksum:
             set_upgrade_needed()
 
 
+def calculate_and_store_resource_checksums():
+    for resource in snap_resources:
+        key = get_resource_checksum_db_key(resource)
+        checksum = calculate_resource_checksum(resource)
+        db.set(key, checksum)
+
+
 def set_upgrade_needed():
     set_state('kubernetes-worker.snaps.upgrade-needed')
     config = hookenv.config()
@@ -151,7 +198,6 @@ def upgrade_needed_status():
 
 @when('kubernetes-worker.snaps.upgrade-specified')
 def install_snaps():
-    check_resources_for_upgrade_needed()
     channel = hookenv.config('channel')
     hookenv.status_set('maintenance', 'Installing kubectl snap')
     snap.install('kubectl', channel=channel, classic=True)
@@ -159,6 +205,7 @@ def install_snaps():
     snap.install('kubelet', channel=channel, classic=True)
     hookenv.status_set('maintenance', 'Installing kube-proxy snap')
     snap.install('kube-proxy', channel=channel, classic=True)
+    calculate_and_store_resource_checksums()
     set_state('kubernetes-worker.snaps.installed')
     set_state('kubernetes-worker.restart-needed')
     remove_state('kubernetes-worker.snaps.upgrade-needed')