Skip to content

rbd: add one depth for softlimit of snapshot for restore PVC #5217

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 14, 2025
Merged

rbd: add one depth for softlimit of snapshot for restore PVC #5217

merged 1 commit into from
Mar 14, 2025

Conversation

@Rakshith-R
Copy link
Contributor

@Rakshith-R Rakshith-R commented Mar 13, 2025 •
edited
Loading

Describe what this PR does

Currently, while preparing a volume for snapshot,
the depthToAvoidFlatten is set to 2. This accounts one for snapshot and another since parent of the volume is being flattened.
This commit modifies the depth to 3 to also account for future PVC restore since

  • snapshot alone is useless and it is very likely to be restored at one point in time.
  • this ensures snapshot is not flattened when restore does occur.
  • flattening of snapshot in the above case will make the snapshot no longer eligible for changed block tracking(snap diff) operation.
  • maintain similarity with PVC-PVC clone operation which currently depthToAvoidFlatten set to 3.

ceph-csi/internal/rbd/controllerserver.go

Lines 480 to 493 in a81dc2c

if rbdVol != nil {
// choosing 3, since cloning image creates a temp clone and a final clone which
// will add a total depth of 2 and the parent image itself adds one depth.
const depthToAvoidFlatten = 3
if rbdHardMaxCloneDepth > depthToAvoidFlatten {
hardLimit = rbdHardMaxCloneDepth - depthToAvoidFlatten
}
if rbdSoftMaxCloneDepth > depthToAvoidFlatten {
softLimit = rbdSoftMaxCloneDepth - depthToAvoidFlatten
}
err := rbdVol.flattenParent(ctx, hardLimit, softLimit)
if err != nil {
return getGRPCErrorForCreateVolume(err)
}

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes
    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
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 unrelated
    failure (please report the failure too!)

@Rakshith-R
Copy link
Contributor Author

/test ci/centos/mini-e2e/k8s-1.31

@Rakshith-R
Copy link
Contributor Author

/test ci/centos/mini-e2e-helm/k8s-1.31

@mergify mergify bot added the component/rbd Issues related to RBD label Mar 13, 2025
@Rakshith-R Rakshith-R requested review from a team, Madhu-1 and nixpanic March 13, 2025 11:24
@Rakshith-R Rakshith-R marked this pull request as ready for review March 13, 2025 11:24
@iPraveenParihar
Copy link
Contributor

@Mergifyio rebase

@Rakshith-R @iPraveenParihar
rbd: add one depth for softlimit of snapshot for restore PVC
91ba365 
Currently, while preparing a volume for snapshot,
the depthToAvoidFlatten is set to 2. This accounts one
for snapshot and another since parent of the volume is
flattened.
This commit modifies the depth to 3 to also account for
future PVC restore since
- snapshot alone is useless and it is very likely to be restore
  at one point in time.
- this ensures snapshot is not flattened when restore does occur.
- flattening of snapshot in the above case will make the snapshot
  no longer eligible for changed block tracking(snap diff)
  operation.
- maintain similarity with PVC-PVC clone operation which currently
  depthToAvoidFlatten set to 1.

Signed-off-by: Rakshith R <[email protected]>
@iPraveenParihar iPraveenParihar force-pushed the modify-depth-for-snapshot branch from 564450b to 91ba365 Compare March 14, 2025 12:18
@mergify
Copy link
Contributor

mergify bot commented Mar 14, 2025

rebase

✅ Branch has been successfully rebased

@iPraveenParihar
Copy link
Contributor

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Mar 14, 2025 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Mar 14, 2025
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.32

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.32

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.32

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Mar 14, 2025
@mergify
Copy link
Contributor

mergify bot commented Mar 14, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You may have to fix your CI before adding the pull request to the queue again.

If you want to requeue this pull request, you can post a @mergifyio requeue comment.

@Rakshith-R
Copy link
Contributor Author

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You may have to fix your CI before adding the pull request to the queue again.

If you want to requeue this pull request, you can post a @mergifyio requeue comment.

Job seemed to be interrupted for some reason, it did not fail

@Rakshith-R
Copy link
Contributor Author

/test ci/centos/mini-e2e/k8s-1.31

@Rakshith-R
Copy link
Contributor Author

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Mar 14, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mergify mergify bot merged commit 6f80258 into ceph:devel Mar 14, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@iPraveenParihar iPraveenParihar iPraveenParihar approved these changes

@nixpanic nixpanic Awaiting requested review from nixpanic

Assignees

No one assigned

Labels

component/rbd Issues related to RBD

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Rakshith-R @iPraveenParihar @ceph-csi-bot @Madhu-1