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

[Matthew Ruffell]

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

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

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

[Chris Johnston (cjohnston)]

review: Approve

[Chris Johnston (cjohnston)]

Drew has requested a functional test be added.

review: Needs Fixing

[Drew Freiberger (afreiberger)]

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.

[Paul Goins (vultaire)]

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

review: Needs Fixing

[Matthew Ruffell (mruffell)]

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.

[Drew Freiberger (afreiberger)]

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

[Matthew Ruffell (mruffell)]

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

[Drew Freiberger (afreiberger)]

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

[Chris Johnston (cjohnston)]

review: Approve

[Matthew Ruffell (mruffell)]

Hi Paul, can you please review?

[James Hebden (ec0)]

+1

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 534c1f1c9e05e02e66cd55087d29a360c4f6b83c

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):
 
 
 def main(args):
-    base_url = 'http://localhost:3000/api/'
+    base_url = 'http://localhost:{port}/api/'.format(port=args.port)
     for key in args.api_keys:
         headers = {'Authorization': 'Bearer {}'.format(key)}
         org_name = requests.get(base_url+'org', headers=headers).json()['name']
@@ -37,6 +37,7 @@ if __name__ == '__main__':
     parser = argparse.ArgumentParser(description='Back up grafana dashboards to disk')
     parser.add_argument('-d', '--directory', help='Directory where to store backups',
                         default='/srv/backups')
+    parser.add_argument('-p', '--port', help='Port to access grafana API', default='3000')
     parser.add_argument('api_keys', help='List of API keys to use for backups', nargs='+')
     args = parser.parse_args()
     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():
                 SNAP_NAME, channel=config["snap_channel"], force_dangerous=False,
             )
     remove_state("grafana.configured")
-    if config.changed("dashboards_backup_schedule") or config.changed(
-        "dashboards_backup_dir"
+    if (
+        config.changed("dashboards_backup_schedule")
+        or config.changed("dashboards_backup_dir")
+        or config.changed("port")
     ):
         remove_state("grafana.backup.configured")
     if config.changed("admin_password") or config.changed("nagios_context"):
@@ -481,6 +483,7 @@ def setup_backup_shedule():
         settings = {
             "schedule": config.get("dashboards_backup_schedule"),
             "directory": config.get("dashboards_backup_dir"),
+            "port": config.get("port"),
             "backup_keys": " ".join(add_backup_api_keys()),
         }
         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 @@
 #
 # Please do not edit. Juju will overwrite it.
 #
-{{ schedule }} root /usr/local/bin/dashboards_backup -d {{ directory }} {{ backup_keys }}
+{{ schedule }} root /usr/local/bin/dashboards_backup -d {{ directory }} -p {{ port }} {{ backup_keys }}
 
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
 TEST_TIMEOUT = 600
 DEFAULT_API_PORT = "3000"
 DEFAULT_API_URL = "/"
+DEFAULT_BACKUP_DIRECTORY = "/srv/backups"
 
 
 class BaseGrafanaTest(unittest.TestCase):
@@ -73,6 +74,37 @@ class BaseGrafanaTest(unittest.TestCase):
         cmd = ["sed", "-i", "-e", expr, file]
         model.run_on_unit(unit, " ".join(cmd))
 
+    def verify_iterative_backups(self):
+        """Wait for new backups to appear to ensure they work"""
+        time.sleep(65)
+        backupdir = model.run_on_unit(
+            self.lead_unit_name,
+            "ls {directory}".format(
+                directory=DEFAULT_BACKUP_DIRECTORY
+            )
+        )
+        self.assertTrue("No such file or directory" not in backupdir['Stderr'])
+        backups_start = model.run_on_unit(
+            self.lead_unit_name,
+            "ls {project}/{backup}".format(
+                project=DEFAULT_BACKUP_DIRECTORY,
+                backup=backupdir['Stdout'].strip()
+            )
+        )
+        self.assertTrue(".json" in backups_start['Stdout'])
+        number_of_backups_start = backups_start['Stdout'].count('.json')
+        self.assertTrue(number_of_backups_start >= 1)
+        time.sleep(65)
+        backups_end = model.run_on_unit(
+            self.lead_unit_name,
+            "ls {project}/{backup}".format(
+                project=DEFAULT_BACKUP_DIRECTORY,
+                backup=backupdir['Stdout'].strip()
+            )
+        )
+        number_of_backups_end = backups_end['Stdout'].count('.json')
+        self.assertTrue(number_of_backups_end > number_of_backups_start)
+
 
 class CharmOperationTest(BaseGrafanaTest):
     """Verify operations."""
@@ -187,6 +219,37 @@ class CharmOperationTest(BaseGrafanaTest):
         self.assertTrue(req.status_code == 200)
         self.assertTrue("name" in req.json())
 
+    def test_13_grafana_dashboard_backup(self):
+        """Run dashboard-backup items"""
+        model.set_application_config(
+            self.application_name,
+            {"dashboards_backup_schedule": "* * * * *"},
+        )
+        model.block_until_all_units_idle()
+        crontab = model.run_on_unit(
+            self.lead_unit_name,
+            "cat /etc/cron.d/juju-dashboards-backup",
+        )
+        self.assertTrue(DEFAULT_BACKUP_DIRECTORY in crontab['Stdout'])
+        self.assertTrue(DEFAULT_API_PORT in crontab['Stdout'])
+        self.verify_iterative_backups()
+
+    def test_14_grafana_dashboard_backup_port_change(self):
+        """Run dashboard backup and test a port change"""
+        port = "4321"
+        model.set_application_config(
+            self.application_name, {"dashboards_backup_schedule": "* * * * *"}
+        )
+        model.block_until_all_units_idle()
+        model.set_application_config(
+            self.application_name, {"port": port}
+        )
+        model.block_until_all_units_idle()
+        crontab = model.run_on_unit(
+            self.lead_unit_name, "cat /etc/cron.d/juju-dashboards-backup"
+        )
+        self.assertTrue(port in crontab['Stdout'])
+        self.verify_iterative_backups()
 
 class SnappedGrafanaTest(BaseGrafanaTest):
     """Verify Grafana installed as a snap."""