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
qa: switch to use the merge fragment for fscrypt #50728
Conversation
All the fscrypt qa tests are running. |
Will use the postmerge fragment to check this. Fixes: https://tracker.ceph.com/issues/59195 Signed-off-by: Xiubo Li <xiubli@redhat.com>
Fixes: https://tracker.ceph.com/issues/59195 Signed-off-by: Xiubo Li <xiubli@redhat.com>
Currently only the upstream kclient supports fscrypt feature. Fixes: https://tracker.ceph.com/issues/59195 Signed-off-by: Xiubo Li <xiubli@redhat.com>
Only updated the commit comments. |
# Once can we make sure the distro kernels have included the fscrypt feature | ||
# or the ceph-fuse have supported the fscrypt feature we can remove this | ||
# restriction. |
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.
we will have to keep track on this right whether and when this feature is added to distro kernels and/or ceph-fuse?
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 added this comment just as Patrick mentioned in the previous PR, just to make it clear why we add this restriction here and in which case we can remove it. But couldn't foresee when.
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.
right, i mean once the distro kernels/ceph-fuse are capable of fscrypt then we need to change it here and also here https://github.com/ceph/ceph/pull/48183/files#diff-63be9c532a24d81b6391bb01e3d994eef8eb6a948154c227164063e6acd592f7R3-R7. So it would be better that we note it down somewhere, or better way is to link these two in the fscrypt feature PR
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 linked this to the fscrypt feature PR #50728. Thanks.
there's one job running close to 5 hours - https://pulpito.ceph.com/xiubli-2023-03-29_05:17:18-fs:fscrypt-wip-lxb-20230316-2212-unlink-revert-distro-default-smithi/7224905/, I see a lot of failures in generic/XXX that are not visible in other jobs. |
Yeah, I have checked it. It's because the
|
I ran the qa tests again. This time the
This is because in
This is a known bug, and the fixing patch has been in the |
This is the same issue with #50728 (comment). |
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. I'll this a second look and approve if after some time.
okay so once your fix has been merged, it should go fine right |
Yeah, once that fix is applied to the |
Or if the job is scheduled with the |
okay but when running full suite, it will show failures for |
[...]
This failure is not related to this PR. |
right, i was just concerned if merging patch that fail even if not related should be done straightway(if the code looks good) or wait until the unrelated issue is patched, but anyways changes look good to me |
@lxbsz This is ready for QA, should I pick it up? |
Sure, please. Thanks. |
@rishabh-d-dave BTW, What's the status for the tests ? The |
jenkins test make check arm64 |
I will run all the relevant tests myself today. |
I initiated testing a couple of times but there were lots of infra-related failures in both runs. I'll initiate it a third time today. |
Sure, thanks. |
I have run the and passed: https://pulpito.ceph.com/xiubli-2023-06-09_04:16:04-fs:functional-wip-lxb-fscrypt-20230607-0901-distro-default-smithi/ If no objection I will merge this and the kclient fscrypt patches need to run these tests, we need to push the kclient fscrypt patches further. Thanks |
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
Talked with @rishabh-d-dave, this is fine with him. Since we need to push the fscrypt patches in kclient and the tests passed. Merge it. Thanks! |
PR #48183 have add the merge fragment support. This will switch to use fragment to simplify the code.
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows