Add storageClassName as an image resource field

- This can be used to set the storageClassName used for the cache pvc - The field is immutable as the pvc field is also immutable - The field is not considered when checking if the image volume cache config has changed
buildpacks-community · Sep 15, 2022 · 29d6f1c · 29d6f1c
1 parent 18fb2c8
commit 29d6f1c
Show file tree

Hide file tree

Showing 6 changed files with 102 additions and 10 deletions.
diff --git a/docs/image.md b/docs/image.md
@@ -14,6 +14,7 @@ The following defines the relevant fields of the `image` resource spec in more d
 - `source`: The source code that will be monitored/built into images. See the [Source Configuration](#source-config) section below.
 - `cache`: Caching configuration, two variants are available:
   - `volume.size`: Creates a Volume Claim of the given size
+  - `volume.storageClassName`: (Optional) Creates a Volume Claim of the given storageClassName. If unset, the default storage class is used. The field is immutable.
   - `registry.tag`: Creates an image with cached contents
 - `failedBuildHistoryLimit`: The maximum number of failed builds for an image that will be retained.
 - `successBuildHistoryLimit`: The maximum number of successful builds for an image that will be retained.
@@ -254,6 +255,7 @@ spec:
   cache:
     volume:
       size: "1.5Gi" # Optional, if not set then the caching feature is disabled
+      storageClassName: "my-storage-class" # Optional, if not set then the default storageclass is used
   failedBuildHistoryLimit: 5 # Optional, if not present defaults to 10
   successBuildHistoryLimit: 5 # Optional, if not present defaults to 10
   source:
@@ -299,6 +301,7 @@ spec:
   cache:
     volume:
       size: "1.5Gi" # Optional, if not set then the caching feature is disabled
+      storageClassName: "my-storage-class" # Optional, if not set then the default storageclass is used
   failedBuildHistoryLimit: 5 # Optional, if not present defaults to 10
   successBuildHistoryLimit: 5 # Optional, if not present defaults to 10
   source:

diff --git a/pkg/apis/build/v1alpha2/image_builds.go b/pkg/apis/build/v1alpha2/image_builds.go
@@ -205,6 +205,10 @@ func (im *Image) CacheName() string {
 }
 
 func (im *Image) BuildCache() *corev1.PersistentVolumeClaim {
+	var storageClassName *string
+	if im.Spec.Cache.Volume.StorageClassName != "" {
+		storageClassName = &im.Spec.Cache.Volume.StorageClassName
+	}
 	return &corev1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      im.CacheName(),
@@ -221,6 +225,7 @@ func (im *Image) BuildCache() *corev1.PersistentVolumeClaim {
 					corev1.ResourceStorage: *im.Spec.Cache.Volume.Size,
 				},
 			},
+			StorageClassName: storageClassName,
 		},
 	}
 }

