Fix PVC deletion (#1221)

* fix pvc deletion

* local copy

* add pvc list permission

* improve pvc reconciliation

* fix order

* cancel resource

* cancel namespace deletion when pvcs remain

* fix labels

helm/kvm-operator/templates/rbac.yaml

@@ -88,12 +88,14 @@ rules:
       - get
       - create
       - delete
+      - list
   - apiGroups:
       - ""
     resources:
       - persistentvolumes
     verbs:
       - list
+      - update
   - apiGroups:
       - ""
     resources:

service/controller/cluster_resource_set.go

@@ -280,9 +280,9 @@ func newClusterResources(config ClusterConfig) ([]resource.Interface, error) {
 		namespaceResource,
 		serviceAccountResource,
 		configMapResource,
+		pvcResource,
 		deploymentResource,
 		ingressResource,
-		pvcResource,
 		serviceResource,
 		nodeControllerResource,
 	}

service/controller/key/key.go

@@ -109,6 +109,7 @@ const (
 	LabelCluster       = "giantswarm.io/cluster"
 	LabelCustomer      = "customer"
 	LabelManagedBy     = "giantswarm.io/managed-by"
+	LabelMountTag      = "mount-tag"
 	LabelOrganization  = "giantswarm.io/organization"
 	LabelVersionBundle = "giantswarm.io/version-bundle"
 
@@ -626,16 +627,6 @@ func PortMappings(customObject v1alpha1.KVMConfig) []corev1.ServicePort {
 	return ports
 }
 
-func PVCNames(customObject v1alpha1.KVMConfig) []string {
-	var names []string
-
-	for i := range customObject.Spec.Cluster.Masters {
-		names = append(names, EtcdPVCName(ClusterID(customObject), VMNumber(i)))
-	}
-
-	return names
-}
-
 func ReleaseVersion(cr v1alpha1.KVMConfig) string {
 	return cr.GetLabels()[label.ReleaseVersion]
 }
@@ -656,7 +647,7 @@ func ShutdownDeferrerPollPath(customObject v1alpha1.KVMConfig) string {
 	return fmt.Sprintf("%s/v1/defer/", ShutdownDeferrerListenAddress(customObject))
 }
 
-func StorageType(customObject v1alpha1.KVMConfig) string {
+func EtcdStorageType(customObject v1alpha1.KVMConfig) string {
 	return customObject.Spec.KVM.K8sKVM.StorageType
 }
 

service/controller/resource/deployment/master_deployment.go

@@ -42,7 +42,7 @@ func newMasterDeployments(customResource v1alpha1.KVMConfig, release *releasev1a
 			return nil, microerror.Maskf(invalidConfigError, "error creating memory quantity: %s", err)
 		}
 
-		storageType := key.StorageType(customResource)
+		storageType := key.EtcdStorageType(customResource)
 
 		// During migration, some TPOs do not have storage type set.
 		// This specifies a default, until all TPOs have the correct storage type set.
@@ -71,7 +71,7 @@ func newMasterDeployments(customResource v1alpha1.KVMConfig, release *releasev1a
 				},
 			}
 		} else {
-			return nil, microerror.Maskf(wrongTypeError, "unknown storageType: '%s'", key.StorageType(customResource))
+			return nil, microerror.Maskf(wrongTypeError, "unknown storageType: '%s'", key.EtcdStorageType(customResource))
 		}
 		deployment := &v1.Deployment{
 			TypeMeta: metav1.TypeMeta{

service/controller/resource/namespace/current.go

@@ -72,18 +72,29 @@ func (r *Resource) GetCurrentState(ctx context.Context, obj interface{}) (interf
 	// we get an empty list here after the delete event got replayed. Then we just
 	// remove the namespace as usual.
 	if key.IsDeleted(&customObject) {
-		n := key.ClusterNamespace(customObject)
+		namespace := key.ClusterNamespace(customObject)
 
-		lo := metav1.ListOptions{
+		deployments, err := r.k8sClient.AppsV1().Deployments(namespace).List(ctx, metav1.ListOptions{
 			LabelSelector: fmt.Sprintf("%s=%s", label.ManagedBy, key.OperatorName),
+		})
+		if err != nil {
+			return nil, microerror.Mask(err)
 		}
 
-		list, err := r.k8sClient.AppsV1().Deployments(n).List(ctx, lo)
+		// Ensure PVCs have been deleted so that bound PVs are properly cleaned up.
+		volumeClaims, err := r.k8sClient.CoreV1().PersistentVolumeClaims(namespace).List(ctx, metav1.ListOptions{
+			LabelSelector: fmt.Sprintf("%s=%s", label.ManagedBy, key.OperatorName),
+		})
 		if err != nil {
 			return nil, microerror.Mask(err)
 		}
-		if len(list.Items) != 0 {
-			r.logger.Debugf(ctx, "cannot finish deletion of namespace due to existing deployments")
+
+		if len(deployments.Items) != 0 || len(volumeClaims.Items) != 0 {
+			if len(deployments.Items) != 0 {
+				r.logger.Debugf(ctx, "cannot finish deletion of namespace due to existing deployments")
+			} else {
+				r.logger.Debugf(ctx, "cannot finish deletion of namespace due to existing PVCs")
+			}
 			resourcecanceledcontext.SetCanceled(ctx)
 			finalizerskeptcontext.SetKept(ctx)
 			r.logger.Debugf(ctx, "canceling resource")

service/controller/resource/pvc/create.go

@@ -25,8 +25,8 @@ func (r *Resource) ApplyCreateChange(ctx context.Context, obj, createChange inte
 		r.logger.Debugf(ctx, "creating the PVCs in the Kubernetes API")
 
 		namespace := key.ClusterNamespace(customObject)
-		for _, PVC := range pvcsToCreate {
-			_, err := r.k8sClient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, PVC, v1.CreateOptions{})
+		for _, persistentVolumeClaim := range pvcsToCreate {
+			_, err := r.k8sClient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, persistentVolumeClaim.DeepCopy(), v1.CreateOptions{})
 			if apierrors.IsAlreadyExists(err) {
 				// fall through
 			} else if err != nil {
@@ -54,7 +54,7 @@ func (r *Resource) newCreateChange(ctx context.Context, obj, currentState, desir
 
 	r.logger.Debugf(ctx, "finding out which PVCs have to be created")
 
-	var pvcsToCreate []*corev1.PersistentVolumeClaim
+	var pvcsToCreate []corev1.PersistentVolumeClaim
 
 	for _, desiredPVC := range desiredPVCs {
 		if !containsPVC(currentPVCs, desiredPVC) {

service/controller/resource/pvc/create_test.go

@@ -28,8 +28,8 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 					},
 				},
 			},
-			CurrentState:     []*corev1.PersistentVolumeClaim{},
-			DesiredState:     []*corev1.PersistentVolumeClaim{},
+			CurrentState:     []corev1.PersistentVolumeClaim{},
+			DesiredState:     []corev1.PersistentVolumeClaim{},
 			ExpectedPVCNames: []string{},
 		},
 
@@ -43,14 +43,14 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 					},
 				},
 			},
-			CurrentState: []*corev1.PersistentVolumeClaim{
+			CurrentState: []corev1.PersistentVolumeClaim{
 				{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "pvc-1",
 					},
 				},
 			},
-			DesiredState: []*corev1.PersistentVolumeClaim{
+			DesiredState: []corev1.PersistentVolumeClaim{
 				{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "pvc-1",
@@ -70,8 +70,8 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 					},
 				},
 			},
-			CurrentState: []*corev1.PersistentVolumeClaim{},
-			DesiredState: []*corev1.PersistentVolumeClaim{
+			CurrentState: []corev1.PersistentVolumeClaim{},
+			DesiredState: []corev1.PersistentVolumeClaim{
 				{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "pvc-1",
@@ -93,8 +93,8 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 					},
 				},
 			},
-			CurrentState: []*corev1.PersistentVolumeClaim{},
-			DesiredState: []*corev1.PersistentVolumeClaim{
+			CurrentState: []corev1.PersistentVolumeClaim{},
+			DesiredState: []corev1.PersistentVolumeClaim{
 				{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "pvc-1",
@@ -123,14 +123,14 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 					},
 				},
 			},
-			CurrentState: []*corev1.PersistentVolumeClaim{
+			CurrentState: []corev1.PersistentVolumeClaim{
 				{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "pvc-1",
 					},
 				},
 			},
-			DesiredState:     []*corev1.PersistentVolumeClaim{},
+			DesiredState:     []corev1.PersistentVolumeClaim{},
 			ExpectedPVCNames: []string{},
 		},
 
@@ -145,7 +145,7 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 					},
 				},
 			},
-			CurrentState: []*corev1.PersistentVolumeClaim{
+			CurrentState: []corev1.PersistentVolumeClaim{
 				{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "pvc-1",
@@ -157,7 +157,7 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 					},
 				},
 			},
-			DesiredState:     []*corev1.PersistentVolumeClaim{},
+			DesiredState:     []corev1.PersistentVolumeClaim{},
 			ExpectedPVCNames: []string{},
 		},
 
@@ -172,7 +172,7 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 					},
 				},
 			},
