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.

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

rbd: add validation to ToCSI() for rbdVolume and rbdSnapshot #5151

Merged
merged 2 commits into from
Feb 20, 2025
Merged

rbd: add validation to ToCSI() for rbdVolume and rbdSnapshot #5151

merged 2 commits into from
Feb 20, 2025

Conversation

nixpanic
Copy link
Member

@nixpanic nixpanic commented Feb 17, 2025 •
edited
Loading

After an unfortnate timed restart of the provisioner, a volume that got
cloned did not get a rbdVolume.VolID set. The .VolID is used as the
CSI Volume Handle, and is a required attribute.

The rbdVolume and rbdSnapshot structs have a .ToCSI() function
that can do the validation of required attributes. This is now added,
including unit-tests.

@nixpanic nixpanic requested a review from a team February 17, 2025 15:05
@mergify mergify bot added the component/rbd Issues related to RBD label Feb 17, 2025
@nixpanic nixpanic requested a review from Madhu-1 February 17, 2025 15:32
Madhu-1
Madhu-1 previously approved these changes Feb 17, 2025
View reviewed changes
@nixpanic nixpanic requested a review from a team February 17, 2025 15:57
Rakshith-R
Rakshith-R requested changes Feb 17, 2025
View reviewed changes
@mergify mergify bot dismissed Madhu-1’s stale review February 18, 2025 08:27

Pull request has been modified.

@nixpanic nixpanic requested a review from a team February 18, 2025 08:33
Rakshith-R
Rakshith-R previously approved these changes Feb 18, 2025
View reviewed changes
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.

Thanks !

@nixpanic nixpanic requested a review from a team February 18, 2025 11:15
Madhu-1
Madhu-1 previously approved these changes Feb 18, 2025
View reviewed changes
@nixpanic
Copy link
Member Author

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 18, 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 Feb 18, 2025
@ceph-csi-bot
Copy link
Collaborator

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

@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.31

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@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
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

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

@Madhu-1 Madhu-1 requested a review from a team February 19, 2025 08:35
Rakshith-R
Rakshith-R approved these changes Feb 19, 2025
View reviewed changes
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.

Thanks !

internal/rbd/controllerserver.go
@@ -1248,7 +1259,11 @@ func (cs *ControllerServer) CreateSnapshot(
return nil, status.Error(codes.Internal, err.Error())
}

csiSnap, err := vol.toSnapshot().ToCSI(ctx)
// FIXME: doSnapshotClone() returns a rbdVolume, some attributes may be missing?
snap := vol.toSnapshot()
Copy link
Contributor

Choose a reason for hiding this comment

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

@Madhu-1, that can not be done, as an rbdVolume does not have a SourceVolumeID, it is only part of rbdSnapshot. Maybe doSnapshotClone() should return an rbdSnapshot, or something else in the flow needs to be adjusted. I know that this isn't very clean, hence the FIXME comment.

Can we please have an issue open for this too ?

@nixpanic nixpanic mentioned this pull request Feb 19, 2025
Open
@iPraveenParihar
Copy link
Contributor

iPraveenParihar commented Feb 20, 2025 •
edited
Loading

Thanks! @nixpanic

@iPraveenParihar
Copy link
Contributor

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 20, 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.

@nixpanic
rbd: always include the SourceVolumeID when returning a Snapshot
532dea6 
`doSnapshotClone()` returns a new `rbdVolume` object from a temporary
snapshot. This conversion drops the `SourceVolumeID` attribute, as a
`rbdVolume` does not have that.

After converting the `rbdVolume` back to a `rbdSnapshot`, the
`SourceVolumeID` attribute can be set again, and the `ToCSI()` function
can create an appropriate CSI Snapshot struct.

Signed-off-by: Niels de Vos <[email protected]>
@nixpanic
rbd: add validation to ToCSI() for rbdVolume and rbdSnapshot
140ee15 
After an unfortnate timed restart of the provisioner, a volume that got
cloned did not get a `rbdVolume.VolID` set. The `.VolID` is used as the
CSI Volume Handle, and is a required attribute.

The `rbdVolume` and `rbdSnapshot` structs have a `.ToCSI()` function
that can do the validation of required attributes. This is now added,
including unit-tests.

Signed-off-by: Niels de Vos <[email protected]>
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Feb 20, 2025
@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.32

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

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

@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/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/k8s-1.31

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

mergify bot commented Feb 20, 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 should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@nixpanic
Copy link
Member Author

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

@nixpanic
Copy link
Member Author

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

@Mergifyio requeue

Failed with:

[FAILED] backend images not matching kubernetes resource count,image count 1 kubernetes resource count 0
  backend image Info:
   [csi-vol-47dd54b3-94f8-4647-9e2a-196304d5302b]
   images information and status Pool: replicapool, Image: csi-vol-47dd54b3-94f8-4647-9e2a-196304d5302b, Info: {"name":"csi-vol-47dd54b3-94f8-4647-9e2a-196304d5302b","id":"16eb56e3021b","size":1073741824,"objects":256,"order":22,"object_size":4194304,"snapshot_count":0,"block_name_prefix":"rbd_data.16eb56e3021b","format":2,"features":["layering"],"op_features":[],"flags":[],"create_timestamp":"Thu Feb 20 07:12:17 2025","access_timestamp":"Thu Feb 20 07:12:17 2025","modify_timestamp":"Thu Feb 20 07:12:17 2025"}
  , Status: {"watchers":[]}

logs

@nixpanic
Copy link
Member Author

@Mergifyio requeue

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 20, 2025

requeue

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

@mergify mergify bot merged commit ac8cda5 into ceph:devel Feb 20, 2025
37 checks passed
@mergify mergify bot mentioned this pull request Feb 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rakshith-R Rakshith-R Rakshith-R approved these changes

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

Assignees
No one assigned
Labels
backport-to-release-v3.13 Label to backport from devel to release-v3.13 branch bug Something isn't working component/rbd Issues related to RBD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nixpanic @ceph-csi-bot @Madhu-1 @iPraveenParihar @Rakshith-R