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

Proposed by Edin S
Status: Rejected
Rejected by: Jeremy Lounder
Proposed branch: lp:~exsdev/landscape-client-charm/juju-app-annotator
Merge into: lp:landscape-client-charm
Diff against target: 339 lines (+268/-1)
6 files modified
files/landscape-annotator.cron (+1/-0)
files/landscape-annotator.sudoers (+1/-0)
files/landscape_annotator.py (+105/-0)
hooks/hooks.py (+2/-1)
hooks/install.py (+28/-0)
tests/20-landscape-annotator.py (+131/-0)
To merge this branch: bzr merge lp:~exsdev/landscape-client-charm/juju-app-annotator
Reviewer Review Type Date Requested Status
Landscape Builder test results Needs Fixing
Landscape Pending
Review via email: mp+408794@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Jose Guedez (jfguedez) wrote :

Hi, added some inline comments...

Revision history for this message
Edin S (exsdev) wrote :

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.

73. By Edin S

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

Add Landscape annotator tests

75. By Edin S

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

Revision history for this message
Simon Poirier (simpoir) wrote :

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.

Revision history for this message
Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
Landscape Builder (landscape-builder) wrote :
review: Needs Fixing (test results)
Revision history for this message
Stephan Pampel (stephanpampel) wrote :

@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.

Revision history for this message
Edin S (exsdev) wrote :

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.

Revision history for this message
Stephan Pampel (stephanpampel) wrote :

> 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.

Revision history for this message
Stephan Pampel (stephanpampel) wrote :
Revision history for this message
Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
Landscape Builder (landscape-builder) wrote :
review: Needs Fixing (test results)
Revision history for this message
Jeremy Lounder (jldev) wrote :

Unmerged revisions

75. By Edin S

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

74. By Edin S

Add Landscape annotator tests

73. By Edin S

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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*/1 * * * * landscape flock --nonblock /var/lib/landscape/client/annotator.lock /var/lib/landscape/client/landscape_annotator.py
02
=== 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 @@
1landscape ALL=NOPASSWD: /bin/systemctl restart landscape-client
02
=== 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 @@
1#!/usr/bin/env python3
2
3import argparse
4import logging
5import os
6import pathlib
7import re
8import subprocess
9
10JUJU_AGENT_DIR = "/var/lib/juju/agents"
11LANDSCAPE_ANNOTATIONS_DIR = "/var/lib/landscape/client/annotations.d/"
12
13
14def cli_arg_parse():
15 """Configure CLI argument parser for enabling simple debugging."""
16 parser = argparse.ArgumentParser()
17 parser.add_argument("-d", "--debug", action="store_true")
18 return parser.parse_args()
19
20
21def set_landscape_annotations(annotation_list, value, from_value=""):
22 """Create/update annotations files."""
23 replaced = False
24 for annotation in annotation_list:
25 annotation_file = os.path.join(LANDSCAPE_ANNOTATIONS_DIR, annotation)
26
27 if text_replace(annotation_file,
28 from_value,
29 value):
30 replaced = True
31 return replaced
32
33
34def get_existing_annotations():
35 """Get a list of currently existing annotations."""
36 annotation_list = os.listdir(LANDSCAPE_ANNOTATIONS_DIR)
37 logging.debug("existing annotations: {}".format(annotation_list))
38 return annotation_list
39
40
41def get_deployed_juju_app_list():
42 """Return a list of deployed juju apps."""
43 agent_dirs = os.listdir(JUJU_AGENT_DIR)
44 regex = re.compile(r"^unit-([\w-]+)-\d+")
45 app_list = [regex.match(agent)[1] for agent in
46 agent_dirs if regex.match(agent)]
47 return app_list
48
49
50def text_replace(filepath, orig, replacement):
51 replaced = False
52 # create file if it doesn't exist to stop subsequent block from
53 # throwing an exception
54 if not os.path.exists(filepath):
55 pathlib.Path(filepath).touch(exist_ok=True)
56
57 with open(filepath, "r+") as f:
58 if orig in f.read():
59 log_line = "annotation {} change value to {}"
60 logging.debug(log_line.format(os.path.basename(filepath),
61 replacement))
62 f.seek(0)
63 f.write(replacement)
64 f.truncate()
65 replaced = True
66
67 return replaced
68
69
70def configure_annotations():
71 existing_annotations = set(get_existing_annotations())
72 installed_apps = set(get_deployed_juju_app_list())
73
74 new_apps = list(installed_apps - existing_annotations)
75 logging.debug("new apps: {}".format(new_apps))
76
77 # apps may have been redeployed, these files need to be rechecked
78 check_annotations = list(set.intersection(installed_apps,
79 existing_annotations))
80 logging.debug("check annotations: {}".format(check_annotations))
81
82 deleted_apps = list(existing_annotations - installed_apps)
83 logging.debug("deleted apps: {}".format(deleted_apps))
84
85 # specifying from_value guards against always replacing (requires restart)
86 restart = any([set_landscape_annotations(deleted_apps,
87 "deployed=false",
88 from_value="deployed=true"),
89 set_landscape_annotations(new_apps,
90 "deployed=true"),
91 set_landscape_annotations(check_annotations,
92 "deployed=true",
93 from_value="deployed=false")])
94
95 if restart:
96 logging.debug("restarting landscape-client")
97 subprocess.Popen(["sudo", "systemctl", "restart", "landscape-client"])
98
99
100if __name__ == "__main__":
101 args = cli_arg_parse()
102 if args.debug:
103 logging.basicConfig(level=logging.DEBUG)
104
105 configure_annotations()
0106
=== 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 @@
18from ceph import (18from ceph import (
19 get_ceph_client_path, write_ceph_client_keyring, write_ceph_client_config)19 get_ceph_client_path, write_ceph_client_keyring, write_ceph_client_config)
2020
21from install import install21from install import install, deploy_annotator
2222
23CERT_FILE = "/etc/ssl/certs/landscape_server_ca.crt"23CERT_FILE = "/etc/ssl/certs/landscape_server_ca.crt"
2424
@@ -82,6 +82,7 @@
82@hooks.hook("upgrade-charm")82@hooks.hook("upgrade-charm")
83def upgrade_charm(landscape_broker=LANDSCAPE_BROKER):83def upgrade_charm(landscape_broker=LANDSCAPE_BROKER):
84 """Idempotently upgrades state from older charms."""84 """Idempotently upgrades state from older charms."""
85 deploy_annotator()
85 landscape_broker.config.reload()86 landscape_broker.config.reload()
86 return migrate_old_juju_info(landscape_broker.config)87 return migrate_old_juju_info(landscape_broker.config)
8788
8889
=== 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 @@
5functions, and landscape is not installed until this hook runs.5functions, and landscape is not installed until this hook runs.
6"""6"""
77
8import os
9import shutil
8import subprocess10import subprocess
9import sys11import sys
1012
11from charmhelpers import fetch13from charmhelpers import fetch
14from charmhelpers.core.host import rsync
12import charmhelpers.core.hookenv as hookenv15import charmhelpers.core.hookenv as hookenv
1316
1417
18ANNOTATIONS_DIR = "/var/lib/landscape/client/annotations.d/"
15hooks = hookenv.Hooks()19hooks = hookenv.Hooks()
1620
1721
@@ -34,6 +38,8 @@
34 "install", "-o", "landscape", "-g", "root", "-m", "755", "-d",38 "install", "-o", "landscape", "-g", "root", "-m", "755", "-d",
35 data_path])39 data_path])
3640
41 deploy_annotator()
42
3743
38def add_apt_source(url, fetch=fetch):44def add_apt_source(url, fetch=fetch):
39 """Add an apt source entry, with passed in key.45 """Add an apt source entry, with passed in key.
@@ -54,6 +60,28 @@
54 return fetch.add_source(url, key)60 return fetch.add_source(url, key)
5561
5662
63def deploy_annotator():
64 """Setup files for creating annotations."""
65
66 # model name is only available from within the juju context
67 # (i.e. not script run by cron) so register annotation here.
68 juju_model = hookenv.model_name()
69 annotation_path = os.path.join(ANNOTATIONS_DIR, "model")
70 with open(annotation_path, "w") as f:
71 f.write(juju_model)
72
73 # can't use common.chown because it imports landscape-client
74 # (potentially before it's installed)
75 shutil.chown(annotation_path, user="landscape", group="landscape")
76
77 rsync("files/landscape_annotator.py",
78 "/var/lib/landscape/client/landscape_annotator.py")
79 rsync("files/landscape-annotator.sudoers",
80 "/etc/sudoers.d/landscape-annotator")
81 rsync("files/landscape-annotator.cron",
82 "/etc/cron.d/landscape-annotator")
83
84
57if __name__ == '__main__':85if __name__ == '__main__':
58 try:86 try:
59 sys.exit(hooks.execute(sys.argv))87 sys.exit(hooks.execute(sys.argv))
6088
=== 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 @@
1#!/usr/bin/env python3
2
3import unittest
4
5from collections import namedtuple
6from unittest.mock import patch, mock_open
7
8import landscape_annotator as annotator
9
10
11class AnnotatorTest(unittest.TestCase):
12 """Create a class for testing the landscape annotator code."""
13
14 @patch("os.listdir")
15 def test_get_existing_annotations(self, mock_os_listdir):
16 """Test to see existing annotations are detected."""
17 dir_contents = []
18 mock_os_listdir.return_value = dir_contents
19 annotations = annotator.get_existing_annotations()
20 self.assertEqual(annotations, dir_contents)
21
22 dir_contents = ["rabbitmq-server", "landscape-client"]
23 mock_os_listdir.return_value = dir_contents
24 annotations = annotator.get_existing_annotations()
25 self.assertEqual(annotations, dir_contents)
26
27 @patch("os.listdir")
28 def test_get_deployed_juju_app_list(self, mock_os_listdir):
29 """Test to see if juju agents (dirs) are listed as installed apps."""
30 dir_contents = []
31 app_list = annotator.get_deployed_juju_app_list()
32 self.assertEqual(app_list, [])
33
34 dir_contents = ["unit-landscape-server-0",
35 "unit-landscape-client-1",
36 "unit-nova-compute-kvm-0",
37 "somedir"]
38 mock_os_listdir.return_value = dir_contents
39 app_list = annotator.get_deployed_juju_app_list()
40
41 self.assertEqual(app_list, ["landscape-server",
42 "landscape-client",
43 "nova-compute-kvm"])
44
45 def test_text_replace(self):
46 """Test to see if text replace is being done/ignored as expected."""
47 TestData = namedtuple("TestData", ["file_contents",
48 "orig_text",
49 "replace_text",
50 "is_replaced"])
51
52 test_data_list = [
53 TestData("deployed=true", "deployed=true", "deployed=false", True),
54 TestData("deployed=true", "", "deployed=false", True),
55 TestData("deployed=true", "deployed=false", "xxx", False)
56 ]
57
58 for test_data in test_data_list:
59 fake_open = mock_open(read_data=test_data.file_contents)
60 with patch("builtins.open", fake_open):
61 replaced = annotator.text_replace("fake.txt",
62 test_data.orig_text,
63 test_data.replace_text)
64 self.assertEqual(replaced, test_data.is_replaced)
65
66 @patch("landscape_annotator.text_replace")
67 def test_set_landscape_annotations(self, mock_text_replace):
68 """Test to see if setting new annotation values are registered."""
69 annotation_list = ["filebeat", "rabbitmq-server"]
70 mock_text_replace.side_effect = [True, False]
71 replaced = annotator.set_landscape_annotations(annotation_list, "x")
72 self.assertTrue(mock_text_replace.call_count == len(annotation_list))
73 self.assertTrue(replaced)
74
75 mock_text_replace.side_effect = [False, False]
76 replaced = annotator.set_landscape_annotations(annotation_list,
77 "x")
78 self.assertTrue(not replaced)
79
80 @patch("landscape_annotator.get_existing_annotations")
81 @patch("landscape_annotator.get_deployed_juju_app_list")
82 @patch("landscape_annotator.subprocess.Popen")
83 @patch("landscape_annotator.set_landscape_annotations")
84 def test_configure_annotations(self, mock_set_annotations,
85 mock_restart_client_process,
86 mock_get_deployed_juju_app_list,
87 mock_get_existing_annotations):
88 """Test to see if changes to annotations are handled properly."""
89 # test adding new application
90 mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server"]
91 mock_get_existing_annotations.return_value = []
92 mock_set_annotations.return_value = True
93 annotator.configure_annotations()
94 mock_set_annotations.assert_any_call(["rabbitmq-server"],
95 "deployed=true")
96 mock_restart_client_process.assert_called_once()
97
98 # test no change in annotations
99 mock_set_annotations.reset_mock()
100 mock_restart_client_process.reset_mock()
101 mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server"]
102 mock_get_existing_annotations.return_value = ["rabbitmq-server"]
103 mock_set_annotations.return_value = False
104 annotator.configure_annotations()
105 mock_restart_client_process.assert_not_called()
106
107 # test additional new application
108 mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server",
109 "filebeats"]
110 mock_get_existing_annotations.return_value = ["rabbitmq-server"]
111 mock_set_annotations.return_value = True
112 annotator.configure_annotations()
113 mock_set_annotations.assert_any_call(["filebeats"], "deployed=true")
114 mock_restart_client_process.assert_called_once()
115
116 # test deletion of application
117 mock_set_annotations.reset_mock()
118 mock_restart_client_process.reset_mock()
119 mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server"]
120 mock_get_existing_annotations.return_value = ["rabbitmq-server",
121 "filebeats"]
122 mock_set_annotations.return_value = True
123 annotator.configure_annotations()
124 mock_set_annotations.assert_any_call(["filebeats"],
125 "deployed=false",
126 from_value="deployed=true")
127 mock_restart_client_process.assert_called_once()
128
129
130if __name__ == '__main__':
131 unittest.main()

Subscribers

People subscribed via source and target branches

to all changes: