-
Notifications
You must be signed in to change notification settings - Fork 549
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: do not return an error when deleting a non-existing image #4885
Conversation
Deleting an image that has been already remove, should not be reported as a failure. This improves the idempotency of the `rbdImage.Delete()` function, making it easier for callers to detect real errors. Signed-off-by: Niels de Vos <[email protected]>
We need to not have this check in order to trigger ceph-csi/internal/rbd/rbd_util.go Line 623 in 611e5d9
|
Sorry, I am not sure what you mean by that... |
@@ -694,7 +694,7 @@ func (ri *rbdImage) trashRemoveImage(ctx context.Context) error { | |||
_, err = ta.AddTrashRemove(admin.NewImageSpec(ri.Pool, ri.RadosNamespace, ri.ImageID)) | |||
|
|||
rbdCephMgrSupported := isCephMgrSupported(ctx, ri.ClusterID, err) | |||
if rbdCephMgrSupported && err != nil { | |||
if rbdCephMgrSupported && err != nil && !errors.Is(err, librbd.ErrNotFound) { |
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.
@nixpanic is AddTrashRemove
Mrg API is also support this error message?
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.
yes, it seemed so. I have been testing with the Ceph main branch, maybe things changed there?
If the image does not exist, then the flow should not have reached that point at all. |
I don't think it multiple processes are trying to delete the same volume at the same time. However a 2nd |
we do have in memory lock to ensure that this should not happen. @Rakshith-R any further comments on this one? |
Yea, I think We'll encounter this error only when another process puts the image to trash while this process reaches the We should treat such occurrences as a race condition happening somewhere in the code and fix that instead of the current error check to avoid it. (sorry, missed this comment earlier) |
@nixpanic I think we can close this pr ? |
I have not seen this issue anymore, closing. |
Deleting an image that has been already remove, should not be reported
as a failure. This improves the idempotency of the
rbdImage.Delete()
function, making it easier for callers to detect real errors.
Related issues
Found while working on #4502.
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>
: retest the<job-name>
after unrelatedfailure (please report the failure too!)