Merge lp:~hloeung/ubuntu-repository-cache/monitor-path-to-synchost into lp:ubuntu-repository-cache

[Haw Loeung]

Enable option to capture and monitor path to the sync host

This is useful for debugging reachability issues with specific vendor/provider/region.

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

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

[lp:~hloeung/ubuntu-repository-cache/monitor-path-to-synchost]

367. By Haw Loeung on 2022-08-03

Fix errors 'Executable path is not absolute' and 'Service lacks both ExecStart= and ExecStop= setting.'

368. By Haw Loeung on 2022-08-03

Fixed the unit test too

369. By Haw Loeung on 2022-08-03

Bump interval to 5mins at first, we can reduce it further if needed

370. By Haw Loeung on 2022-08-03

Fixed to use tmpdir

[Thomas Cuthbert (tcuthbert)]

looking good! can you update the commit message as to why this change is happening? rest of my comments are inline.

review: Needs Fixing

[lp:~hloeung/ubuntu-repository-cache/monitor-path-to-synchost]

371. By Haw Loeung on 2022-08-04

Fixed based on review

372. By Haw Loeung on 2022-08-04

Allow disabling of monitoring of path to the sync-host

373. By Haw Loeung on 2022-08-04

Make it disabled by default

[Haw Loeung (hloeung)]

Commit message updated.

[lp:~hloeung/ubuntu-repository-cache/monitor-path-to-synchost]

374. By Haw Loeung on 2022-08-04

Fixed based on review

375. By Haw Loeung on 2022-08-04

We can't use DynamicUser as that's not supported in the systemd version shipped with Xenial

376. By Haw Loeung on 2022-08-04

Using one isn't enough to reveal some of the hops likely due to ICMP rate limiting

377. By Haw Loeung on 2022-08-04

Bump count / report cycles some more

[Thomas Cuthbert (tcuthbert)]

LGTM +1 with a comment inline.

review: Approve

[lp:~hloeung/ubuntu-repository-cache/monitor-path-to-synchost]

378. By Haw Loeung on 2022-08-04

7 looks like a good balance with enough to reach archive.u.c during testing from AWS.

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

Change successfully merged at revision 364

=== modified file 'config.yaml'
--- config.yaml	2022-07-25 00:51:24 +0000
+++ config.yaml	2022-08-04 02:35:54 +0000
@@ -198,6 +198,11 @@
       This endpoint will check both metadata served from local storage as well
       as pool files via the locally running squid-deb-proxy service.
     default: True
+  enable_sync-host_monitoring:
+    type: boolean
+    description: |
+      Enable periodic mtr outputs to configured sync-host
+    default: False
   leader_unit:
     type: string
     default: ''

=== added directory 'files/systemd'
=== added file 'files/systemd/mtr-synchost.timer'
--- files/systemd/mtr-synchost.timer	1970-01-01 00:00:00 +0000
+++ files/systemd/mtr-synchost.timer	2022-08-04 02:35:54 +0000
@@ -0,0 +1,12 @@
+[Unit]
+Description=Monitor path to sync host regularly
+
+[Timer]
+Unit=mtr-synchost.service
+OnActiveSec=5min
+OnUnitActiveSec=5min
+RemainAfterElapse=no
+Persistent=false
+
+[Install]
+WantedBy=timers.target

=== modified file 'reactive/ubuntu_repository_cache.py'
--- reactive/ubuntu_repository_cache.py	2022-08-01 01:24:02 +0000
+++ reactive/ubuntu_repository_cache.py	2022-08-04 02:35:54 +0000
@@ -10,6 +10,8 @@
 import shutil
 import subprocess
 
+import jinja2
+
 from charms import reactive
 from charms.layer import status
 from charmhelpers.core import hookenv, host, unitdata
@@ -146,6 +148,8 @@
     reactive.clear_flag('ubuntu-repository-cache.configured')
     reactive.clear_flag('nagios-nrpe.configured')
 
+    reactive.set_flag('ubuntu-repository-cache.update-systemd-mtr-required')
+
 
 @reactive.hook('start')
 def start():
@@ -320,6 +324,50 @@
     update_website_relation_data()
 
 
[e-mail]_any('config.changed.sync_host', 'config.changed.enable_sync-host_monitoring')
+def update_systemd_mtr_required():
+    reactive.set_flag('ubuntu-repository-cache.update-systemd-mtr-required')
+
+
[e-mail]('ubuntu-repository-cache.update-systemd-mtr-required')
+def update_systemd_mtr(etc_systemd_path='/etc/systemd/system'):
+    config = hookenv.config()
+    reactive.clear_flag('ubuntu-repository-cache.active')
+
+    mtr_service = os.path.join(etc_systemd_path, 'mtr-synchost.service')
+    mtr_timer = os.path.join(etc_systemd_path, 'mtr-synchost.timer')
+
+    if config['enable_sync-host_monitoring']:
+        status.maintenance('Updating systemd mtr timer & service with sync-host {}'.format(config['sync-host']))
+
+        changed = False
+
+        base = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
+        env = jinja2.Environment(loader=jinja2.FileSystemLoader(base), keep_trailing_newline=True)
+        template = env.get_template('templates/systemd/mtr-synchost.service')
+        contents = template.render({'sync_host': config['sync-host']})
+        changed = _write_file(contents, mtr_service) or changed
+        changed = _copy_file('files/systemd/mtr-synchost.timer', mtr_timer) or changed
+
+        if changed:
+            status.maintenance('Calling systemctl daemon-reload')
+            subprocess.call(['systemctl', 'daemon-reload'])
+            subprocess.call(['systemctl', 'enable', '--now', 'mtr-synchost.timer'])
+
+    else:
+        if os.path.exists(mtr_service) or os.path.exists(mtr_timer):
+            status.maintenance('Disabling systemd mtr timer & service with sync-host {}'.format(config['sync-host']))
+            if os.path.exists(mtr_service):
+                subprocess.call(['systemctl', 'disable', '--now', 'mtr-synchost.service'])
+                os.unlink(mtr_service)
+            if os.path.exists(mtr_timer):
+                subprocess.call(['systemctl', 'disable', '--now', 'mtr-synchost.timer'])
+                os.unlink(mtr_timer)
+            subprocess.call(['systemctl', 'daemon-reload'])
+
+    reactive.clear_flag('ubuntu-repository-cache.update-systemd-mtr-required')
+
+
 # LP:1770071: Work around charm-helper's unison ownership
 #             https://github.com/juju/charm-helpers/issues/487
 def _fix_ssh_ownership(user='www-sync'):
@@ -331,6 +379,12 @@
             os.chown(fname, uid, -1)
 
 
+def _copy_file(source_path, dest_path, **kwargs):
+    with open(source_path, 'r') as f:
+        source = f.read()
+    return _write_file(source, dest_path, **kwargs)
+
+
 def _write_file(source, dest_path, perms=0o644, owner=None, group=None):
     """Write file only on changes and return True if changes written."""
     # Compare and only write out file on change.

=== added directory 'templates/systemd'
=== added file 'templates/systemd/mtr-synchost.service'
--- templates/systemd/mtr-synchost.service	1970-01-01 00:00:00 +0000
+++ templates/systemd/mtr-synchost.service	2022-08-04 02:35:54 +0000
@@ -0,0 +1,15 @@
+[Unit]
+Description=Monitor path to sync host regularly
+Wants=network-online.target
+After=network-online.target
+
+[Service]
+Type=simple
+ExecStart=/usr/bin/mtr -z -c7 --tcp -P 80 --report-wide {{sync_host}}
+ExecStop=
+User=nobody
+Restart=on-failure
+TimeoutSec=5m
+
+[Install]
+WantedBy=multi-user.target

=== modified file 'tests/unit/test_ubuntu_repository_cache.py'
--- tests/unit/test_ubuntu_repository_cache.py	2022-08-01 01:24:02 +0000
+++ tests/unit/test_ubuntu_repository_cache.py	2022-08-04 02:35:54 +0000
@@ -48,7 +48,10 @@
         patcher = mock.patch('charmhelpers.core.hookenv.config')
         self.mock_config = patcher.start()
         self.addCleanup(patcher.stop)
-        self.mock_config.return_value = {'nagios_context': 'juju'}
+        self.mock_config.return_value = {
+            'nagios_context': 'juju',
+            'sync-host': 'archive.ubuntu.com',
+        }
 
         patcher = mock.patch('charmhelpers.core.host.log')
         self.mock_log = patcher.start()
@@ -62,6 +65,19 @@
         if os.path.exists('.juju-persistent-config'):
             os.unlink('.juju-persistent-config')
 
+    @mock.patch('charms.reactive.clear_flag')
+    @mock.patch('charms.reactive.set_flag')
+    def test_upgrade_charm(self, set_flag, clear_flag):
+        ubuntu_repository_cache.upgrade_charm()
+        status.maintenance.assert_called_once_with('Upgrading ubuntu-repository-cache')
+        want = [
+            mock.call('ubuntu-repository-cache.active'),
+            mock.call('ubuntu-repository-cache.configured'),
+            mock.call('nagios-nrpe.configured'),
+        ]
+        clear_flag.assert_has_calls(want, any_order=True)
+        set_flag.assert_called_once_with('ubuntu-repository-cache.update-systemd-mtr-required')
+
     @mock.patch('charmhelpers.core.hookenv.leader_get')
     @mock.patch('charms.reactive.set_flag')
     def test_set_active(self, set_flag, leader_get):
@@ -113,6 +129,115 @@
         ubuntu_repository_cache.set_active(os.path.join(self.charm_dir, 'tests/unit/files/version5'))
         status.active.assert_called_once_with('Ready (source version/commit commit-id)')
 
+    @mock.patch('charms.reactive.set_flag')
+    def test_update_systemd_mtr_required(self, set_flag):
+        ubuntu_repository_cache.update_systemd_mtr_required()
+        set_flag.assert_called_once_with('ubuntu-repository-cache.update-systemd-mtr-required')
+
+    @mock.patch('charms.reactive.clear_flag')
+    @mock.patch('subprocess.call')
+    def test_update_systemd_mtr(self, call, clear_flag):
+        self.mock_config.return_value['enable_sync-host_monitoring'] = True
+        ubuntu_repository_cache.update_systemd_mtr(self.tmpdir)
+        want = [
+            mock.call('ubuntu-repository-cache.active'),
+            mock.call('ubuntu-repository-cache.update-systemd-mtr-required'),
+        ]
+        clear_flag.assert_has_calls(want, any_order=True)
+        want = [
+            mock.call(['systemctl', 'daemon-reload']),
+            mock.call(['systemctl', 'enable', '--now', 'mtr-synchost.timer']),
+        ]
+        call.assert_has_calls(want, any_order=True)
+
+        want = '''[Unit]
+Description=Monitor path to sync host regularly
+Wants=network-online.target
+After=network-online.target
+
+[Service]
+Type=simple
+ExecStart=/usr/bin/mtr -z -c7 --tcp -P 80 --report-wide archive.ubuntu.com
+ExecStop=
+User=nobody
+Restart=on-failure
+TimeoutSec=5m
+
+[Install]
+WantedBy=multi-user.target
+'''  # NOQA: E501
+        with open(os.path.join(self.tmpdir, 'mtr-synchost.service'), 'r', encoding='utf-8') as f:
+            got = f.read()
+        self.assertEqual(want, got)
+
+        with open('files/systemd/mtr-synchost.timer', 'r', encoding='utf-8') as f:
+            want = f.read()
+        with open(os.path.join(self.tmpdir, 'mtr-synchost.timer'), 'r', encoding='utf-8') as f:
+            got = f.read()
+        self.assertEqual(want, got)
+
+        call.reset_mock()
+        ubuntu_repository_cache.update_systemd_mtr(self.tmpdir)
+        call.assert_not_called()
+
+        call.reset_mock()
+        self.mock_config.return_value['enable_sync-host_monitoring'] = False
+        ubuntu_repository_cache.update_systemd_mtr(self.tmpdir)
+        want = [
+            mock.call('ubuntu-repository-cache.active'),
+            mock.call('ubuntu-repository-cache.update-systemd-mtr-required'),
+        ]
+        clear_flag.assert_has_calls(want, any_order=True)
+        want = [
+            mock.call(['systemctl', 'disable', '--now', 'mtr-synchost.service']),
+            mock.call(['systemctl', 'disable', '--now', 'mtr-synchost.timer']),
+            mock.call(['systemctl', 'daemon-reload']),
+        ]
+        call.assert_has_calls(want, any_order=True)
+
+        call.reset_mock()
+        self.mock_config.return_value['enable_sync-host_monitoring'] = False
+        ubuntu_repository_cache.update_systemd_mtr(self.tmpdir)
+        call.assert_not_called()
+
+        call.reset_mock()
+        with open(os.path.join(self.tmpdir, 'mtr-synchost.service'), 'w') as f:
+            f.write("Testing")
+        self.mock_config.return_value['enable_sync-host_monitoring'] = False
+        ubuntu_repository_cache.update_systemd_mtr(self.tmpdir)
+        want = [
+            mock.call(['systemctl', 'disable', '--now', 'mtr-synchost.service']),
+            mock.call(['systemctl', 'daemon-reload']),
+        ]
+        call.assert_has_calls(want, any_order=True)
+
+        call.reset_mock()
+        self.mock_config.return_value['enable_sync-host_monitoring'] = False
+        with open(os.path.join(self.tmpdir, 'mtr-synchost.timer'), 'w') as f:
+            f.write("Testing")
+        ubuntu_repository_cache.update_systemd_mtr(self.tmpdir)
+        ubuntu_repository_cache.update_systemd_mtr(self.tmpdir)
+        want = [
+            mock.call(['systemctl', 'disable', '--now', 'mtr-synchost.timer']),
+            mock.call(['systemctl', 'daemon-reload']),
+        ]
+        call.assert_has_calls(want, any_order=True)
+
+    def test__copy_file(self):
+        source = os.path.join(self.charm_dir, 'metadata.yaml')
+        dest = os.path.join(self.tmpdir, os.path.basename(source))
+
+        self.assertTrue(ubuntu_repository_cache._copy_file(source, dest))
+        # Write again, should return False and not True per above.
+        self.assertFalse(ubuntu_repository_cache._copy_file(source, dest))
+
+        # Check contents
+        with open(source, 'r') as f:
+            want = f.read()
+        with open(dest, 'r') as f:
+            got = f.read()
+        self.assertEqual(got, want)
+
     def test__write_file(self):
         source = '# User-provided config added here'
         dest = os.path.join(self.tmpdir, 'my-test-file')