Merge ~hopem/charm-sysconfig:bug/1864217 into charm-sysconfig:master

[Edward Hope-Morley]

Check file content sha256sum when deciding if they have changed

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Xav Paice (xavpaice)]

LGTM, tests OK (manually) and does appear to resolve the bug.

review: Approve

[Alvaro Uria (aluria)]

Hey Edward, thank you for the fix. However, I would suggest using charmhelpers' host.write [1] which already checks if the content has changed (and skips re-writing the file if so).

1. https://github.com/juju/charm-helpers/blob/master/charmhelpers/core/host.py#L536

review: Needs Fixing

[Alvaro Uria (aluria)]

I meant host.write_file. BTW, the render helper also uses host.write_file underneath [1].

1. https://github.com/juju/charm-helpers/blob/master/charmhelpers/core/templating.py#L22

review: Needs Fixing

[Edward Hope-Morley (hopem)]

@aluria The existing code in the charm that tracks if resources have changed does not use the charm-helpers code so I don't want to rewrite the code in this patch. I have extensively tested this patch and confirm that it resolves the problem as well as doing so for pre-existing resources so would rather stick with this implementation. If somebody wants to rewrite the code to use charm-helpers that's fine but not in this patch please.

[Giuseppe Petralia (peppepetra)]

Hi Edward,

I have tested the MR. I see two issues:

1. on a fresh install the message of the unit is always "ready" even if the resources are updated.
2. on two consecutive update-status the message of the unit will be always set to ready.

I have suggested how to fix that (comments inline)

Also the MR needs to be rebased.

Thanks,
Giuseppe

review: Needs Fixing

[Edward Hope-Morley (hopem)]

@peppepetra86 thanks yeah it does need a rebase to get https://code.launchpad.net/~hopem/charm-sysconfig/+git/charm-sysconfig/+merge/379879 ill do that now and check the issue you mention.

[Edward Hope-Morley (hopem)]

@peppepetra86 thank you for your feedback you were absolutely right, fresh install was falling into this trap. I have updated the logic to fix (and actually think it is much cleaner now). I have tested:

  * fresh install
  * upgrade (patched to patched)
  * config change
  * upgrade (current to patched)

Thing to note is that you do need to wait for update-status hook to run in order for the status message to go away (in my test i set to 10s refresh to speed things up). Since no hooks are executed on reboot I don't think there is another way to resolve this.

[Giuseppe Petralia (peppepetra)]

lgtm! Thank you, Giuseppe

review: Approve

[Jeremy Lounder (jldev)]

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 34bca16d53b97d8671ca2b4f9d237b28dd32147a

diff --git a/src/lib/lib_sysconfig.py b/src/lib/lib_sysconfig.py
index 862a484..7d4513d 100644
--- a/src/lib/lib_sysconfig.py
+++ b/src/lib/lib_sysconfig.py
@@ -2,6 +2,7 @@
 
 Manage grub, systemd, coufrequtils and kernel version configuration.
 """
+import hashlib
 import os
 import subprocess
 from datetime import datetime, timedelta, timezone
@@ -75,10 +76,31 @@ class BootResourceState:
         """Return db key for a given resource."""
         return "sysconfig.boot_resource.{}".format(resource_name)
 
+    def calculate_resource_sha256sum(self, resource_name):
+        """Calcucate sha256sum of contents of provided resource."""
+        sha = hashlib.sha256()
+        sha.update(open(resource_name, 'rb').read())
+        return sha.hexdigest()
+
+    def update_resource_checksums(self, resources):
+        """Update db entry for the resource_name with sha256sum of its contents."""
+        for resource in resources:
+            if not os.path.exists(resource):
+                continue
+
+            self.db.set("{}.sha256sum".format(self.key_for(resource)),
+                        self.calculate_resource_sha256sum(resource))
+
     def set_resource(self, resource_name):
         """Update db entry for the resource_name with time.now."""
         timestamp = datetime.now(timezone.utc)
         self.db.set(self.key_for(resource_name), timestamp.timestamp())
+        # NOTE: don't set checksum here
+
+    def get_resource_sha256sum(self, resource_name):
+        """Get db record of sha256sum of contents of provided resource."""
+        key = self.key_for(resource_name)
+        return self.db.get("{}.sha256sum".format(key))
 
     def get_resource_changed_timestamp(self, resource_name):
         """Retrieve timestamp of last resource change recorded.
