-
Notifications
You must be signed in to change notification settings - Fork 2
fix(vd): respect user-specified storage class when restoring from snapshot #1417
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
fix(vd): respect user-specified storage class when restoring from snapshot #1417
Conversation
Signed-off-by: Daniil Loktev <[email protected]>
Workflow has started. The target step completed with status: failure. |
Workflow has started. The target step completed with status: failure. |
Workflow has started. The target step completed with status: success. |
Signed-off-by: Daniil Loktev <[email protected]>
Workflow has started. The target step completed with status: failure. |
Workflow has started. The target step completed with status: failure. |
Workflow has started. The target step completed with status: failure. |
Workflow has started. The target step completed with status: failure. |
Workflow has started. The target step completed with status: failure. |
Reviewer's GuideThis PR updates the VirtualDisk controller to respect a user-specified storageClassName when restoring from a snapshot, adds a pre-flight compatibility check to prevent cross-provider restores, and introduces annotation constants for storage provisioner tracking. Sequence diagram for VirtualDisk restore with user-specified storageClassName and compatibility checksequenceDiagram
participant User
participant Controller
participant KubernetesAPI
participant StorageClass
participant PVC
participant VolumeSnapshot
User->>Controller: Request restore VirtualDisk from snapshot
Controller->>VolumeSnapshot: Fetch snapshot metadata
Controller->>KubernetesAPI: Fetch user-specified StorageClass
Controller->>KubernetesAPI: Fetch original PVC from snapshot
Controller->>KubernetesAPI: Fetch original storage provisioner annotation
Controller->>StorageClass: Compare provisioner with original
alt Provisioners are compatible
Controller->>PVC: Create PVC with user-specified storageClassName
Controller->>User: Success event
else Provisioners are incompatible
Controller->>User: Error event (cross-provider restore not supported)
end
Class diagram for CreatePVCFromVDSnapshotStep changesclassDiagram
class CreatePVCFromVDSnapshotStep {
+Take(ctx, vd)
+AddOriginalMetadata(vd)
+buildPVC(vd, vs)
+validateStorageClassCompatibility(ctx, vd, vdSnapshot, vs)
}
class VirtualDisk {
+Spec: PersistentVolumeClaimSpec
+Status: DiskStatus
}
class PersistentVolumeClaimSpec {
+StorageClass: string
}
class VirtualDiskSnapshot {
+Namespace: string
}
class VolumeSnapshot {
+Annotations: map[string]string
+Spec: VolumeSnapshotSpec
}
class StorageClass {
+Provisioner: string
}
class PersistentVolumeClaim {
+Annotations: map[string]string
}
CreatePVCFromVDSnapshotStep --> VirtualDisk
CreatePVCFromVDSnapshotStep --> VirtualDiskSnapshot
CreatePVCFromVDSnapshotStep --> VolumeSnapshot
CreatePVCFromVDSnapshotStep --> StorageClass
CreatePVCFromVDSnapshotStep --> PersistentVolumeClaim
Class diagram for new annotation constants in annotations.goclassDiagram
class Annotations {
+AnnStorageProvisioner: string
+AnnStorageProvisionerDeprecated: string
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- In validateStorageClassCompatibility, add a nil check for vs.Spec.Source.PersistentVolumeClaimName before dereferencing it to avoid potential panics when the snapshot source isn’t a PVC.
- Consider extracting the repeated annotation-reading logic (storageClassName, provisioner, volumeMode, etc.) into a shared helper to reduce duplication between buildPVC and compatibility validation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In validateStorageClassCompatibility, add a nil check for vs.Spec.Source.PersistentVolumeClaimName before dereferencing it to avoid potential panics when the snapshot source isn’t a PVC.
- Consider extracting the repeated annotation-reading logic (storageClassName, provisioner, volumeMode, etc.) into a shared helper to reduce duplication between buildPVC and compatibility validation.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Daniil Loktev <[email protected]>
Signed-off-by: Daniil Loktev <[email protected]>
…shot Signed-off-by: Daniil Loktev <[email protected]>
Signed-off-by: Daniil Loktev <[email protected]>
Workflow has started. The target step completed with status: failure. |
Description
Fix the VirtualDisk controller to respect user-specified storageClassName when creating disks from VirtualDiskSnapshot. Previously, the controller was always using the storage class from the original disk (stored in VolumeSnapshot annotations), completely ignoring the user-specified value in
spec.persistentVolumeClaim.storageClassName
. Also add error message when user tries to perform cross-provider restore.Why do we need it, and what problem does it solve?
When users tried to restore a VirtualDisk from a snapshot with a different storage class, the parameter
spec.persistentVolumeClaim.storageClassName
was silently ignored. The restored disk would always use the same storage class as the original disk.What is the expected result?
Checklist
Changelog entries