Skip to content
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 if mirror image has parent image #4941

Closed
wants to merge 1 commit into from

Conversation

yati1998
Copy link
Contributor

@yati1998 yati1998 commented Nov 4, 2024

since enable mirroring fails if it has parent image this commit adds a check for that and returns
suitable error which can be picke by VR status

@mergify mergify bot added the component/rbd Issues related to RBD label Nov 4, 2024
since enable mirroring fails if it has parent image
this commit adds a check for that and returns
suitable error which can be picke by VR status

Signed-off-by: yati1998 <[email protected]>
@iPraveenParihar
Copy link
Contributor

@yati1998, parent image check is already handled in HandleParentImageExistence()

if info.GetState() != librbd.MirrorImageEnabled.String() {
err = rbdVol.HandleParentImageExistence(ctx, flattenMode)
if err != nil {
log.ErrorLog(ctx, err.Error())
return nil, getGRPCError(err)
}
err = mirror.EnableMirroring(ctx, mirroringMode)
if err != nil {
log.ErrorLog(ctx, err.Error())
return nil, status.Error(codes.Internal, err.Error())
}
}

@yati1998
Copy link
Contributor Author

yati1998 commented Nov 5, 2024

@yati1998, parent image check is already handled in HandleParentImageExistence()

if info.GetState() != librbd.MirrorImageEnabled.String() {
err = rbdVol.HandleParentImageExistence(ctx, flattenMode)
if err != nil {
log.ErrorLog(ctx, err.Error())
return nil, getGRPCError(err)
}
err = mirror.EnableMirroring(ctx, mirroringMode)
if err != nil {
log.ErrorLog(ctx, err.Error())
return nil, status.Error(codes.Internal, err.Error())
}
}

okay, missed it, it seems we need to skip returning grpc error and instead return some meaningful error
cc @nixpanic any suggestions on how we can improvise the error?

@nixpanic
Copy link
Member

nixpanic commented Nov 5, 2024

okay, missed it, it seems we need to skip returning grpc error and instead return some meaningful error

gRPC functions should return a gRPC error witch codes.* as a status and a message. These are sent over the gRPC protocol back to the caller. The message that is part of the gRPC error should contain as many details as posssible, but user understandable. In the end, these should land in a .Status entry.

@@ -91,6 +91,10 @@ func (ri *rbdImage) EnableMirroring(_ context.Context, mode librbd.ImageMirrorMo

err = image.MirrorEnable(mode)
if err != nil {
if ri.ParentName != "" {
return fmt.Errorf("failed to enable mirroring as image %q has a parent %q", ri.ImageID, ri.ParentName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget to include the original error too

@yati1998
Copy link
Contributor Author

yati1998 commented Nov 6, 2024

I dont think this change is required as parent existence check it anyways verified.
Closing this PR

@yati1998 yati1998 closed this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/rbd Issues related to RBD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants