Merge ~mruffell/charm-grafana:lp1894011 into charm-grafana:master

Proposed by Matthew Ruffell
Status: Merged
Approved by: James Hebden
Approved revision: 5f1843b3f25e7ed7b4a117b3ef942a124fe5f137
Merged at revision: 534c1f1c9e05e02e66cd55087d29a360c4f6b83c
Proposed branch: ~mruffell/charm-grafana:lp1894011
Merge into: charm-grafana:master
Diff against target: 145 lines (+71/-4)
4 files modified
src/files/dashboards_backup (+2/-1)
src/reactive/grafana.py (+5/-2)
src/templates/juju-dashboards-backup.j2 (+1/-1)
src/tests/functional/tests/test_grafana.py (+63/-0)
Reviewer Review Type Date Requested Status
James Hebden (community) Approve
Chris Johnston (community) Approve
Drew Freiberger Approve
Paul Goins Needs Fixing
Review via email: mp+390215@code.launchpad.net

Commit message

Enable support for custom ports for dashboard backup

Currently dashboard backup uses a hardcoded base_url of localhost:3000,
which breaks the backup script if the user changes the port to anything
else.

Add some logic to fetch the port config and pass it into the dashboard
backup script. Also make sure dashboard backups are kept in sync with
any port changes.

LP: #1894011

To post a comment you must log in.
Revision history for this message
Canonical IS Mergebot (canonical-is-mergebot) wrote :

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

Revision history for this message
Chris Johnston (cjohnston) :
review: Approve
Revision history for this message
Chris Johnston (cjohnston) wrote :

Drew has requested a functional test be added.

review: Needs Fixing
Revision history for this message
Drew Freiberger (afreiberger) wrote :

One inline (cosmetic) comment, but would mainly suggest that a functional test be added to ensure this is functioning and doesn't suffer a regression.

Revision history for this message
Paul Goins (vultaire) wrote :

LGTM but I agree with Drew. I'm +1 if we get a functional test added.

review: Needs Fixing
Revision history for this message
Matthew Ruffell (mruffell) wrote :

Okay, I fixed up the cosmetic change in the base_url string, and it is now a format string, as requested.

I added two functional tests. The first ensures that dashboard backups function with their default settings, and the second one tests a port change, and ensures that backups keep working as desired when the API endpoint has changed ports.

Both tests share the verify_iterative_backups() function, which finds the backup directory, and checks to see if new backups are being added over the duration of a minute.

Hopefully this should be ready to roll.

Revision history for this message
Drew Freiberger (afreiberger) wrote :

Thank you for the updates. Unfortunately, using sleep within functional tests is an anti-pattern.

Please see the documentation on available block_until methods that can check for file contents, missing files, and ready files.

You can then set timeouts on these block_until calls to limit max time that the function will block for. This may also greatly simplify your code, as you could just block until the file has the contents you're looking for after executing the backup.

https://zaza.readthedocs.io/en/latest/model-api.html#zaza.model.async_block_until_file_has_contents

review: Needs Fixing
Revision history for this message
Matthew Ruffell (mruffell) wrote :
Download full text (5.5 KiB)

I understand why usage of time.sleep() can lead to flaky tests, but in this specific example, I think it is a necessary evil, and it actually simplifies my code instead of making it more complicated.

Let me explain.

When we enable dashboard backup by setting

juju config grafana dashboards_backup_schedule="* * * * *"

This creates a cronjob, in /etc/cron.d/juju-dashboards-backup, which runs once every 60 seconds, due to the timer being "* * * * *".

At this point in time, the backup directory, /srv/backups doesn't exist, and we have no idea what the project / organisation name is, so we cannot watch the backup directory /srv/backups/<project name>. Although so far I have only seen "Main_Org_" as the project name.

When the cronjob runs, it backs up the dashboard configuration to /srv/backups/<project name>/<dashboard name>.json.<timestamp>

An few examples are:

/srv/backups/Main_Org_/juju-prometheus-libvirt-exporter-functionaltestmarker-metrics.json.20200915_233401
/srv/backups/Main_Org_/juju-prometheus-libvirt-exporter-functionaltestmarker-metrics.json.20200915_233501
/srv/backups/Main_Org_/juju-prometheus-libvirt-exporter-functionaltestmarker-metrics.json.20200915_233602
/srv/backups/Main_Org_/juju-prometheus-libvirt-exporter-libvirt-metrics.json.20200915_233401
/srv/backups/Main_Org_/juju-prometheus-libvirt-exporter-libvirt-metrics.json.20200915_233501
/srv/backups/Main_Org_/juju-prometheus-libvirt-exporter-libvirt-metrics.json.20200915_233602

Now, filenames are dependant on dashboard data sources, so the charms that grafana have a relation to cannot change.

There is also a small problem of the timestamp at the end, since it has seconds present.

I left the backup setting enabled for ten minutes or so:

https://paste.ubuntu.com/p/RVmDCqTPGx/

We can see the seconds vary between 01 and 02, depending on machine load.

Now, If I used model.block_until_file_has_contents(), I would have to be able to predict the exact filename, which is impossible, as some files will have 00 seconds, 01, 02, 03 or 04 or more seconds.

