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

test: failure in journal.sh workunit test #9322

Merged
merged 2 commits into from May 27, 2016
Merged

Conversation

trociny
Copy link
Contributor

@trociny trociny commented May 25, 2016

No description provided.

@dillaman
Copy link

@trociny The new 'rbd journal client status' seems to overlap with 'rbd journal status'? Also, while I can see it might be a useful tool to have 'rbd journal client unregister', I have a feeling that 'rbd journal client register' could lead to some abuse / confusion. Should this command just be moved to a standalone support tool?

@dillaman dillaman changed the title Failure in journal.sh workunit test test: failure in journal.sh workunit test May 25, 2016
@dillaman dillaman added the tests label May 25, 2016
@trociny
Copy link
Contributor Author

trociny commented May 25, 2016

@dillaman Agree, these commands are better to have in a standalone support tool. Moreover, I think all rbd journal commands are better to be moved to a standalone tool because they are not supposed to be used normally (may be except rbd journal status and rbd journal info but this could be moved to rbd status and rbd info).

So, what do you think about creating a rbd-journal-tool, which would support info, status, export, import, reset, client register ... commands, and making rbd journal commands deprecated (invisible in help output and printing a warning on their use)?

Just making a tool for register/unregister does not look very useful...

@dillaman
Copy link

@trociny Another possibility is to just write a small Python script that handles this special case (see test_lock_fence.sh for an example). I think moving all the journal-related commands to a new CLI is fine, but probably not worth the effort at this point since in a perfect world it would be nice to have feature parity (where it makes sense) with cephfs-journal-tool.

@trociny
Copy link
Contributor Author

trociny commented May 25, 2016

@dillaman I think a Python script whould have worked if we had had journal API?

@dillaman
Copy link

@trociny Whoops -- totally forgot that we didn't implement an API. 😢 How about running the following before bench-write:

rados -p rbd getomapval journal.{journal id} client_ {temp file}

After bench-write you can reset the journal position:

rados -p rbd setomapval journal.{journal id} client_ < {temp file}

@dillaman
Copy link

@trociny ... or modify the cls_journal client_register method to only do that min commit position reset if the min set != 0 (i.e. we have had trimming)

Mykola Golub added 2 commits May 25, 2016 22:05
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
With the changes to ensure that the commit position of a new
client is initialized to the minimum position of other clients,
the 'journal inspect/export' commands return zero records because
the master client has committed all of its entries.

Workaround this by restoring the initial commit position after
writing to the image.

Fixes: http://tracker.ceph.com/issues/16011
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
@trociny
Copy link
Contributor Author

trociny commented May 25, 2016

@dillaman I changed it to store/restore commit position as you proposed.

Still I think for the future it will be useful to have a support tool to register/unregister client. Then it could be used in journal.sh too to make the test more valuable.

Also, I left the patch " treat empty commit position as minimal" in the PR. The problem was observed when a new (non-master) client was registered on a virgin image, so it had empty commit position. When later the master position was updated, it was incorrectly used as minimal.

@dillaman
Copy link

lgtm

@dillaman dillaman merged commit 27a92ce into ceph:master May 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants