Merge pull request #2371 from tylerslaton/revert-volume-size-removal

Author: tylerslaton

pkg/apis/internal.acorn.io/v1/appinstance.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -229,9 +230,10 @@ type AppStatusStaged struct {
 }
 
 type Defaults struct {
-	Volumes map[string]VolumeDefault `json:"volumes,omitempty"`
-	Memory  map[string]*int64        `json:"memory,omitempty"`
-	Region  string                   `json:"region,omitempty"`
+	VolumeSize *resource.Quantity       `json:"volumeSize,omitempty"`
+	Volumes    map[string]VolumeDefault `json:"volumes,omitempty"`
+	Memory     map[string]*int64        `json:"memory,omitempty"`
+	Region     string                   `json:"region,omitempty"`
 }
 
 type VolumeDefault struct {

pkg/apis/internal.acorn.io/v1/zz_generated.deepcopy.go

@@ -30,6 +30,12 @@ func Calculate(req router.Request, resp router.Response) (err error) {
 		}
 	}()
 
+	// 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 {
 		if err = calculate(req, appInstance); err != nil {
 			return err
@@ -49,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
 	}

pkg/controller/defaults/defaults.go

@@ -30,10 +30,12 @@ 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: {}
   namespace: app-created-namespace

pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-same-gen/expected.golden

@@ -20,8 +20,6 @@ status:
             volume: foo
     volumes:
       foo: {}
-  defaults:
-    region: local
   conditions:
     - error: true
       message: 'cannot establish defaults because two defaults volume classes exist: test-volume-class and test-volume-class-1'

pkg/controller/defaults/testdata/volumeclass/volume-class-two-cluster-defaults/expected.yaml

@@ -20,8 +20,6 @@ status:
             volume: foo
     volumes:
       foo: {}
-  defaults:
-    region: local
   conditions:
     - error: true
       message: 'cannot establish defaults because two defaults volume classes exist: test-volume-class and test-volume-class-1'

pkg/controller/defaults/testdata/volumeclass/volume-class-two-project-defaults/expected.yaml

@@ -37,28 +37,38 @@ func addVolumeClassDefaults(ctx context.Context, c kclient.Client, app *v1.AppIn
 	})
 
 	for name, vol := range app.Status.AppSpec.Volumes {
-		if _, alreadySet := app.Status.Defaults.Volumes[name]; alreadySet {
-			continue
-		}
-
 		volDefaults := app.Status.Defaults.Volumes[name]
 		vol = volume.CopyVolumeDefaults(vol, volumeBindings[name], volDefaults)
 
-		if vol.Class == "" && defaultVolumeClass != nil {
-			volDefaults.Class = defaultVolumeClass.Name
-			vol.Class = volDefaults.Class
-		}
-		if len(vol.AccessModes) == 0 {
-			volDefaults.AccessModes = volumeClasses[vol.Class].AllowedAccessModes
+		// This is a bit of a hack as we're migrating away from the VolumeSize field. Essentially,
+		// we want to ensure that app.Status.Volumes[name] always has a size set. If the VolumeSize
+		// field has been set in the past, we want to mirgrate that over to be set on app.Status.Volumes[name].
+		// There is another edge case where the Size field was set by a VolumeClass's default size. In this
+		// case we want to leave the Size field alone.
+		if app.Status.Defaults.VolumeSize != nil && volDefaults.Size == "" {
+			volDefaults.Size = v1.Quantity(app.Status.Defaults.VolumeSize.String())
 		}
-		if vol.Size == "" {
-			volDefaults.Size = volumeClasses[vol.Class].Size.Default
-			if volDefaults.Size == "" {
-				defaultSize, err := getDefaultVolumeSize(ctx, c)
-				if err != nil {
-					return err
+
+		// If the Volume already has defaults, skip these steps. We don't want to overwrite
+		// default class or access modes for volumes as it can lead to unexpected behavior when
+		// volume classes are updated.
+		if _, alreadySet := app.Status.Defaults.Volumes[name]; !alreadySet {
+			if vol.Class == "" && defaultVolumeClass != nil {
+				volDefaults.Class = defaultVolumeClass.Name
+				vol.Class = volDefaults.Class
+			}
+			if len(vol.AccessModes) == 0 {
+				volDefaults.AccessModes = volumeClasses[vol.Class].AllowedAccessModes
+			}
+			if vol.Size == "" {
+				volDefaults.Size = volumeClasses[vol.Class].Size.Default
+				if volDefaults.Size == "" {
+					defaultSize, err := getDefaultVolumeSize(ctx, c)
+					if err != nil {
+						return err
+					}
+					volDefaults.Size = defaultSize
 				}
-				volDefaults.Size = defaultSize
 			}
 		}