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: include trashed parent images while calculating the clone depth #4029

Draft
wants to merge 8 commits into
base: devel
Choose a base branch
from

Conversation

nixpanic
Copy link
Member

@nixpanic nixpanic commented Aug 2, 2023

The getCloneDepth() function did not account for images that are in
the trash. A trashed image can only be opened by the image-id, and not
by name anymore.

Closes: #4013
Depends-on: #4064 #4273


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!)

@mergify mergify bot added the component/rbd Issues related to RBD label Aug 2, 2023
@nixpanic
Copy link
Member Author

nixpanic commented Aug 4, 2023

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

Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

This pr looks good to me.

internal/rbd/rbd_util.go Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Aug 8, 2023

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@nixpanic
Copy link
Member Author

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

@nixpanic
Copy link
Member Author

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

@nixpanic
Copy link
Member Author

Manual stress testing with the scripts from rook/rook#12312 passes.

Need to fix the golangci-lint issues and have the go-ceph rebase merged before this is completely ready.

//
// This function re-uses the ioctx of the image to open all images in the
// chain. There is no need to open new ioctx's for every image.
func (ri *rbdImage) getCloneDepth() (uint, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

hey @nixpanic,
Can we add another arguement like maxDepthToTraverse which can be used to exit and not traverse further unnecessarily?

We can pass hard rbdHardMaxCloneDepth+1 to the function call then.

wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, I'll add that

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a 2nd commit to address this.

@nixpanic nixpanic marked this pull request as ready for review August 24, 2023 09:52
@nixpanic nixpanic requested review from Rakshith-R and a team August 25, 2023 07:27
riya-singhal31
riya-singhal31 previously approved these changes Aug 25, 2023
Copy link
Contributor

@riya-singhal31 riya-singhal31 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Niels

@nixpanic nixpanic requested a review from a team August 29, 2023 11:57
@nixpanic
Copy link
Member Author

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 30, 2023

rebase

✅ Branch has been successfully rebased

Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM, i will let @Rakshith-R to approve it as had some suggestions.

Rakshith-R
Rakshith-R previously approved these changes Sep 4, 2023
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

@Rakshith-R
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Sep 4, 2023

refresh

✅ Pull request refreshed

@Rakshith-R
Copy link
Contributor

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Sep 4, 2023

queue

🛑 The pull request has been removed from the queue default

The queue 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 Sep 4, 2023
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@nixpanic
Copy link
Member Author

For some reason, these commits prevent setting the meta-data for a cloned image (e2e suggests). When testing manually, no RBD-images get any meta-data set at all. The OMAP for all images (including the created snapshot) does have the expected meta-data... Still trying to understand what the e2e tests expect, and why they do not fail on previous test-cases.

It seemed that my deployment did not include --setmetadata=true for the provisioner... When setting it, the volume gets the expected (I think) metadata set. Trying again in the CI, now with enhanced logging.

@nixpanic
Copy link
Member Author

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

@nixpanic
Copy link
Member Author

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

@nixpanic
Copy link
Member Author

For anyone watching this, I have now noticed that the ImageID of a rbdSnapshot is not set to the right value, it is set to the ImageID of the original parent volume. Using the ImageID to open the rbdSnapshot, actually opens the parent image.

@nixpanic
Copy link
Member Author

Dropped the commit

rbd: use librbd.OpenImageById() if rbdVol.ImageID is set

as it seems that the ImageID is not always set to the correct value, but to the ImageID of a cloned image. Without this commit, e2e should pass.

@nixpanic
Copy link
Member Author

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

@nixpanic
Copy link
Member Author

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

@nixpanic
Copy link
Member Author

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

@nixpanic
Copy link
Member Author

logs

  I1127 15:20:04.710772       1 utils.go:164] ID: 39 Req-ID: 0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 GRPC call: /csi.v1.Controller/DeleteVolume
  I1127 15:20:04.710860       1 utils.go:165] ID: 39 Req-ID: 0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 GRPC request: {"secrets":"***stripped***","volume_id":"0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6"}
  I1127 15:20:04.712116       1 omap.go:88] ID: 39 Req-ID: 0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 got omap values: (pool="replicapool", namespace="", name="csi.volume.ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6"): map[csi.imageid:1af286243815 csi.imagename:csi-vol-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 csi.volname:pvc-3d33943d-80f0-46d1-ad6e-d6165ee1b7e5 csi.volume.owner:rbd-6238]
  I1127 15:20:04.757621       1 rbd_util.go:637] ID: 39 Req-ID: 0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 rbd: delete csi-vol-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6-temp using mon rook-ceph-mon-a.rook-ceph.svc.cluster.local:6789, pool replicapool
  I1127 15:20:04.762284       1 controllerserver.go:1021] ID: 39 Req-ID: 0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 deleting image csi-vol-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6
  I1127 15:20:04.762309       1 rbd_util.go:637] ID: 39 Req-ID: 0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 rbd: delete csi-vol-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 using mon rook-ceph-mon-a.rook-ceph.svc.cluster.local:6789, pool replicapool
  E1127 15:20:04.763705       1 rbd_util.go:667] ID: 39 Req-ID: 0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 failed to delete rbd image: replicapool/csi-vol-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6, error: RBD image not found
  E1127 15:20:04.763736       1 controllerserver.go:1023] ID: 39 Req-ID: 0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 failed to delete rbd image: replicapool/csi-vol-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 with error: RBD image not found
  E1127 15:20:04.763801       1 utils.go:169] ID: 39 Req-ID: 0001-0024-65e89782-f2bd-4b30-85f4-fcad649cdded-0000000000000001-ccfe21c4-54cf-44cc-9a75-72ce1ad9adf6 GRPC error: rpc error: code = Internal desc = RBD image not found

