Merge pull request #2297 from tylerslaton/volume-class-defaults

Only set Status.Defaults.Volumes if it hasn't been set
acorn-io · Nov 3, 2023 · ba42df7 · ba42df7
2 parents 39e8aed + c23d6fa
commit ba42df7
Show file tree

Hide file tree

Showing 38 changed files with 185 additions and 101 deletions.
diff --git a/pkg/controller/appdefinition/testdata/deployspec/filter-user-labels/expected.golden b/pkg/controller/appdefinition/testdata/deployspec/filter-user-labels/expected.golden
@@ -415,8 +415,7 @@ status:
     status: "True"
     success: true
     type: defined
-  defaults:
-    volumeSize: 10G
+  defaults: {}
   namespace: app-created-namespace
   staged:
     appImage:

diff --git a/pkg/controller/appdefinition/testdata/deployspec/labels/expected.golden b/pkg/controller/appdefinition/testdata/deployspec/labels/expected.golden
@@ -423,8 +423,7 @@ status:
     status: "True"
     success: true
     type: defined
-  defaults:
-    volumeSize: 10G
+  defaults: {}
   namespace: app-created-namespace
   staged:
     appImage:

diff --git a/pkg/controller/appdefinition/testdata/deployspec/no-user-labels/expected.golden b/pkg/controller/appdefinition/testdata/deployspec/no-user-labels/expected.golden
@@ -297,8 +297,7 @@ status:
     status: "True"
     success: true
     type: defined
-  defaults:
-    volumeSize: 10G
+  defaults: {}
   namespace: app-created-namespace
   staged:
     appImage:

diff --git a/pkg/controller/appdefinition/testdata/volumes/empty/expected.golden b/pkg/controller/appdefinition/testdata/volumes/empty/expected.golden
@@ -175,8 +175,7 @@ status:
     status: "True"
     success: true
     type: defined
-  defaults:
-    volumeSize: 10G
+  defaults: {}
   namespace: app-created-namespace
   staged:
     appImage:

diff --git a/pkg/controller/appdefinition/testdata/volumes/no-default-class/expected.golden b/pkg/controller/appdefinition/testdata/volumes/no-default-class/expected.golden
@@ -175,8 +175,7 @@ status:
     status: "True"
     success: true
     type: defined
-  defaults:
-    volumeSize: 10G
+  defaults: {}
   namespace: app-created-namespace
   staged:
     appImage:

