Merge lp:~exsdev/landscape-client-charm/juju-app-annotator into lp:landscape-client-charm

[Edin S]

Add script and supporting files for registering juju apps as Landscape annotations. There's an accompanying piece of code (in landscape-tagger) that converts these annotations into tags.

A few notes about the design choices:
1. The script is run via a cronjob
The alternative (which was attempted previously) was to run register annotation code whenever charms were deployed/removed. The issue with this design is that removing other subordinate charms (e.g. filebeat) did not trigger any code in this charm (e.g. config-changed, or update-status) to ensure frequent updates of annotations.

2. Annotation deployment status (e.g. deployed/removed) are tracked via annotation values: deployed={true,false}.
This is required to keep track of which applications have been removed. While annotations are registered by creating a file in annotations.d, removing the same file does not remove the annotation. As such annotations are always kept around, but their values/contents are changed (e.g. deployed=true -> deployed=false). The accompanying landscape-tagger code then adds/removes the annotations as tags on landscape-server.

[Jose Guedez (jfguedez)]

Hi, added some inline comments...

[Edin S (exsdev)]

Next reviewer: please decide if line 61 should be changed. It's potentially inefficient in that it makes two calls to regex.match(agent) for every match, but it feels cleaner than the alternatives: a ~5 line for-loop or a double-nested list-comprehension.

[lp:~exsdev/landscape-client-charm/juju-app-annotator]

73. By Edin S on 2021-09-22

Add script and supporting files for registering juju apps as Landscape annotations

A few notes about the design choices:
1. The script is run via a cronjob
The alternative (which was attempted previously) was to run register
annotation code whenever charms were deployed/removed. The issue with
this design is that removing other subordinate charms (e.g. filebeat)
did not trigger any code in this charm (e.g. config-changed, or
update-status) to ensure frequent updates of annotations.

2. Annotation deployment status (e.g. deployed/removed) are tracked
via annotation values: deployed={true,false}.
This is required to keep track of which applications have been
removed. While annotations are registered by creating a file in
annotations.d, removing the same file does not remove the
annotation. As such annotations are always kept around, but their
values/contents are changed (e.g. deployed=true ->
deployed=false). The accompanying landscape-tagger code then
adds/removes the annotations as tags on landscape-server.

74. By Edin S on 2021-09-22

Add Landscape annotator tests

75. By Edin S on 2021-09-22

Add hooks for deploying Landscape annotator scripts (and supporting files)

[Simon Poirier (simpoir)]

The first thing which comes to mind is we should do without client restarts (and the sudoers bit).
Annotations already get monitored at regular intervals (5min) and pushed with other monitor exchanges (as frequently as configured). There is no need to force restart services.

Bonus is that preserves the throttling of server-bound exchanges.
Extra bonus, the code would be way simpler.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Fail
Revno: 75
Branch: lp:~exsdev/landscape-client-charm/juju-app-annotator
Jenkins: https://jenkins.canonical.com/landscape/job/latch-test-xenial/1447/

review: Needs Fixing (test results)

[Stephan Pampel (stephanpampel)]

@exsdev I looked into improving the things that Simon mentioned. One thing I am wondering is why the cronjob is necessary?
I checked locally and on some deployments and the update-status hook is called on each client every ~5 min. Is that not frequent enough to set the annotations?

The update-status hook is currently not implemented but could be added. Avoiding the cronjob might make the code simpler and less error prone.

[Edin S (exsdev)]

Thanks for everyone's feedback so far.

Stephan, I did try running the code via update-status (see my original commit message), but based on my observations the update-status hook did not run in a scheduled/timely/consistent manner (e.g. like a cron) -- but perhaps I wasn't patient enough in my testing :) If you can confirm that update-status runs in a scheduled/timely/consistent manner, then I agree, we should get rid of the cronjob as it simplifies the code.

One thing to note is that you said update-status is called consistently "on some deployments". Does this mean there are environments where it doesn't run consistently? If yes, then we may need to investigate why, or stay with the cron solution.

[Stephan Pampel (stephanpampel)]

> One thing to note is that you said update-status is called consistently "on
> some deployments". Does this mean there are environments where it doesn't run
> consistently? If yes, then we may need to investigate why, or stay with the
> cron solution.

I only checked some, that is why I could not say it for all.

[Stephan Pampel (stephanpampel)]

Created an updated implementation: https://code.launchpad.net/~stephanpampel/landscape-client-charm/landscape-client-charm/+merge/411409

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Fail
Revno: 75
Branch: lp:~exsdev/landscape-client-charm/juju-app-annotator
Jenkins: https://jenkins.canonical.com/landscape/job/latch-test-xenial/1456/

