-
Notifications
You must be signed in to change notification settings - Fork 554
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
support nbd on euler or arm #4291
Conversation
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions. |
I think this looks reasonable. Could you update the title of the commit to
so that commitlint will accept it? You'll need to force-push that change to your branch that you used to create this PR from. Thanks! |
isUnmap := CheckSliceContains(cmdArgs, "unmap") | ||
if !isUnmap { | ||
if !strings.Contains(userOptions, useNbdNetlink) { | ||
cmdArgs = append(cmdArgs, "--options", useNbdNetlink) | ||
cmdArgs = append(cmdArgs, "--"+useNbdNetlink) |
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.
My main concern with this patch is that the --options
parameter seems to be dropped. Is that intentional, and does it keep working with the currently maintained Ceph versions?
Inspired by this PR, I've created #4364 which will cause a little conflict, sorry! |
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
1 similar comment
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
/test ci/centos/mini-e2e/k8s-1.29 |
This passed, so I think all still works. Ideally it is more efficient now too. @pkalever can you have a quick look and check if it doesn't break anything? |
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.
CI job passed, so functionality should be OK.
@Mergifyio rebase |
✅ Branch has been successfully rebased |
@Mergifyio queue |
🛑 The pull request has been removed from the queue
|
/test ci/centos/k8s-e2e-external-storage/1.29 |
/test ci/centos/mini-e2e-helm/k8s-1.29 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/k8s-e2e-external-storage/1.28 |
/test ci/centos/mini-e2e/k8s-1.29 |
/test ci/centos/mini-e2e-helm/k8s-1.28 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/mini-e2e/k8s-1.28 |
/test ci/centos/mini-e2e/k8s-1.27 |
@Mergifyio rebase |
Signed-off-by: muxuelan <muxuelan@cmss.chinamobile.com>
✅ Branch has been successfully rebased |
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 4f04748 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/k8s-e2e-external-storage/1.29 |
/test ci/centos/mini-e2e-helm/k8s-1.29 |
/test ci/centos/mini-e2e/k8s-1.29 |
/test ci/centos/k8s-e2e-external-storage/1.28 |
/test ci/centos/mini-e2e-helm/k8s-1.28 |
/test ci/centos/mini-e2e/k8s-1.28 |
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/mini-e2e/k8s-1.27 |
Describe what this PR does
For euler or arm,command “rbd device list --format=json --device-type=nbd” can be response slowly, often more than 5 miniutes. So we use rbd-nbd list-mapped instead.
Is there anything that requires special attention
Do you have any questions?
Is the change backward compatible?
Are there concerns around backward compatibility?
Provide any external context for the change, if any.