Conversation
CreateSnapshotResponse and ListSnapshots entries never set SizeBytes, so VolumeSnapshot.status.restoreSize was always 0. Kubernetes uses this value to show the minimum restore size and to reject PVCs that are too small. Use the snapshot virtualsize from CloudStack, falling back to the source volume size. Also populate Size/CreatedAt in GetSnapshotByID and Size in GetSnapshotByName so callers see the real snapshot size.
CloudStack creates a volume from a snapshot at the snapshot's size and ignores a larger requested size. The driver returned that smaller capacity, so external-provisioner rejected it with 'created volume capacity X less than requested capacity Y', deleted the volume and retried forever. After creating the volume, resize it to the requested size when it is smaller. If the resize fails, delete the undersized volume and return an error. The node plugin already grows the filesystem on NodeStage.
CreateSnapshot always called CloudStack createSnapshot, so a retry from csi-snapshotter (e.g. after a timeout) created a duplicate snapshot. The CSI spec requires CreateSnapshot to return the existing snapshot when one with the same name and source volume already exists. Look up the snapshot by name first; return it if it belongs to the same volume, return AlreadyExists if it belongs to a different volume, and only create a new snapshot when none exists.
… state A timed-out CreateSnapshot keeps running against CloudStack, so a retry could miss the in-progress snapshot and create a duplicate. Hold a per-name lock for the call and return Aborted to concurrent calls. CreateSnapshot and ListSnapshots always returned ReadyToUse: true, even while CloudStack was still creating or backing up the snapshot. Report ReadyToUse=false until the snapshot leaves Allocated/Creating/ CreatedOnPrimary/BackingUp/Copying, and an error for the Error state.
If a snapshot was destroyed outside the driver, or an earlier delete completed after the RPC timed out, CloudStack returns errorcode 431 '... is already destroyed'. That was not mapped to ErrNotFound, so DeleteSnapshot returned Internal and the VolumeSnapshotContent (and its VolumeSnapshot) could never be deleted. The CSI spec requires DeleteSnapshot to succeed when the snapshot no longer exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #37, Fixes #38, Fixes #39, Fixes #40, Fixes #41
Description of changes:
Each change is a separate commit.
Report snapshot size (#37). CreateSnapshot and ListSnapshots set SizeBytes, using CloudStack's virtualsize and falling back to the source volume's size. GetSnapshotByID and GetSnapshotByName now populate Size (and GetSnapshotByID populates CreatedAt).
Resize restored volumes to the requested size (#38). After CreateVolumeFromSnapshot, a volume smaller than requested is grown with ExpandVolume. If the resize fails, the undersized volume is deleted so the provisioner's retry starts clean.
Make CreateSnapshot idempotent (#39). The snapshot is looked up by name first:
if it exists for the same volume, it is returned;
if it exists for a different volume, the call returns AlreadyExists;
otherwise a new snapshot is created.
The lookup uses the client's default options, so it respects the project ID.
Move the restore path into createVolumeFromSnapshot. No behaviour change; this keeps CreateVolume within the nestif limit, and the now-unused //nolint:gocognit is removed.
Serialize CreateSnapshot per name and report ReadyToUse from the snapshot state (#39, #40).
A per-name lock returns Aborted to a concurrent call while the first is still running against CloudStack, so a retry can't race it.
cloud.Snapshot gains a State field. ReadyToUse is false while the snapshot is in progress, and a snapshot in Error state is returned as an error.
Treat already-destroyed snapshots as deleted (#41). DeleteSnapshot maps "is already destroyed" to not-found, so it succeeds idempotently.
New unit tests:
TestCreateSnapshotReportsSize
TestGrowVolumeFromSnapshot (resize, no-op and failure paths)
TestCreateSnapshotIsIdempotent
TestIsSnapshotReady
TestIsSnapshotGoneError
Testing performed:
golangci-lint run ./... (v1.63.4, repo config), make test and make test-sanity all pass.
Lab: CloudStack 4.23(KVM, NFS), a CKS cluster on Kubernetes 1.37.0. I ran the same end-to-end script against the stock image and against this branch
Additional stress test with csi-snapshotter --timeout=5s:
Stock: 6 duplicate snapshots, and the VolumeSnapshot never became ready.
This PR: one snapshot, READYTOUSE false until CloudStack showed BackedUp, and retries returned the existing snapshot.
If you want a bash script to run against fixed and main releases I can upload it.