replace osdmonitor_prepare_command with --mon_debug_idempotency #2413
Conversation
ghost
self-assigned this
|
trivial rebase |
ghost
changed the title
ghost
removed their assignment
|
@gregsfortytwo would you like to take a look ? You did not like osdmonitor_prepare_command and figure you will be happy to see it go away :-) |
|
Hm. Adding an option that just double-runs every command is actually a pretty good way to test for idempotency. But I'm nervous about tests that rely on loopback hitting the monitor correctly — do we have other tests doing the same thing? Can we not just use the admin socket instead of tell injectargs? |
|
Oh, and this will need to be pushed to the gitbuilders to run through testing before it gets merged, and this is all about the unit tests anyway, so you might as well push it there? |
|
@gregsfortytwo added to gitbuilder. I'm not sure about what you mean by tests that rely on loopback hitting the monitor correctly could you please expand or point to an example ? |
|
Sorry @dachary, lost this in my inbox noise. I was referring to the use of 127.0.0.1 as the monitor address, instead of looking it up somewhere. But I gather the whole unit test framework is actually set up that way, so whatever. :) |
|
@gregsfortytwo indeed the whole unit tests rely on 127.0.0.1 ;-) |
|
trivial rebase |
|
gitbuilder running. @gregsfortytwo do you have other concerns ? |
The mon command is run twice to check for idempotency and cover the case where the paxos proposal with the pending incremental changes have not yet been submitted. For instance: ceph tell 'mon.*' injectargs -- --mon-debug-idempotency ceph osd pool create mypool 12 ceph tell 'mon.*' injectargs -- --no-mon-debug-idempotency will create mypool twice. This is equivalent to CEPH_CLI_TEST_DUP_COMMAND in ceph.in except it is done within the monitor and is not subject to race conditions where the second command arrives after the first command changes have been submitted. Signed-off-by: Loic Dachary <loic-201408@dachary.org>
Signed-off-by: Loic Dachary <loic-201408@dachary.org>
Signed-off-by: Loic Dachary <loic-201408@dachary.org>
There are two tests using osdmonitor_prepare_command and they cannot be replaced with --mon_debug_idempotency. Since osdmonitor_prepare_command was introduced it has mostly been useful in the context created by --mon_debug_idempotency and only marginally in the context of the two remaining tests. It is not worth keeping them : there is more infrastructure and fragility than actual benefit. Signed-off-by: Loic Dachary <loic-201408@dachary.org>
And the mon_advanced_debug_mode option that is only used in the context of osdmonitor_prepare_command. http://tracker.ceph.com/issues/9245 Fixes: #9245 Signed-off-by: Loic Dachary <loic-201408@dachary.org>
Prior to discarding the implicit creation of a ruleset (in 8b27997) it could be done by duplicating the pool create command. There now is no convenient way to test this code path. Signed-off-by: Loic Dachary <loic-201408@dachary.org>
|
the rebase included 8b27997 which required a change in the tests (no implicit creation of a ruleset in pool create https://github.com/dachary/ceph/commit/b30fd4e35658753b9ad0f28a0dba65f60dde5528) now running on gitbuilder again |
|
Reviewed-by: Should probably get @jecluis to check it out, though; it's his system. ;) |
|
@jecluis what do you think ? |
|
I am glad to see this going away as well. However I'm concerned about the current approach. Unlike submitting the same message twice, as the ceph tool does it, we are crafting a blank message here and submitting it along with the I don't think we can safely rely on the current state of this patch. I think that we should aim to copying the original message as best as we can and pass a flag to prepare_command_impl() so that it does not reply to the message. This also has to undergo a few runs before I feel comfortable merging it. |
|
I'll split this in two then. First remove the current strategy and then implement something else. I'll have to learn more about the code base before I can implement the second step. We however need to get rid of the current test method because it has caused more trouble than good in the past months and we don't want it to keep doing this. |
ghost
closed this
Obsolete osdmonitor_prepare_command and replace it (mostly) with --mon_debug_idempotency