diff --git a/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml b/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml index cf407516d..2c9a17e12 100644 --- a/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml +++ b/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml @@ -42,7 +42,7 @@ spec: type: string driver: description: driver is the name of the storage driver that handles this - VolumeSnapshotClass. + VolumeSnapshotClass. Required. type: string kind: description: 'Kind is a string value representing the REST resource this diff --git a/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml b/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml index 8033a350a..edbb69ef4 100644 --- a/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml +++ b/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml @@ -35,7 +35,7 @@ spec: type: object spec: description: spec defines properties of a VolumeSnapshotContent created - by the underlying storage system. + by the underlying storage system. Required. properties: deletionPolicy: allOf: @@ -68,15 +68,17 @@ spec: type: string source: description: source specifies from where a snapshot will be created. - Required. + This field is immutable after creation. Required. properties: snapshotHandle: description: snapshotHandle specifies the CSI name of a pre-existing - snapshot on the underlying storage system. + snapshot on the underlying storage system. This field is immutable + once specified. type: string volumeHandle: description: volumeHandle specifies the CSI name of the volume from - which a snapshot should be dynamically taken from. + which a snapshot should be dynamically taken from. This field + is immutable once specified. type: string type: object volumeSnapshotRef: @@ -85,7 +87,7 @@ spec: field must reference to this VolumeSnapshotContent's name for the bidirectional binding to be valid. For a pre-existing VolumeSnapshotContent object, name and namespace of the VolumeSnapshot object MUST be provided - for binding to happen. Required. + for binding to happen. This field is immutable after creation. Required. properties: apiVersion: description: API version of the referent. @@ -177,7 +179,6 @@ spec: type: object required: - spec - - status type: object version: v1beta1 versions: diff --git a/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml b/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml index ed0f0b28c..78d2e4552 100644 --- a/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml +++ b/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml @@ -41,17 +41,17 @@ spec: properties: source: description: source specifies where a snapshot will be created from. - Required. + This field is immutable after creation. Required. properties: persistentVolumeClaimName: description: persistentVolumeClaimName specifies the name of the PersistentVolumeClaim object in the same namespace as the VolumeSnapshot - object where the snapshot should be dynamically taken from. + object where the snapshot should be dynamically taken from. This + field is immutable once specified. type: string volumeSnapshotContentName: description: volumeSnapshotContentName specifies the name of a pre-existing - VolumeSnapshotContent object a user asks to statically bind the - VolumeSnapshot object to. + VolumeSnapshotContent object. This field is immutable once specified. type: string type: object volumeSnapshotClassName: @@ -72,11 +72,14 @@ spec: is accurate and complete.' properties: boundVolumeSnapshotContentName: - description: boundVolumeSnapshotContentName represents the name of the - VolumeSnapshotContent object to which the VolumeSnapshot object is - bound. If not specified, it indicates that the VolumeSnapshot object + description: 'boundVolumeSnapshotContentName represents the name of + the VolumeSnapshotContent object to which the VolumeSnapshot object + is bound. If not specified, it indicates that the VolumeSnapshot object has not been successfully bound to a VolumeSnapshotContent object - yet. + yet. NOTE: Specified boundVolumeSnapshotContentName alone does not + mean binding is valid. Controllers MUST always verify bidirectional + binding between VolumeSnapshot and VolumeSnapshotContent to + avoid possible security issues.' type: string creationTime: description: creationTime, if not nil, represents the timestamp when @@ -113,10 +116,9 @@ spec: finished all out-of-bound procedures to make a snapshot ready to be used to restore a volume. For a pre-existing snapshot, readyToUse will be set to the value returned from CSI "ListSnapshots" gRPC call - if the matching CSI driver exists and supports. Otherwise, if there - exists no CSI driver or the driver does not support "ListSnapshots", - this field will be set to "True". If not specified, it indicates that - the readiness of a snapshot is unknown. + if the matching CSI driver exists and supports. Otherwise, this field + will be set to "True". If not specified, it indicates that the readiness + of a snapshot is unknown. type: boolean restoreSize: description: restoreSize represents the complete size of the snapshot diff --git a/pkg/apis/volumesnapshot/v1beta1/types.go b/pkg/apis/volumesnapshot/v1beta1/types.go index 53842c336..9236a8d47 100644 --- a/pkg/apis/volumesnapshot/v1beta1/types.go +++ b/pkg/apis/volumesnapshot/v1beta1/types.go @@ -1,5 +1,5 @@ /* -Copyright 2018 The Kubernetes Authors. +Copyright 2019 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -50,7 +50,7 @@ type VolumeSnapshot struct { // Controllers should only use information from the VolumeSnapshotContent object // after verifying that the binding is accurate and complete. // +optional - Status VolumeSnapshotStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"` + Status *VolumeSnapshotStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -67,6 +67,7 @@ type VolumeSnapshotList struct { // VolumeSnapshotSpec describes the common attributes of a volume snapshot. type VolumeSnapshotSpec struct { // source specifies where a snapshot will be created from. + // This field is immutable after creation. // Required. Source VolumeSnapshotSource `json:"source" protobuf:"bytes,1,opt,name=source"` @@ -78,7 +79,9 @@ type VolumeSnapshotSpec struct { VolumeSnapshotClassName *string `json:"volumeSnapshotClassName,omitempty" protobuf:"bytes,2,opt,name=volumeSnapshotClassName"` } -// VolumeSnapshotSource represents the source of a snapshot. +// VolumeSnapshotSource specifies whether the underlying snapshot should be +// dynamically taken upon creation or if a pre-existing VolumeSnapshotContent +// object should be used. // Exactly one of its members must be set. // Members in VolumeSnapshotSource are immutable. // TODO(xiangqian): Add a webhook to ensure that VolumeSnapshotSource members @@ -87,24 +90,26 @@ type VolumeSnapshotSource struct { // persistentVolumeClaimName specifies the name of the PersistentVolumeClaim // object in the same namespace as the VolumeSnapshot object where the // snapshot should be dynamically taken from. + // This field is immutable once specified. // +optional PersistentVolumeClaimName *string `json:"persistentVolumeClaimName,omitempty" protobuf:"bytes,1,opt,name=persistentVolumeClaimName"` // volumeSnapshotContentName specifies the name of a pre-existing VolumeSnapshotContent - // object a user asks to statically bind the VolumeSnapshot object to. + // object. + // This field is immutable once specified. // +optional VolumeSnapshotContentName *string `json:"volumeSnapshotContentName,omitempty" protobuf:"bytes,2,opt,name=volumeSnapshotContentName"` } // VolumeSnapshotStatus is the status of the VolumeSnapshot type VolumeSnapshotStatus struct { - // NOTE: All fields in VolumeSnapshotStatus are informational for user references. - // Controllers MUST NOT rely on any fields programmatically. - // boundVolumeSnapshotContentName represents the name of the VolumeSnapshotContent // object to which the VolumeSnapshot object is bound. // If not specified, it indicates that the VolumeSnapshot object has not been // successfully bound to a VolumeSnapshotContent object yet. + // NOTE: Specified boundVolumeSnapshotContentName alone does not mean binding + // is valid. Controllers MUST always verify bidirectional binding between + // VolumeSnapshot and VolumeSnapshotContent to avoid possible security issues. // +optional BoundVolumeSnapshotContentName *string `json:"boundVolumeSnapshotContentName,omitempty" protobuf:"bytes,1,opt,name=boundVolumeSnapshotContentName"` @@ -124,8 +129,7 @@ type VolumeSnapshotStatus struct { // be used to restore a volume. // For a pre-existing snapshot, readyToUse will be set to the value returned // from CSI "ListSnapshots" gRPC call if the matching CSI driver exists and supports. - // Otherwise, if there exists no CSI driver or the driver does not support "ListSnapshots", - // this field will be set to "True". + // Otherwise, this field will be set to "True". // If not specified, it indicates that the readiness of a snapshot is unknown. // +optional ReadyToUse *bool `json:"readyToUse,omitempty" protobuf:"varint,3,opt,name=readyToUse"` @@ -166,6 +170,7 @@ type VolumeSnapshotClass struct { metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` // driver is the name of the storage driver that handles this VolumeSnapshotClass. + // Required. Driver string `json:"driver" protobuf:"bytes,2,opt,name=driver"` // parameters is a key-value map with storage driver specific parameters for creating snapshots. @@ -215,10 +220,12 @@ type VolumeSnapshotContent struct { metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` // spec defines properties of a VolumeSnapshotContent created by the underlying storage system. + // Required. Spec VolumeSnapshotContentSpec `json:"spec" protobuf:"bytes,2,opt,name=spec"` // status represents the current information of a snapshot. - Status VolumeSnapshotContentStatus `json:"status" protobuf:"bytes,3,opt,name=status"` + // +optional + Status *VolumeSnapshotContentStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -242,6 +249,7 @@ type VolumeSnapshotContentSpec struct { // this VolumeSnapshotContent's name for the bidirectional binding to be valid. // For a pre-existing VolumeSnapshotContent object, name and namespace of the // VolumeSnapshot object MUST be provided for binding to happen. + // This field is immutable after creation. // Required. VolumeSnapshotRef core_v1.ObjectReference `json:"volumeSnapshotRef" protobuf:"bytes,1,opt,name=volumeSnapshotRef"` @@ -269,23 +277,26 @@ type VolumeSnapshotContentSpec struct { SnapshotClassName *string `json:"snapshotClassName,omitempty" protobuf:"bytes,4,opt,name=snapshotClassName"` // source specifies from where a snapshot will be created. + // This field is immutable after creation. // Required. Source VolumeSnapshotContentSource `json:"source" protobuf:"bytes,5,opt,name=source"` } -// VolumeSnapshotContentSource represents the source of a snapshot. +// VolumeSnapshotContentSource represents the CSI source of a snapshot. // Exactly one of its members must be set. // Members in VolumeSnapshotContentSource are immutable. // TODO(xiangqian): Add a webhook to ensure that VolumeSnapshotContentSource members -// will not be updated once specified. +// will be immutable once specified. type VolumeSnapshotContentSource struct { // volumeHandle specifies the CSI name of the volume from which a snapshot // should be dynamically taken from. + // This field is immutable once specified. // +optional VolumeHandle *string `json:"volumeHandle,omitempty" protobuf:"bytes,1,opt,name=volumeHandle"` // snapshotHandle specifies the CSI name of a pre-existing snapshot on the // underlying storage system. + // This field is immutable once specified. // +optional SnapshotHandle *string `json:"snapshotHandle,omitempty" protobuf:"bytes,2,opt,name=snapshotHandle"` } @@ -322,6 +333,7 @@ type VolumeSnapshotContentStatus struct { // If not specified, it means the readiness of a snapshot is unknown. // +optional. ReadyToUse *bool `json:"readyToUse,omitempty" protobuf:"varint,4,opt,name=readyToUse"` + // error is the latest observed error during snapshot creation, if any. // +optional Error *VolumeSnapshotError `json:"error,omitempty" protobuf:"bytes,5,opt,name=error,casttype=VolumeSnapshotError"` @@ -332,10 +344,12 @@ type VolumeSnapshotContentStatus struct { type DeletionPolicy string const ( - // volumeSnapshotContentDelete means the snapshot will be deleted from Kubernetes on release from its volume snapshot. + // volumeSnapshotContentDelete means the snapshot will be deleted from the + // underlying storage system on release from its volume snapshot. VolumeSnapshotContentDelete DeletionPolicy = "Delete" - // volumeSnapshotContentRetain means the snapshot will be left in its current state on release from its volume snapshot. + // volumeSnapshotContentRetain means the snapshot will be left in its current + // state on release from its volume snapshot. VolumeSnapshotContentRetain DeletionPolicy = "Retain" ) diff --git a/pkg/apis/volumesnapshot/v1beta1/zz_generated.deepcopy.go b/pkg/apis/volumesnapshot/v1beta1/zz_generated.deepcopy.go index 6e2c637ca..98514e2b7 100644 --- a/pkg/apis/volumesnapshot/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/volumesnapshot/v1beta1/zz_generated.deepcopy.go @@ -27,7 +27,11 @@ func (in *VolumeSnapshot) DeepCopyInto(out *VolumeSnapshot) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - in.Status.DeepCopyInto(&out.Status) + if in.Status != nil { + in, out := &in.Status, &out.Status + *out = new(VolumeSnapshotStatus) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new VolumeSnapshot. @@ -118,7 +122,11 @@ func (in *VolumeSnapshotContent) DeepCopyInto(out *VolumeSnapshotContent) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - in.Status.DeepCopyInto(&out.Status) + if in.Status != nil { + in, out := &in.Status, &out.Status + *out = new(VolumeSnapshotContentStatus) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new VolumeSnapshotContent. diff --git a/pkg/controller/framework_test.go b/pkg/controller/framework_test.go index 4c3539e2f..f8b415177 100644 --- a/pkg/controller/framework_test.go +++ b/pkg/controller/framework_test.go @@ -786,7 +786,7 @@ func newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHa Driver: mockDriverName, DeletionPolicy: deletionPolicy, }, - Status: crdv1.VolumeSnapshotContentStatus{ + Status: &crdv1.VolumeSnapshotContentStatus{ CreationTime: creationTime, RestoreSize: size, }, @@ -869,7 +869,7 @@ func newSnapshot( Spec: crdv1.VolumeSnapshotSpec{ VolumeSnapshotClassName: nil, }, - Status: crdv1.VolumeSnapshotStatus{ + Status: &crdv1.VolumeSnapshotStatus{ CreationTime: creationTime, ReadyToUse: readyToUse, Error: err, diff --git a/pkg/controller/snapshot_controller.go b/pkg/controller/snapshot_controller.go index 45d1c6b09..40f1502e6 100644 --- a/pkg/controller/snapshot_controller.go +++ b/pkg/controller/snapshot_controller.go @@ -195,7 +195,7 @@ func (ctrl *csiSnapshotController) syncSnapshot(snapshot *crdv1.VolumeSnapshot) ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "ErrorSnapshotSourceFinalizer", "Error check and remove PVC Finalizer for VolumeSnapshot") } - if snapshot.Status.ReadyToUse == nil || !*snapshot.Status.ReadyToUse { + if !isSnapshotReadyToUse(snapshot) { return ctrl.syncUnreadySnapshot(snapshot) } return ctrl.syncReadySnapshot(snapshot) @@ -274,15 +274,11 @@ func (ctrl *csiSnapshotController) syncUnreadySnapshot(snapshot *crdv1.VolumeSna // find a matching volume snapshot content if contentObj := ctrl.getMatchSnapshotContent(snapshot); contentObj != nil { klog.V(5).Infof("Find VolumeSnapshotContent object %s for snapshot %s", contentObj.Name, uniqueSnapshotName) - newSnapshot, err := ctrl.bindandUpdateVolumeSnapshot(contentObj, snapshot) - if err != nil { - return err - } - if err = ctrl.checkandUpdateBoundSnapshotStatus(newSnapshot, contentObj); err != nil { + if err := ctrl.checkandUpdateBoundSnapshotStatus(snapshot, contentObj); err != nil { return err } - klog.V(5).Infof("bindandUpdateVolumeSnapshot %v", newSnapshot) - } else if snapshot.Status.BoundVolumeSnapshotContentName != nil { + klog.V(5).Infof("checkandUpdateBoundSnapshotStatus %v", snapshot) + } else if snapshot.Status != nil && snapshot.Status.BoundVolumeSnapshotContentName != nil { contentObj, found, err := ctrl.contentStore.GetByKey(*snapshot.Status.BoundVolumeSnapshotContentName) if err != nil { return err @@ -295,7 +291,7 @@ func (ctrl *csiSnapshotController) syncUnreadySnapshot(snapshot *crdv1.VolumeSna ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "InvalidSnapshotBinding", fmt.Sprintf("Snapshot is bound to a VolumeSnapshotContent which is bound to other Snapshot")) return fmt.Errorf("snapshot %s is bound, but VolumeSnapshotContent %s is not bound to the VolumeSnapshot correctly", uniqueSnapshotName, content.Name) } - } else if snapshot.Status.Error == nil || isControllerUpdateFailError(snapshot.Status.Error) { + } else if snapshot.Status == nil || snapshot.Status.Error == nil || isControllerUpdateFailError(snapshot.Status.Error) { if err := ctrl.createSnapshot(snapshot); err != nil { ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot with error %v", err)) return err @@ -416,7 +412,7 @@ func (ctrl *csiSnapshotController) checkandUpdateBoundSnapshotStatus(snapshot *c func (ctrl *csiSnapshotController) updateSnapshotErrorStatusWithEvent(snapshot *crdv1.VolumeSnapshot, eventtype, reason, message string) error { klog.V(5).Infof("updateSnapshotStatusWithEvent[%s]", snapshotKey(snapshot)) - if snapshot.Status.Error != nil && snapshot.Status.Error.Message != nil && *snapshot.Status.Error.Message == message { + if snapshot.Status != nil && snapshot.Status.Error != nil && snapshot.Status.Error.Message != nil && *snapshot.Status.Error.Message == message { klog.V(4).Infof("updateSnapshotStatusWithEvent[%s]: the same error %v is already set", snapshot.Name, snapshot.Status.Error) return nil } @@ -451,13 +447,10 @@ func (ctrl *csiSnapshotController) updateSnapshotErrorStatusWithEvent(snapshot * // Stateless functions func getSnapshotStatusForLogging(snapshot *crdv1.VolumeSnapshot) string { snapshotContentName := "" - readyToUse := false - if snapshot.Status.BoundVolumeSnapshotContentName != nil { + readyToUse := isSnapshotReadyToUse(snapshot) + if snapshot.Status != nil && snapshot.Status.BoundVolumeSnapshotContentName != nil { snapshotContentName = *snapshot.Status.BoundVolumeSnapshotContentName } - if snapshot.Status.ReadyToUse != nil { - readyToUse = *snapshot.Status.ReadyToUse - } return fmt.Sprintf("bound to: %q, Completed: %v", snapshotContentName, readyToUse) } @@ -479,7 +472,7 @@ func (ctrl *csiSnapshotController) isSnapshotContentBeingUsed(content *crdv1.Vol } // Check if the snapshot content is bound to the snapshot - if IsSnapshotBound(snapshotObj, content) && *snapshotObj.Status.BoundVolumeSnapshotContentName == content.Name { + if IsSnapshotBound(snapshotObj, content) && snapshotObj.Status != nil && *snapshotObj.Status.BoundVolumeSnapshotContentName == content.Name { klog.Infof("isSnapshotContentBeingUsed: VolumeSnapshot %s is bound to volumeSnapshotContent [%s]", snapshotObj.Name, content.Name) return true } @@ -622,7 +615,7 @@ func (ctrl *csiSnapshotController) checkandUpdateBoundSnapshotStatusOperation(sn func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshot, error) { klog.Infof("createSnapshot: Creating snapshot %s through the plugin ...", snapshotKey(snapshot)) - if snapshot.Status.Error != nil && snapshot.Status.Error.Message != nil && !isControllerUpdateFailError(snapshot.Status.Error) { + if snapshot.Status != nil && snapshot.Status.Error != nil && snapshot.Status.Error.Message != nil && !isControllerUpdateFailError(snapshot.Status.Error) { klog.V(4).Infof("error is already set in snapshot, do not retry to create: %s", *snapshot.Status.Error.Message) return snapshot, nil } @@ -686,12 +679,6 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum VolumeHandle: &volume.Spec.CSI.VolumeHandle, }, }, - Status: crdv1.VolumeSnapshotContentStatus{ - SnapshotHandle: &snapshotID, - CreationTime: ×tamp, - RestoreSize: &size, - ReadyToUse: &readyToUse, - }, } // Set AnnDeletionSecretRefName and AnnDeletionSecretRefNamespace @@ -707,14 +694,19 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum // Try to create the VolumeSnapshotContent object several times for i := 0; i < ctrl.createSnapshotContentRetryCount; i++ { klog.V(5).Infof("createSnapshot [%s]: trying to save volume snapshot content %s", snapshotKey(snapshot), snapshotContent.Name) - if _, err = ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Create(snapshotContent); err == nil || apierrs.IsAlreadyExists(err) { - // Save succeeded. + var newContent *crdv1.VolumeSnapshotContent + if newContent, err = ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Create(snapshotContent); err == nil || apierrs.IsAlreadyExists(err) { + // creation succeeded. if err != nil { klog.V(3).Infof("volume snapshot content %q for snapshot %q already exists, reusing", snapshotContent.Name, snapshotKey(snapshot)) err = nil } else { klog.V(3).Infof("volume snapshot content %q for snapshot %q saved, %v", snapshotContent.Name, snapshotKey(snapshot), snapshotContent) } + newContent, err = ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Get(snapshotContent.Name, metav1.GetOptions{}) + if err == nil { + err = ctrl.updateSnapshotContentStatus(newContent, snapshotID, readyToUse, timestamp, size) + } break } // Save failed, try again after a while. @@ -731,13 +723,7 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum ctrl.eventRecorder.Event(newSnapshot, v1.EventTypeWarning, "CreateSnapshotContentFailed", strerr) return nil, newControllerUpdateError(snapshotKey(snapshot), err.Error()) } - - // save succeeded, bind and update status for snapshot. - result, err := ctrl.bindandUpdateVolumeSnapshot(snapshotContent, newSnapshot) - if err != nil { - return nil, err - } - return result, nil + return newSnapshot, nil } // Delete a snapshot @@ -784,66 +770,50 @@ func (ctrl *csiSnapshotController) deleteSnapshotContentOperation(content *crdv1 return nil } -func (ctrl *csiSnapshotController) bindandUpdateVolumeSnapshot(snapshotContent *crdv1.VolumeSnapshotContent, snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshot, error) { - klog.V(5).Infof("bindandUpdateVolumeSnapshot for snapshot [%s]: snapshotContent [%s]", snapshot.Name, snapshotContent.Name) - snapshotObj, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshots(snapshot.Namespace).Get(snapshot.Name, metav1.GetOptions{}) - if err != nil { - return nil, fmt.Errorf("error get snapshot %s from api server: %v", snapshotKey(snapshot), err) - } - - // Copy the snapshot object before updating it - snapshotCopy := snapshotObj.DeepCopy() - - if snapshotObj.Status.BoundVolumeSnapshotContentName != nil && *snapshotObj.Status.BoundVolumeSnapshotContentName == snapshotContent.Name { - klog.Infof("bindVolumeSnapshotContentToVolumeSnapshot: VolumeSnapshot %s already bind to volumeSnapshotContent [%s]", snapshot.Name, snapshotContent.Name) - } else { - klog.Infof("bindVolumeSnapshotContentToVolumeSnapshot: before bind VolumeSnapshot %s to volumeSnapshotContent [%s]", snapshot.Name, snapshotContent.Name) - snapshotCopy.Status.BoundVolumeSnapshotContentName = &snapshotContent.Name - updateSnapshot, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshots(snapshot.Namespace).Update(snapshotCopy) - if err != nil { - klog.Infof("bindVolumeSnapshotContentToVolumeSnapshot: Error binding VolumeSnapshot %s to volumeSnapshotContent [%s]. Error [%#v]", snapshot.Name, snapshotContent.Name, err) - return nil, newControllerUpdateError(snapshotKey(snapshot), err.Error()) - } - snapshotCopy = updateSnapshot - _, err = ctrl.storeSnapshotUpdate(snapshotCopy) - if err != nil { - klog.Errorf("%v", err) - } - } - - klog.V(5).Infof("bindandUpdateVolumeSnapshot for snapshot completed [%#v]", snapshotCopy) - return snapshotCopy, nil -} - // TODO(xiangqian) This is a temp implementation to make sure the test cases pass +// updateSnapshotContentStatus updates status of a content object func (ctrl *csiSnapshotController) updateSnapshotContentStatus( content *crdv1.VolumeSnapshotContent, snapshotHandle string, readyToUse bool, createdAt int64, size int64) error { - currentStatus := content.Status + var newStatus *crdv1.VolumeSnapshotContentStatus updated := false - if currentStatus.SnapshotHandle == nil { - currentStatus.SnapshotHandle = &snapshotHandle - updated = true - } - if currentStatus.ReadyToUse == nil || *currentStatus.ReadyToUse != readyToUse { - currentStatus.ReadyToUse = &readyToUse - updated = true - } - if currentStatus.CreationTime == nil { - currentStatus.CreationTime = &createdAt - updated = true - } - if currentStatus.RestoreSize == nil { - currentStatus.RestoreSize = &size + if content.Status == nil { + newStatus = &crdv1.VolumeSnapshotContentStatus{ + SnapshotHandle: &snapshotHandle, + ReadyToUse: &readyToUse, + CreationTime: &createdAt, + RestoreSize: &size, + } updated = true + } else { + newStatus = content.Status.DeepCopy() + if newStatus.SnapshotHandle == nil { + newStatus.SnapshotHandle = &snapshotHandle + updated = true + } + if newStatus.ReadyToUse == nil || *newStatus.ReadyToUse != readyToUse { + newStatus.ReadyToUse = &readyToUse + updated = true + if readyToUse && newStatus.Error != nil { + newStatus.Error = nil + } + } + if newStatus.CreationTime == nil { + newStatus.CreationTime = &createdAt + updated = true + } + if newStatus.RestoreSize == nil { + newStatus.RestoreSize = &size + updated = true + } } if updated { contentClone := content.DeepCopy() - contentClone.Status = currentStatus + contentClone.Status = newStatus _, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().UpdateStatus(contentClone) if err != nil { return newControllerUpdateError(content.Name, err.Error()) @@ -860,32 +830,43 @@ func (ctrl *csiSnapshotController) updateSnapshotStatus( createdAt time.Time, size int64) (*crdv1.VolumeSnapshot, error) { klog.V(5).Infof("updating VolumeSnapshot[]%s, readyToUse %v, timestamp %v", snapshotKey(snapshot), readyToUse, createdAt) - status := snapshot.Status - updated := false - - if status.BoundVolumeSnapshotContentName == nil { - status.BoundVolumeSnapshotContentName = &boundContentName - updated = true - } - - if status.CreationTime == nil { - status.CreationTime = &metav1.Time{Time: createdAt} - updated = true - } - - if status.ReadyToUse == nil || *status.ReadyToUse != readyToUse { - status.ReadyToUse = &readyToUse - updated = true - } - if status.RestoreSize == nil { - status.RestoreSize = resource.NewQuantity(size, resource.BinarySI) + var newStatus *crdv1.VolumeSnapshotStatus + updated := false + if snapshot.Status == nil { + newStatus = &crdv1.VolumeSnapshotStatus{ + BoundVolumeSnapshotContentName: &boundContentName, + CreationTime: &metav1.Time{Time: createdAt}, + ReadyToUse: &readyToUse, + RestoreSize: resource.NewQuantity(size, resource.BinarySI), + } updated = true + } else { + newStatus = snapshot.Status.DeepCopy() + if newStatus.BoundVolumeSnapshotContentName == nil { + newStatus.BoundVolumeSnapshotContentName = &boundContentName + updated = true + } + if newStatus.CreationTime == nil { + newStatus.CreationTime = &metav1.Time{Time: createdAt} + updated = true + } + if newStatus.ReadyToUse == nil || *newStatus.ReadyToUse != readyToUse { + newStatus.ReadyToUse = &readyToUse + updated = true + if readyToUse && newStatus.Error != nil { + newStatus.Error = nil + } + } + if newStatus.RestoreSize == nil { + newStatus.RestoreSize = resource.NewQuantity(size, resource.BinarySI) + updated = true + } } if updated { snapshotClone := snapshot.DeepCopy() - snapshotClone.Status = status + snapshotClone.Status = newStatus newSnapshotObj, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshots(snapshotClone.Namespace).UpdateStatus(snapshotClone) if err != nil { return nil, newControllerUpdateError(snapshotKey(snapshot), err.Error()) @@ -1187,7 +1168,7 @@ func (ctrl *csiSnapshotController) isSnapshotSourceBeingUsed(snapshot *crdv1.Vol klog.V(4).Infof("Skipping static bound snapshot %s when checking PVC %s/%s", snap.Name, pvc.Namespace, pvc.Name) continue } - if pvc.Name == *snap.Spec.Source.PersistentVolumeClaimName && (snap.Status.ReadyToUse == nil || *snap.Status.ReadyToUse == false) { + if pvc.Name == *snap.Spec.Source.PersistentVolumeClaimName && !isSnapshotReadyToUse(snap) { klog.V(2).Infof("Keeping PVC %s/%s, it is used by snapshot %s/%s", pvc.Namespace, pvc.Name, snap.Namespace, snap.Name) return true } @@ -1226,3 +1207,11 @@ func (ctrl *csiSnapshotController) checkandRemoveSnapshotSourceFinalizer(snapsho return nil } + +// isSnapshotReadyToUse checks if a snapshot object has ReadyToUse in Status set to true +func isSnapshotReadyToUse(snapshot *crdv1.VolumeSnapshot) bool { + if snapshot.Status == nil || snapshot.Status.ReadyToUse == nil { + return false + } + return *snapshot.Status.ReadyToUse +} diff --git a/pkg/controller/snapshot_ready_test.go b/pkg/controller/snapshot_ready_test.go index 5a6c4e5fc..e39b9f1f2 100644 --- a/pkg/controller/snapshot_ready_test.go +++ b/pkg/controller/snapshot_ready_test.go @@ -196,11 +196,11 @@ func TestSync(t *testing.T) { test: testSyncSnapshot, }, { - name: "2-9 - bind when snapshot and content matches, fail on status update as there is not pvc provided", + name: "2-9 - fail on status update as there is not pvc provided", initialContents: newContentArray("content2-9", "snapuid2-9", "snap2-9", "sid2-9", validSecretClass, "", "", deletionPolicy, nil, nil, false), expectedContents: newContentArray("content2-9", "snapuid2-9", "snap2-9", "sid2-9", validSecretClass, "", "", deletionPolicy, nil, nil, false), initialSnapshots: newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "", &False, nil, nil, nil), - expectedSnapshots: newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "content2-9", &False, nil, nil, newVolumeError("Failed to check and update snapshot: failed to get input parameters to create snapshot snap2-9: \"failed to retrieve PVC claim2-9 from the lister: \\\"persistentvolumeclaim \\\\\\\"claim2-9\\\\\\\" not found\\\"\"")), + expectedSnapshots: newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "", &False, nil, nil, newVolumeError("Failed to check and update snapshot: failed to get input parameters to create snapshot snap2-9: \"failed to retrieve PVC claim2-9 from the lister: \\\"persistentvolumeclaim \\\\\\\"claim2-9\\\\\\\" not found\\\"\"")), errors: noerrors, test: testSyncSnapshot, },