-
Notifications
You must be signed in to change notification settings - Fork 547
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
rbd: check volume details from original volumeID #2931
Conversation
/retest all |
something wrong with CI restarting all tests |
hmmm.. just wondering how it react to migrated volume handle. |
@humblec what is the concern here? |
/test all |
looking some more into this, ideally we should be good, that said the static volume ID is untouched and should folow the code path as before, for new volume Handles only the change in effect . @Madhu-1 can you please double confirm ? |
Yes this is not for static volume its for dynamic volume already getting called in DeleteVolume Code ceph-csi/internal/rbd/controllerserver.go Lines 762 to 766 in c3e35f8
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
/retest ci/centos/mini-e2e-helm/k8s-1.23 |
/retest ci/centos/mini-e2e/k8s-1.21 |
/retest ci/centos/mini-e2e/k8s-1.23 |
@Mergifyio rebase |
/test ci/centos/mini-e2e/k8s-1.21 |
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
/retest ci/centos/mini-e2e/k8s-1.21 |
@Mergifyio rebase |
✅ Branch has been successfully rebased |
@Mergifyio rebase |
✅ Branch has been successfully rebased |
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
Pull request has been modified.
/retest ci/centos/mini-e2e/k8s-1.25 |
internal/rbd/nodeserver.go
Outdated
return nil, status.Error(codes.Internal, err.Error()) | ||
} | ||
|
||
rv := &rbdVolume{} |
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.
Nit: This works, but creating an empty object before overwriting it is a little ugly.
I prefer to use this instead:
var rv *rbdVolume
var vi util.CSIIdentifier | ||
var imageAttributes *journal.ImageAttributes | ||
err = vi.DecomposeCSIID(volID) | ||
rv, err = GenVolFromVolID(ctx, volID, cr, req.GetSecrets()) |
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.
thanks!
Pull request has been modified.
@Mergifyio rebase |
Checking volume details for the existing volumeID first. if details like OMAP, RBD Image, Pool doesnot exists try to use clusterIDMapping to look for the correct informations. fixes: ceph#2929 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
✅ Branch has been successfully rebased |
/test ci/centos/k8s-e2e-external-storage/1.23 |
/test ci/centos/k8s-e2e-external-storage/1.24 |
/test ci/centos/k8s-e2e-external-storage/1.25 |
/test ci/centos/mini-e2e-helm/k8s-1.23 |
/test ci/centos/mini-e2e-helm/k8s-1.24 |
/test ci/centos/mini-e2e-helm/k8s-1.25 |
/test ci/centos/mini-e2e/k8s-1.23 |
/test ci/centos/mini-e2e/k8s-1.24 |
/test ci/centos/mini-e2e/k8s-1.25 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/upgrade-tests-rbd |
Checking volume details for the existing volumeID first. if details like OMAP, RBD Image, Pool does not exist try to use clusterIDMapping to look for the correct pieces of information.
fixes: #2929
Signed-off-by: Madhu Rajanna madhupr007@gmail.com
Facing some CI issue and looks like its due to the rbd issue https://tracker.ceph.com/issues/54593