diff --git a/pkg/apis/build/v1alpha2/image_types.go b/pkg/apis/build/v1alpha2/image_types.go
@@ -88,7 +88,8 @@ type ImageCacheConfig struct {
 
 // +k8s:openapi-gen=true
 type ImagePersistentVolumeCache struct {
-	Size *resource.Quantity `json:"size,omitempty"`
+	Size             *resource.Quantity `json:"size,omitempty"`
+	StorageClassName string             `json:"storageClassName,omitempty"`
 }
 
 // +k8s:openapi-gen=true

diff --git a/pkg/apis/build/v1alpha2/image_validation.go b/pkg/apis/build/v1alpha2/image_validation.go
@@ -127,8 +127,8 @@ func (is *ImageSpec) validateSameRegistry() *apis.FieldError {
 }
 
 func (is *ImageSpec) validateVolumeCache(ctx context.Context) *apis.FieldError {
-	if is.Cache != nil && is.Cache.Volume != nil && ctx.Value(HasDefaultStorageClass) == nil {
-		return apis.ErrGeneric("spec.cache.volume.size cannot be set with no default StorageClass")
+	if is.Cache != nil && is.Cache.Volume != nil && is.Cache.Volume.StorageClassName == "" && ctx.Value(HasDefaultStorageClass) == nil {
+		return apis.ErrGeneric("spec.cache.volume.size cannot be set without spec.cache.volume.storageClassName or a default StorageClass")
 	}
 
 	if apis.IsInUpdate(ctx) {
@@ -147,6 +147,8 @@ func (is *ImageSpec) validateVolumeCache(ctx context.Context) *apis.FieldError {
 					Details: fmt.Sprintf("current: %v, requested: %v", original.Spec.Cache.Volume.Size, is.Cache.Volume.Size),
 				}
 			}
+
+			return validate.ImmutableField(original.Spec.Cache.Volume.StorageClassName, is.Cache.Volume.StorageClassName, "cache.volume.storageClassName")
 		}
 	}
 
@@ -194,7 +196,7 @@ func (is *ImageSpec) validateBuildHistoryLimit() *apis.FieldError {
 	return nil
 }
 
-func (c *ImageCacheConfig) Validate(context context.Context) *apis.FieldError {
+func (c *ImageCacheConfig) Validate(ctx context.Context) *apis.FieldError {
 	if c != nil && c.Volume != nil && c.Registry != nil {
 		return apis.ErrGeneric("only one type of cache can be specified", "volume", "registry")
 	}

diff --git a/pkg/apis/build/v1alpha2/image_validation_test.go b/pkg/apis/build/v1alpha2/image_validation_test.go
@@ -44,7 +44,8 @@ func testImageValidation(t *testing.T, when spec.G, it spec.S) {
 			},
 			Cache: &ImageCacheConfig{
 				Volume: &ImagePersistentVolumeCache{
-					Size: &cacheSize,
+					Size:             &cacheSize,
+					StorageClassName: "sc-name",
 				},
 			},
 			FailedBuildHistoryLimit:  &limit,
@@ -285,10 +286,11 @@ func testImageValidation(t *testing.T, when spec.G, it spec.S) {
 
 		})
 
-		it("validates cache size is not set when there is no default StorageClass", func() {
+		it("validates cache size is not set when there is no default StorageClass and no storageClassName", func() {
 			ctx = context.TODO()
+			image.Spec.Cache.Volume.StorageClassName = ""
 
-			assertValidationError(image, ctx, apis.ErrGeneric("spec.cache.volume.size cannot be set with no default StorageClass"))
+			assertValidationError(image, ctx, apis.ErrGeneric("spec.cache.volume.size cannot be set without spec.cache.volume.storageClassName or a default StorageClass"))
 		})
 
 		it("combining errors", func() {
@@ -315,6 +317,14 @@ func testImageValidation(t *testing.T, when spec.G, it spec.S) {
 			assert.EqualError(t, err, "Field cannot be decreased: spec.cache.volume.size\ncurrent: 5G, requested: 4G")
 		})
 
+		it("image.cache.volume.storageClassName has not changed", func() {
+			original := image.DeepCopy()
+
+			image.Spec.Cache.Volume.StorageClassName = "sc-different"
+			err := image.Validate(apis.WithinUpdate(ctx, original))
+			assert.EqualError(t, err, "Immutable field changed: spec.cache.volume.storageClassName\ngot: sc-different, want: sc-name")
+		})
+
 		when("validating the cosign config", func() {
 			// cosign: nil
 			it("handles nil cosign", func() {

diff --git a/pkg/reconciler/image/image_test.go b/pkg/reconciler/image/image_test.go
@@ -408,7 +408,6 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
 			}
 
 			it("creates a cache if requested", func() {
-
 				rt.Test(rtesting.TableRow{
 					Key: key,
 					Objects: []runtime.Object{
@@ -460,20 +459,41 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
 			it("does not create a cache if a cache already exists", func() {
 				image.Spec.Cache.Volume.Size = &cacheSize
 				image.Status.BuildCacheName = image.CacheName()
+				storageClassName := "some-name"
 
 				rt.Test(rtesting.TableRow{
 					Key: key,
 					Objects: []runtime.Object{
 						image,
 						image.SourceResolver(),
-						image.BuildCache(),
+						&corev1.PersistentVolumeClaim{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      image.CacheName(),
+								Namespace: namespace,
+								OwnerReferences: []metav1.OwnerReference{
+									*kmeta.NewControllerRef(image),
+								},
+								Labels: map[string]string{
+									someLabelKey: someValueToPassThrough,
+								},
+							},
+							Spec: corev1.PersistentVolumeClaimSpec{
+								AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
+								Resources: corev1.ResourceRequirements{
+									Requests: corev1.ResourceList{
+										corev1.ResourceStorage: cacheSize,
+									},
+								},
+								StorageClassName: &storageClassName,
+							},
+						},
 						builder,
 					},
 					WantErr: false,
 				})
 			})
 
-			it("updates build cache if desired configuration changed", func() {
+			it("updates build cache if size changes", func() {
 				var imageCacheName = image.CacheName()
 
 				image.Status.BuildCacheName = imageCacheName
@@ -622,6 +642,57 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
 					},
 				})
 			})
+
+			it("uses the storageClassName if provided", func() {
+				image.Spec.Cache.Volume.StorageClassName = "some-storage-class"
+				rt.Test(rtesting.TableRow{
+					Key: key,
+					Objects: []runtime.Object{
+						image,
+						image.SourceResolver(),
+						builder,
+					},
+					WantErr: false,
+					WantCreates: []runtime.Object{
+						&corev1.PersistentVolumeClaim{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      image.CacheName(),
+								Namespace: namespace,
+								OwnerReferences: []metav1.OwnerReference{
+									*kmeta.NewControllerRef(image),
+								},
+								Labels: map[string]string{
+									someLabelKey: someValueToPassThrough,
+								},
+							},
+							Spec: corev1.PersistentVolumeClaimSpec{
+								AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
+								Resources: corev1.ResourceRequirements{
+									Requests: corev1.ResourceList{
+										corev1.ResourceStorage: cacheSize,
+									},
+								},
+								StorageClassName: &image.Spec.Cache.Volume.StorageClassName,
+							},
+						},
+					},
+					WantStatusUpdates: []clientgotesting.UpdateActionImpl{
+						{
+							Object: &buildapi.Image{
+								ObjectMeta: image.ObjectMeta,
+								Spec:       image.Spec,
+								Status: buildapi.ImageStatus{
+									BuildCacheName: image.CacheName(),
+									Status: corev1alpha1.Status{
+										ObservedGeneration: originalGeneration,
+										Conditions:         conditionReadyUnknown(),
+									},
+								},
+							},
+						},
+					},
+				})
+			})
 		})
 
 		when("reconciling builds", func() {