Skip to content

Add Finalizer for VolumeSnapshot and VolumeSnapshotContent #39

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

Merged
merged 1 commit into from
Nov 27, 2018
Merged

Add Finalizer for VolumeSnapshot and VolumeSnapshotContent #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 18 additions & 5 deletions pkg/controller/framework_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,16 @@ type reactorError struct {
error error
}

func withSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot) *crdv1.VolumeSnapshot {
snapshot.ObjectMeta.Finalizers = append(snapshot.ObjectMeta.Finalizers, VolumeSnapshotFinalizer)
return snapshot
}

func withContentFinalizer(content *crdv1.VolumeSnapshotContent) *crdv1.VolumeSnapshotContent {
content.ObjectMeta.Finalizers = append(content.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer)
return content
}

// React is a callback called by fake kubeClient from the controller.
// In other words, every snapshot/content change performed by the controller ends
// here.
Expand Down Expand Up @@ -744,7 +754,7 @@ func newTestController(kubeClient kubernetes.Interface, clientset clientset.Inte
}

// newContent returns a new content with given attributes
func newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, deletionPolicy *crdv1.DeletionPolicy, size *int64, creationTime *int64) *crdv1.VolumeSnapshotContent {
func newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, deletionPolicy *crdv1.DeletionPolicy, size *int64, creationTime *int64, withFinalizer bool) *crdv1.VolumeSnapshotContent {
content := crdv1.VolumeSnapshotContent{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand Down Expand Up @@ -779,17 +789,20 @@ func newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToS
}
}

if withFinalizer {
return withContentFinalizer(&content)
}
return &content
}

func newContentArray(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, deletionPolicy *crdv1.DeletionPolicy, size *int64, creationTime *int64) []*crdv1.VolumeSnapshotContent {
func newContentArray(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, deletionPolicy *crdv1.DeletionPolicy, size *int64, creationTime *int64, withFinalizer bool) []*crdv1.VolumeSnapshotContent {
return []*crdv1.VolumeSnapshotContent{
newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName, deletionPolicy, size, creationTime),
newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName, deletionPolicy, size, creationTime, withFinalizer),
}
}

func newContentWithUnmatchDriverArray(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, deletionPolicy *crdv1.DeletionPolicy, size *int64, creationTime *int64) []*crdv1.VolumeSnapshotContent {
content := newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName, deletionPolicy, size, creationTime)
content := newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName, deletionPolicy, size, creationTime, false)
content.Spec.VolumeSnapshotSource.CSI.Driver = "fake"
return []*crdv1.VolumeSnapshotContent{
content,
Expand Down Expand Up @@ -821,7 +834,7 @@ func newSnapshot(name, className, boundToContent, snapshotUID, claimName string,
},
}

return &snapshot
return withSnapshotFinalizer(&snapshot)
}

func newSnapshotArray(name, className, boundToContent, snapshotUID, claimName string, ready bool, err *storagev1beta1.VolumeError, creationTime *metav1.Time, size *resource.Quantity) []*crdv1.VolumeSnapshot {
Expand Down
155 changes: 155 additions & 0 deletions pkg/controller/snapshot_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
ref "k8s.io/client-go/tools/reference"
"k8s.io/kubernetes/pkg/util/goroutinemap"
"k8s.io/kubernetes/pkg/util/goroutinemap/exponentialbackoff"
"k8s.io/kubernetes/pkg/util/slice"
)