-			CurrentState: []*corev1.PersistentVolumeClaim{
+			CurrentState: []corev1.PersistentVolumeClaim{
 				{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "pvc-1",
@@ -184,7 +184,7 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 					},
 				},
 			},
-			DesiredState: []*corev1.PersistentVolumeClaim{
+			DesiredState: []corev1.PersistentVolumeClaim{
 				{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "pvc-1",
@@ -231,9 +231,9 @@ func Test_Resource_PVC_newCreateChange(t *testing.T) {
 			t.Fatalf("case %d expected %#v got %#v", i+1, nil, err)
 		}
 
-		configMaps, ok := result.([]*corev1.PersistentVolumeClaim)
+		configMaps, ok := result.([]corev1.PersistentVolumeClaim)
 		if !ok {
-			t.Fatalf("case %d expected %T got %T", i+1, []*corev1.PersistentVolumeClaim{}, result)
+			t.Fatalf("case %d expected %T got %T", i+1, []corev1.PersistentVolumeClaim{}, result)
 		}
 
 		if len(configMaps) != len(tc.ExpectedPVCNames) {

service/controller/resource/pvc/current.go

@@ -2,10 +2,9 @@ package pvc
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/giantswarm/microerror"
-	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/giantswarm/kvm-operator/service/controller/key"
@@ -19,25 +18,15 @@ func (r *Resource) GetCurrentState(ctx context.Context, obj interface{}) (interf
 
 	r.logger.Debugf(ctx, "looking for PVCs in the Kubernetes API")
 
-	var PVCs []*corev1.PersistentVolumeClaim
-
 	namespace := key.ClusterNamespace(customObject)
-	pvcNames := key.PVCNames(customObject)
-
-	for _, name := range pvcNames {
-		manifest, err := r.k8sClient.CoreV1().PersistentVolumeClaims(namespace).Get(ctx, name, metav1.GetOptions{})
-		if apierrors.IsNotFound(err) {
-			r.logger.Debugf(ctx, "did not find a PVC in the Kubernetes API")
-			// fall through
-		} else if err != nil {
-			return nil, microerror.Mask(err)
-		} else {
-			r.logger.Debugf(ctx, "found a PVC in the Kubernetes API")
-			PVCs = append(PVCs, manifest)
-		}
+	pvcs, err := r.k8sClient.CoreV1().PersistentVolumeClaims(namespace).List(ctx, metav1.ListOptions{
+		LabelSelector: fmt.Sprintf("%s=%s", key.LegacyLabelCluster, key.ClusterID(customObject)),
+	})
+	if err != nil {
+		return nil, microerror.Mask(err)
 	}
 
-	r.logger.Debugf(ctx, "found %d PVCs in the Kubernetes API", len(PVCs))
+	r.logger.Debugf(ctx, "found %d PVCs in the Kubernetes API", len(pvcs.Items))
 
-	return PVCs, nil
+	return pvcs.Items, nil
 }

service/controller/resource/pvc/delete.go

@@ -25,7 +25,9 @@ func (r *Resource) ApplyDeleteChange(ctx context.Context, obj, deleteChange inte
 	if len(pvcsToDelete) != 0 {
 		r.logger.Debugf(ctx, "deleting the PVCs in the Kubernetes API")
 
-		pvsList, err := r.k8sClient.CoreV1().PersistentVolumes().List(ctx, metav1.ListOptions{LabelSelector: LabelMountTag})
+		persistentVolumes, err := r.k8sClient.CoreV1().PersistentVolumes().List(ctx, metav1.ListOptions{
+			LabelSelector: key.LabelMountTag,
+		})
 		if err != nil {
 			return microerror.Mask(err)
 		}
@@ -45,27 +47,23 @@ func (r *Resource) ApplyDeleteChange(ctx context.Context, obj, deleteChange inte
 				continue
 			}
 
-			var boundPV *corev1.PersistentVolume
-			for _, pv := range pvsList.Items {
-				ref := pv.Spec.ClaimRef
-				if ref.Name != pvc.Name && ref.Namespace != namespace {
-					continue
+			// Find PV with claim pointing to the deleted PVC
+			for _, persistentVolume := range persistentVolumes.Items {
+				if persistentVolume.Spec.ClaimRef != nil &&
+					persistentVolume.Spec.ClaimRef.Name == pvc.Name &&
+					persistentVolume.Spec.ClaimRef.Namespace == namespace {
+					persistentVolume := persistentVolume.DeepCopy()
+					// Remove the claim
+					persistentVolume.Spec.ClaimRef = nil
+					_, err = r.k8sClient.CoreV1().PersistentVolumes().Update(ctx, persistentVolume, metav1.UpdateOptions{})
+					if err != nil {
+						return microerror.Mask(err)
+					}
+
+					r.logger.Debugf(ctx, "unbound PV %#q from PVC %#q", persistentVolume.Name, pvc.Name)
+					break
 				}
-
-				boundPV = pv.DeepCopy()
-			}
-
-			if boundPV == nil || boundPV.Spec.ClaimRef == nil {
-				continue
 			}
-
-			boundPV.Spec.ClaimRef = nil
-			_, err = r.k8sClient.CoreV1().PersistentVolumes().Update(ctx, boundPV, metav1.UpdateOptions{})
-			if err != nil {
-				return microerror.Mask(err)
-			}
-
-			r.logger.Debugf(ctx, "unbound PV %s from pvc %s", boundPV.Name, pvc.Name)
 		}
 
 		r.logger.Debugf(ctx, "deleted the PVCs in the Kubernetes API")
@@ -77,13 +75,13 @@ func (r *Resource) ApplyDeleteChange(ctx context.Context, obj, deleteChange inte
 }
 
 func (r *Resource) NewDeletePatch(ctx context.Context, obj, currentState, desiredState interface{}) (*crud.Patch, error) {
-	delete, err := r.newDeleteChange(ctx, obj, currentState, desiredState)
+	deleteChange, err := r.newDeleteChange(ctx, obj, currentState, desiredState)
 	if err != nil {
 		return nil, microerror.Mask(err)
 	}
 
 	patch := crud.NewPatch()
-	patch.SetDeleteChange(delete)
+	patch.SetDeleteChange(deleteChange)
 
 	return patch, nil
 }
@@ -100,7 +98,7 @@ func (r *Resource) newDeleteChange(ctx context.Context, obj, currentState, desir
 
 	r.logger.Debugf(ctx, "finding out which PVCs have to be deleted")
 
-	var pvcsToDelete []*corev1.PersistentVolumeClaim
+	var pvcsToDelete []corev1.PersistentVolumeClaim
 
 	for _, currentPVC := range currentPVCs {
 		if containsPVC(desiredPVCs, currentPVC) {