Grafana Charm

Merge ~peter-sabaini/charm-grafana:bug/1875691-dashboard-upgrade into charm-grafana:master

Proposed by Peter Sabaini on 2020-06-30

Status:

Merged

Approved by:

Alvaro Uria on 2020-07-24

Approved revision:

af7cea436183383c6a7af66a5c352f5e5ed8f1d8

Merged at revision:

963e29018ea7ec13e17b1bc69b6f8d697a8f4ed0

Proposed branch:

~peter-sabaini/charm-grafana:bug/1875691-dashboard-upgrade

Merge into:

charm-grafana:master

Diff against target:

256 lines (+131/-34)

6 files modified

reactive/grafana.py (+40/-15)
tests/functional/tests/bundles/base.yaml (+9/-0)
tests/functional/tests/bundles/overlays/focal-snap.yaml.j2 (+2/-0)
tests/functional/tests/bundles/overlays/focal.yaml.j2 (+2/-0)
tests/functional/tests/test_grafana.py (+76/-19)
tests/functional/tests/tests.yaml (+2/-0)

Medium

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Alvaro Uria (community)	2020-06-30	Approve on 2020-07-24
Giuseppe Petralia		Approve on 2020-07-07
Zachary Zehring (community)		Needs Fixing on 2020-07-01
BootStack Reviewers	2020-07-23	Pending
Review via email: mp+386564@code.launchpad.net

Commit message

Dashboard upgrade fix

Revision history for this message

🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote on 2020-06-30:

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

Revision history for this message

Peter Sabaini (peter-sabaini) wrote on 2020-06-30:

Note: functests require an update to the prometheus-libvirt-exporter from

https://code.launchpad.net/~peter-sabaini/charm-prometheus-libvirt-exporter/+git/charm-prometheus-libvirt-exporter/+merge/386434

Revision history for this message

James Troup (elmo) wrote on 2020-06-30:

Some drive by commentary.

Revision history for this message

Zachary Zehring (zzehring) wrote on 2020-07-01:

Added comments inline.

review: Needs Fixing

Revision history for this message

Peter Sabaini (peter-sabaini) wrote on 2020-07-01:

Download full text (10.6 KiB)

Thanks for the review, I've added some comments and will push some changes shortly

On 01.07.20 17:52, Zachary Zehring wrote:
> Review: Needs Fixing
>
> Added comments inline.
>
> Diff comments:
>
>> diff --git a/reactive/grafana.py b/reactive/grafana.py
>> index c64743a..1b78d77 100644
>> --- a/reactive/grafana.py
>> +++ b/reactive/grafana.py
>> @@ -1006,22 +1006,59 @@ def hpwgen(passwd, salt):
>> return hpasswd
>>
>>
>> -@when("grafana.started", "endpoint.dashboards.requests")
>> +@when("grafana.started", "endpoint.dashboards.has_requests")
>> @when_not("grafana.change-block")
>> def import_dashboards(dashboards):
>> - for request in dashboards.new_requests:
>> + kv = unitdata.kv()
>> + dashboard_digests = kv.get("dash_digests", {})
>> +
>> + def dash_import(name, dash):
>
> Is there any reason that this is a nested function? I see it's really only to avoid passing in dashboard_digests. I'd prefer to move this out into it's own private function, as it's clunky to read here.

Reasoning behind making it nested is to hide it from outside use, as it's not meant to be used directly.

>> try:
>> - dash = request.dashboard
>> - if isinstance(request.dashboard, str):
>> - dash = json.loads(request.dashboard)
>> - success, reason = import_dashboard(dash, request.name)
>> + if isinstance(dash, str):
>> + # Safety: per the dashboard interface the dash should already
>
> I personally really dislike comments that describe intention that could be communicated through code. This check and possible assignment could be abstracted into a private function like 'ensure_dash_is_dict' which would be readable, less code in top function, and you could remove this block comment. Comments are often not updated when code changes, leading to misleading documentation. You could put any pertinent documentation into the docstring of the resulting function, which should be true describing the intention of the function.

I'm ok with abstracting this into a sep. function, but would hate to lose the comment as this is meant to explain the reason why this is being done.

>> + # be de-serialized json, but older dashboard providers used
>> + # to send a json string, so attempt to deserialize if we find
>> + # a string
>> + dash = json.loads(dash)
>> + success, reason = import_dashboard(dash, name)
>> + dashboard_digests[name] = digest
>> except json.decoder.JSONDecodeError as error:
>> success, reason = False, str(error)
>> -
>> hookenv.log(
>> - "import_dashboards via grafana-dashboards: {} [{}]".format(
>> - success, reason
>> - ),
>> - hookenv.DEBUG,
>> + "import_dashboards {}, digest {}: {} [{}]".format(
>> + name, digest, success, reason
>
> Since success is a boolean, this log message would be a bit awkward (like what exactly does true/false mean here). I'd adjust somewhat to be more clear in what just happened.

Fair

>> + )
>> )
>> + return succes...

Thanks for the review, I've added some comments and will push some changes shortly

On 01.07.20 17:52, Zachary Zehring wrote:
> Review: Needs Fixing
> 
> Added comments inline. 
> 
> Diff comments:
> 
>> diff --git a/reactive/grafana.py b/reactive/grafana.py
>> index c64743a..1b78d77 100644
>> --- a/reactive/grafana.py
>> +++ b/reactive/grafana.py
>> @@ -1006,22 +1006,59 @@ def hpwgen(passwd, salt):
>>      return hpasswd
>>  
>>  
>> -@when("grafana.started", "endpoint.dashboards.requests")
>> +@when("grafana.started", "endpoint.dashboards.has_requests")
>>  @when_not("grafana.change-block")
>>  def import_dashboards(dashboards):
>> -    for request in dashboards.new_requests:
>> +    kv = unitdata.kv()
>> +    dashboard_digests = kv.get("dash_digests", {})
>> +
>> +    def dash_import(name, dash):
> 
> Is there any reason that this is a nested function? I see it's really only to avoid passing in dashboard_digests. I'd prefer to move this out into it's own private function, as it's clunky to read here.

Reasoning behind making it nested is to hide it from outside use, as it's not meant to be used directly.

>>          try:
>> -            dash = request.dashboard
>> -            if isinstance(request.dashboard, str):
>> -                dash = json.loads(request.dashboard)
>> -            success, reason = import_dashboard(dash, request.name)
>> +            if isinstance(dash, str):
>> +                # Safety: per the dashboard interface the dash should already
> 
> I personally really dislike comments that describe intention that could be communicated through code. This check and possible assignment could be abstracted into a private function like 'ensure_dash_is_dict' which would be readable, less code in top function, and you could remove this block comment. Comments are often not updated when code changes, leading to misleading documentation. You could put any pertinent documentation into the docstring of the resulting function, which should be true describing the intention of the function.

I'm ok with abstracting this into a sep. function, but would hate to lose the comment as this is meant to explain the reason why this is being done.

>> +                # be de-serialized json, but older dashboard providers used
>> +                # to send a json string, so attempt to deserialize if we find
>> +                # a string
>> +                dash = json.loads(dash)
>> +            success, reason = import_dashboard(dash, name)
>> +            dashboard_digests[name] = digest
>>          except json.decoder.JSONDecodeError as error:
>>              success, reason = False, str(error)
>> -
>>          hookenv.log(
>> -            "import_dashboards via grafana-dashboards: {} [{}]".format(
>> -                success, reason
>> -            ),
>> -            hookenv.DEBUG,
>> +            "import_dashboards {}, digest {}: {} [{}]".format(
>> +                name, digest, success, reason
> 
> Since success is a boolean, this log message would be a bit awkward (like what exactly does true/false mean here). I'd adjust somewhat to be more clear in what just happened.

Fair

>> +            )
>>          )
>> +        return success, reason
>> +
>> +    # Check for updates
>> +    for existing_req in dashboards.all_requests:
>> +        dash = existing_req.dashboard
>> +        if isinstance(dash, str):
>> +            # Similar to the above: attempt to deserialize for older providers
> 
> Definitely need to abstract this since you're doing it twice.

>> +            dash = json.loads(dash)
>> +        digest = dash.get("digest")
>> +        if not digest:
> 
> Would it be possible to add all the possible paths for a request here, in one for loop? You could add an if check for digest in the request and an elif check for it's existence in dashboards.new_requests, then process accordingly. That way you can just loop through once and remove the hacky 'continue' logic.

I've handled updates and new requests separately as I wanted to make it very clear that new requests are going to be imported no matter what, independent of the existence of a digest or not, so as not to break the existing behaviour. I don't think 'continue' is particularly hacky, but I know that opinions differ here.

>> +            # Backward compat: if we don't see a digest in the payload, it's
>> +            # coming from an older provider, skip import of an existing
>> +            # request
>> +            continue
>> +        name = existing_req.name
>> +        hookenv.log(
>> +            "import_dashboards: found existing dash for {}, "
>> +            "with digest {}".format(name, digest)
>> +        )
>> +        if dashboard_digests.get(name) != digest:
>> +            hookenv.log(
>> +                "import_dashboards: updated digest, old: {}, new: {}. "
>> +                "Importing dashboard {}".format(
>> +                    dashboard_digests.get(name), digest, name
>> +                )
>> +            )
>> +            dash_import(name, dash)
>> +    # Import new requests
>> +    for request in dashboards.new_requests:
>> +        success, reason = dash_import(request.name, request.dashboard)
>>          request.respond(success, reason)
>> +    # Save back digests
> 
> Unnecessary comment. Code is already describing what it's doing.
> 
>> +    kv.set("dash_digests", dashboard_digests)
>> diff --git a/tests/functional/tests/test_grafana.py b/tests/functional/tests/test_grafana.py
>> index 5f452d9..7e34ac8 100644
>> --- a/tests/functional/tests/test_grafana.py
>> +++ b/tests/functional/tests/test_grafana.py
>> @@ -34,6 +34,42 @@ class BaseGrafanaTest(unittest.TestCase):
>>          u = model.get_unit_from_name(unit)
>>          return u.workload_status_message
>>  
>> +    def get_admin_password(self):
> 
> Would some version of caching the password as an instance variable work here? Not sure if this gets instantiated upon every test (i.e. new unit so same password).

Good point, the admin password shouldn't change

