Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configurable storage class name for image resource #1029

Merged
merged 2 commits into from
Sep 26, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/image.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/build/v1alpha2/image_builds.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -221,6 +225,7 @@ func (im *Image) BuildCache() *corev1.PersistentVolumeClaim {
corev1.ResourceStorage: *im.Spec.Cache.Volume.Size,
},
},
StorageClassName: storageClassName,
},
}
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/apis/build/v1alpha2/image_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions pkg/apis/build/v1alpha2/image_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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")
}
}

Expand Down Expand Up @@ -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")
}
Expand Down
16 changes: 13 additions & 3 deletions pkg/apis/build/v1alpha2/image_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand Down
77 changes: 74 additions & 3 deletions pkg/reconciler/image/image_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down