-
Notifications
You must be signed in to change notification settings - Fork 147
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
[ceph][luks] Fix ceph cephvolumescan for cephadm #1180
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp from PR#42, use It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported,
[Deprecated] To launch on-demand regression testing public members of oamg organization can leave the following comment:
Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra. |
It looks good to me. |
repos/system_upgrade/common/actors/cephvolumescan/libraries/cephvolumescan.py
Outdated
Show resolved
Hide resolved
Please make our linter happy, added a suggestion in the comment. |
186cb35
to
b9caf27
Compare
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.
LGTM. One thing - given that the behavior has changed and this actor already has unit tests - could you please consider adding a unit test?
@fernflower Note this PR is bounded to RHEL ticket, so it cannot be merged before the ticket is approved for the release. |
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.
@xbezdick seems good to me. but I am rather setting req_changes to ensure the PR is not merged before it's approved to get to RHEL.
repos/system_upgrade/common/actors/cephvolumescan/libraries/cephvolumescan.py
Outdated
Show resolved
Hide resolved
LGTM |
/packit build |
I am not able to reopen it. for some reason, I cannot see here the pushed commit and seems I have problem to push here from a different branch-name when master already exists on my system too. so moving to another pr instead of playing around.. #1230 |
For cephadm the containers are named ceph--osd... while ceph-ansible still uses the ceph-osd-...
Other issue is that OSDs can have multiple volumes in them so filtering just for the first one is wrong and we need to check each volume for the encryption.
Resolves: rhbz#2264543
jira: RHEL-25838