-
Notifications
You must be signed in to change notification settings - Fork 32
qa/iscsi_test: test also user:rbd (tcmu-runner) backstore #699
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
&& falsehere? I realise that construct is also present in a couple other places in_iscsi_test(), but looking at it I feel like that should cause those lines to just always fail and abort the script. What am I missing?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed response, my sesdev github notifications seem to be filtered-out some how, will need to check the filters.
Yes, the idea to check that it is not present in
rbd showmappedoutput. We do this several times, e.g. at the beginning of the test to check the state clean and at the end of the test. But in this particular case (backstore=user:rbd) we check that is not in the list, becauserbd mapis used only for 'rbd' (kernel) backstore. And this check is to test that the 'backstore' setting is not ignored and is actually applied, and the default 'rbd' backstore is not used. It is not strictly necessary, because it would fail on the next check anyway (where we test that is is indeed tcmu), on the other hand the cause would not be so clear. Anyway, it is not mandatory and may safely remove it if you think it is confusing or unnecessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, actually now I understand what you mean. It looks like it should fail for both case (i.e. if the image is present in output or not). Hm, interesting why the test did not fail for me. I will need to recheck this
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked, and it works as it was intended, i.e. if there is no the image in the list (
grepfails) it does not abort and proceeds. And if the image is in the list (grepsucceeds) then it aborts due to&& false. Although if you run it from the command line and check$?it is 1 in both cases. I suppose it is a peculiarity of howset -eworks that I am not aware of. I just saw this idiom in ceph tests many times and used it here not thinking much about it, so for me it did not look confusing, but if you think it is we may change it to something more clear.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the man bash explains this, and I remember I read it in the past (probably several times) but forgot after some time had passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looking at http://see.prv.suse.net:8080/blue/organizations/jenkins/sesdev-integration/detail/PR-699/1/pipeline, that CI test seems to have failed because:
So with
backstore=user:rbdthe volume is (incorrectly) mapped in this CI test. Note that the CI does a single node deployment on openSUSE 15.3, in case that makes any difference. I'll try to reproduce locally myself and poke around a bit...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, interesting. I will try to reproduce it with one node scenario, although I can't imaging how it could be related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it is failing for openSUSE. I think it is more important here. Right now I have no idea how iscsi support is built for openSUSE, if it has necessary bits. I will investigate. As the last resort we may enable this test for SES only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm... openSUSE 15.3 is EOL since the end of December 2022. Under the circumstances, I wonder if we should maybe switch the CI to deploying ses7p on SLE, rather than pacific on openSUSE? I've opened #702 to see if that might work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried
sesdev pacificdeploy and reproduced the issue. The backstore=user:rbd fails because of this tcmu-runner error:And it is because tcmu-runner is built without rbd support here:
There is no handler_rbd.so here.
I will modify the test to test backstore=user:rbd only if it is running on ses deployment.