// ==================================================================
Expand Down Expand Up @@ -77,6 +78,9 @@ import (

const pvcKind = "PersistentVolumeClaim"
const apiGroup = ""
const snapshotKind = "VolumeSnapshot"
const snapshotAPIGroup = crdv1.GroupName

const controllerUpdateFailMsg = "snapshot controller failed to update"

const IsDefaultSnapshotClassAnnotation = "snapshot.storage.kubernetes.io/is-default-class"
Expand All @@ -85,6 +89,23 @@ const IsDefaultSnapshotClassAnnotation = "snapshot.storage.kubernetes.io/is-defa
func (ctrl *csiSnapshotController) syncContent(content *crdv1.VolumeSnapshotContent) error {
glog.V(5).Infof("synchronizing VolumeSnapshotContent[%s]", content.Name)

if isContentDeletionCandidate(content) {
// Volume snapshot content should be deleted. Check if it's used
// and remove finalizer if it's not.
// Check if snapshot content is still bound to a snapshot.
isUsed := ctrl.isSnapshotContentBeingUsed(content)
if !isUsed {
glog.V(5).Infof("syncContent: Remove Finalizer for VolumeSnapshotContent[%s]", content.Name)
return ctrl.removeContentFinalizer(content)
}
}

if needToAddContentFinalizer(content) {
// Content is not being deleted -> it should have the finalizer.
glog.V(5).Infof("syncContent: Add Finalizer for VolumeSnapshotContent[%s]", content.Name)
return ctrl.addContentFinalizer(content)
}

// VolumeSnapshotContent is not bound to any VolumeSnapshot, in this case we just return err
if content.Spec.VolumeSnapshotRef == nil {
// content is not bound
Expand Down Expand Up @@ -154,6 +175,23 @@ func (ctrl *csiSnapshotController) syncContent(content *crdv1.VolumeSnapshotCont
func (ctrl *csiSnapshotController) syncSnapshot(snapshot *crdv1.VolumeSnapshot) error {
glog.V(5).Infof("synchonizing VolumeSnapshot[%s]: %s", snapshotKey(snapshot), getSnapshotStatusForLogging(snapshot))

if isSnapshotDeletionCandidate(snapshot) {
// Volume snapshot should be deleted. Check if it's used
// and remove finalizer if it's not.
// Check if a volume is being created from snapshot.
isUsed := ctrl.isVolumeBeingCreatedFromSnapshot(snapshot)
if !isUsed {
glog.V(5).Infof("syncSnapshot: Remove Finalizer for VolumeSnapshot[%s]", snapshotKey(snapshot))
return ctrl.removeSnapshotFinalizer(snapshot)
}
}

if needToAddSnapshotFinalizer(snapshot) {
// Snapshot is not being deleted -> it should have the finalizer.
glog.V(5).Infof("syncSnapshot: Add Finalizer for VolumeSnapshot[%s]", snapshotKey(snapshot))
return ctrl.addSnapshotFinalizer(snapshot)
}

if !snapshot.Status.ReadyToUse {
return ctrl.syncUnreadySnapshot(snapshot)
} else {
Expand Down Expand Up @@ -410,6 +448,48 @@ func IsSnapshotBound(snapshot *crdv1.VolumeSnapshot, content *crdv1.VolumeSnapsh
return false
}

// isSnapshotConentBeingUsed checks if snapshot content is bound to snapshot.
func (ctrl *csiSnapshotController) isSnapshotContentBeingUsed(content *crdv1.VolumeSnapshotContent) bool {
if content.Spec.VolumeSnapshotRef != nil {
snapshotObj, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshots(content.Spec.VolumeSnapshotRef.Namespace).Get(content.Spec.VolumeSnapshotRef.Name, metav1.GetOptions{})
if err != nil {
glog.Infof("isSnapshotContentBeingUsed: Cannot get snapshot %s from api server: [%v]. VolumeSnapshot object may be deleted already.", content.Spec.VolumeSnapshotRef.Name, err)
return false
}

// Check if the snapshot content is bound to the snapshot
if IsSnapshotBound(snapshotObj, content) && snapshotObj.Spec.SnapshotContentName == content.Name {
glog.Infof("isSnapshotContentBeingUsed: VolumeSnapshot %s is bound to volumeSnapshotContent [%s]", snapshotObj.Name, content.Name)
return true
}
}

glog.V(5).Infof("isSnapshotContentBeingUsed: Snapshot content %s is not being used", content.Name)
return false
}

// isVolumeBeingCreatedFromSnapshot checks if an volume is being created from the snapshot.
func (ctrl *csiSnapshotController) isVolumeBeingCreatedFromSnapshot(snapshot *crdv1.VolumeSnapshot) bool {
pvcList, err := ctrl.client.CoreV1().PersistentVolumeClaims(snapshot.Namespace).List(metav1.ListOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use pvcLister to reduce the server pressure instead of using client

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion. I think a separate PR should be submitted to add pvcLister to the snapshot controller first, and after that this can be updated to use that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you create an issue for it so that we won't forget?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Opened issue #70

if err != nil {
glog.Errorf("Failed to retrieve PVCs from the API server to check if volume snapshot %s is being used by a volume: %q", snapshotKey(snapshot), err)
return false
}
for _, pvc := range pvcList.Items {
if pvc.Spec.DataSource != nil && len(pvc.Spec.DataSource.Name) > 0 && pvc.Spec.DataSource.Name == snapshot.Name {
if pvc.Spec.DataSource.Kind == snapshotKind && *(pvc.Spec.DataSource.APIGroup) == snapshotAPIGroup {
if pvc.Status.Phase == v1.ClaimPending {
// A volume is being created from the snapshot
glog.Infof("isVolumeBeingCreatedFromSnapshot: volume %s is being created from snapshot %s", pvc.Name, pvc.Spec.DataSource.Name)
return true
}
}
}
}
glog.V(5).Infof("isVolumeBeingCreatedFromSnapshot: no volume is being created from snapshot %s", snapshotKey(snapshot))
return false
}

// The function checks whether the volumeSnapshotRef in snapshot content matches the given snapshot. If match, it binds the content with the snapshot
func (ctrl *csiSnapshotController) checkandBindSnapshotContent(snapshot *crdv1.VolumeSnapshot, content *crdv1.VolumeSnapshotContent) error {
if content.Spec.VolumeSnapshotRef == nil || content.Spec.VolumeSnapshotRef.Name != snapshot.Name {
Expand Down Expand Up @@ -899,3 +979,78 @@ func isControllerUpdateFailError(err *storage.VolumeError) bool {
}
return false
}

// addContentFinalizer adds a Finalizer for VolumeSnapshotContent.
func (ctrl *csiSnapshotController) addContentFinalizer(content *crdv1.VolumeSnapshotContent) error {
contentClone := content.DeepCopy()
contentClone.ObjectMeta.Finalizers = append(contentClone.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer)

_, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshotContents().Update(contentClone)
if err != nil {
return newControllerUpdateError(content.Name, err.Error())
}

_, err = ctrl.storeContentUpdate(contentClone)
if err != nil {
glog.Errorf("failed to update content store %v", err)
}

glog.V(5).Infof("Added protection finalizer to volume snapshot content %s", content.Name)
return nil
}

// removeContentFinalizer removes a Finalizer for VolumeSnapshotContent.
func (ctrl *csiSnapshotController) removeContentFinalizer(content *crdv1.VolumeSnapshotContent) error {
contentClone := content.DeepCopy()
contentClone.ObjectMeta.Finalizers = slice.RemoveString(contentClone.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer, nil)

_, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshotContents().Update(contentClone)
if err != nil {
return newControllerUpdateError(content.Name, err.Error())
}

_, err = ctrl.storeContentUpdate(contentClone)
if err != nil {
glog.Errorf("failed to update content store %v", err)
}

glog.V(5).Infof("Removed protection finalizer from volume snapshot content %s", content.Name)
return nil
}

// addSnapshotFinalizer adds a Finalizer for VolumeSnapshot.
func (ctrl *csiSnapshotController) addSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot) error {
snapshotClone := snapshot.DeepCopy()
snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, VolumeSnapshotFinalizer)
_, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshots(snapshotClone.Namespace).Update(snapshotClone)
if err != nil {
return newControllerUpdateError(snapshot.Name, err.Error())
}

_, err = ctrl.storeSnapshotUpdate(snapshotClone)
if err != nil {
glog.Errorf("failed to update snapshot store %v", err)
}

glog.V(5).Infof("Added protection finalizer to volume snapshot %s", snapshotKey(snapshot))
return nil
}

// removeContentFinalizer removes a Finalizer for VolumeSnapshot.
func (ctrl *csiSnapshotController) removeSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot) error {
snapshotClone := snapshot.DeepCopy()
snapshotClone.ObjectMeta.Finalizers = slice.RemoveString(snapshotClone.ObjectMeta.Finalizers, VolumeSnapshotFinalizer, nil)

_, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshots(snapshotClone.Namespace).Update(snapshotClone)
if err != nil {
return newControllerUpdateError(snapshot.Name, err.Error())
}

_, err = ctrl.storeSnapshotUpdate(snapshotClone)
if err != nil {
glog.Errorf("failed to update snapshot store %v", err)
}

glog.V(5).Infof("Removed protection finalizer from volume snapshot %s", snapshotKey(snapshot))
return nil
}
4 changes: 2 additions & 2 deletions pkg/controller/snapshot_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
)

func storeVersion(t *testing.T, prefix string, c cache.Store, version string, expectedReturn bool) {
content := newContent("contentName", classEmpty, "sid1-1", "vuid1-1", "volume1-1", "snapuid1-1", "snap1-1", nil, nil, nil)
content := newContent("contentName", classEmpty, "sid1-1", "vuid1-1", "volume1-1", "snapuid1-1", "snap1-1", nil, nil, nil, false)
content.ResourceVersion = version
ret, err := storeObjectUpdate(c, content, "content")
if err != nil {
Expand Down Expand Up @@ -82,7 +82,7 @@ func TestControllerCacheParsingError(t *testing.T) {
// There must be something in the cache to compare with
storeVersion(t, "Step1", c, "1", true)

content := newContent("contentName", classEmpty, "sid1-1", "vuid1-1", "volume1-1", "snapuid1-1", "snap1-1", nil, nil, nil)
content := newContent("contentName", classEmpty, "sid1-1", "vuid1-1", "volume1-1", "snapuid1-1", "snap1-1", nil, nil, nil, false)
content.ResourceVersion = "xxx"
_, err := storeObjectUpdate(c, content, "content")
if err == nil {
Expand Down
12 changes: 6 additions & 6 deletions pkg/controller/snapshot_create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestCreateSnapshotSync(t *testing.T) {
{
name: "6-1 - successful create snapshot with snapshot class gold",
initialContents: nocontents,
expectedContents: newContentArray("snapcontent-snapuid6-1", classGold, "sid6-1", "pv-uid6-1", "volume6-1", "snapuid6-1", "snap6-1", &deletePolicy, &defaultSize, &timeNow),
expectedContents: newContentArray("snapcontent-snapuid6-1", classGold, "sid6-1", "pv-uid6-1", "volume6-1", "snapuid6-1", "snap6-1", &deletePolicy, &defaultSize, &timeNow, false),
initialSnapshots: newSnapshotArray("snap6-1", classGold, "", "snapuid6-1", "claim6-1", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap6-1", classGold, "snapcontent-snapuid6-1", "snapuid6-1", "claim6-1", false, nil, metaTimeNowUnix, getSize(defaultSize)),
initialClaims: newClaimArray("claim6-1", "pvc-uid6-1", "1Gi", "volume6-1", v1.ClaimBound, &classEmpty),
Expand All @@ -92,7 +92,7 @@ func TestCreateSnapshotSync(t *testing.T) {
{
name: "6-2 - successful create snapshot with snapshot class silver",
initialContents: nocontents,
expectedContents: newContentArray("snapcontent-snapuid6-2", classSilver, "sid6-2", "pv-uid6-2", "volume6-2", "snapuid6-2", "snap6-2", &deletePolicy, &defaultSize, &timeNow),
expectedContents: newContentArray("snapcontent-snapuid6-2", classSilver, "sid6-2", "pv-uid6-2", "volume6-2", "snapuid6-2", "snap6-2", &deletePolicy, &defaultSize, &timeNow, false),
initialSnapshots: newSnapshotArray("snap6-2", classSilver, "", "snapuid6-2", "claim6-2", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap6-2", classSilver, "snapcontent-snapuid6-2", "snapuid6-2", "claim6-2", false, nil, metaTimeNowUnix, getSize(defaultSize)),
initialClaims: newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
Expand All @@ -116,7 +116,7 @@ func TestCreateSnapshotSync(t *testing.T) {
{
name: "6-3 - successful create snapshot with snapshot class valid-secret-class",
initialContents: nocontents,
expectedContents: newContentArray("snapcontent-snapuid6-3", validSecretClass, "sid6-3", "pv-uid6-3", "volume6-3", "snapuid6-3", "snap6-3", &deletePolicy, &defaultSize, &timeNow),
expectedContents: newContentArray("snapcontent-snapuid6-3", validSecretClass, "sid6-3", "pv-uid6-3", "volume6-3", "snapuid6-3", "snap6-3", &deletePolicy, &defaultSize, &timeNow, false),
initialSnapshots: newSnapshotArray("snap6-3", validSecretClass, "", "snapuid6-3", "claim6-3", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap6-3", validSecretClass, "snapcontent-snapuid6-3", "snapuid6-3", "claim6-3", false, nil, metaTimeNowUnix, getSize(defaultSize)),
initialClaims: newClaimArray("claim6-3", "pvc-uid6-3", "1Gi", "volume6-3", v1.ClaimBound, &classEmpty),
Expand All @@ -142,7 +142,7 @@ func TestCreateSnapshotSync(t *testing.T) {
{
name: "6-4 - successful create snapshot with snapshot class empty-secret-class",
initialContents: nocontents,
expectedContents: newContentArray("snapcontent-snapuid6-4", emptySecretClass, "sid6-4", "pv-uid6-4", "volume6-4", "snapuid6-4", "snap6-4", &deletePolicy, &defaultSize, &timeNow),
expectedContents: newContentArray("snapcontent-snapuid6-4", emptySecretClass, "sid6-4", "pv-uid6-4", "volume6-4", "snapuid6-4", "snap6-4", &deletePolicy, &defaultSize, &timeNow, false),
initialSnapshots: newSnapshotArray("snap6-4", emptySecretClass, "", "snapuid6-4", "claim6-4", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap6-4", emptySecretClass, "snapcontent-snapuid6-4", "snapuid6-4", "claim6-4", false, nil, metaTimeNowUnix, getSize(defaultSize)),
initialClaims: newClaimArray("claim6-4", "pvc-uid6-4", "1Gi", "volume6-4", v1.ClaimBound, &classEmpty),
Expand All @@ -168,7 +168,7 @@ func TestCreateSnapshotSync(t *testing.T) {
{
name: "6-5 - successful create snapshot with status uploading",
initialContents: nocontents,
expectedContents: newContentArray("snapcontent-snapuid6-5", classGold, "sid6-5", "pv-uid6-5", "volume6-5", "snapuid6-5", "snap6-5", &deletePolicy, &defaultSize, &timeNow),
expectedContents: newContentArray("snapcontent-snapuid6-5", classGold, "sid6-5", "pv-uid6-5", "volume6-5", "snapuid6-5", "snap6-5", &deletePolicy, &defaultSize, &timeNow, false),
initialSnapshots: newSnapshotArray("snap6-5", classGold, "", "snapuid6-5", "claim6-5", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap6-5", classGold, "snapcontent-snapuid6-5", "snapuid6-5", "claim6-5", false, nil, metaTimeNowUnix, getSize(defaultSize)),
initialClaims: newClaimArray("claim6-5", "pvc-uid6-5", "1Gi", "volume6-5", v1.ClaimBound, &classEmpty),
Expand All @@ -192,7 +192,7 @@ func TestCreateSnapshotSync(t *testing.T) {
{
name: "6-6 - successful create snapshot with status error uploading",
initialContents: nocontents,
expectedContents: newContentArray("snapcontent-snapuid6-6", classGold, "sid6-6", "pv-uid6-6", "volume6-6", "snapuid6-6", "snap6-6", &deletePolicy, &defaultSize, &timeNow),
expectedContents: newContentArray("snapcontent-snapuid6-6", classGold, "sid6-6", "pv-uid6-6", "volume6-6", "snapuid6-6", "snap6-6", &deletePolicy, &defaultSize, &timeNow, false),
initialSnapshots: newSnapshotArray("snap6-6", classGold, "", "snapuid6-6", "claim6-6", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap6-6", classGold, "snapcontent-snapuid6-6", "snapuid6-6", "claim6-6", false, nil, metaTimeNowUnix, getSize(defaultSize)),
initialClaims: newClaimArray("claim6-6", "pvc-uid6-6", "1Gi", "volume6-6", v1.ClaimBound, &classEmpty),
Expand Down
Loading