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
1diff --git a/src/files/dashboards_backup b/src/files/dashboards_backup
2index 32d837c..30299a2 100755
3--- a/src/files/dashboards_backup
4+++ b/src/files/dashboards_backup
5@@ -18,7 +18,7 @@ def get_backup_filename(directory, org_name, uri):
6
7
8 def main(args):
9- base_url = 'http://localhost:3000/api/'
10+ base_url = 'http://localhost:{port}/api/'.format(port=args.port)
11 for key in args.api_keys:
12 headers = {'Authorization': 'Bearer {}'.format(key)}
13 org_name = requests.get(base_url+'org', headers=headers).json()['name']
14@@ -37,6 +37,7 @@ if __name__ == '__main__':
15 parser = argparse.ArgumentParser(description='Back up grafana dashboards to disk')
16 parser.add_argument('-d', '--directory', help='Directory where to store backups',
17 default='/srv/backups')
18+ parser.add_argument('-p', '--port', help='Port to access grafana API', default='3000')
19 parser.add_argument('api_keys', help='List of API keys to use for backups', nargs='+')
20 args = parser.parse_args()
21 main(args)
22diff --git a/src/reactive/grafana.py b/src/reactive/grafana.py
23index ff26b3e..31e0881 100644
24--- a/src/reactive/grafana.py
25+++ b/src/reactive/grafana.py
26@@ -321,8 +321,10 @@ def config_changed():
27 SNAP_NAME, channel=config["snap_channel"], force_dangerous=False,
28 )
29 remove_state("grafana.configured")
30- if config.changed("dashboards_backup_schedule") or config.changed(
31- "dashboards_backup_dir"
32+ if (
33+ config.changed("dashboards_backup_schedule")
34+ or config.changed("dashboards_backup_dir")
35+ or config.changed("port")
36 ):
37 remove_state("grafana.backup.configured")
38 if config.changed("admin_password") or config.changed("nagios_context"):
39@@ -481,6 +483,7 @@ def setup_backup_shedule():
40 settings = {
41 "schedule": config.get("dashboards_backup_schedule"),
42 "directory": config.get("dashboards_backup_dir"),
43+ "port": config.get("port"),
44 "backup_keys": " ".join(add_backup_api_keys()),
45 }
46 render(
47diff --git a/src/templates/juju-dashboards-backup.j2 b/src/templates/juju-dashboards-backup.j2
48index b3c546b..00f0737 100644
49--- a/src/templates/juju-dashboards-backup.j2
50+++ b/src/templates/juju-dashboards-backup.j2
51@@ -1,5 +1,5 @@
52 #
53 # Please do not edit. Juju will overwrite it.
54 #
55-{{ schedule }} root /usr/local/bin/dashboards_backup -d {{ directory }} {{ backup_keys }}
56+{{ schedule }} root /usr/local/bin/dashboards_backup -d {{ directory }} -p {{ port }} {{ backup_keys }}
57
58diff --git a/src/tests/functional/tests/test_grafana.py b/src/tests/functional/tests/test_grafana.py
59index f848077..4bc3585 100644
60--- a/src/tests/functional/tests/test_grafana.py
61+++ b/src/tests/functional/tests/test_grafana.py
62@@ -12,6 +12,7 @@ import zaza.model as model
63 TEST_TIMEOUT = 600
64 DEFAULT_API_PORT = "3000"
65 DEFAULT_API_URL = "/"
66+DEFAULT_BACKUP_DIRECTORY = "/srv/backups"
67
68
69 class BaseGrafanaTest(unittest.TestCase):
70@@ -73,6 +74,37 @@ class BaseGrafanaTest(unittest.TestCase):
71 cmd = ["sed", "-i", "-e", expr, file]
72 model.run_on_unit(unit, " ".join(cmd))
73
74+ def verify_iterative_backups(self):
75+ """Wait for new backups to appear to ensure they work"""
76+ time.sleep(65)
77+ backupdir = model.run_on_unit(
78+ self.lead_unit_name,
79+ "ls {directory}".format(
80+ directory=DEFAULT_BACKUP_DIRECTORY
81+ )
82+ )
83+ self.assertTrue("No such file or directory" not in backupdir['Stderr'])
84+ backups_start = model.run_on_unit(
85+ self.lead_unit_name,
86+ "ls {project}/{backup}".format(
87+ project=DEFAULT_BACKUP_DIRECTORY,
88+ backup=backupdir['Stdout'].strip()
89+ )
90+ )
91+ self.assertTrue(".json" in backups_start['Stdout'])
92+ number_of_backups_start = backups_start['Stdout'].count('.json')
93+ self.assertTrue(number_of_backups_start >= 1)
94+ time.sleep(65)
95+ backups_end = model.run_on_unit(
96+ self.lead_unit_name,
97+ "ls {project}/{backup}".format(
98+ project=DEFAULT_BACKUP_DIRECTORY,
99+ backup=backupdir['Stdout'].strip()
100+ )
101+ )
102+ number_of_backups_end = backups_end['Stdout'].count('.json')
103+ self.assertTrue(number_of_backups_end > number_of_backups_start)
104+
105
106 class CharmOperationTest(BaseGrafanaTest):
107 """Verify operations."""
108@@ -187,6 +219,37 @@ class CharmOperationTest(BaseGrafanaTest):
109 self.assertTrue(req.status_code == 200)
110 self.assertTrue("name" in req.json())
111
112+ def test_13_grafana_dashboard_backup(self):
113+ """Run dashboard-backup items"""
114+ model.set_application_config(
115+ self.application_name,
116+ {"dashboards_backup_schedule": "* * * * *"},
117+ )
118+ model.block_until_all_units_idle()
119+ crontab = model.run_on_unit(
120+ self.lead_unit_name,
121+ "cat /etc/cron.d/juju-dashboards-backup",
122+ )
123+ self.assertTrue(DEFAULT_BACKUP_DIRECTORY in crontab['Stdout'])
124+ self.assertTrue(DEFAULT_API_PORT in crontab['Stdout'])
125+ self.verify_iterative_backups()
126+
127+ def test_14_grafana_dashboard_backup_port_change(self):
128+ """Run dashboard backup and test a port change"""
129+ port = "4321"
130+ model.set_application_config(
131+ self.application_name, {"dashboards_backup_schedule": "* * * * *"}
132+ )
133+ model.block_until_all_units_idle()
134+ model.set_application_config(
135+ self.application_name, {"port": port}
136+ )
137+ model.block_until_all_units_idle()
138+ crontab = model.run_on_unit(
139+ self.lead_unit_name, "cat /etc/cron.d/juju-dashboards-backup"
140+ )
141+ self.assertTrue(port in crontab['Stdout'])
142+ self.verify_iterative_backups()
143
144 class SnappedGrafanaTest(BaseGrafanaTest):
145 """Verify Grafana installed as a snap."""

Subscribers

People subscribed via source and target branches