Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify mgr-influx module database check to not require admin privileges #18102

Merged
merged 2 commits into from Oct 11, 2017

Conversation

Projects
None yet
2 participants
@bmeekhof
Copy link
Contributor

commented Oct 3, 2017

  • existing check tried to list all DB and fails even if DB exists if user is not admin level
  • still tries to create database if not found and user has privs
  • modified measurement for daemon stats to be ceph_daemon_stats (was osd)

Signed-off-by: Benjeman Meekhof bmeekhof@umich.edu

@jcsp
Copy link
Contributor

left a comment

Looks good in principle, just some small changes requested.

Please could you also update the commit message to have the "mgr/influx: " prefix (we use a ": what changed" style).

@@ -82,7 +83,7 @@ def get_daemon_stats(self):
value = counter_info['value']

data.append({
"measurement": "ceph_osd_stats",

This comment has been minimized.

Copy link
@jcsp

jcsp Oct 4, 2017

Contributor

This change appears to be unrelated to the commit message -- if it's intentional, please separate it out into a different commit.

client.write_points(self.get_daemon_stats(), 'ms')
# using influx client get_list_database requires admin privs, instead we'll catch the not found exception and inform the user if db can't be created
try:
client.write_points(self.get_df_stats(), 'ms')

This comment has been minimized.

Copy link
@jcsp

jcsp Oct 4, 2017

Contributor

Please use 4-space indentation (no tabs) for python code.

except InfluxDBClientError as e:
if e.code == 404:
self.log.info("Database '{0}' not found, trying to create (requires admin privs). You can also create manually and grant write privs to user '{1}'".format(database,username))
client.create_database(database)

This comment has been minimized.

Copy link
@jcsp

jcsp Oct 4, 2017

Contributor

Should also do this:

else:
    raise

So that if the error is anything other than 404, we're not silently dropping it.

client.write_points(self.get_daemon_stats(), 'ms')
except InfluxDBClientError as e:
if e.code == 404:
self.log.info("Database '{0}' not found, trying to create (requires admin privs). You can also create manually and grant write privs to user '{1}'".format(database,username))

This comment has been minimized.

Copy link
@jcsp

jcsp Oct 4, 2017

Contributor

Please could you update the docs in doc/mgr/influx.rst to explain that the user must either have admin privs, or that the database must have been precreated

@jcsp jcsp added bug fix mgr labels Oct 4, 2017

bmeekhof added some commits Oct 3, 2017

mgr/influx: modify module database check to not require admin privileges
- existing check tried to list all DB and fails even if DB exists if user is not admin level
- still tries to create database if not found and user has privs

Signed-off-by: Benjeman Meekhof <bmeekhof@umich.edu>
mgr/influx: Correct name of daemon stat measurement to 'ceph_daemon_s…
…tats'

Signed-off-by: Benjeman Meekhof <bmeekhof@umich.edu>

@bmeekhof bmeekhof force-pushed the MI-OSiRIS:mgr_influx_dbcheck branch from faa0da1 to f9014a1 Oct 4, 2017

@bmeekhof

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2017

@jcsp requested changes made, thanks!

@jcsp

jcsp approved these changes Oct 4, 2017

@jcsp jcsp merged commit fd7bcb2 into ceph:master Oct 11, 2017

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.