@@ -91,6 +113,20 @@ class BootResourceState:
             return datetime.fromtimestamp(tfloat, timezone.utc)
         return datetime.min.replace(tzinfo=timezone.utc)  # We don't have a ts -> changed at dawn of time
 
+    def checksum_changed(self, resource_name):
+        """Return True if checksum has changed since last recorded."""
+        # NOTE: we treat checksum == None as True because this is required for
+        #       backwards compatibility (see bug 1864217) since new resources
+        #       created since the charm was patched will always have a checksum.
+        if not self.get_resource_sha256sum(resource_name):
+            return True
+
+        new_sum = self.calculate_resource_sha256sum(resource_name)
+        if self.get_resource_sha256sum(resource_name) != new_sum:
+            return True
+
+        return False
+
     def resources_changed_since_boot(self, resource_names):
         """Given a list of resource names return those that have changed since boot.
 
@@ -98,8 +134,25 @@ class BootResourceState:
         :return: list of names
         """
         boot_ts = boot_time()
-        changed = [name for name in resource_names if boot_ts < self.get_resource_changed_timestamp(name)]
-        return changed
+        time_changed = [name for name in resource_names
+                        if boot_ts < self.get_resource_changed_timestamp(name)]
+
+        csum_changed = [name for name in resource_names
+                        if self.checksum_changed(name)]
+
+        a = set(time_changed)
+        b = set(csum_changed)
+        c = set(csum_changed).difference(set(time_changed))
+        d = set(b).difference(c)
+        # i.e. update resources that have csum mismatch but did not change
+        # since boot (we only ever update csum here).
+        self.update_resource_checksums(c)
+
+        # i.e. resources that have changed since boot time that do have csum
+        # mismatch.
+        changed = a.intersection(d)
+
+        return list(changed)
 
 
 class SysConfigHelper:
diff --git a/src/reactive/sysconfig.py b/src/reactive/sysconfig.py
index 85beb8c..2237caa 100644
--- a/src/reactive/sysconfig.py
+++ b/src/reactive/sysconfig.py
@@ -114,6 +114,16 @@ def config_changed():
     update_status()
 
 
+@hook('upgrade-charm')
+def upgrade_charm():
+    """Extras to run when charm is upgraded."""
+    # NOTE(hopem): do this for backwards compatibility to ensure that db
+    # records of resources are updated with a sha256sum prior to config-changed
+    # being run where they may be overwritten with no content change - see bug
+    # 1864217 for context.
+    update_status()
+
+
 @hook('update-status')
 def update_status():
     """Update the workload message checking if reboot is needed.
diff --git a/src/tests/unit/test_lib.py b/src/tests/unit/test_lib.py
index ceca18c..455018b 100644
--- a/src/tests/unit/test_lib.py
+++ b/src/tests/unit/test_lib.py
@@ -1,7 +1,8 @@
 #!/usr/bin/python3
 """Unit tests for SysConfigHelper and BootResourceState classes."""
 import subprocess
-from datetime import datetime, timezone
+from datetime import datetime, timedelta, timezone
+from tempfile import NamedTemporaryFile
 
 import lib_sysconfig
 
@@ -13,16 +14,28 @@ import pytest
 class TestBootResourceState:
     """Test BootResourceState class."""
 
+    @property
+    def datetime(self):
+        """Return a datetime object."""
+        return datetime(2019, 1, 1, tzinfo=timezone.utc)
+
     def boot_resource(self):
         """Mock unitdata.kv()."""
         db = mock.MagicMock()
-        db.get.return_value = datetime(2019, 1, 1, tzinfo=timezone.utc).timestamp()
+
+        def fake_db_get(key):
+            if key.endswith("sha256sum"):
+                return "1234"
+            else:
+                return self.datetime.timestamp()
+
+        db.get.side_effect = fake_db_get
         return lib_sysconfig.BootResourceState(db=db)
 
     @mock.patch("lib_sysconfig.datetime")
     def test_set_resource(self, mock_datetime):
         """Test updating resource entry in the db."""
-        test_time = datetime(2019, 1, 1, tzinfo=timezone.utc)
+        test_time = self.datetime
         mock_datetime.now.return_value = test_time
         boot_resource = self.boot_resource()
         boot_resource.set_resource("foofile")
@@ -34,7 +47,7 @@ class TestBootResourceState:
 
     def test_get_resource(self):
         """Test retrieving timestamp of last resource update."""
-        test_time = datetime(2019, 1, 1, tzinfo=timezone.utc)
+        test_time = self.datetime
         boot_resource = self.boot_resource()
         timestamp = boot_resource.get_resource_changed_timestamp("foofile")
         assert timestamp == test_time
@@ -54,17 +67,43 @@ class TestBootResourceState:
         """Test retrieving of resources changed since last boot."""
         mock_boot_time.return_value = datetime.min.replace(tzinfo=timezone.utc)
         boot_resource = self.boot_resource()
-        changed = boot_resource.resources_changed_since_boot(["foofile"])
-        assert len(changed) == 1
-        assert changed[0] == "foofile"
+        with NamedTemporaryFile() as ftmp:
+            changed = boot_resource.resources_changed_since_boot([ftmp.name])
+            assert len(changed) == 1
+            assert changed[0] == ftmp.name
+
+    @mock.patch("lib_sysconfig.boot_time")
+    def test_resources_not_changed(self, mock_boot_time):
+        """Test resource is not changed since last boot."""
+        mock_boot_time.return_value = self.datetime
+        boot_resource = self.boot_resource()
+        with NamedTemporaryFile() as ftmp:
+            changed = boot_resource.resources_changed_since_boot([ftmp.name])
+            assert not changed
 
     @mock.patch("lib_sysconfig.boot_time")
-    def test_resources_changed_future(self, mock_boot_time):
+    def test_resources_time_changed_contents_no_change(self, mock_boot_time):
         """Test resource is not changed since last boot."""
-        mock_boot_time.return_value = datetime.max.replace(tzinfo=timezone.utc)
+        mock_boot_time.return_value = self.datetime - timedelta(1)
         boot_resource = self.boot_resource()
-        changed = boot_resource.resources_changed_since_boot(["foofile"])
-        assert not changed
+        with NamedTemporaryFile() as ftmp:
+            with mock.patch.object(boot_resource,
+                                   'calculate_resource_sha256sum') as mock_calc:
+                mock_calc.return_value = "1234"
+                changed = boot_resource.resources_changed_since_boot([ftmp.name])
+                assert not changed
+
+    @mock.patch("lib_sysconfig.boot_time")
+    def test_resources_time_changed_contents_changed(self, mock_boot_time):
+        """Test resource is not changed since last boot."""
+        mock_boot_time.return_value = self.datetime - timedelta(1)
+        boot_resource = self.boot_resource()
+        with NamedTemporaryFile() as ftmp:
+            with mock.patch.object(boot_resource,
+                                   'calculate_resource_sha256sum') as mock_calc:
+                mock_calc.return_value = "2345"
+                changed = boot_resource.resources_changed_since_boot([ftmp.name])
+                assert changed
 
 
 class TestLib:
@@ -411,10 +450,11 @@ class TestLib:
         }
         exists.return_value = True
 
-        sysh = lib_sysconfig.SysConfigHelper()
-        sysh.remove_grub_configuration()
-
-        os_remove.assert_called_with(lib_sysconfig.GRUB_CONF)
+        with NamedTemporaryFile() as ftmp:
+            lib_sysconfig.GRUB_CONF = ftmp.name
+            sysh = lib_sysconfig.SysConfigHelper()
+            sysh.remove_grub_configuration()
+            os_remove.assert_called_with(lib_sysconfig.GRUB_CONF)
 
     @mock.patch("lib_sysconfig.hookenv.config")
     @mock.patch("lib_sysconfig.os.remove")