Disable default PV for Experiment with resume from volume (#1552)

* Remove default PV creation from resume from volume

* Modify error msg

* Remove PV check from Suggestion controller test

* Use go 1.15.13

Author: andreyvelich

.github/workflows/test-go.yaml

@@ -23,7 +23,7 @@ jobs:
       - name: Setup Go
         uses: actions/setup-go@v2
         with:
-          go-version: 1.15.8
+          go-version: 1.15.13
 
       # Verify that go.mod and go.sum is synchronized
       - name: Check Go modules

go.sum

@@ -1368,6 +1368,7 @@ k8s.io/csi-translation-lib v0.20.4/go.mod h1:WisLItCdoDKB4lr6nkhzQsfgBNBJDI/TD9m
 k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
 k8s.io/gengo v0.0.0-20190822140433-26a664648505/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
 k8s.io/gengo v0.0.0-20200413195148-3a45101e95ac/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
+k8s.io/gengo v0.0.0-20201113003025-83324d819ded h1:JApXBKYyB7l9xx+DK7/+mFjC7A9Bt5A93FPvFD0HIFE=
 k8s.io/gengo v0.0.0-20201113003025-83324d819ded/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
 k8s.io/heapster v1.2.0-beta.1/go.mod h1:h1uhptVXMwC8xtZBYsPXKVi8fpdlYkTs6k949KozGrM=
 k8s.io/klog v1.0.0 h1:Pt+yjF5aB1xDSVbau4VsWe+dQNzA0qv1LlXdC2dF6Q8=

pkg/controller.v1beta1/consts/const.go

@@ -119,19 +119,12 @@ const (
 	// DefaultContainerSuggestionVolumeMountPath is the default mount path in suggestion container
 	DefaultContainerSuggestionVolumeMountPath = "/opt/katib/data"
 
-	// DefaultSuggestionStorageClassName is the default value for suggestion's volume storage class name
-	DefaultSuggestionStorageClassName = "katib-suggestion"
-
 	// DefaultSuggestionVolumeStorage is the default value for suggestion's volume storage
 	DefaultSuggestionVolumeStorage = "1Gi"
 
 	// DefaultSuggestionVolumeAccessMode is the default value for suggestion's volume access mode
 	DefaultSuggestionVolumeAccessMode = corev1.ReadWriteOnce
 
-	// DefaultSuggestionVolumeLocalPathPrefix is the default cluster local path prefix for suggestion volume
-	// Full default local path = /tmp/katib/suggestions/<suggestion-name>-<suggestion-algorithm>-<suggestion-namespace>
-	DefaultSuggestionVolumeLocalPathPrefix = "/tmp/katib/suggestions/"
-
 	// ReconcileErrorReason is the reason when there is a reconcile error.
 	ReconcileErrorReason = "ReconcileError"
 

pkg/controller.v1beta1/suggestion/composer/composer.go

@@ -23,6 +23,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -257,17 +258,15 @@ func (g *General) desiredContainers(s *suggestionsv1beta1.Suggestion,
 	return containers
 }
 
-// DesiredVolume returns desired PVC and PV for suggestion.
-// If StorageClassName != DefaultSuggestionStorageClassName returns only PVC.
+// DesiredVolume returns desired PVC and PV for Suggestion.
+// If PV doesn't exist in Katib config return nil for PV.
 func (g *General) DesiredVolume(s *suggestionsv1beta1.Suggestion) (*corev1.PersistentVolumeClaim, *corev1.PersistentVolume, error) {
 
 	suggestionConfigData, err := katibconfig.GetSuggestionConfigData(s.Spec.Algorithm.AlgorithmName, g.Client)
 	if err != nil {
 		return nil, nil, err
 	}
 
-	persistentVolumeName := util.GetSuggestionPersistentVolumeName(s)
-
 	pvc := &corev1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      util.GetSuggestionPersistentVolumeClaimName(s),
@@ -282,24 +281,19 @@ func (g *General) DesiredVolume(s *suggestionsv1beta1.Suggestion) (*corev1.Persi
 	}
 
 	var pv *corev1.PersistentVolume
-	// Create PV with local hostPath by default
-	if *pvc.Spec.StorageClassName == consts.DefaultSuggestionStorageClassName {
-		localLabel := map[string]string{"type": "local"}
+	// Create PV if Katib config contains it.
+	if !equality.Semantic.DeepEqual(suggestionConfigData.PersistentVolumeSpec, corev1.PersistentVolumeSpec{}) {
+
+		persistentVolumeName := util.GetSuggestionPersistentVolumeName(s)
 
 		pv = &corev1.PersistentVolume{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:   persistentVolumeName,
-				Labels: localLabel,
+				Labels: suggestionConfigData.PersistentVolumeLabels,
 			},
 			Spec: suggestionConfigData.PersistentVolumeSpec,
 		}
 
-		// If default host path is specified attach pv name to the path.
-		// Full default local path = DefaultSuggestionVolumeLocalPathPrefix<suggestion-name>-<suggestion-algorithm>-<suggestion-namespace>
-		if pv.Spec.PersistentVolumeSource.HostPath != nil &&
-			pv.Spec.PersistentVolumeSource.HostPath.Path == consts.DefaultSuggestionVolumeLocalPathPrefix {
-			pv.Spec.PersistentVolumeSource.HostPath.Path = pv.Spec.PersistentVolumeSource.HostPath.Path + persistentVolumeName
-		}
 	}
 
 	return pvc, pv, nil

pkg/controller.v1beta1/suggestion/composer/composer_test.go

@@ -92,7 +92,8 @@ var (
 
 	refFlag bool = true
 
-	storageClassName = consts.DefaultSuggestionStorageClassName
+	storageClassName = "test-storage-class"
+	pvLabels         = map[string]string{"type": "local"}
 )
 
 func TestMain(m *testing.M) {
@@ -401,106 +402,36 @@ func TestDesiredVolume(t *testing.T) {
 			suggestion:      newFakeSuggestion(),
 			configMap:       newFakeKatibConfig(newFakeSuggestionConfig(), newFakeEarlyStoppingConfig()),
 			expectedPVC:     newFakePVC(),
-			expectedPV:      newFakePV(),
-			err:             false,
-			testDescription: "Desired Volume valid run with default pvc and pv",
-		},
-		{
-			suggestion: newFakeSuggestion(),
-			configMap: func() *corev1.ConfigMap {
-				sc := newFakeSuggestionConfig()
-				storageClass := "custom-storage-class"
-				volumeStorage, _ := resource.ParseQuantity("5Gi")
-
-				sc.PersistentVolumeClaimSpec = corev1.PersistentVolumeClaimSpec{
-					StorageClassName: &storageClass,
-					AccessModes: []corev1.PersistentVolumeAccessMode{
-						corev1.ReadWriteOnce,
-						corev1.ReadOnlyMany,
-					},
-					Resources: corev1.ResourceRequirements{
-						Requests: corev1.ResourceList{
-							corev1.ResourceStorage: volumeStorage,
-						},
-					},
-				}
-				cm := newFakeKatibConfig(sc, newFakeEarlyStoppingConfig())
-				return cm
-			}(),
-			expectedPVC: func() *corev1.PersistentVolumeClaim {
-				pvc := newFakePVC()
-				storageClass := "custom-storage-class"
-				volumeStorage, _ := resource.ParseQuantity("5Gi")
-
-				pvc.Spec.StorageClassName = &storageClass
-				pvc.Spec.AccessModes = append(pvc.Spec.AccessModes, corev1.ReadOnlyMany)
-				pvc.Spec.Resources.Requests[corev1.ResourceStorage] = volumeStorage
-				return pvc
-			}(),
 			expectedPV:      nil,
 			err:             false,
-			testDescription: "Custom PVC with not default storage class",
+			testDescription: "Desired Volume valid run with default PVC",
 		},
 		{
 			suggestion: newFakeSuggestion(),
 			configMap: func() *corev1.ConfigMap {
 				sc := newFakeSuggestionConfig()
-				mode := corev1.PersistentVolumeFilesystem
-				accessModes := []corev1.PersistentVolumeAccessMode{
-					corev1.ReadWriteOnce,
-					corev1.ReadOnlyMany,
-				}
-				volumeStorage, _ := resource.ParseQuantity("10Gi")
-
-				sc.PersistentVolumeClaimSpec = corev1.PersistentVolumeClaimSpec{
-					VolumeMode:  &mode,
-					AccessModes: accessModes,
-				}
-
-				sc.PersistentVolumeSpec = corev1.PersistentVolumeSpec{
-					VolumeMode:  &mode,
-					AccessModes: accessModes,
-					PersistentVolumeSource: corev1.PersistentVolumeSource{
-						GCEPersistentDisk: &corev1.GCEPersistentDiskVolumeSource{
-							PDName: "pd-name",
-							FSType: "fs-type",
-						},
-					},
-					Capacity: corev1.ResourceList{
-						corev1.ResourceStorage: volumeStorage,
-					},
-				}
+
+				sc.PersistentVolumeClaimSpec = newFakePVC().Spec
+
+				// Change StorageClass and volume storage.
+				sc.PersistentVolumeClaimSpec.StorageClassName = &storageClassName
+
+				sc.PersistentVolumeSpec = newFakePV().Spec
+				// This policy will be changed to "Delete".
+				sc.PersistentVolumeSpec.PersistentVolumeReclaimPolicy = corev1.PersistentVolumeReclaimRetain
+				sc.PersistentVolumeLabels = pvLabels
+
 				cm := newFakeKatibConfig(sc, newFakeEarlyStoppingConfig())
 				return cm
 			}(),
 			expectedPVC: func() *corev1.PersistentVolumeClaim {
 				pvc := newFakePVC()
-				mode := corev1.PersistentVolumeFilesystem
-
-				pvc.Spec.VolumeMode = &mode
-				pvc.Spec.AccessModes = append(pvc.Spec.AccessModes, corev1.ReadOnlyMany)
+				pvc.Spec.StorageClassName = &storageClassName
 				return pvc
 			}(),
-			expectedPV: func() *corev1.PersistentVolume {
-				pv := newFakePV()
-				mode := corev1.PersistentVolumeFilesystem
-				volumeStorage, _ := resource.ParseQuantity("10Gi")
-
-				pv.Spec.VolumeMode = &mode
-				pv.Spec.AccessModes = append(pv.Spec.AccessModes, corev1.ReadOnlyMany)
-				pv.Spec.PersistentVolumeSource = corev1.PersistentVolumeSource{
-					GCEPersistentDisk: &corev1.GCEPersistentDiskVolumeSource{
-						PDName: "pd-name",
-						FSType: "fs-type",
-					},
-				}
-				pv.Spec.Capacity = corev1.ResourceList{
-					corev1.ResourceStorage: volumeStorage,
-				}
-				return pv
-			}(),
+			expectedPV:      newFakePV(),
 			err:             false,
-			testDescription: "Custom PVC and PV with default storage class",
+			testDescription: "Custom PVC and PV",
 		},
 	}
 
@@ -915,7 +846,6 @@ func newFakePVC() *corev1.PersistentVolumeClaim {
 			},
 		},
 		Spec: corev1.PersistentVolumeClaimSpec{
-			StorageClassName: &storageClassName,
 			AccessModes: []corev1.PersistentVolumeAccessMode{
 				consts.DefaultSuggestionVolumeAccessMode,
 			},
@@ -934,10 +864,8 @@ func newFakePV() *corev1.PersistentVolume {
 
 	return &corev1.PersistentVolume{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: pvName,
-			Labels: map[string]string{
-				"type": "local",
-			},
+			Name:   pvName,
+			Labels: pvLabels,
 		},
 		Spec: corev1.PersistentVolumeSpec{
 			StorageClassName:              storageClassName,
@@ -947,7 +875,7 @@ func newFakePV() *corev1.PersistentVolume {
 			},
 			PersistentVolumeSource: corev1.PersistentVolumeSource{
 				HostPath: &corev1.HostPathVolumeSource{
-					Path: consts.DefaultSuggestionVolumeLocalPathPrefix + pvName,
+					Path: "tmp/katib/suggestion" + pvName,
 				},
 			},
 			Capacity: corev1.ResourceList{

pkg/controller.v1beta1/suggestion/suggestion_controller_test.go

@@ -164,11 +164,6 @@ func TestReconcile(t *testing.T) {
 		return c.Get(context.TODO(), types.NamespacedName{Namespace: namespace, Name: resourceName}, &corev1.Service{})
 	}, timeout).Should(gomega.Succeed())
 
-	// Expect that PV with appropriate name is created
-	g.Eventually(func() error {
-		return c.Get(context.TODO(), types.NamespacedName{Name: resourceName + "-" + namespace}, &corev1.PersistentVolume{})
-	}, timeout).Should(gomega.Succeed())
-
 	// Expect that PVC with appropriate name is created
 	g.Eventually(func() error {
 		return c.Get(context.TODO(), types.NamespacedName{Namespace: namespace, Name: resourceName}, &corev1.PersistentVolumeClaim{})

pkg/controller.v1beta1/suggestion/suggestion_controller_util.go

@@ -65,13 +65,13 @@ func (r *ReconcileSuggestion) reconcileVolume(
 	foundPVC := &corev1.PersistentVolumeClaim{}
 	foundPV := &corev1.PersistentVolume{}
 
-	// Try to find/create PV, if PV has to be created
+	// Try to find/create PV, if PV has to be created.
 	if pv != nil {
 		err := r.Get(context.TODO(), types.NamespacedName{Name: pv.Name}, foundPV)
 		if err != nil && errors.IsNotFound(err) {
 			logger.Info("Creating Persistent Volume", "name", pv.Name)
 			err = r.Create(context.TODO(), pv)
-			// Return only if Create was failed, otherwise try to find/create PVC
+			// Return only if Create was failed, otherwise try to find/create PVC.
 			if err != nil {
 				return nil, nil, err
 			}
@@ -80,7 +80,7 @@ func (r *ReconcileSuggestion) reconcileVolume(
 		}
 	}
 
-	// Try to find/create PVC
+	// Try to find/create PVC.
 	err := r.Get(context.TODO(), types.NamespacedName{Name: pvc.Name, Namespace: pvc.Namespace}, foundPVC)
 	if err != nil && errors.IsNotFound(err) {
 		logger.Info("Creating Persistent Volume Claim", "name", pvc.Name)