diff --git a/pkg/controller/appdefinition/volume.go b/pkg/controller/appdefinition/volume.go
@@ -17,9 +17,9 @@ import (
 	"github.com/acorn-io/runtime/pkg/volume"
 	"github.com/acorn-io/z"
 	name2 "github.com/rancher/wrangler/pkg/name"
-	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	klabels "k8s.io/apimachinery/pkg/labels"
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -196,14 +196,16 @@ func toPVCs(req router.Request, appInstance *v1.AppInstance) (result []kclient.O
 			pvc.Spec.VolumeName = pvName
 
 			if volumeRequest.Size == "" {
-				// This shouldn't happen because we set a default in the status. However
-				// if this situation does occur, we'll just use the default size for runtime
-				// and log it at the debug level. As an example, our unit tests hit this case.
-				if appInstance.Status.Defaults.VolumeSize == nil {
-					logrus.Debugf("no default volume size found in status, using static default size of %v", v1.DefaultSize)
-					appInstance.Status.Defaults.VolumeSize = v1.DefaultSize
+				// If the volumeRequest does not have a size set, then we need to determine what default to use. If status.Defaults.Volumes has this
+				// volume set, then we use that. Otherwise, we use v1 package level default size.
+				defaultSize := *v1.DefaultSize
+				if defaultVolume, hasDefaultSet := appInstance.Status.Defaults.Volumes[vol]; hasDefaultSet {
+					defaultSize, err = resource.ParseQuantity(string(defaultVolume.Size))
+					if err != nil {
+						return nil, fmt.Errorf("failed to parse default volume size %q for volume %q: %w", defaultVolume.Size, vol, err)
+					}
 				}
-				pvc.Spec.Resources.Requests[corev1.ResourceStorage] = *appInstance.Status.Defaults.VolumeSize
+				pvc.Spec.Resources.Requests[corev1.ResourceStorage] = defaultSize
 			} else {
 				pvc.Spec.Resources.Requests[corev1.ResourceStorage] = *v1.MustParseResourceQuantity(volumeRequest.Size)
 			}

diff --git a/pkg/controller/defaults/defaults.go b/pkg/controller/defaults/defaults.go
@@ -30,11 +30,10 @@ func Calculate(req router.Request, resp router.Response) (err error) {
 		}
 	}()
 
-	// Only set the default volume size if it hasn't been set yet.
-	if appInstance.Status.Defaults.Volumes == nil {
-		if err := addDefaultVolumeSize(req.Ctx, req.Client, appInstance); err != nil {
-			return err
-		}
+	// addVolumeClassDefaults should run everytime as the function itself will not overwrite any existing
+	// defaults. Effectively, this means that volume defaults only get set if they have not been set before.
+	if err = addVolumeClassDefaults(req.Ctx, req.Client, appInstance); err != nil {
+		return err
 	}
 
 	if appInstance.Generation != appInstance.Status.ObservedGeneration {
@@ -56,10 +55,6 @@ func calculate(req router.Request, appInstance *internalv1.AppInstance) error {
 		return err
 	}
 
-	if err = addVolumeClassDefaults(req.Ctx, req.Client, appInstance); err != nil {
-		return err
-	}
-
 	if err = addDefaultMemory(req, cfg, appInstance); err != nil {
 		return err
 	}

diff --git a/pkg/controller/defaults/testdata/memory/all-set-overwrite/expected.golden b/pkg/controller/defaults/testdata/memory/all-set-overwrite/expected.golden
@@ -51,7 +51,6 @@ status:
       left: 0
       oneimage: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/memory/all-set/expected.golden b/pkg/controller/defaults/testdata/memory/all-set/expected.golden
@@ -50,7 +50,6 @@ status:
       left: 0
       oneimage: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/memory/container/expected.golden b/pkg/controller/defaults/testdata/memory/container/expected.golden
@@ -50,7 +50,6 @@ status:
       left: 0
       oneimage: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/memory/job/expected.golden b/pkg/controller/defaults/testdata/memory/job/expected.golden
@@ -50,7 +50,6 @@ status:
       left: 0
       oneimage: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/memory/overwrite-acornfile-memory/expected.golden b/pkg/controller/defaults/testdata/memory/overwrite-acornfile-memory/expected.golden
@@ -51,7 +51,6 @@ status:
       left: 0
       oneimage: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/memory/same-generation/expected.golden b/pkg/controller/defaults/testdata/memory/same-generation/expected.golden
@@ -49,7 +49,6 @@ status:
       "": 0
       left: 0
       oneimage: 0
-    volumeSize: 10G
   namespace: app-created-namespace
   staged:
     appImage:

diff --git a/pkg/controller/defaults/testdata/memory/sidecar/expected.golden b/pkg/controller/defaults/testdata/memory/sidecar/expected.golden
@@ -50,7 +50,6 @@ status:
       left: 0
       oneimage: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/memory/two-ccc-defaults-should-error/expected.yaml b/pkg/controller/defaults/testdata/memory/two-ccc-defaults-should-error/expected.yaml
@@ -11,7 +11,6 @@ spec:
 status:
   defaults:
     region: local
-    volumeSize: 10G
   observedGeneration: 1
   namespace: app-created-namespace
   appImage:

diff --git a/pkg/controller/defaults/testdata/memory/two-containers/expected.golden b/pkg/controller/defaults/testdata/memory/two-containers/expected.golden
@@ -52,7 +52,6 @@ status:
       oneimage: 0
       twoimage: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/memory/two-pcc-defaults-should-error/expected.yaml b/pkg/controller/defaults/testdata/memory/two-pcc-defaults-should-error/expected.yaml
@@ -11,7 +11,6 @@ spec:
 status:
   defaults:
     region: local
-    volumeSize: 10G
   observedGeneration: 1
   namespace: app-created-namespace
   appImage:

diff --git a/pkg/controller/defaults/testdata/memory/with-acornfile-memory/expected.golden b/pkg/controller/defaults/testdata/memory/with-acornfile-memory/expected.golden
@@ -49,7 +49,6 @@ status:
       left: 0
       oneimage: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/region/default/expected.golden b/pkg/controller/defaults/testdata/region/default/expected.golden
@@ -31,7 +31,6 @@ status:
       "": 0
       container-name: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/region/project-default-status/expected.golden b/pkg/controller/defaults/testdata/region/project-default-status/expected.golden
@@ -31,7 +31,6 @@ status:
       "": 0
       container-name: 0
     region: local
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/region/region-on-spec/expected.golden b/pkg/controller/defaults/testdata/region/region-on-spec/expected.golden
@@ -31,7 +31,6 @@ status:
     memory:
       "": 0
       container-name: 0
-    volumeSize: 10G
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/...troller/defaults/testdata/volumeclass/cluster-and-project-class-same-name/expected.golden b/...troller/defaults/testdata/volumeclass/cluster-and-project-class-same-name/expected.golden
@@ -38,7 +38,6 @@ status:
       "": 0
       container-name: 0
     region: local
-    volumeSize: 10G
     volumes:
       foo:
         accessModes:

diff --git a/pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-same-gen/expected.golden b/pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-same-gen/expected.golden
@@ -30,13 +30,14 @@ status:
   appStatus: {}
   columns: {}
   conditions:
+  - error: true
+    message: 'cannot establish defaults because two defaults volume classes exist:
+      test-volume-class and test-volume-class-1'
     observedGeneration: 1
-    reason: Success
-    status: "True"
-    success: true
+    reason: Error
+    status: "False"
     type: defaults
-  defaults:
-    volumeSize: 10G
+  defaults: {}
   namespace: app-created-namespace
   observedGeneration: 1
   staged:

diff --git a/pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-set/existing.yaml b/pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-set/existing.yaml
@@ -0,0 +1,24 @@
+---
+kind: ProjectVolumeClassInstance
+apiVersion: internal.admin.acorn.io/v1
+metadata:
+  name: test-volume-class
+  namespace: app-namespace
+description: Just a simple test volume class
+default: true
+storageClassName: test-storage-class
+size:
+  min: 1Gi
+  max: 10Gi
+  default: 2Gi
+allowedAccessModes: ["readWriteOnce"]
+---
+kind: ProjectInstance
+apiVersion: internal.acorn.io/v1
+metadata:
+  name: app-namespace
+spec: {}
+status:
+  defaultRegion: local
+  supportedRegions:
+    - local
diff --git a/pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-set/expected.golden b/pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-set/expected.golden
@@ -0,0 +1,54 @@
+`apiVersion: internal.acorn.io/v1
+kind: AppInstance
+metadata:
+  creationTimestamp: null
+  generation: 2
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  image: test
+status:
+  appImage:
+    buildContext: {}
+    id: test
+    imageData: {}
+    vcs: {}
+  appSpec:
+    containers:
+      container-name:
+        dirs:
+          /var/tmp:
+            secret: {}
+            volume: foo
+        image: image-name
+        metrics: {}
+        probes: null
+    volumes:
+      foo: {}
+  appStatus: {}
+  columns: {}
+  conditions:
+    observedGeneration: 2
+    reason: Success
+    status: "True"
+    success: true
+    type: defaults
+  defaults:
+    memory:
+      "": 0
+      container-name: 0
+    region: local
+    volumes:
+      foo:
+        class: test-volume-class
+        size: 4Gi
+  namespace: app-created-namespace
+  observedGeneration: 1
+  staged:
+    appImage:
+      buildContext: {}
+      imageData: {}
+      vcs: {}
+  summary: {}
+`
diff --git a/pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-set/input.yaml b/pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-set/input.yaml
@@ -0,0 +1,26 @@
+kind: AppInstance
+apiVersion: internal.acorn.io/v1
+metadata:
+  generation: 2
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  image: test
+status:
+  observedGeneration: 1
+  namespace: app-created-namespace
+  appImage:
+    id: test
+  appSpec:
+    containers:
+      container-name:
+        image: "image-name"
+        dirs:
+          "/var/tmp":
+            volume: foo
+  defaults:
+    volumes:
+      foo:
+        class: test-volume-class
+        size: 4Gi
diff --git a/...ontroller/defaults/testdata/volumeclass/volume-class-fill-cluster-default/expected.golden b/...ontroller/defaults/testdata/volumeclass/volume-class-fill-cluster-default/expected.golden
@@ -37,7 +37,6 @@ status:
       "": 0
       container-name: 0
     region: local
-    volumeSize: 10G
     volumes:
       foo:
         accessModes:

diff --git a/...roller/defaults/testdata/volumeclass/volume-class-fill-defaults-with-bind/expected.golden b/...roller/defaults/testdata/volumeclass/volume-class-fill-defaults-with-bind/expected.golden
@@ -41,7 +41,6 @@ status:
       "": 0
       container-name: 0
     region: local
-    volumeSize: 10G
     volumes:
       foo:
         accessModes:

diff --git a/pkg/controller/defaults/testdata/volumeclass/volume-class-fill-defaults/expected.golden b/pkg/controller/defaults/testdata/volumeclass/volume-class-fill-defaults/expected.golden
@@ -38,7 +38,6 @@ status:
       "": 0
       container-name: 0
     region: local
-    volumeSize: 10G
     volumes:
       foo:
         accessModes:

diff --git a/...ontroller/defaults/testdata/volumeclass/volume-class-fill-project-default/expected.golden b/...ontroller/defaults/testdata/volumeclass/volume-class-fill-project-default/expected.golden
@@ -37,7 +37,6 @@ status:
       "": 0
       container-name: 0
     region: local
-    volumeSize: 10G
     volumes:
       foo:
         accessModes:

diff --git a/pkg/controller/defaults/testdata/volumeclass/volume-class-fill-size/expected.golden b/pkg/controller/defaults/testdata/volumeclass/volume-class-fill-size/expected.golden
@@ -40,7 +40,6 @@ status:
       "": 0
       container-name: 0
     region: local
-    volumeSize: 10G
     volumes:
       foo:
         size: 2Gi