I would also have to wait for a file one minute in the future, since if the cronjob did not fire in the current minute, no files would exist, so I would need to wait for the next one.

This would turn my test code for verify_iterative_backups() into something like:

from datetime import datetime, timedelta

DEFAULT_BACKUP_ORG = "Main_Org_"
DEFAULT_FUNCTEST_PROJECT = "juju-prometheus-libvirt-exporter"
DEFAULT_FUNCTEST_DASHBOARD = "functionaltestmarker-metrics"

def verify_iterative_backups(self):
    """Wait for new backups to appear to ensure they work"""
    current_time = datetime.utcnow()
    time_in_1_minutes = current_time + timedelta(minutes = 1)
    time_in_2_minutes = current_time + timedelta(minutes = 2)

    file_name_1_minutes = "{project}-{dashboard}.json.{date}_{time}".format(
        project=DEFAULT_FUNCTEST_PROJECT,
        dashboard=DEFAULT_FUNCTEST_DASHBOARD,
        date=time_in_1_minutes.strftime('%Y%m%d'),
        time=time_in_1_minutes.strftime('%H%M00')
        )
    )
    file_name_2_minutes = "{project}-{dashboard}.json.{date}_{time}".format(
        project=DEFAULT_FUNCTEST_PROJECT,
        dashb...

Read more...

Revision history for this message
Drew Freiberger (afreiberger) wrote :

Your comment is well stated and given that the intention is to wait for a 60 second cron tick, I'm +1 for this method with the sleep.

Thank you for explaining that so well.

review: Approve
Revision history for this message
Chris Johnston (cjohnston) :
review: Approve
Revision history for this message
Matthew Ruffell (mruffell) wrote :

Hi Paul, can you please review?

Revision history for this message
James Hebden (ec0) wrote :

+1

review: Approve
Revision history for this message
Canonical IS Mergebot (canonical-is-mergebot) wrote :

Change successfully merged at revision 534c1f1c9e05e02e66cd55087d29a360c4f6b83c

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/src/files/dashboards_backup b/src/files/dashboards_backup
index 32d837c..30299a2 100755
--- a/src/files/dashboards_backup
+++ b/src/files/dashboards_backup
@@ -18,7 +18,7 @@ def get_backup_filename(directory, org_name, uri):
1818
1919
20def main(args):20def main(args):
21 base_url = 'http://localhost:3000/api/'21 base_url = 'http://localhost:{port}/api/'.format(port=args.port)
22 for key in args.api_keys:22 for key in args.api_keys:
23 headers = {'Authorization': 'Bearer {}'.format(key)}23 headers = {'Authorization': 'Bearer {}'.format(key)}
24 org_name = requests.get(base_url+'org', headers=headers).json()['name']24 org_name = requests.get(base_url+'org', headers=headers).json()['name']
@@ -37,6 +37,7 @@ if __name__ == '__main__':
37 parser = argparse.ArgumentParser(description='Back up grafana dashboards to disk')37 parser = argparse.ArgumentParser(description='Back up grafana dashboards to disk')
38 parser.add_argument('-d', '--directory', help='Directory where to store backups',38 parser.add_argument('-d', '--directory', help='Directory where to store backups',
39 default='/srv/backups')39 default='/srv/backups')
40 parser.add_argument('-p', '--port', help='Port to access grafana API', default='3000')
40 parser.add_argument('api_keys', help='List of API keys to use for backups', nargs='+')41 parser.add_argument('api_keys', help='List of API keys to use for backups', nargs='+')
41 args = parser.parse_args()42 args = parser.parse_args()
42 main(args)43 main(args)
diff --git a/src/reactive/grafana.py b/src/reactive/grafana.py
index ff26b3e..31e0881 100644
--- a/src/reactive/grafana.py
+++ b/src/reactive/grafana.py
@@ -321,8 +321,10 @@ def config_changed():
321 SNAP_NAME, channel=config["snap_channel"], force_dangerous=False,321 SNAP_NAME, channel=config["snap_channel"], force_dangerous=False,
322 )322 )
323 remove_state("grafana.configured")323 remove_state("grafana.configured")
324 if config.changed("dashboards_backup_schedule") or config.changed(324 if (
325 "dashboards_backup_dir"325 config.changed("dashboards_backup_schedule")
326 or config.changed("dashboards_backup_dir")
327 or config.changed("port")
326 ):328 ):
327 remove_state("grafana.backup.configured")329 remove_state("grafana.backup.configured")
328 if config.changed("admin_password") or config.changed("nagios_context"):330 if config.changed("admin_password") or config.changed("nagios_context"):
@@ -481,6 +483,7 @@ def setup_backup_shedule():
481 settings = {483 settings = {
482 "schedule": config.get("dashboards_backup_schedule"),484 "schedule": config.get("dashboards_backup_schedule"),
483 "directory": config.get("dashboards_backup_dir"),485 "directory": config.get("dashboards_backup_dir"),
486 "port": config.get("port"),
484 "backup_keys": " ".join(add_backup_api_keys()),487 "backup_keys": " ".join(add_backup_api_keys()),
485 }488 }
486 render(489 render(
diff --git a/src/templates/juju-dashboards-backup.j2 b/src/templates/juju-dashboards-backup.j2
index b3c546b..00f0737 100644
--- a/src/templates/juju-dashboards-backup.j2
+++ b/src/templates/juju-dashboards-backup.j2
@@ -1,5 +1,5 @@
1#1#
2# Please do not edit. Juju will overwrite it.2# Please do not edit. Juju will overwrite it.
3#3#
4{{ schedule }} root /usr/local/bin/dashboards_backup -d {{ directory }} {{ backup_keys }}4{{ schedule }} root /usr/local/bin/dashboards_backup -d {{ directory }} -p {{ port }} {{ backup_keys }}
55
diff --git a/src/tests/functional/tests/test_grafana.py b/src/tests/functional/tests/test_grafana.py
index f848077..4bc3585 100644
--- a/src/tests/functional/tests/test_grafana.py
+++ b/src/tests/functional/tests/test_grafana.py
@@ -12,6 +12,7 @@ import zaza.model as model
12TEST_TIMEOUT = 60012TEST_TIMEOUT = 600
13DEFAULT_API_PORT = "3000"13DEFAULT_API_PORT = "3000"
14DEFAULT_API_URL = "/"14DEFAULT_API_URL = "/"
15DEFAULT_BACKUP_DIRECTORY = "/srv/backups"
1516
1617
17class BaseGrafanaTest(unittest.TestCase):18class BaseGrafanaTest(unittest.TestCase):
@@ -73,6 +74,37 @@ class BaseGrafanaTest(unittest.TestCase):
73 cmd = ["sed", "-i", "-e", expr, file]74 cmd = ["sed", "-i", "-e", expr, file]
74 model.run_on_unit(unit, " ".join(cmd))75 model.run_on_unit(unit, " ".join(cmd))
7576
77 def verify_iterative_backups(self):
78 """Wait for new backups to appear to ensure they work"""
79 time.sleep(65)
80 backupdir = model.run_on_unit(
81 self.lead_unit_name,
82 "ls {directory}".format(
83 directory=DEFAULT_BACKUP_DIRECTORY
84 )
85 )
86 self.assertTrue("No such file or directory" not in backupdir['Stderr'])
87 backups_start = model.run_on_unit(
88 self.lead_unit_name,
89 "ls {project}/{backup}".format(
90 project=DEFAULT_BACKUP_DIRECTORY,
91 backup=backupdir['Stdout'].strip()
92 )
93 )
94 self.assertTrue(".json" in backups_start['Stdout'])
95 number_of_backups_start = backups_start['Stdout'].count('.json')
96 self.assertTrue(number_of_backups_start >= 1)
97 time.sleep(65)
98 backups_end = model.run_on_unit(
99 self.lead_unit_name,
100 "ls {project}/{backup}".format(
101 project=DEFAULT_BACKUP_DIRECTORY,
102 backup=backupdir['Stdout'].strip()
103 )
104 )
105 number_of_backups_end = backups_end['Stdout'].count('.json')
106 self.assertTrue(number_of_backups_end > number_of_backups_start)
107
76108
77class CharmOperationTest(BaseGrafanaTest):109class CharmOperationTest(BaseGrafanaTest):
78 """Verify operations."""110 """Verify operations."""
@@ -187,6 +219,37 @@ class CharmOperationTest(BaseGrafanaTest):
187 self.assertTrue(req.status_code == 200)219 self.assertTrue(req.status_code == 200)
188 self.assertTrue("name" in req.json())220 self.assertTrue("name" in req.json())
189221
222 def test_13_grafana_dashboard_backup(self):
223 """Run dashboard-backup items"""
224 model.set_application_config(
225 self.application_name,
226 {"dashboards_backup_schedule": "* * * * *"},
227 )
228 model.block_until_all_units_idle()
229 crontab = model.run_on_unit(
230 self.lead_unit_name,
231 "cat /etc/cron.d/juju-dashboards-backup",
232 )
233 self.assertTrue(DEFAULT_BACKUP_DIRECTORY in crontab['Stdout'])
234 self.assertTrue(DEFAULT_API_PORT in crontab['Stdout'])
235 self.verify_iterative_backups()
236
237 def test_14_grafana_dashboard_backup_port_change(self):
238 """Run dashboard backup and test a port change"""
239 port = "4321"
240 model.set_application_config(
241 self.application_name, {"dashboards_backup_schedule": "* * * * *"}
242 )
243 model.block_until_all_units_idle()
244 model.set_application_config(
245 self.application_name, {"port": port}
246 )
247 model.block_until_all_units_idle()
248 crontab = model.run_on_unit(
249 self.lead_unit_name, "cat /etc/cron.d/juju-dashboards-backup"
250 )
251 self.assertTrue(port in crontab['Stdout'])
252 self.verify_iterative_backups()
190253
191class SnappedGrafanaTest(BaseGrafanaTest):254class SnappedGrafanaTest(BaseGrafanaTest):
192 """Verify Grafana installed as a snap."""255 """Verify Grafana installed as a snap."""

Subscribers

People subscribed via source and target branches