review: Needs Fixing (test results)

[Jeremy Lounder (jldev)]

Superseded by https://code.launchpad.net/~stephanpampel/landscape-client-charm/landscape-client-charm/+merge/411409

Unmerged revisions

75. By Edin S on 2021-09-22

Add hooks for deploying Landscape annotator scripts (and supporting files)

74. By Edin S on 2021-09-22

Add Landscape annotator tests

73. By Edin S on 2021-09-22

Add script and supporting files for registering juju apps as Landscape annotations

A few notes about the design choices:
1. The script is run via a cronjob
The alternative (which was attempted previously) was to run register
annotation code whenever charms were deployed/removed. The issue with
this design is that removing other subordinate charms (e.g. filebeat)
did not trigger any code in this charm (e.g. config-changed, or
update-status) to ensure frequent updates of annotations.

2. Annotation deployment status (e.g. deployed/removed) are tracked
via annotation values: deployed={true,false}.
This is required to keep track of which applications have been
removed. While annotations are registered by creating a file in
annotations.d, removing the same file does not remove the
annotation. As such annotations are always kept around, but their
values/contents are changed (e.g. deployed=true ->
deployed=false). The accompanying landscape-tagger code then
adds/removes the annotations as tags on landscape-server.

=== added directory 'files'
=== added file 'files/landscape-annotator.cron'
--- files/landscape-annotator.cron	1970-01-01 00:00:00 +0000
+++ files/landscape-annotator.cron	2021-09-22 04:34:41 +0000
@@ -0,0 +1,1 @@
+*/1 * * * * landscape flock --nonblock /var/lib/landscape/client/annotator.lock /var/lib/landscape/client/landscape_annotator.py

=== added file 'files/landscape-annotator.sudoers'
--- files/landscape-annotator.sudoers	1970-01-01 00:00:00 +0000
+++ files/landscape-annotator.sudoers	2021-09-22 04:34:41 +0000
@@ -0,0 +1,1 @@
+landscape ALL=NOPASSWD: /bin/systemctl restart landscape-client

=== added file 'files/landscape_annotator.py'
--- files/landscape_annotator.py	1970-01-01 00:00:00 +0000
+++ files/landscape_annotator.py	2021-09-22 04:34:41 +0000
@@ -0,0 +1,105 @@
+#!/usr/bin/env python3
+
+import argparse
+import logging
+import os
+import pathlib
+import re
+import subprocess
+
+JUJU_AGENT_DIR = "/var/lib/juju/agents"
+LANDSCAPE_ANNOTATIONS_DIR = "/var/lib/landscape/client/annotations.d/"
+
+
+def cli_arg_parse():
+    """Configure CLI argument parser for enabling simple debugging."""
+    parser = argparse.ArgumentParser()
+    parser.add_argument("-d", "--debug", action="store_true")
+    return parser.parse_args()
+
+
+def set_landscape_annotations(annotation_list, value, from_value=""):
+    """Create/update annotations files."""
+    replaced = False
+    for annotation in annotation_list:
+        annotation_file = os.path.join(LANDSCAPE_ANNOTATIONS_DIR, annotation)
+
+        if text_replace(annotation_file,
+                        from_value,
+                        value):
+            replaced = True
+    return replaced
+
+
+def get_existing_annotations():
+    """Get a list of currently existing annotations."""
+    annotation_list = os.listdir(LANDSCAPE_ANNOTATIONS_DIR)
+    logging.debug("existing annotations: {}".format(annotation_list))
+    return annotation_list
+
+
+def get_deployed_juju_app_list():
+    """Return a list of deployed juju apps."""
+    agent_dirs = os.listdir(JUJU_AGENT_DIR)
+    regex = re.compile(r"^unit-([\w-]+)-\d+")
+    app_list = [regex.match(agent)[1] for agent in
+                agent_dirs if regex.match(agent)]
+    return app_list
+
+
+def text_replace(filepath, orig, replacement):
+    replaced = False
+    # create file if it doesn't exist to stop subsequent block from
+    # throwing an exception
+    if not os.path.exists(filepath):
+        pathlib.Path(filepath).touch(exist_ok=True)
+
+    with open(filepath, "r+") as f:
+        if orig in f.read():
+            log_line = "annotation {} change value to {}"
+            logging.debug(log_line.format(os.path.basename(filepath),
+                                          replacement))
+            f.seek(0)
+            f.write(replacement)
+            f.truncate()
+            replaced = True
+
+    return replaced
+
+
+def configure_annotations():
+    existing_annotations = set(get_existing_annotations())
+    installed_apps = set(get_deployed_juju_app_list())
+
+    new_apps = list(installed_apps - existing_annotations)
+    logging.debug("new apps: {}".format(new_apps))
+
+    # apps may have been redeployed, these files need to be rechecked
+    check_annotations = list(set.intersection(installed_apps,
+                                              existing_annotations))
+    logging.debug("check annotations: {}".format(check_annotations))
+
+    deleted_apps = list(existing_annotations - installed_apps)
+    logging.debug("deleted apps: {}".format(deleted_apps))
+
+    # specifying from_value guards against always replacing (requires restart)
+    restart = any([set_landscape_annotations(deleted_apps,
+                                             "deployed=false",
+                                             from_value="deployed=true"),
+                   set_landscape_annotations(new_apps,
+                                             "deployed=true"),
+                   set_landscape_annotations(check_annotations,
+                                             "deployed=true",
+                                             from_value="deployed=false")])
+
+    if restart:
+        logging.debug("restarting landscape-client")
+        subprocess.Popen(["sudo", "systemctl", "restart", "landscape-client"])
+
+
+if __name__ == "__main__":
+    args = cli_arg_parse()
+    if args.debug:
+        logging.basicConfig(level=logging.DEBUG)
+
+    configure_annotations()

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2019-04-04 16:07:48 +0000
+++ hooks/hooks.py	2021-09-22 04:34:41 +0000
@@ -18,7 +18,7 @@
 from ceph import (
     get_ceph_client_path, write_ceph_client_keyring, write_ceph_client_config)
 
