-
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: add support for flattenMode option for replication #4678
Conversation
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
0839781
to
5588b73
Compare
I've added testing results in pr description. |
@Rakshith-R, I don't see the testing results in the PR description. can you re-check if you have missed it? |
forgot to press update button. |
return corerbd.FlattenModeNever | ||
} | ||
|
||
switch corerbd.FlattenMode(val) { |
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.
switch corerbd.FlattenMode(val) { | |
mode := corerbd.FlattenMode(val); | |
switch mode { | |
case corerbd.FlattenModeForce, corerbd.FlattenModeNever: | |
return mode,nil | |
default: | |
log.DebugLog(ctx, "flattenMode %v not supported",mode) | |
return mode,err | |
} |
lets return error in case of its a user mistake instead of choosing FlattenModeNever
if err != nil { | ||
return nil, err | ||
} |
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.
this check seems to be wrong?
internal/rbd/mirror.go
Outdated
const ( | ||
// FlattenModeNever indicates that the image should never be flattened. | ||
FlattenModeNever FlattenMode = "never" | ||
// FlattenModeForce indicates that the image with parent should definitely |
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.
FlattenModeForce indicates that the image with the parent must be flattened
internal/rbd/mirror.go
Outdated
// if the flattenMode is FlattenModeForce, it flattens the image itself. | ||
// if the parent image is in trash, it returns an error. | ||
// if the parent image exists and is not enabled for mirroring, it returns an error. | ||
func (rv *rbdVolume) HandleParentImage( |
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.
should we rename this to MirrorImageIfParentExists or something else as we are not handling the ParentImage rather we are flattening of the image only if the parent exists ?
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.
HandleParentImage can also be read as handling the cases where parent image may exist and the method to handle such parent depends.
Currently we either error out or flatten the live image.
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 are acting/handling on the actual image not on the parent, IMO we don't handle anything in parentImage we are having few checks on it, the main idea is to flatten the main/live image. For me, it make sense to have something like MirrorImageIfParentExists
and HandleParentImage
looks to be generic and doesn't mention what exactly we are handing here. please feel free to rename to anything else that gives a sense of what we are doing in this function.
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.
Renamed it to HandleParentImageExistence
to imply this function contains logic to handle existence of a parent
image
} | ||
|
||
if flattenMode == FlattenModeForce { | ||
err := rv.DeleteTempImage(ctx) |
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.
please add a comment why we are deleting the temporary clone here
internal/rbd/mirror.go
Outdated
"failed to get mirroring info of parent %q of image %q: %w", | ||
rv, parent, err) |
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.
"failed to get mirroring info of parent %q of image %q: %w", | |
rv, parent, err) | |
"failed to get mirroring info of parent %q of image %q: %w", | |
parent, rv, err) |
internal/rbd/mirror.go
Outdated
rv, parent, err) | ||
} | ||
|
||
if parentMirroringInfo.State == librbd.MirrorImageEnabled { |
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.
if parentMirroringInfo.State == librbd.MirrorImageEnabled { | |
if parentMirroringInfo.State != librbd.MirrorImageEnabled { | |
fmt.Errorf("%w:failed to enable mirroring on image %q: "+ | |
"parent image %q is not enabled for mirroring", | |
ErrFailedPrecondition, rv, parent) | |
} | |
return nil |
internal/rbd/rbd_util.go
Outdated
@@ -703,6 +703,27 @@ func (ri *rbdImage) trashRemoveImage(ctx context.Context) error { | |||
return nil | |||
} | |||
|
|||
// DeleteTempImage deletes the temporary image created for pvc-pvc clone. |
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.
// DeleteTempImage deletes the temporary image created for pvc-pvc clone. | |
// DeleteTempImage deletes the temporary image created for volume datasource. |
internal/rbd/rbd_util.go
Outdated
log.ErrorLog(ctx, "failed to delete rbd image: %s with error: %v", | ||
tempClone, err) |
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.
lets not log here, dont pass the ctx and log it in caller if there is any error?
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.
lets not log here, dont pass the ctx and log it in caller if there is any error?
ctx is definitely required for other function calls in this function.
I'll move the log upwards.
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.
it looks like its not addressed, can you please check this one again
e582ff7
to
6c74635
Compare
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.
small nits, LGTM
case corerbd.FlattenModeForce, corerbd.FlattenModeNever: | ||
return mode, nil | ||
} | ||
log.DebugLog(ctx, "%q=%q is not supported", flattenModeKey, val) |
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.
lets log it as in error message in caller not as debug here
internal/rbd/mirror.go
Outdated
// if the flattenMode is FlattenModeForce, it flattens the image itself. | ||
// if the parent image is in trash, it returns an error. | ||
// if the parent image exists and is not enabled for mirroring, it returns an error. | ||
func (rv *rbdVolume) HandleParentImage( |
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 are acting/handling on the actual image not on the parent, IMO we don't handle anything in parentImage we are having few checks on it, the main idea is to flatten the main/live image. For me, it make sense to have something like MirrorImageIfParentExists
and HandleParentImage
looks to be generic and doesn't mention what exactly we are handing here. please feel free to rename to anything else that gives a sense of what we are doing in this function.
internal/rbd/mirror.go
Outdated
log.ErrorLog(ctx, "failed to delete temporary rbd image: %v", err) | ||
|
||
return err |
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.
log.ErrorLog(ctx, "failed to delete temporary rbd image: %v", err) | |
return err | |
return fmt.Errorf("failed to delete temporary rbd image: %w", err) |
internal/rbd/rbd_util.go
Outdated
err = tempClone.ensureImageCleanup(ctx) | ||
|
||
return err |
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.
err = tempClone.ensureImageCleanup(ctx) | |
return err | |
return tempClone.ensureImageCleanup(ctx) |
internal/rbd/rbd_util.go
Outdated
log.ErrorLog(ctx, "failed to delete rbd image: %s with error: %v", | ||
tempClone, err) |
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.
it looks like its not addressed, can you please check this one again
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
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at d166229 |
This commit adds support for flattenMode option for replication. If the flattenMode is set to "force" in volumereplicationclass parameters, cephcsi will add a task to flatten the image if it has parent. This enable cephcsi to then mirror such images after flattening them. The error message when the image's parent is in trash or unmirrored is improved as well. Signed-off-by: Rakshith R <[email protected]>
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/k8s-e2e-external-storage/1.28 |
/test ci/centos/k8s-e2e-external-storage/1.29 |
/test ci/centos/mini-e2e-helm/k8s-1.28 |
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/mini-e2e-helm/k8s-1.29 |
/test ci/centos/mini-e2e/k8s-1.28 |
/test ci/centos/k8s-e2e-external-storage/1.30 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/mini-e2e/k8s-1.29 |
/test ci/centos/mini-e2e/k8s-1.27 |
/test ci/centos/mini-e2e-helm/k8s-1.30 |
/test ci/centos/mini-e2e/k8s-1.30 |
This commit adds support for flattenMode option
for replication.
If the flattenMode is set to "force" in
volumereplicationclass parameters, cephcsi will
add a task to flatten the image if it has parent.
This enable cephcsi to then mirror such images after flattening them.
The error message when the image's parent is
in trash or unmirrored is improved as well.
Checklist:
guidelines in the developer
guide.
Request
notes
updated with breaking and/or notable changes for the next major release.
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!)
test results
Restored PVC
PVC-PVC clone