Merge ~brettmilford/charm-grafana:lp-1893320 into charm-grafana:master

Proposed by Brett Milford
Status: Merged
Approved by: Drew Freiberger
Approved revision: 872692b53b18224959ffa197f08d131650968bd3
Merged at revision: 7929c5b8ef30a3f4e61b5aa55d6600dca62110a0
Proposed branch: ~brettmilford/charm-grafana:lp-1893320
Merge into: charm-grafana:master
Diff against target: 53 lines (+22/-2)
2 files modified
src/reactive/grafana.py (+11/-2)
src/tests/functional/tests/test_grafana.py (+11/-0)
Reviewer Review Type Date Requested Status
Drew Freiberger Approve
Paul Goins Approve
Review via email: mp+393348@code.launchpad.net

Commit message

Catch ConnectionError and update constraints

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
Drew Freiberger (afreiberger) wrote :

I commented on bug that I'm okay with the implications of the revert of 7540b, as the ds_name can't be changed in the UI and have any of our dashboards work properly, anyways.

I would like to know if this change survives an upgrade-charm well before approving.

Revision history for this message
Brett Milford (brettmilford) wrote :

Here is what I've tested.

I deployed the grafana portion of the LMA stack like so:

https://pastebin.canonical.com/p/b4TqNcF3nc/

Upgraded charm to the local repository built with the patch.

juju upgrade-charm grafana --path ./.build/grafana

And monitored for issues - none were seen.

Also tested upgrading the charm from an error state after changing prometheus listen port. --force-units is required so that the new charm code can be delivered and executed.

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

Thanks. As I was reviewing this again, I'd love to see a blocked status set in the exception handling. I realize there may not be one in the exception handling just below, but since you're returning without failing, the operator won't know that dashboards didn't update or that there's an error in the log to review.

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

+1 from me; one more review from the BootStack team needed for merge.

I did leave some review comments; my final comment suggests adding a source code comment for clarification and noting reason for adding the test in this MR. If you can spare the time, please consider doing that.

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

Thanks Brett!

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

LGTM +1

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

Change successfully merged at revision 7929c5b8ef30a3f4e61b5aa55d6600dca62110a0

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/reactive/grafana.py b/src/reactive/grafana.py
2index d6718eb..04b7c27 100644
3--- a/src/reactive/grafana.py
4+++ b/src/reactive/grafana.py
5@@ -701,7 +701,7 @@ def check_datasource(ds):
6 rows = query.fetchall()
7 ds_name = "{} - {}".format(ds["service_name"], ds["description"])
8 for row in rows:
9- if row[1] == ds["type"] and row[3] == ds["url"]:
10+ if row[1] == ds["type"] and row[2] == ds_name:
11 hookenv.log("Datasource already exist, updating: {}".format(ds_name))
12 stmt, values = generate_query(ds, row[4], id=row[0])
13 print(stmt, values)
14@@ -880,7 +880,16 @@ def generate_prometheus_dashboards(gf_adminpasswd, ds):
15 config = hookenv.config()
16
17 # https://prometheus.io/docs/prometheus/latest/querying/api/#querying-label-values
18- response = requests.get("{}/api/v1/label/__name__/values".format(ds["url"]))
19+ try:
20+ response = requests.get("{}/api/v1/label/__name__/values".format(ds["url"]))
21+ except requests.exceptions.ConnectionError as e:
22+ hookenv.log(
23+ "Exception while trying to reach prometheus API: {}".format(e),
24+ "ERROR",
25+ )
26+ hookenv.status_set("blocked", "Exception reaching prometheus API whilst updating dashboards")
27+ return
28+
29 if response.status_code != 200:
30 hookenv.log(
31 "Could not reach prometheus API status code: "
32diff --git a/src/tests/functional/tests/test_grafana.py b/src/tests/functional/tests/test_grafana.py
33index 4fa664a..5832fe0 100644
34--- a/src/tests/functional/tests/test_grafana.py
35+++ b/src/tests/functional/tests/test_grafana.py
36@@ -251,6 +251,17 @@ class CharmOperationTest(BaseGrafanaTest):
37 self.assertTrue(port in crontab["Stdout"])
38 self.verify_iterative_backups()
39
40+ def test_15_grafana_datasource_updates(self):
41+ """
42+ Change the port on the associated datasource.
43+ Ensures the model returns to idle after changing the prometheus port post deployment.
44+ As per LP #1893320
45+ """
46+ datasource_application = "prometheus"
47+ port_config = {"web-listen-port": "1234"}
48+ model.set_application_config(datasource_application, port_config)
49+ model.block_until_all_units_idle()
50+
51
52 class SnappedGrafanaTest(BaseGrafanaTest):
53 """Verify Grafana installed as a snap."""

Subscribers

People subscribed via source and target branches