>> +        action = model.run_action(
>> +            self.lead_unit_name, "get-admin-password", raise_on_failure=True,
>> +        )
>> +        passwd = action.data["results"]["password"]
>> +        return passwd
>> +
>> +    def get_and_check_dashboards(self, cond):
>> +        """Retrieve dashboard from grafana
>> +
>> +        Check if cond(dashboards) is true, otherwise retry up to TEST_TIMEOUT
>> +        """
>> +        passwd = self.get_admin_password()
>> +        timeout = time.time() + TEST_TIMEOUT
>> +        dashboards = []
>> +        while True:
>> +            req = requests.get(
>> +                "http://{host}:{port}"
>> +                "/api/search?dashboardIds".format(
>> +                    host=self.grafana_ip, port=DEFAULT_API_PORT
>> +                ),
>> +                auth=("admin", passwd),
>> +            )
>> +            self.assertTrue(req.status_code == 200)
>> +            dashboards = json.loads(req.text)
>> +            if time.time() > timeout or cond(dashboards):
>> +                # Break out if the time is up or our condition becomes true
> 
> Comment doesn't seem necessary. It's pretty clear from if condition what it's doing.
> 
>> +                break
>> +            # Wait before retrying
> 
> Also pretty unnecessary comment.
> 
>> +            time.sleep(30)
>> +        return dashboards
>> +
>> +    def remote_sed(self, unit, file, expr):
>> +        cmd = ["sed", "-i", "-e", expr, file]
>> +        model.run_on_unit(unit, " ".join(cmd))
>> +
>>  
>>  class CharmOperationTest(BaseGrafanaTest):
>>      """Verify operations."""
>> @@ -99,25 +135,11 @@ class CharmOperationTest(BaseGrafanaTest):
>>  
>>      def test_10_grafana_imported_dashboards(self):
>>          """Check that Grafana dashboards expected are there"""
>> -        action = model.run_action(
>> -            self.lead_unit_name, "get-admin-password", raise_on_failure=True,
>> -        )
>> -        passwd = action.data["results"]["password"]
>>          # Loop this, it takes a while for the data to arrive and dashboards
> 
> Incorrect comment based on new code as the loop lives in another function entirely. I'd just remove.
>>          # to populate
>> -        timeout = time.time() + TEST_TIMEOUT
>> -        dashboards = []
>> -        while time.time() < timeout and len(dashboards) < 4:
>> -            req = requests.get(
>> -                "http://{host}:{port}"
>> -                "/api/search?dashboardIds".format(
>> -                    host=self.grafana_ip, port=DEFAULT_API_PORT
>> -                ),
>> -                auth=("admin", passwd),
>> -            )
>> -            self.assertTrue(req.status_code == 200)
>> -            dashboards = json.loads(req.text)
>> -            time.sleep(30)
>> +        dashboards = self.get_and_check_dashboards(
>> +            lambda dashes: len(dashes) >= 4
>> +        )
>>          self.assertTrue(len(dashboards) >= 4)
>>          self.assertTrue(
>>              all(
>> @@ -127,6 +149,40 @@ class CharmOperationTest(BaseGrafanaTest):
>>              )
>>          )
>>  
>> +    def test_11_dashboard_upgrade(self):
>> +        """Test upgraded dashboards are imported."""
>> +        exporter = "prometheus-libvirt-exporter/0"
>> +        self.remote_sed(
>> +            exporter,
>> +            "/var/lib/juju/agents/unit-prometheus-libvirt-exporter-0/charm/"
>> +            "files/grafana-dashboards/libvirt.json",
>> +            "s/Libvirt/FunctionalTestMarker/",
>> +        )
>> +        model.run_on_unit(
>> +            exporter, "JUJU_HOOK_NAME=upgrade-charm ./hooks/upgrade-charm"
>> +        )
>> +        model.block_until_all_units_idle()
>> +
>> +        def chk(dashes):
>> +            marker_dashes = [
>> +                d for d in dashes if "FunctionalTestMarker" in d["title"]
>> +            ]
>> +            return len(dashes) >= 4 and len(marker_dashes) == 1
>> +
>> +        dashes = self.get_and_check_dashboards(chk)
>> +        self.assertTrue(chk(dashes))
>> +
>> +        self.remote_sed(
>> +            exporter,
>> +            "/var/lib/juju/agents/unit-prometheus-libvirt-exporter-0/charm/"
>> +            "files/grafana-dashboards/libvirt.json",
>> +            "s/FunctionalTestMarker/Libvirt/",
>> +        )
>> +        model.run_on_unit(
>> +            exporter, "JUJU_HOOK_NAME=upgrade-charm ./hooks/upgrade-charm"
>> +        )
>> +        model.block_until_all_units_idle()
> 
> Doesn't look like there's a check for the new updated dashboard back to Libvirt. Is this intended? Or is this to put it back to consistent state?

Indeed the intention is to put it back into it's original state in case more tests are being written that might assume this state

