Merge ~jamesbeedy/charm-grafana:relation_base_to_endpoint into charm-grafana:master

Proposed by james beedy
Status: Work in progress
Proposed branch: ~jamesbeedy/charm-grafana:relation_base_to_endpoint
Merge into: charm-grafana:master
Diff against target: 52 lines (+24/-18)
1 file modified
src/reactive/grafana.py (+24/-18)
Reviewer Review Type Date Requested Status
Drew Freiberger Needs Fixing
BootStack Reviewers Pending
Review via email: mp+393218@code.launchpad.net

Description of the change

Accompanying changes to interface-grafana-source changes (https://code.launchpad.net/~jamesbeedy/interface-grafana-source/+git/interface-grafana-source/+ref/relation_base_to_endpoint) in respect to replacing RelationBase with Endpoint in the grafana-source requires.

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
Paul Goins (vultaire) wrote :

Did an initial review.

It looks sane. I'm not sure I've personally dealt with conversations vs. endpoints, but the change makes sense.

I'm not yet ready to +1, as I think some hands-on checking and testing is appropriate here. Further, there's the related changes to interface-grafana-source (https://code.launchpad.net/~jamesbeedy/interface-grafana-source/+git/interface-grafana-source/+merge/393217) which I'm guessing is related here - I haven't looked deeply enough quite to know for sure.

To be clear, this isn't waiting on the author; this is waiting for reviewers (maybe me) to dig into the above points and make sure this won't break.

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

inline comment below. I think there's an issue with either implementation's logic when we migrate prometheus/influxdb/other grafana-source units but keep the relation.

Either way, I think we have an issue and we'd have to break and re-create the relation if datasources move, if I'm reading the surrounding code accurately.

Agree, this needs some real world testing. I'll address this tomorrow.

Revision history for this message
Drew Freiberger (afreiberger) wrote :
Download full text (4.4 KiB)

I built this branch along with the interface update and it fails grafana-source-relation-departed due to lack of relation.application_name. when removing one of two prometheus units.

unit-grafana-0: 11:53:51 INFO unit.grafana/0.juju-log grafana-source:3: Reactive main running for hook grafana-source-relation-departed
unit-grafana-0: 11:53:51 INFO unit.grafana/0.juju-log grafana-source:3: Initializing Snap Layer
unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed All snaps up to date.
unit-grafana-0: 11:53:51 INFO unit.grafana/0.juju-log grafana-source:3: Invoking reactive handler: reactive/grafana.py:609:sources_gone
unit-grafana-0: 11:53:51 INFO unit.grafana/0.juju-log grafana-source:3: sources gone
unit-grafana-0: 11:53:51 INFO unit.grafana/0.juju-log grafana-source:3: <relations.grafana-source.requires.GrafanaRequires object at 0x7fcb186afc88>
unit-grafana-0: 11:53:51 ERROR unit.grafana/0.juju-log grafana-source:3: Hook error:
Traceback (most recent call last):
  File "/var/lib/juju/agents/unit-grafana-0/.venv/lib/python3.6/site-packages/charms/reactive/__init__.py", line 74, in main
    bus.dispatch(restricted=restricted_mode)
  File "/var/lib/juju/agents/unit-grafana-0/.venv/lib/python3.6/site-packages/charms/reactive/bus.py", line 379, in dispatch
    _invoke(hook_handlers)
  File "/var/lib/juju/agents/unit-grafana-0/.venv/lib/python3.6/site-packages/charms/reactive/bus.py", line 359, in _invoke
    handler.invoke()
  File "/var/lib/juju/agents/unit-grafana-0/.venv/lib/python3.6/site-packages/charms/reactive/bus.py", line 181, in invoke
    self._action(*args)
  File "/var/lib/juju/agents/unit-grafana-0/charm/reactive/grafana.py", line 625, in sources_gone
    "service_name": relation.application_name,
AttributeError: 'GrafanaRequires' object has no attribute 'application_name'

unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed Traceback (most recent call last):
unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed File "/var/lib/juju/agents/unit-grafana-0/charm/hooks/grafana-source-relation-departed", line 22, in <module>
unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed main()
unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed File "/var/lib/juju/agents/unit-grafana-0/.venv/lib/python3.6/site-packages/charms/reactive/__init__.py", line 74, in main
unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed bus.dispatch(restricted=restricted_mode)
unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed File "/var/lib/juju/agents/unit-grafana-0/.venv/lib/python3.6/site-packages/charms/reactive/bus.py", line 379, in dispatch
unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed _invoke(hook_handlers)
unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed File "/var/lib/juju/agents/unit-grafana-0/.venv/lib/python3.6/site-packages/charms/reactive/bus.py", line 359, in _invoke
unit-grafana-0: 11:53:51 WARNING unit.grafana/0.grafana-source-relation-departed handler.invoke()
...

Read more...

review: Needs Fixing

Unmerged commits

4491ac8... by james beedy

refactor sources_gone to accommodate for Endpoint relation

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..a95b271 100644
3--- a/src/reactive/grafana.py
4+++ b/src/reactive/grafana.py
5@@ -616,25 +616,31 @@ def sources_gone(relation):
6 hookenv.log(relation)
7 # Update the datasources now with the new information that something is
8 # not available.
9- conv = relation.conversation()
10- ds = {
11- "service_name": conv.scope.split("/")[0],
12- "type": conv.get_remote("type"),
13- "url": conv.get_remote("url"),
14- "description": conv.get_remote("description"),
15- }
16- hookenv.log("Removing datasource:")
17- hookenv.log(ds)
18+ units = relation.all_joined_units
19+ if len(units) > 0:
20+
21+ unit = units[0]
22+
23+ ds = {
24+ "service_name": relation.application_name,
25+ "type": unit.received.get('type'),
26+ "url": unit.received.get('url'),
27+ "description": unit.received.get('description'),
28+ }
29+
30+ hookenv.log("Removing datasource:")
31+ hookenv.log(ds)
32+
33+ if ds:
34+ conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
35+ cur = conn.cursor()
36+ cur.execute(
37+ "DELETE FROM DATA_SOURCE WHERE type=? AND url=?",
38+ (ds["type"], ds["url"]),
39+ )
40+ conn.commit()
41+ cur.close()
42
43- if ds:
44- conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
45- cur = conn.cursor()
46- cur.execute(
47- "DELETE FROM DATA_SOURCE WHERE type=? AND url=?",
48- (ds["type"], ds["url"]),
49- )
50- conn.commit()
51- cur.close()
52 relation.set_local = None
53
54

Subscribers

People subscribed via source and target branches