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

KEP-3476: Add Volume Group Snapshot KEP #1551

Merged
merged 19 commits into from
Feb 8, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 28 additions & 35 deletions keps/sig-storage/3476-volume-group-snapshot/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,13 @@ Note: In the following, we will use VolumeGroupSnapshot Controller to refer to t
* The controller will retrieve all volumeSnapshotHandles in the Volume Group Snapshot from the CSI CreateVolumeGroupSnapshotResponse, create VolumeSnapshotContents pointing to the volumeSnapshotHandles. Then the controller will create VolumeSnapshots pointing to the VolumeSnapshotContents.
* CreateVolumeGroupSnapshot CSI function response
* The CreateVolumeGroupSnapshot CSI function should return a list of snapshots (Snapshot message defined in CSI Spec) in its response. The VolumeGroupSnapshot controller can use the returned list of snapshots to construct corresponding individual VolumeSnapshotContents and VolumeSnapshots, wait for VolumeSnapshots and VolumeSnapshotContents to be bound, and update SnapshotList in the VolumeGroupSnapshot Status and SnapshotContentList in the VolumeGroupSnapshotContent Status.
* Individual VolumeSnapshots will be named in this format:
* snap+<the first 4 letters of VolumeGroupSnapshot name>+<PVC name>
(truncate if it exceeds the max length)
* If the exact same name already exists, append a "1" at the end. If that still exists, replace the suffix "1" with "2", and so on.

apiVersion: snapshot.storage.k8s.io/v1
```
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshot
metadata:
name: snapshot1
Expand All @@ -172,14 +176,14 @@ status:
volumeGroupSnapshotName: groupSnapshot1
```

* An admissions controller or finalizer should be added to prevent an individual snapshot from being deleted that belongs to a VolumeGroupSnapshot.
* An admissions controller or finalizer should be added to prevent an individual snapshot from being deleted that belongs to a VolumeGroupSnapshot. Note that there is a [KEP](https://github.com/kubernetes/enhancements/pull/2840/files) that is proposing the Liens feature which could potentially be used for this purpose.
* In the CSI spec, it is specified that it is required for individual snapshots to be returned along with the group snapshot.

#### Pre-provisioned VolumeGroupSnapshot

Admin can create a VolumeGroupSnapshotContent, specifying an existing VolumeGroupSnapshotHandle in the storage system and specifying a VolumeGroupSnapshot name and namespace. Then the user creates a VolumeGroupSnapshot that points to the VolumeGroupSnapshotContent name.

The controller will retrieve all volumeSnapshotHandles in the Volume Group Snapshot from the storage system, create VolumeSnapshotContents pointing to the volumeSnapshotHandles. Then the controller will create VolumeSnapshots pointing to the VolumeSnapshotContents.
The controller will call the CSI GetVolumeGroupSnapshot method to retrieve all volumeSnapshotHandles in the Volume Group Snapshot from the storage system, create VolumeSnapshotContents pointing to the volumeSnapshotHandles. Then the controller will create VolumeSnapshots pointing to the VolumeSnapshotContents.

### Delete VolumeGroupSnapshot

Expand Down Expand Up @@ -339,12 +343,19 @@ type VolumeGroupSnapshot struct {

// VolumeGroupSnapshotSpec describes the common attributes of a group snapshot
type VolumeGroupSnapshotSpec struct {
// VolumeGroupSnapshotClassName may be left nil to indicate that
// the default class will be used.
// Empty string is not allowed for this field.
// +optional
VolumeSnapshotClassName *string
VolumeGroupSnapshotClassName *string

// A label query over persistent volume claims to be grouped together
// for snapshotting.
// This labelSelector will be used to match the label added to a PVC.
// Note that if volumes are added/removed from the label after a group snapshot
// is created, the existing snapshots won't be modified.
// Once a VolumeGroupSnapshotContent is created and the sidecar starts to process it,
// the volume list will not change with retries.
Selector *metav1.LabelSelector

// VolumeGroupSnapshotSecretRef is a reference to the secret object containing
Expand All @@ -368,22 +379,12 @@ Type VolumeGroupSnapshotStatus struct {
CreationTime *metav1.Time

// +optional
Error *VolumeGroupSnapshotError

// List of volume snapshots
// +optional
SnapshotList []VolumeSnapshot
}

// Describes an error encountered on the group snapshot
type VolumeGroupSnapshotError struct {
// time is the timestamp when the error was encountered.
// +optional
Time *metav1.Time
Error *VolumeSnapshotError

// message details the encountered error
// List of volume snapshot refs
// The max number of snapshots in the group is 100
// +optional
Message *string
VolumeSnapshotRefList []core_v1.ObjectReference
Copy link
Member

Choose a reason for hiding this comment

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

Since these can only be snapshot kinds, we could make our own SnapshotReference type that only includes name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we only need snapshot names, can it just be a list of strings?

Copy link
Member

Choose a reason for hiding this comment

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

We can follow up on this during a more detailed API review, but I believe the guidance is to define a type scoped to your supported sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

}
```

Expand Down Expand Up @@ -418,6 +419,8 @@ type VolumeGroupSnapshotContentSpec struct {
// Required
Driver string

// This field may be unset for pre-provisioned snapshots.
// For dynamic provisioning, this field must be set.
// +optional
VolumeGroupSnapshotClassName *string

Expand All @@ -427,7 +430,7 @@ type VolumeGroupSnapshotContentSpec struct {

// OneOf
type VolumeGroupSnapshotContentSource struct {
// Dynamical provisioning of VolumeGroupSnapshot
// Dynamic provisioning of VolumeGroupSnapshot
// A list of PersistentVolume names to be snapshotted together
// +optional
PersistentVolumeNames []string
Expand All @@ -453,11 +456,12 @@ Type VolumeGroupSnapshotContentStatus struct {
CreationTime *int64

// +optional
Error *VolumeGroupSnapshotError
Error *VolumeSnapshotError

// List of volume group snapshot contents
// List of volume snapshot content refs
// The max number of snapshots in a group is 100
// +optional
VolumeSnapshotContentList []VolumeSnapshotContent
VolumeSnapshotContentRefList []core_v1.ObjectReference
}
```

Expand Down Expand Up @@ -1460,23 +1464,12 @@ Type VolumeGroupSnapshotStatus struct {
CreationTime *metav1.Time

// +optional
Error *VolumeGroupSnapshotError
Error *VolumeSnapshotError

// List of volume snapshots
// +optional
SnapshotList []VolumeSnapshot
}

// Describes an error encountered on the group snapshot
type VolumeGroupSnapshotError struct {
// time is the timestamp when the error was encountered.
// +optional
Time *metav1.Time

// message details the encountered error
// +optional
Message *string
}
```

#### VolumeGroupSnapshotContent
Expand Down Expand Up @@ -1544,7 +1537,7 @@ Type VolumeGroupSnapshotContentStatus struct {
CreationTime *int64

// +optional
Error *VolumeGroupSnapshotError
Error *VolumeSnapshotError

// List of volume group snapshot contents
// +optional
Expand Down