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

qa/mgr/selftest: handle always-on module fall out #23681

Merged
merged 1 commit into from Aug 29, 2018

Conversation

tchaikov
Copy link
Contributor

…disabled command test

Per Noah Watkins,

hello isn't packaged so it can't announce its commands.

so we should use "selftest" instead "hello" for disabled command test.

in this change, we make sure "selftest" is disabled and then, try to run
one of its announced command. then re-enable this module, so the
following tests can use this module for sending commands like 'mgr self-test'

Fixes: http://tracker.ceph.com/issues/26994
Signed-off-by: Noah Watkins nwatkins@redhat.com

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

@tchaikov
Copy link
Contributor Author

@tchaikov
Copy link
Contributor Author

tchaikov commented Aug 22, 2018

we cannot enable "status" module anymore -- it's listed as an always-on module. i am copying my comment at https://github.com/ceph/ceph/pull/23558/files#r211944383 for as a reference

@noahdesu http://pulpito.ceph.com/kchai-2018-08-22_08:29:54-rados-wip-26994-distro-basic-smithi/2936591/

"status" is always on, hence it's not announced to the monitor in MgrStandby::send_beacon(). and monitor will not consider it when handling mgr module enable.

i am wondering if we can announce "always-on" modules in the beacon message? and ignore the request to enabled it? and return EINVAL when mgr module disable it?

@jcsp
Copy link
Contributor

jcsp commented Aug 23, 2018

For this test, we need a module that really can be enabled and disabled (it was added to check that commands properly wait for a newly enabled module to start).

We can avoid needing another module if we add a call to "self-test run" immediately after the _load_module("selftest") -- that would provide the same coverage.

@tchaikov
Copy link
Contributor Author

@tchaikov
Copy link
Contributor Author

now it failed in test_selftest_config_update.

2018-08-28T14:47:55.662 INFO:tasks.cephfs_test_runner:FAIL: test_selftest_config_update (tasks.mgr.test_module_selftest.TestModuleSelftest)
2018-08-28T14:47:55.662 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2018-08-28T14:47:55.663 INFO:tasks.cephfs_test_runner:Traceback (most recent call last):
2018-08-28T14:47:55.663 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/git.ceph.com_ceph-c_wip-26994-kefu/qa/tasks/mgr/test_module_selftest.py",
line 86, in test_selftest_config_update
2018-08-28T14:47:55.663 INFO:tasks.cephfs_test_runner:    self.assertEqual(get_value(), "None")
2018-08-28T14:47:55.663 INFO:tasks.cephfs_test_runner:AssertionError: 'testvalue' != 'None'

@tchaikov
Copy link
Contributor Author

@jcsp instead of using self-test run, i am using self-test config get. because run changes the settings self-test used for testing, and it failed to reset them before exits. so self-test config get is a safer option.

@tchaikov
Copy link
Contributor Author

@dotnwat
Copy link
Contributor

dotnwat commented Aug 28, 2018

@tchaikov ill work on this today. sorry for the troubles!

need a non-always-on module. hello doesn't work because it isn't
installed. so switch to selftest.

Signed-off-by: Noah Watkins <nwatkins@redhat.com>
@dotnwat
Copy link
Contributor

dotnwat commented Aug 28, 2018

@tchaikov i wasn't able to reproduce the assertion error you mentioned above. was that fixed by the switch to "self-test config get" rather than "self-test run"?

I was testing with this dotnwat@ea15b62

and it passed tests with

python ../qa/tasks/vstart_runner.py --create tasks.mgr.test_module_selftest

that patch is effectively this patch, but I loaded selftest at the beginning before disabling it to explicitly show we are testing that it is disabled rather than assuming it is enabled.

diff --git a/qa/tasks/mgr/test_module_selftest.py b/qa/tasks/mgr/test_module_selftest.py
index 3851a5be8672..30968118f822 100644
--- a/qa/tasks/mgr/test_module_selftest.py
+++ b/qa/tasks/mgr/test_module_selftest.py
@@ -231,15 +231,14 @@ def test_module_commands(self):
         disabled/failed/recently-enabled modules.
         """
 
-        self._load_module("selftest")
-
         # Calling a command on a disabled module should return the proper
         # error code.
+        self._load_module("selftest")
         self.mgr_cluster.mon_manager.raw_cluster_cmd(
-            "mgr", "module", "disable", "hello")
+            "mgr", "module", "disable", "selftest")
         with self.assertRaises(CommandFailedError) as exc_raised:
             self.mgr_cluster.mon_manager.raw_cluster_cmd(
-                "hello")
+                "mgr", "self-test", "run")
 
         self.assertEqual(exc_raised.exception.exitstatus, errno.EOPNOTSUPP)
 
@@ -252,9 +251,9 @@ def test_module_commands(self):
 
         # Enabling a module and then immediately using ones of its commands
         # should work (#21683)
+        self._load_module("selftest")
         self.mgr_cluster.mon_manager.raw_cluster_cmd(
-            "mgr", "module", "enable", "status")
-        self.mgr_cluster.mon_manager.raw_cluster_cmd("osd", "status")
+            "mgr", "self-test", "config", "get", "testkey")
 
         # Calling a command for a failed module should return the proper
         # error code.

@tchaikov tchaikov force-pushed the wip-26994 branch 2 times, most recently from b963c09 to ea15b62 Compare August 29, 2018 00:50
@tchaikov
Copy link
Contributor Author

tchaikov commented Aug 29, 2018

was that fixed by the switch to "self-test config get" rather than "self-test run"?

@noahdesu yeah. and i cherry-picked your change into this PR. could you help me approve it? or i can do so if you want to create a PR by yourself.

@tchaikov
Copy link
Contributor Author

@tchaikov tchaikov changed the title qa/tasks/mgr/test_module_selftest: use selftest instead of hello for … qa/mgr/selftest: handle always-on module fall out Aug 29, 2018
@tchaikov tchaikov merged commit 786bec6 into ceph:master Aug 29, 2018
@tchaikov tchaikov deleted the wip-26994 branch August 29, 2018 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants