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: librbd features test case should only disable active features #10713

Merged
merged 1 commit into from Nov 2, 2016

Conversation

dillaman
Copy link

Fixes: http://tracker.ceph.com/issues/16898
Signed-off-by: Jason Dillaman dillaman@redhat.com

@dillaman dillaman added this to the infernalis milestone Aug 12, 2016
@trociny trociny assigned trociny and unassigned trociny Aug 14, 2016
@trociny trociny self-assigned this Sep 13, 2016
trociny pushed a commit to ceph/ceph-qa-suite that referenced this pull request Sep 13, 2016
trociny pushed a commit to ceph/ceph-qa-suite that referenced this pull request Sep 13, 2016
trociny pushed a commit to ceph/ceph-qa-suite that referenced this pull request Sep 13, 2016
trociny pushed a commit to ceph/ceph-qa-suite that referenced this pull request Sep 13, 2016
trociny pushed a commit to ceph/ceph-qa-suite that referenced this pull request Sep 13, 2016
@trociny
Copy link
Contributor

trociny commented Sep 13, 2016

@dillaman Testing this on teuthology, unfortunately it does not work:

http://qa-proxy.ceph.com/teuthology/trociny-2016-09-13_13:47:46-upgrade-jewel---basic-vps/413558/teuthology.log

I have addded some debugging and increased debug rbd to have more details:

2016-09-13T14:11:10.263 INFO:tasks.workunit.client.0.vpm069.stdout:[ RUN      ] TestLibRBD.UpdateFeatures
2016-09-13T14:11:10.264 INFO:tasks.workunit.client.0.vpm069.stdout:using new format!
2016-09-13T14:11:10.330 INFO:tasks.workunit.client.0.vpm069.stdout:XXXMG: features=13
2016-09-13T14:11:10.330 INFO:tasks.workunit.client.0.vpm069.stdout:XXXMG: disable_features=12
2016-09-13T14:11:10.330 INFO:tasks.workunit.client.0.vpm069.stdout:XXXMG: image.update_features(12, false)
2016-09-13T14:11:10.357 INFO:tasks.workunit.client.0.vpm069.stdout:test/librbd/test_librbd.cc:3036: Failure
2016-09-13T14:11:10.357 INFO:tasks.workunit.client.0.vpm069.stdout:Value of: image.update_features(disable_features, false)
2016-09-13T14:11:10.357 INFO:tasks.workunit.client.0.vpm069.stdout:  Actual: -22
2016-09-13T14:11:10.358 INFO:tasks.workunit.client.0.vpm069.stdout:Expected: 0
2016-09-13T14:11:10.379 INFO:tasks.workunit.client.0.vpm069.stdout:[  FAILED  ] TestLibRBD.UpdateFeatures (116 ms)

So, it failed for disable_features=12. In client logs, there is 'update requires at least one feature' error but this was from the previous test (expected result), while the error for this failed case is generated by cls_client::set_features:

2016-09-13 14:11:10.356179 7f83dde21840 10 librbd: update_features: features=1, mask=76
2016-09-13 14:11:10.358015 7f83dde21840 -1 librbd: failed to update features: (22) Invalid argument

I suppose because jewel library adds RBD_FEATURE_JOURNALING to features_mask, which is unknown on infernalis.

@dillaman
Copy link
Author

@trociny Definitely related to the mask -- any idea why we didn't see this before? The "cls_rbd" set_features function should mask the incoming mask with its version of RBD_FEATURES_ALL at the start to avoid this issue.

@dillaman
Copy link
Author

@trociny Whoops -- didn't look close enough. I thought this was your dynamic features PR. Thanks!

@trociny
Copy link
Contributor

trociny commented Sep 13, 2016

@dillaman I don't have a good idea why we didn't see this before -- not sure when it was noticed the first time. I see the qa test was added in Nov 2015, and then it worked because the default features did not include exclusive lock. In Feb 2016 the default features was changed, and there were also related changes in update_features, so I expect it should have started to fail since then.

@trociny trociny removed their assignment Sep 16, 2016
@dillaman
Copy link
Author

Requires PR #11155 to be merged to the jewel branch

@trociny trociny self-assigned this Sep 21, 2016
@trociny
Copy link
Contributor

trociny commented Nov 2, 2016

@trociny trociny merged commit baf17c9 into ceph:infernalis Nov 2, 2016
@dillaman dillaman deleted the wip-16898-infernalis branch November 2, 2016 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants