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
1=== added directory 'files'
2=== added file 'files/landscape-annotator.cron'
3--- files/landscape-annotator.cron 1970-01-01 00:00:00 +0000
4+++ files/landscape-annotator.cron 2021-09-22 04:34:41 +0000
5@@ -0,0 +1,1 @@
6+*/1 * * * * landscape flock --nonblock /var/lib/landscape/client/annotator.lock /var/lib/landscape/client/landscape_annotator.py
7
8=== added file 'files/landscape-annotator.sudoers'
9--- files/landscape-annotator.sudoers 1970-01-01 00:00:00 +0000
10+++ files/landscape-annotator.sudoers 2021-09-22 04:34:41 +0000
11@@ -0,0 +1,1 @@
12+landscape ALL=NOPASSWD: /bin/systemctl restart landscape-client
13
14=== added file 'files/landscape_annotator.py'
15--- files/landscape_annotator.py 1970-01-01 00:00:00 +0000
16+++ files/landscape_annotator.py 2021-09-22 04:34:41 +0000
17@@ -0,0 +1,105 @@
18+#!/usr/bin/env python3
19+
20+import argparse
21+import logging
22+import os
23+import pathlib
24+import re
25+import subprocess
26+
27+JUJU_AGENT_DIR = "/var/lib/juju/agents"
28+LANDSCAPE_ANNOTATIONS_DIR = "/var/lib/landscape/client/annotations.d/"
29+
30+
31+def cli_arg_parse():
32+ """Configure CLI argument parser for enabling simple debugging."""
33+ parser = argparse.ArgumentParser()
34+ parser.add_argument("-d", "--debug", action="store_true")
35+ return parser.parse_args()
36+
37+
38+def set_landscape_annotations(annotation_list, value, from_value=""):
39+ """Create/update annotations files."""
40+ replaced = False
41+ for annotation in annotation_list:
42+ annotation_file = os.path.join(LANDSCAPE_ANNOTATIONS_DIR, annotation)
43+
44+ if text_replace(annotation_file,
45+ from_value,
46+ value):
47+ replaced = True
48+ return replaced
49+
50+
51+def get_existing_annotations():
52+ """Get a list of currently existing annotations."""
53+ annotation_list = os.listdir(LANDSCAPE_ANNOTATIONS_DIR)
54+ logging.debug("existing annotations: {}".format(annotation_list))
55+ return annotation_list
56+
57+
58+def get_deployed_juju_app_list():
59+ """Return a list of deployed juju apps."""
60+ agent_dirs = os.listdir(JUJU_AGENT_DIR)
61+ regex = re.compile(r"^unit-([\w-]+)-\d+")
62+ app_list = [regex.match(agent)[1] for agent in
63+ agent_dirs if regex.match(agent)]
64+ return app_list
65+
66+
67+def text_replace(filepath, orig, replacement):
68+ replaced = False
69+ # create file if it doesn't exist to stop subsequent block from
70+ # throwing an exception
71+ if not os.path.exists(filepath):
72+ pathlib.Path(filepath).touch(exist_ok=True)
73+
74+ with open(filepath, "r+") as f:
75+ if orig in f.read():
76+ log_line = "annotation {} change value to {}"
77+ logging.debug(log_line.format(os.path.basename(filepath),
78+ replacement))
79+ f.seek(0)
80+ f.write(replacement)
81+ f.truncate()
82+ replaced = True
83+
84+ return replaced
85+
86+
87+def configure_annotations():
88+ existing_annotations = set(get_existing_annotations())
89+ installed_apps = set(get_deployed_juju_app_list())
90+
91+ new_apps = list(installed_apps - existing_annotations)
92+ logging.debug("new apps: {}".format(new_apps))
93+
94+ # apps may have been redeployed, these files need to be rechecked
95+ check_annotations = list(set.intersection(installed_apps,
96+ existing_annotations))
97+ logging.debug("check annotations: {}".format(check_annotations))
98+
99+ deleted_apps = list(existing_annotations - installed_apps)
100+ logging.debug("deleted apps: {}".format(deleted_apps))
101+
102+ # specifying from_value guards against always replacing (requires restart)
103+ restart = any([set_landscape_annotations(deleted_apps,
104+ "deployed=false",
105+ from_value="deployed=true"),
106+ set_landscape_annotations(new_apps,
107+ "deployed=true"),
108+ set_landscape_annotations(check_annotations,
109+ "deployed=true",
110+ from_value="deployed=false")])
111+
112+ if restart:
113+ logging.debug("restarting landscape-client")
114+ subprocess.Popen(["sudo", "systemctl", "restart", "landscape-client"])
115+
116+
117+if __name__ == "__main__":
118+ args = cli_arg_parse()
119+ if args.debug:
120+ logging.basicConfig(level=logging.DEBUG)
121+
122+ configure_annotations()
123
124=== modified file 'hooks/hooks.py'
125--- hooks/hooks.py 2019-04-04 16:07:48 +0000
126+++ hooks/hooks.py 2021-09-22 04:34:41 +0000
127@@ -18,7 +18,7 @@
128 from ceph import (
129 get_ceph_client_path, write_ceph_client_keyring, write_ceph_client_config)
130
131-from install import install
132+from install import install, deploy_annotator
133
134 CERT_FILE = "/etc/ssl/certs/landscape_server_ca.crt"
135
136@@ -82,6 +82,7 @@
137 @hooks.hook("upgrade-charm")
138 def upgrade_charm(landscape_broker=LANDSCAPE_BROKER):
139 """Idempotently upgrades state from older charms."""
140+ deploy_annotator()
141 landscape_broker.config.reload()
142 return migrate_old_juju_info(landscape_broker.config)
143
144
145=== modified file 'hooks/install.py'
146--- hooks/install.py 2019-02-03 14:50:36 +0000
147+++ hooks/install.py 2021-09-22 04:34:41 +0000
148@@ -5,13 +5,17 @@
149 functions, and landscape is not installed until this hook runs.
150 """
151
152+import os
153+import shutil
154 import subprocess
155 import sys
156
157 from charmhelpers import fetch
158+from charmhelpers.core.host import rsync
159 import charmhelpers.core.hookenv as hookenv
160
161
162+ANNOTATIONS_DIR = "/var/lib/landscape/client/annotations.d/"
163 hooks = hookenv.Hooks()
164
165
166@@ -34,6 +38,8 @@
167 "install", "-o", "landscape", "-g", "root", "-m", "755", "-d",
168 data_path])
169
170+ deploy_annotator()
171+
172
173 def add_apt_source(url, fetch=fetch):
174 """Add an apt source entry, with passed in key.
175@@ -54,6 +60,28 @@
176 return fetch.add_source(url, key)
177
178
179+def deploy_annotator():
180+ """Setup files for creating annotations."""
181+
182+ # model name is only available from within the juju context
183+ # (i.e. not script run by cron) so register annotation here.
184+ juju_model = hookenv.model_name()
185+ annotation_path = os.path.join(ANNOTATIONS_DIR, "model")
186+ with open(annotation_path, "w") as f:
187+ f.write(juju_model)
188+
189+ # can't use common.chown because it imports landscape-client
190+ # (potentially before it's installed)
191+ shutil.chown(annotation_path, user="landscape", group="landscape")
192+
193+ rsync("files/landscape_annotator.py",
194+ "/var/lib/landscape/client/landscape_annotator.py")
195+ rsync("files/landscape-annotator.sudoers",
196+ "/etc/sudoers.d/landscape-annotator")
197+ rsync("files/landscape-annotator.cron",
198+ "/etc/cron.d/landscape-annotator")
199+
200+
201 if __name__ == '__main__':
202 try:
203 sys.exit(hooks.execute(sys.argv))
204
205=== added file 'tests/20-landscape-annotator.py'
206--- tests/20-landscape-annotator.py 1970-01-01 00:00:00 +0000
207+++ tests/20-landscape-annotator.py 2021-09-22 04:34:41 +0000
208@@ -0,0 +1,131 @@
209+#!/usr/bin/env python3
210+
211+import unittest
212+
213+from collections import namedtuple
214+from unittest.mock import patch, mock_open
215+
216+import landscape_annotator as annotator
217+
218+
219+class AnnotatorTest(unittest.TestCase):
220+ """Create a class for testing the landscape annotator code."""
221+
222+ @patch("os.listdir")
223+ def test_get_existing_annotations(self, mock_os_listdir):
224+ """Test to see existing annotations are detected."""
225+ dir_contents = []
226+ mock_os_listdir.return_value = dir_contents
227+ annotations = annotator.get_existing_annotations()
228+ self.assertEqual(annotations, dir_contents)
229+
230+ dir_contents = ["rabbitmq-server", "landscape-client"]
231+ mock_os_listdir.return_value = dir_contents
232+ annotations = annotator.get_existing_annotations()
233+ self.assertEqual(annotations, dir_contents)
234+
235+ @patch("os.listdir")
236+ def test_get_deployed_juju_app_list(self, mock_os_listdir):
237+ """Test to see if juju agents (dirs) are listed as installed apps."""
238+ dir_contents = []
239+ app_list = annotator.get_deployed_juju_app_list()
240+ self.assertEqual(app_list, [])
241+
242+ dir_contents = ["unit-landscape-server-0",
243+ "unit-landscape-client-1",
244+ "unit-nova-compute-kvm-0",
245+ "somedir"]
246+ mock_os_listdir.return_value = dir_contents
247+ app_list = annotator.get_deployed_juju_app_list()
248+
249+ self.assertEqual(app_list, ["landscape-server",
250+ "landscape-client",
251+ "nova-compute-kvm"])
252+
253+ def test_text_replace(self):
254+ """Test to see if text replace is being done/ignored as expected."""
255+ TestData = namedtuple("TestData", ["file_contents",
256+ "orig_text",
257+ "replace_text",
258+ "is_replaced"])
259+
260+ test_data_list = [
261+ TestData("deployed=true", "deployed=true", "deployed=false", True),
262+ TestData("deployed=true", "", "deployed=false", True),
263+ TestData("deployed=true", "deployed=false", "xxx", False)
264+ ]
265+
266+ for test_data in test_data_list:
267+ fake_open = mock_open(read_data=test_data.file_contents)
268+ with patch("builtins.open", fake_open):
269+ replaced = annotator.text_replace("fake.txt",
270+ test_data.orig_text,
271+ test_data.replace_text)
272+ self.assertEqual(replaced, test_data.is_replaced)
273+
274+ @patch("landscape_annotator.text_replace")
275+ def test_set_landscape_annotations(self, mock_text_replace):
276+ """Test to see if setting new annotation values are registered."""
277+ annotation_list = ["filebeat", "rabbitmq-server"]
278+ mock_text_replace.side_effect = [True, False]
279+ replaced = annotator.set_landscape_annotations(annotation_list, "x")
280+ self.assertTrue(mock_text_replace.call_count == len(annotation_list))
281+ self.assertTrue(replaced)
282+
283+ mock_text_replace.side_effect = [False, False]
284+ replaced = annotator.set_landscape_annotations(annotation_list,
285+ "x")
286+ self.assertTrue(not replaced)
287+
288+ @patch("landscape_annotator.get_existing_annotations")
289+ @patch("landscape_annotator.get_deployed_juju_app_list")
290+ @patch("landscape_annotator.subprocess.Popen")
291+ @patch("landscape_annotator.set_landscape_annotations")
292+ def test_configure_annotations(self, mock_set_annotations,
293+ mock_restart_client_process,
294+ mock_get_deployed_juju_app_list,
295+ mock_get_existing_annotations):
296+ """Test to see if changes to annotations are handled properly."""
297+ # test adding new application
298+ mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server"]
299+ mock_get_existing_annotations.return_value = []
300+ mock_set_annotations.return_value = True
301+ annotator.configure_annotations()
302+ mock_set_annotations.assert_any_call(["rabbitmq-server"],
303+ "deployed=true")
304+ mock_restart_client_process.assert_called_once()
305+
306+ # test no change in annotations
307+ mock_set_annotations.reset_mock()
308+ mock_restart_client_process.reset_mock()
309+ mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server"]
310+ mock_get_existing_annotations.return_value = ["rabbitmq-server"]
311+ mock_set_annotations.return_value = False
312+ annotator.configure_annotations()
313+ mock_restart_client_process.assert_not_called()
314+
315+ # test additional new application
316+ mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server",
317+ "filebeats"]
318+ mock_get_existing_annotations.return_value = ["rabbitmq-server"]
319+ mock_set_annotations.return_value = True
320+ annotator.configure_annotations()
321+ mock_set_annotations.assert_any_call(["filebeats"], "deployed=true")
322+ mock_restart_client_process.assert_called_once()
323+
324+ # test deletion of application
325+ mock_set_annotations.reset_mock()
326+ mock_restart_client_process.reset_mock()
327+ mock_get_deployed_juju_app_list.return_value = ["rabbitmq-server"]
328+ mock_get_existing_annotations.return_value = ["rabbitmq-server",
329+ "filebeats"]
330+ mock_set_annotations.return_value = True
331+ annotator.configure_annotations()
332+ mock_set_annotations.assert_any_call(["filebeats"],
333+ "deployed=false",
334+ from_value="deployed=true")
335+ mock_restart_client_process.assert_called_once()
336+
337+
338+if __name__ == '__main__':
339+ unittest.main()

Subscribers

People subscribed via source and target branches

to all changes: