mon,mgr: improve 'mgr module disable' cmd #21188

guzhongyan · 2018-04-03T02:55:13Z

check if the module do exist or is disabled already.

Signed-off-by: Gu Zhongyan guzhongyan@360.cn

guzhongyan · 2018-04-03T02:56:02Z

$ceph mgr module ls
{
    "enabled_modules": [
        "balancer",
        "dashboard",
        "restful",
        "status"
    ],
    "disabled_modules": [
        {
            "name": "dashboard_v2",
            "can_run": true,
            "error_string": ""
        },
        {
            "name": "influx",
            "can_run": false,
            "error_string": "influxdb python module not found"
        },
        {
            "name": "localpool",
            "can_run": true,
            "error_string": ""
        },
        {
            "name": "prometheus",
            "can_run": true,
            "error_string": ""
        },
        {
            "name": "selftest",
            "can_run": true,
            "error_string": ""
        },
        {
            "name": "smart",
            "can_run": true,
            "error_string": ""
        },
        {
            "name": "zabbix",
            "can_run": true,
            "error_string": ""
        }
    ]
}
disable an already disabled module
$ceph mgr module disable smart
Error EINVAL: module  smart is already disabled

disable an non existed module
$ceph mgr module disable non_exist_module
Error ENOENT: module  non_exist_module does not exist

enable an already enabled module
$ bin/ceph mgr module enable balancer
Error EINVAL: module balancer is already enabled

LenzGr · 2018-04-04T13:44:27Z

src/mon/MgrMap.h

@@ -179,6 +179,17 @@ class MgrMap
    return false;
  }

+  bool enabled_module(const std::string &module_name) const


Just wondering: would it make sense to rename this to module_enabled or is_enabled?

jcsp · 2018-04-04T15:50:23Z

src/mon/MgrMonitor.cc

+    }
+    if (!pending_map.enabled_module(module)) {
+      ss << "module " << module << " is already disabled";
+      r = -EINVAL;


This is not desired behaviour -- all Ceph CLI operations should be idempotent, so disabling a disabled module should return 0.

jcsp · 2018-04-04T16:08:16Z

src/mon/MgrMonitor.cc

@@ -895,6 +900,16 @@ bool MgrMonitor::prepare_command(MonOpRequestRef op)
      r = -EINVAL;
      goto out;
    }
+    if (!pending_map.all_support_module(module)) {
+      ss << "module " << module << " does not exist";
+      r = -ENOENT;


I would agree that disabling a nonexistent module should be an error (in order to detect mistyped module names), but I don't think all_support_module is the right check. We should accept a disable command if any mgr has reported the module.

@jcsp , I re-checked the all_support_module(), it seems it included all mgrs' reported modules(both active and standby). Could you point out why it is not the right check?

If module foo is enabled, and module foo is installed on mgr.x, but not mgr.y, then all_support_module would return false and we would return -ENOENT. The correct behaviour in that case would be to proceed with successfully disabling the module, even if some mgr daemons do not have it.

@jcsp got it. I updated the fix, please take a look.

jcsp · 2018-04-04T16:08:47Z

src/mon/MgrMonitor.cc

@@ -887,6 +887,11 @@ bool MgrMonitor::prepare_command(MonOpRequestRef op)
      r = -ENOENT;
      goto out;
    }
+    if (pending_map.enabled_module(module)) {
+      ss << "module " << module << " is already enabled";
+      r = -EINVAL;


Same as in the disable case, if the module is already enabled then we must return 0 to be idempotent.

jcsp · 2018-04-10T11:06:58Z

src/mon/MgrMap.h

+    return false;
+  }
+
+  bool module_existed(const std::string& module) {


I suggest naming this function any_support_module so that its relationship with all_support_module is more obvious

@jcsp fixed.

jcsp · 2018-04-10T11:08:56Z

src/mon/MgrMap.h

@@ -179,6 +179,29 @@ class MgrMap
    return false;
  }

+  bool module_enabled(const std::string &module_name) const
+  {
+    for (const auto &name : modules) {


Since modules is a std::set, you can make this function a one-liner modules.find(module_name) != modules.end()

@jcsp fixed.

tchaikov · 2018-04-13T09:21:47Z

src/mon/MgrMonitor.cc

@@ -895,6 +900,16 @@ bool MgrMonitor::prepare_command(MonOpRequestRef op)
      r = -EINVAL;
      goto out;
    }
+    if (!pending_map.any_support_module(module)) {
+      ss << "module " << module << " does not exist";


this breaks the test of test_mon_misc() in qa/workunits/cephtool/test.sh, see http://pulpito.ceph.com/kchai-2018-04-13_07:54:02-rados-wip-kefu-testing-2018-04-13-1431-distro-basic-smithi/2394008/

2018-04-13T08:13:18.738 INFO:tasks.workunit.client.0.smithi109.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:751: test_mon_misc: ceph mgr module disable foodne 2018-04-13T08:13:19.020 INFO:tasks.workunit.client.0.smithi109.stderr:Error ENOENT: module foodne does not exist

probably we could just drop this check?

I think we should move the check inside the !pending_map.module_enabled block, so that the test can still disable a non-existent module if the module was enabled with --force at some point.

Then the test should also be updated on the foodnebizbangbash line to expect that to fail.

@jcsp how about set the retval =0, if pending_map.any_support_module(module) is false?

Actually what I just suggested was wrong because it wouldn't be idempotent. We need to return 0 from all these cases.

However, in the !pending_map.any_support_module(module) we can't just return, because it might still need removing from the list of enabled modules if it was added with --force. We can do that by moving this block after the !pending_map.module_enabled(module) one, and just set the ss output rather then returning.

So I think the correct code would be:

if (!pending_map.module_enabled(module)) { ss << "module " << module << " is already disabled"; r = 0; goto out; } if (!pending_map.any_support_module(module)) { ss << "module " << module << " does not exist"; } pending_map.modules.erase(module);

@jcsp @tchaikov , updated. please take a look.

tchaikov

also, could you squash these two commits into a single one?

tchaikov · 2018-04-13T13:41:31Z

src/mon/MgrMap.h

@@ -179,6 +179,23 @@ class MgrMap
    return false;
  }

+  bool module_enabled(const std::string &module_name) const


please be consistent where to put the &. i'd suggest put it right next to std::string.

tchaikov · 2018-04-13T13:42:15Z

src/mon/MgrMap.h

@@ -179,6 +179,23 @@ class MgrMap
    return false;
  }

+  bool module_enabled(const std::string &module_name) const
+  {
+    return (modules.find(module_name) != modules.end());


nit, remove the parentheses around this statement.

tchaikov · 2018-04-13T13:42:52Z

src/mon/MgrMap.h

+    return (modules.find(module_name) != modules.end());
+  }
+
+  bool any_support_module(const std::string& module) {


mark this method const, if you please.

"any" is singular, so should be "supports", i guess?

Either way is probably fine. I had been reading this as a contraction of "[do] any [daemons] support module" rather than "[if] any [daemon] supports module". I don't even know the right grammar terminology to describe the difference though :-D

tchaikov · 2018-04-13T13:45:46Z

src/mon/MgrMonitor.cc

@@ -895,6 +895,14 @@ bool MgrMonitor::prepare_command(MonOpRequestRef op)
      r = -EINVAL;
      goto out;
    }
+    if (!pending_map.module_enabled(module)) {
+      ss << "module " << module << " is already disabled";


i'd suggest add quotes around "module", like

ss << "module '" << module << "' is already disabled";

tchaikov · 2018-04-13T13:45:58Z

src/mon/MgrMonitor.cc

+      goto out;
+    }
+    if (!pending_map.any_support_module(module)) {
+      ss << "module " << module << " does not exist";


jcsp · 2018-04-13T15:35:39Z

retest this please (jenkins)

the test should pass now.

guzhongyan · 2018-04-16T01:45:31Z

@tchaikov all your comments are addressed. please take a look.

tchaikov · 2018-04-21T16:53:27Z

@guzhongyan needs rebase

when running those cmds, check if the module do exist or is disabled already. also check if the module is enabled already. Signed-off-by: Gu Zhongyan <guzhongyan@360.cn>

guzhongyan · 2018-04-23T01:38:41Z

@tchaikov rebased, Thanks.

tchaikov · 2018-04-23T10:55:06Z

http://pulpito.ceph.com/kchai-2018-04-23_07:38:14-rados-wip-kefu-testing-2018-04-23-1357-distro-basic-smithi/

test failures are tracked / addressed by

smithfarm added the mgr label Apr 3, 2018

guzhongyan force-pushed the mgr-disable-fix branch 2 times, most recently from b1bfe39 to 8d8943d Compare April 3, 2018 03:20

guzhongyan changed the title ~~improve 'mgr module disable' cmd~~ mon,mgr: improve 'mgr module disable' cmd Apr 3, 2018

LenzGr requested a review from jcsp April 4, 2018 13:41

LenzGr reviewed Apr 4, 2018

View reviewed changes

jcsp reviewed Apr 4, 2018

View reviewed changes

guzhongyan force-pushed the mgr-disable-fix branch 2 times, most recently from 091cef2 to f325b1a Compare April 10, 2018 07:26

jcsp reviewed Apr 10, 2018

View reviewed changes

guzhongyan force-pushed the mgr-disable-fix branch from f325b1a to b0c3120 Compare April 11, 2018 01:52

jcsp approved these changes Apr 11, 2018

View reviewed changes

jcsp added the needs-qa label Apr 11, 2018

tchaikov added the wip-kefu-testing label Apr 11, 2018

tchaikov previously requested changes Apr 13, 2018

View reviewed changes

tchaikov removed needs-qa wip-kefu-testing labels Apr 13, 2018

guzhongyan force-pushed the mgr-disable-fix branch from b0c3120 to 2ae847e Compare April 13, 2018 13:31

tchaikov reviewed Apr 13, 2018

View reviewed changes

guzhongyan force-pushed the mgr-disable-fix branch from 2ae847e to 1cabf79 Compare April 16, 2018 01:43

tchaikov added needs-qa wip-kefu-testing and removed wip-kefu-testing labels Apr 16, 2018

tchaikov removed the wip-kefu-testing label Apr 21, 2018

mon,mgr: improve 'mgr module disable/enable' cmds

a7677c7

when running those cmds, check if the module do exist or is disabled already. also check if the module is enabled already. Signed-off-by: Gu Zhongyan <guzhongyan@360.cn>

guzhongyan force-pushed the mgr-disable-fix branch from 1cabf79 to a7677c7 Compare April 23, 2018 01:36

tchaikov added the wip-kefu-testing label Apr 23, 2018

tchaikov removed needs-qa wip-kefu-testing labels Apr 23, 2018

tchaikov self-assigned this Apr 23, 2018

liewegas added this to the mimic milestone Apr 23, 2018

liewegas merged commit 5b99b6f into ceph:master Apr 23, 2018

guzhongyan deleted the mgr-disable-fix branch April 24, 2018 02:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mon,mgr: improve 'mgr module disable' cmd #21188

mon,mgr: improve 'mgr module disable' cmd #21188

guzhongyan commented Apr 3, 2018

guzhongyan commented Apr 3, 2018 •

edited by tchaikov

LenzGr Apr 4, 2018

guzhongyan Apr 9, 2018

jcsp Apr 4, 2018

guzhongyan Apr 9, 2018

jcsp Apr 4, 2018

guzhongyan Apr 9, 2018

jcsp Apr 9, 2018

guzhongyan Apr 10, 2018

jcsp Apr 4, 2018

guzhongyan Apr 9, 2018

jcsp Apr 10, 2018

guzhongyan Apr 11, 2018

jcsp Apr 10, 2018

guzhongyan Apr 11, 2018

tchaikov Apr 13, 2018 •

edited

jcsp Apr 13, 2018

guzhongyan Apr 13, 2018

jcsp Apr 13, 2018 •

edited

guzhongyan Apr 13, 2018

tchaikov left a comment

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

jcsp Apr 13, 2018

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

jcsp commented Apr 13, 2018

guzhongyan commented Apr 16, 2018

tchaikov commented Apr 21, 2018

guzhongyan commented Apr 23, 2018

tchaikov commented Apr 23, 2018

mon,mgr: improve 'mgr module disable' cmd #21188

mon,mgr: improve 'mgr module disable' cmd #21188

Conversation

guzhongyan commented Apr 3, 2018

guzhongyan commented Apr 3, 2018 • edited by tchaikov

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsp Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsp commented Apr 13, 2018

guzhongyan commented Apr 16, 2018

tchaikov commented Apr 21, 2018

guzhongyan commented Apr 23, 2018

tchaikov commented Apr 23, 2018

guzhongyan commented Apr 3, 2018 •

edited by tchaikov

tchaikov Apr 13, 2018 •

edited

jcsp Apr 13, 2018 •

edited