-from install import install
+from install import install, deploy_annotator
 
 CERT_FILE = "/etc/ssl/certs/landscape_server_ca.crt"
 
@@ -82,6 +82,7 @@
 @hooks.hook("upgrade-charm")
 def upgrade_charm(landscape_broker=LANDSCAPE_BROKER):
     """Idempotently upgrades state from older charms."""
+    deploy_annotator()
     landscape_broker.config.reload()
     return migrate_old_juju_info(landscape_broker.config)
 

=== modified file 'hooks/install.py'
--- hooks/install.py	2019-02-03 14:50:36 +0000
+++ hooks/install.py	2021-09-22 04:34:41 +0000
@@ -5,13 +5,17 @@
 functions, and landscape is not installed until this hook runs.
 """
 
+import os
+import shutil
 import subprocess
 import sys
 
 from charmhelpers import fetch
+from charmhelpers.core.host import rsync
 import charmhelpers.core.hookenv as hookenv
 
 
+ANNOTATIONS_DIR = "/var/lib/landscape/client/annotations.d/"
 hooks = hookenv.Hooks()
 
 
@@ -34,6 +38,8 @@
             "install", "-o", "landscape", "-g", "root", "-m", "755", "-d",
             data_path])
 
+    deploy_annotator()
+
 
 def add_apt_source(url, fetch=fetch):
     """Add an apt source entry, with passed in key.
@@ -54,6 +60,28 @@
     return fetch.add_source(url, key)
 
 
+def deploy_annotator():
+    """Setup files for creating annotations."""
+    
+    # model name is only available from within the juju context
+    # (i.e. not script run by cron) so register annotation here.
+    juju_model = hookenv.model_name()
+    annotation_path = os.path.join(ANNOTATIONS_DIR, "model")
+    with open(annotation_path, "w") as f:
+        f.write(juju_model)
+
+    # can't use common.chown because it imports landscape-client
+    # (potentially before it's installed)
+    shutil.chown(annotation_path, user="landscape", group="landscape")
+
+    rsync("files/landscape_annotator.py",
+          "/var/lib/landscape/client/landscape_annotator.py")
+    rsync("files/landscape-annotator.sudoers",
+          "/etc/sudoers.d/landscape-annotator")
+    rsync("files/landscape-annotator.cron",
+          "/etc/cron.d/landscape-annotator")
+
+
 if __name__ == '__main__':
     try:
         sys.exit(hooks.execute(sys.argv))