Deleting the RBD image seems to have resulted in a (spurious?) error. The DeleteVolume CSI procedure is repeated over and over again, because the image was removed already.

@nixpanic
Copy link
Member Author

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

`librbd.OpenImageById()` works if the image is in the trash, so it makes
it possible to get the parent of the image.

Signed-off-by: Niels de Vos <[email protected]>
When a new volume is not created yet, the ImageID should not be set to
the ID of the snapshot.

Signed-off-by: Niels de Vos <[email protected]>
In some places the ImageID is used as the ID of the parent. That is very
confusing and prone to errors. Instead, fetch the right ImageID where
possible, and set ParentID for referencing to parent images.

Signed-off-by: Niels de Vos <[email protected]>
If the RBD-image is deleted already, the DeleteVolume CSI procedure is
expected to report success (as it should be idempotent). In case the
returned error indicates "RBD image not found", the error is ignored and
the DeleteVolume procedure continues.

Signed-off-by: Niels de Vos <[email protected]>
The `getCloneDepth()` function did not account for images that are in
the trash. A trashed image can only be opened by the image-id, and not
by name anymore.

Closes: ceph#4013
Signed-off-by: Niels de Vos <[email protected]>
The `getCloneDepth()` function does not need to traverse the whole chain
of parents when a certain max-limit is configured. The traversing can be
aborted once the hard-limit is reached. This makes the procedure a
little more efficient, as unnecessary traversing is prevented.

Signed-off-by: Niels de Vos <[email protected]>
By returned `ABORTED` or `PRECONDITION_FAILED` in the right places, the
CO will retry with the same arguments until the snapshot is
`ReadyToUse`. This causes restoring a volume from a snapshot to be
delayed, until the snapshot can be used.

Signed-off-by: Niels de Vos <[email protected]>
@nixpanic
Copy link
Member Author

nixpanic commented Dec 1, 2023

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

@nixpanic
Copy link
Member Author

nixpanic commented Dec 1, 2023

From the logs it looks like the flattening of images is in progress. I have not found messages that suggest something is blocked, but it is well possible that there is a deadlock on the PVC-restore and snapshot-delete somewhere (I have seen these in manual testing too, but though I addressed them).

Maybe #1883 (comment) contains more hints on things that I have missed in the current PR.

@nixpanic nixpanic added the keepalive This label can be used to disable stale bot activiity in the repo label Dec 21, 2023
Copy link
Contributor

mergify bot commented Apr 15, 2024

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

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 keepalive This label can be used to disable stale bot activiity in the repo
Projects
None yet
5 participants