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
diff --git a/src/reactive/grafana.py b/src/reactive/grafana.py
index d6718eb..a95b271 100644
--- a/src/reactive/grafana.py
+++ b/src/reactive/grafana.py
@@ -616,25 +616,31 @@ def sources_gone(relation):
616 hookenv.log(relation)616 hookenv.log(relation)
617 # Update the datasources now with the new information that something is617 # Update the datasources now with the new information that something is
618 # not available.618 # not available.
619 conv = relation.conversation()619 units = relation.all_joined_units
620 ds = {620 if len(units) > 0:
621 "service_name": conv.scope.split("/")[0],621
622 "type": conv.get_remote("type"),622 unit = units[0]
623 "url": conv.get_remote("url"),623
624 "description": conv.get_remote("description"),624 ds = {
625 }625 "service_name": relation.application_name,
626 hookenv.log("Removing datasource:")626 "type": unit.received.get('type'),
627 hookenv.log(ds)627 "url": unit.received.get('url'),
628 "description": unit.received.get('description'),
629 }
630
631 hookenv.log("Removing datasource:")
632 hookenv.log(ds)
633
634 if ds:
635 conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
636 cur = conn.cursor()
637 cur.execute(
638 "DELETE FROM DATA_SOURCE WHERE type=? AND url=?",
639 (ds["type"], ds["url"]),
640 )
641 conn.commit()
642 cur.close()
628643
629 if ds:
630 conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
631 cur = conn.cursor()
632 cur.execute(
633 "DELETE FROM DATA_SOURCE WHERE type=? AND url=?",
634 (ds["type"], ds["url"]),
635 )
636 conn.commit()
637 cur.close()
638 relation.set_local = None644 relation.set_local = None
639645
640646

Subscribers

People subscribed via source and target branches