Merge ~rharding/interface-grafana-source:add-mysql-support into interface-grafana-source:master

Proposed by Richard Harding
Status: Merged
Merged at revision: 402c7eda9fedfcfd83ef88e583e1819c58718293
Proposed branch: ~rharding/interface-grafana-source:add-mysql-support
Merge into: interface-grafana-source:master
Diff against target: 77 lines (+40/-2)
2 files modified
provides.py (+37/-2)
requires.py (+3/-0)
Reviewer Review Type Date Requested Status
Jacek Nykis (community) Needs Fixing
Thomas Cuthbert (community) Needs Fixing
Review via email: mp+330041@code.launchpad.net

Description of the change

Add support for mysql as a datasource in the interface.

Make sure it's backward compatible as much as possible.

- takes an optional database argument
- makes sure that the username/password is set
- supports ip:port for the url in the mysql case

To post a comment you must log in.
Revision history for this message
Richard Harding (rharding) wrote :

To support this I've pushed up my built grafana charm with the interface and tested it via:

    juju deploy mysql; juju deploy cs:~rharding/gypsy-danger-3; juju deploy cs:~rharding/grafana-1
    juju relate mysql:db gypsy-danger; juju relate gypsy-danger:grafana-source grafana
    juju expose grafana

And validated I can pull up the list of datasources and successfully "Save and test" the mysql datasource that's added.

Then I made sure it didn't regress promtheus by doing:

    juju deploy prometheus; juju relate prometheus:grafana-source grafana

And again validating the data sources.

Revision history for this message
Thomas Cuthbert (tcuthbert) wrote :

comments inline

review: Needs Fixing
Revision history for this message
Thomas Cuthbert (tcuthbert) wrote :

I also couldn't get your gypsy-danger charm to actually populate the datasource with the correct database name, username and password even though in the logs I saw the sql statements setting it up and the relation data is all there.

Revision history for this message
Richard Harding (rharding) wrote :

@tcuthbert can you specify which charm you used? Let me check the latest code here got pushed to the charmstore and I wonder if the latest gypsy-danger charm made it. I'll double check.

Revision history for this message
Richard Harding (rharding) wrote :

I've pushed an update to check the url/port per the suggestion and rebuilt and pushed updated charms to my account. I used the following and things deployed and loaded correctly using JAAS on a GCE model. I'd be curious to know more about it not working so that I can narrow down what could be wrong here.

I've got a case of WFM and not sure what's different. What cloud are you on? Did you use these commands to something different?

https://usercontent.irccloud-cdn.com/file/D7vdbSOE/Selection_013.png

juju deploy mysql; juju deploy cs:~rharding/gypsy-danger-4; juju deploy cs:~rharding/grafana-2
juju relate mysql:db gypsy-danger; juju relate gypsy-danger:grafana-source grafana
juju expose grafana

Revision history for this message
Thomas Cuthbert (tcuthbert) wrote :

> @tcuthbert can you specify which charm you used? Let me check the latest code
> here got pushed to the charmstore and I wonder if the latest gypsy-danger
> charm made it. I'll double check.

FWIW I was testing on prodstack45 against our staging prometheus environment. I'll retest everything to see if the relationships work tomorrow.

Revision history for this message
Richard Harding (rharding) wrote :

Thanks, I've setup a bundle to test it out with:

    juju deploy cs:~rharding/gypsy-danger-testing-0

One thing is that setting the admin password in the bundle doesn't work. It looks like maybe some sort of race in the grafana charm. I'll file a bug

Revision history for this message
Richard Harding (rharding) wrote :

Had to update the bundle for working on my local maas setup:

cs:~rharding/bundle/gypsy-danger-testing-1

Revision history for this message
Richard Harding (rharding) wrote :

I wanted to see if things were working ok and tested it on AWS, Azure, GCE, and my MAAS setup. Each time it worked and sent the data properly. Any issues please get any info you can get to help diagnose.

Revision history for this message
Thomas Cuthbert (tcuthbert) wrote :

so interestingly when I redeployed gypsy-danger, mysql again and did the relations it populated the connection info fine. When I removed relation and tried again it wouldn't update the connection details.

https://pastebin.canonical.com/197838/
https://pastebin.canonical.com/197840/

everything else is fine now.

Revision history for this message
Richard Harding (rharding) wrote :

Thanks, I've looked into this and it seems it's related into the way the grafana datasources code works. It detects that it's already been done and when you break the relation it doesn't do any deleting. I'll have to dive into this a bit as I'm a bit fuzzy on how the internal code tracking the datasources (not in the grafana db itself) works.

Revision history for this message
Jacek Nykis (jacekn) wrote :

Setting as "needs fixing" since we are waiting for Rick.

review: Needs Fixing
Revision history for this message
Richard Harding (rharding) wrote :

Updated the testing bundle to:

 juju deploy cs:~rharding/gypsy-danger-testing-2

This includes the changes that add the hook to properly remove the datasource when the relation is broken so that it can be readded a second time.

Tested with mysql and prometheus and verified the datasources are removed and re-add successfully.

This goes with the charm changes in:

https://code.launchpad.net/~rharding/grafana-charm/+git/grafana-charm/+merge/329855

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/provides.py b/provides.py
index c23a3cd..ff80b4d 100644
--- a/provides.py
+++ b/provides.py
@@ -15,14 +15,49 @@ class GrafanaProvides(RelationBase):
15 def broken(self):15 def broken(self):
16 self.remove_state('{relation_name}.available')16 self.remove_state('{relation_name}.available')
1717
18 def provide(self, source_type, port, description, username=None, password=None):18 def provide(self, source_type, url_or_port, description,
19 username=None, password=None, database=None):
20 """
21 :param source_type: The type of Grafana data source (prometheus/mysql)
22 :param url_or_port: Details used to build the connection details. If
23 only a port is specified the IP address is pulled
24 from the relating application. If it's a full
25 IP:port string then it's used as is.
26 :param description: Used to build the description of this
27 specific datasource in Grafana.
28 :param username: A username required to auth for connection.
29 :param password: A password required to auth for connection.
30 :param database: The database name required to connect to the
31 datasource
32 """
33 # start out with the old default behavior.
34 url = 'http://{}:{}'.format(hookenv.unit_get('private-address'),
35 url_or_port)
36
37 # This check is built to be py2 and py3 compatible to verify if the
38 # value is a string or not. If the value passed is a string with a :
39 # then use it as the full URL vs the built one.
40 if isinstance(url_or_port, ("".__class__, u"".__class__)):
41 if ':' in url_or_port:
42 # Then a url with a :port at the end was supplied. Just use it.
43 url = url_or_port
44
19 relation_info = {45 relation_info = {
20 'type': source_type,46 'type': source_type,
21 'url': 'http://{}:{}'.format(hookenv.unit_get('private-address'), port),47 'url': url,
22 'description': description,48 'description': description,
23 }49 }
50
51 if source_type == 'mysql' and (not username or not password):
52 # Username and password are required for mysql
53 hookenv.log('ERROR: Missing MySQL username/password')
54
24 if username and password:55 if username and password:
25 relation_info['username'] = username56 relation_info['username'] = username
26 relation_info['password'] = password57 relation_info['password'] = password
2758
59 if database:
60 hookenv.log('Adding the database to the relation_info')
61 relation_info['database'] = database
62
28 self.set_remote(**relation_info)63 self.set_remote(**relation_info)
diff --git a/requires.py b/requires.py
index 1c990ab..862f46b 100644
--- a/requires.py
+++ b/requires.py
@@ -31,6 +31,7 @@ class GrafanaRequires(RelationBase):
31 'description': endpoint description,31 'description': endpoint description,
32 'username': optional_username,32 'username': optional_username,
33 'password': optional_password,33 'password': optional_password,
34 'database': optional_database,
34 },35 },
35 # ...36 # ...
36 ]37 ]
@@ -45,6 +46,8 @@ class GrafanaRequires(RelationBase):
45 if conv.get_remote('username') and conv.get_remote('password'):46 if conv.get_remote('username') and conv.get_remote('password'):
46 ds['username'] = conv.get_remote('username')47 ds['username'] = conv.get_remote('username')
47 ds['password'] = conv.get_remote('password')48 ds['password'] = conv.get_remote('password')
49 if conv.get_remote('database'):
50 ds['database'] = conv.get_remote('database')
48 datasources.append(ds)51 datasources.append(ds)
4952
50 return datasources53 return datasources

Subscribers

People subscribed via source and target branches