>> +
>>      def test_12_grafana_create_user(self):
>>          """Run the create-user action."""
>>          params = {
> 
>

Revision history for this message

Alvaro Uria (aluria) wrote on 2020-07-03:

I've added a few comments inline. Thank you.

review: Needs Fixing

Revision history for this message

Peter Sabaini (peter-sabaini) wrote on 2020-07-03:

Pushed some updates, one comment inline

Revision history for this message

Giuseppe Petralia (peppepetra) wrote on 2020-07-07:

All tests pass. Code looks good.

Just one minor comment inline on debug log, otherwise approved

review: Approve

Revision history for this message

Alvaro Uria (aluria) wrote on 2020-07-23:

Change lgtm, thank you.

review: Approve

Revision history for this message

🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote on 2020-07-23:

Failed to merge change (unable to merge source repository due to conflicts), setting status to needs review.

Revision history for this message

Alvaro Uria (aluria) wrote on 2020-07-23:

It seems it needs to be rebased

review: Needs Fixing

Revision history for this message

Peter Sabaini (peter-sabaini) wrote on 2020-07-24:

Rebase done. I've rerun functional and unit tests (note with the prometheus-libvirt-exporter from https://code.launchpad.net/~peter-sabaini/charm-prometheus-libvirt-exporter/+git/charm-prometheus-libvirt-exporter/+merge/386434), tests are passing

Revision history for this message

Alvaro Uria (aluria) wrote on 2020-07-24:

I've also run all tests and reviewed code one more time, and it lgtm. Cheers!

review: Approve

Revision history for this message

🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote on 2020-07-24:

Change successfully merged at revision 963e29018ea7ec13e17b1bc69b6f8d697a8f4ed0

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Peter Sabaini

 diff --git a/reactive/grafana.py b/reactive/grafana.py
 index 8c39a9d..17dfad1 100644
 --- a/reactive/grafana.py
 +++ b/reactive/grafana.py
@@ -1121,22 +1121,47 @@ def hpwgen(passwd, salt):
      return hpasswd
--@when("grafana.started", "endpoint.dashboards.requests")
++def ensure_dash_deserialized(dash):
++    """Deserialize dash.
++
++    Per the dashboard interface the dashboards transmitted over a relation
++    should already be de-serialized json, but older dashboard providers used
++    to send a json string, so attempt to deserialize if we find a string
++    """
++    if isinstance(dash, str):
++        dash = json.loads(dash)
++    return dash
++
++
++@when("grafana.started", "endpoint.dashboards.has_requests")
  @when_not("grafana.change-block")
  def import_dashboards(dashboards):
--    for request in dashboards.new_requests:
--        try:
--            dash = request.dashboard
--            if isinstance(request.dashboard, str):
--                dash = json.loads(request.dashboard)
--            success, reason = import_dashboard(dash, request.name)
--        except json.decoder.JSONDecodeError as error:
--            success, reason = False, str(error)
--
++    kv = unitdata.kv()
++    dashboard_digests = kv.get("dash_digests", {})
++    new_requests = set([r.request_id for r in dashboards.new_requests])
++    for req in dashboards.all_requests:
++        dash = ensure_dash_deserialized(req.dashboard)
++        is_new = req.request_id in new_requests
++        digest = dash.get("digest")
++        name = req.name
          hookenv.log(
--            "import_dashboards via grafana-dashboards: {} [{}]".format(
--                success, reason
--            ),
--            hookenv.DEBUG,
++            "import_dashboards: {}, digest {}, is_new: {}".format(
++                name, digest, is_new
++            )
+         )
--        request.respond(success, reason)
++        if not digest and not is_new:
++            # Backward compat: if we don't see a digest in the payload, it's
++            # coming from an older provider. Skip unless it's new
++            continue
++        if dashboard_digests.get(name) != digest or is_new:
++            success, reason = import_dashboard(dash, name)
++            dashboard_digests[name] = digest
++            hookenv.log(
++                "import_dashboards {}, digest {}: success: {} [{}]".format(
++                    name, digest, success, reason
++                )
++            )
++            if is_new:
++                req.respond(success, reason)
++
++    kv.set("dash_digests", dashboard_digests)
 diff --git a/tests/functional/tests/bundles/base.yaml b/tests/functional/tests/bundles/base.yaml
 index 7b8d346..7994657 100644
 --- a/tests/functional/tests/bundles/base.yaml
 +++ b/tests/functional/tests/bundles/base.yaml
@@ -49,6 +49,9 @@ applications:
    prometheus-ceph-exporter:
      charm: cs:prometheus-ceph-exporter
      num_units: 1
++  prometheus-libvirt-exporter:
++    charm: cs:prometheus-libvirt-exporter
++
  relations:
  - - keystone:shared-db
@@ -75,3 +78,9 @@ relations:
    - grafana:grafana-source
  - - grafana:nrpe-external-master
    - nrpe
++- - prometheus-libvirt-exporter
++  - mysql
++- - prometheus:target
++  - prometheus-libvirt-exporter
++- - grafana:dashboards
++  - prometheus-libvirt-exporter
 diff --git a/tests/functional/tests/bundles/overlays/focal-snap.yaml.j2 b/tests/functional/tests/bundles/overlays/focal-snap.yaml.j2
 index 49b191a..446a332 100644
 --- a/tests/functional/tests/bundles/overlays/focal-snap.yaml.j2
 +++ b/tests/functional/tests/bundles/overlays/focal-snap.yaml.j2
@@ -21,3 +21,5 @@ applications:
      series: bionic
    prometheus-ceph-exporter:
      series: bionic
++  prometheus-libvirt-exporter:
++    series: bionic
 diff --git a/tests/functional/tests/bundles/overlays/focal.yaml.j2 b/tests/functional/tests/bundles/overlays/focal.yaml.j2
 index 452a798..5734e6c 100644
 --- a/tests/functional/tests/bundles/overlays/focal.yaml.j2
 +++ b/tests/functional/tests/bundles/overlays/focal.yaml.j2
@@ -20,3 +20,5 @@ applications:
      series: bionic
    prometheus-ceph-exporter:
      series: bionic
++  prometheus-libvirt-exporter:
++    series: bionic
 diff --git a/tests/functional/tests/test_grafana.py b/tests/functional/tests/test_grafana.py
 index 5f452d9..31d3b4d 100644
 --- a/tests/functional/tests/test_grafana.py
 +++ b/tests/functional/tests/test_grafana.py
@@ -16,6 +16,8 @@ DEFAULT_API_URL = "/"
  class BaseGrafanaTest(unittest.TestCase):
      """Base for Prometheus-openstack-exporter charm tests."""
++    _admin_pass = None
++
      @classmethod
      def setUpClass(cls):
          """Set up tests."""
@@ -34,6 +36,43 @@ class BaseGrafanaTest(unittest.TestCase):
          u = model.get_unit_from_name(unit)
          return u.workload_status_message
++    @property
++    def admin_password(self):
++        if self._admin_pass is None:
++            action = model.run_action(
++                self.lead_unit_name,
++                "get-admin-password",
++                raise_on_failure=True,
++            )
++            self._admin_pass = action.data["results"]["password"]
++        return self._admin_pass
++
++    def get_and_check_dashboards(self, cond):
++        """Retrieve dashboard from grafana
++
++        Check if cond(dashboards) is true, otherwise retry up to TEST_TIMEOUT
++        """
++        timeout = time.time() + TEST_TIMEOUT
++        dashboards = []
++        while True:
++            req = requests.get(
++                "http://{host}:{port}"
++                "/api/search?dashboardIds".format(
++                    host=self.grafana_ip, port=DEFAULT_API_PORT
++                ),
++                auth=("admin", self.admin_password),
++            )
++            self.assertTrue(req.status_code == 200)
++            dashboards = json.loads(req.text)
++            if time.time() > timeout or cond(dashboards):
++                break
++            time.sleep(30)
++        return dashboards
++
++    def remote_sed(self, unit, file, expr):
++        cmd = ["sed", "-i", "-e", expr, file]
++        model.run_on_unit(unit, " ".join(cmd))
++
  class CharmOperationTest(BaseGrafanaTest):
      """Verify operations."""
@@ -99,25 +138,9 @@ class CharmOperationTest(BaseGrafanaTest):
      def test_10_grafana_imported_dashboards(self):
          """Check that Grafana dashboards expected are there"""
--        action = model.run_action(
--            self.lead_unit_name, "get-admin-password", raise_on_failure=True,
++        dashboards = self.get_and_check_dashboards(
++            lambda dashes: len(dashes) >= 4
+         )
--        passwd = action.data["results"]["password"]
--        # Loop this, it takes a while for the data to arrive and dashboards
--        # to populate
--        timeout = time.time() + TEST_TIMEOUT
--        dashboards = []
--        while time.time() < timeout and len(dashboards) < 4:
--            req = requests.get(
--                "http://{host}:{port}"
--                "/api/search?dashboardIds".format(
--                    host=self.grafana_ip, port=DEFAULT_API_PORT
--                ),
--                auth=("admin", passwd),
--            )
--            self.assertTrue(req.status_code == 200)
--            dashboards = json.loads(req.text)
--            time.sleep(30)
          self.assertTrue(len(dashboards) >= 4)
          self.assertTrue(
              all(
@@ -127,6 +150,40 @@ class CharmOperationTest(BaseGrafanaTest):
+             )
+         )
++    def test_11_dashboard_upgrade(self):
++        """Test upgraded dashboards are imported."""
++        exporter = "prometheus-libvirt-exporter/0"
++        self.remote_sed(
++            exporter,
++            "/var/lib/juju/agents/unit-prometheus-libvirt-exporter-0/charm/"
++            "files/grafana-dashboards/libvirt.json",
++            "s/Libvirt/FunctionalTestMarker/",
++        )
++        model.run_on_unit(
++            exporter, "JUJU_HOOK_NAME=upgrade-charm ./hooks/upgrade-charm"
++        )
++        model.block_until_all_units_idle()
++
++        def chk(dashes):
++            marker_dashes = [
++                d for d in dashes if "FunctionalTestMarker" in d["title"]
++            ]
++            return len(dashes) >= 4 and len(marker_dashes) == 1
++
++        dashes = self.get_and_check_dashboards(chk)
++        self.assertTrue(chk(dashes))
++
++        self.remote_sed(
++            exporter,
++            "/var/lib/juju/agents/unit-prometheus-libvirt-exporter-0/charm/"
++            "files/grafana-dashboards/libvirt.json",
++            "s/FunctionalTestMarker/Libvirt/",
++        )
++        model.run_on_unit(
++            exporter, "JUJU_HOOK_NAME=upgrade-charm ./hooks/upgrade-charm"
++        )
++        model.block_until_all_units_idle()
++
      def test_12_grafana_create_user(self):
          """Run the create-user action."""
          params = {
@@ -153,7 +210,7 @@ class CharmOperationTest(BaseGrafanaTest):
  class SnappedGrafanaTest(BaseGrafanaTest):
      def test_50_snap_upgrade(self):
--        """Test a snap upgrade from 6 to 7"""
++        """Test a snap upgrade from 6 to 7."""
          model.set_application_config(
              self.application_name, {"snap_channel": "7/stable"}
+         )
 diff --git a/tests/functional/tests/tests.yaml b/tests/functional/tests/tests.yaml
 index 56abe83..909b776 100644
 --- a/tests/functional/tests/tests.yaml
 +++ b/tests/functional/tests/tests.yaml
@@ -23,3 +23,5 @@ target_deploy_status:
      workload-status-message: Running
    telegraf:
      workload-status-message: Monitoring
++  prometheus-libvirt-exporter:
++    workload-status-message: "Exporter installed and connected to libvirt slot"

Grafana Charm

Merge ~peter-sabaini/charm-grafana:bug/1875691-dashboard-upgrade into charm-grafana:master

Commit message

Description of the change

Preview Diff

Subscribers