=== added file 'tests/20-landscape-annotator.py'
--- tests/20-landscape-annotator.py	1970-01-01 00:00:00 +0000
+++ tests/20-landscape-annotator.py	2021-09-22 04:34:41 +0000
@@ -0,0 +1,131 @@
+#!/usr/bin/env python3
+
+import unittest
+
+from collections import namedtuple
+from unittest.mock import patch, mock_open
+
+import landscape_annotator as annotator
+
+
+class AnnotatorTest(unittest.TestCase):
+    """Create a class for testing the landscape annotator code."""
+    
+    @patch("os.listdir")
+    def test_get_existing_annotations(self, mock_os_listdir):
+        """Test to see existing annotations are detected."""
+        dir_contents = []
+        mock_os_listdir.return_value = dir_contents
+        annotations = annotator.get_existing_annotations()
+        self.assertEqual(annotations, dir_contents)
+
+        dir_contents = ["rabbitmq-server", "landscape-client"]
+        mock_os_listdir.return_value = dir_contents
+        annotations = annotator.get_existing_annotations()
+        self.assertEqual(annotations, dir_contents)
+
+    @patch("os.listdir")
+    def test_get_deployed_juju_app_list(self, mock_os_listdir):
+        """Test to see if juju agents (dirs) are listed as installed apps."""
+        dir_contents = []
+        app_list = annotator.get_deployed_juju_app_list()
+        self.assertEqual(app_list, [])
+    
+        dir_contents = ["unit-landscape-server-0",
+                        "unit-landscape-client-1",
+                        "unit-nova-compute-kvm-0",
+                        "somedir"]
+        mock_os_listdir.return_value = dir_contents
+        app_list = annotator.get_deployed_juju_app_list()
+
+        self.assertEqual(app_list, ["landscape-server",
+                                    "landscape-client",
+                                    "nova-compute-kvm"])
+
+    def test_text_replace(self):
+        """Test to see if text replace is being done/ignored as expected."""
+        TestData = namedtuple("TestData", ["file_contents",
+                                           "orig_text",
+                                           "replace_text",
+                                           "is_replaced"])
+
+        test_data_list = [
+            TestData("deployed=true", "deployed=true", "deployed=false", True),
+            TestData("deployed=true", "", "deployed=false", True),
+            TestData("deployed=true", "deployed=false", "xxx", False)
+        ]
+
+        for test_data in test_data_list:
+            fake_open = mock_open(read_data=test_data.file_contents)
+            with patch("builtins.open", fake_open):
+                replaced = annotator.text_replace("fake.txt",
+                                                  test_data.orig_text,
+                                                  test_data.replace_text)
+                self.assertEqual(replaced, test_data.is_replaced)
+
+    @patch("landscape_annotator.text_replace")
+    def test_set_landscape_annotations(self, mock_text_replace):
+        """Test to see if setting new annotation values are registered."""
+        annotation_list = ["filebeat", "rabbitmq-server"]
+        mock_text_replace.side_effect = [True, False]
+        replaced = annotator.set_landscape_annotations(annotation_list, "x")
+        self.assertTrue(mock_text_replace.call_count == len(annotation_list))
+        self.assertTrue(replaced)
+
+        mock_text_replace.side_effect = [False, False]
+        replaced = annotator.set_landscape_annotations(annotation_list,
+                                                       "x")
+        self.assertTrue(not replaced)
+
+    @patch("landscape_annotator.get_existing_annotations")
+    @patch("landscape_annotator.get_deployed_juju_app_list")
+    @patch("landscape_annotator.subprocess.Popen")
+    @patch("landscape_annotator.set_landscape_annotations")
+    def test_configure_annotations(self, mock_set_annotations,
+                                   mock_restart_client_process,
+                                   mock_get_deployed_juju_app_list,
+                                   mock_get_existing_annotations):
+        """Test to see if changes to annotations are handled properly."""
+        # test adding new application
+        mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server"]
+        mock_get_existing_annotations.return_value = []
+        mock_set_annotations.return_value = True
+        annotator.configure_annotations()
+        mock_set_annotations.assert_any_call(["rabbitmq-server"],
+                                             "deployed=true")
+        mock_restart_client_process.assert_called_once()
+
+        # test no change in annotations
+        mock_set_annotations.reset_mock()
+        mock_restart_client_process.reset_mock()
+        mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server"]
+        mock_get_existing_annotations.return_value = ["rabbitmq-server"]
+        mock_set_annotations.return_value = False
+        annotator.configure_annotations()
+        mock_restart_client_process.assert_not_called()
+
+        # test additional new application
+        mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server",
+                                                        "filebeats"]
+        mock_get_existing_annotations.return_value = ["rabbitmq-server"]
+        mock_set_annotations.return_value = True
+        annotator.configure_annotations()
+        mock_set_annotations.assert_any_call(["filebeats"], "deployed=true")
+        mock_restart_client_process.assert_called_once()
+
+        # test deletion of application
+        mock_set_annotations.reset_mock()
+        mock_restart_client_process.reset_mock()
+        mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server"]
+        mock_get_existing_annotations.return_value = ["rabbitmq-server",
+                                                        "filebeats"]
+        mock_set_annotations.return_value = True
+        annotator.configure_annotations()
+        mock_set_annotations.assert_any_call(["filebeats"],
+                                             "deployed=false",
+                                             from_value="deployed=true")
+        mock_restart_client_process.assert_called_once()
+
+
+if __name__ == '__main__':
+    unittest.main()