From a5648ac4f935b8a3b494b9da2d0cf59df49357ac Mon Sep 17 00:00:00 2001 From: Dave Walter Date: Mon, 3 Jul 2023 16:30:44 -0700 Subject: [PATCH] handle non-resizable storage classes for build cache - We now set build cache size on kpack Images, but if we already have an Image, and we try to change the size setting and repush the app, the operation will fail if the storage class doesn't support resize. This change checks the storage class and deletes the image if resizing isn't supported. #2634 Co-authored-by: Julian Hjortshoj --- helm/korifi/kpack-image-builder/role.yaml | 7 ++ .../controllers/buildworkload_controller.go | 65 +++++++++++++++++-- .../buildworkload_controller_test.go | 54 ++++++++++++++- kpack-image-builder/controllers/suite_test.go | 28 +++++--- 4 files changed, 141 insertions(+), 13 deletions(-) diff --git a/helm/korifi/kpack-image-builder/role.yaml b/helm/korifi/kpack-image-builder/role.yaml index 767324993..a28116fc1 100644 --- a/helm/korifi/kpack-image-builder/role.yaml +++ b/helm/korifi/kpack-image-builder/role.yaml @@ -124,3 +124,10 @@ rules: verbs: - get - patch +- apiGroups: + - storage.k8s.io + resources: + - storageclasses + verbs: + - list + - watch diff --git a/kpack-image-builder/controllers/buildworkload_controller.go b/kpack-image-builder/controllers/buildworkload_controller.go index 791713eb8..3f321941d 100644 --- a/kpack-image-builder/controllers/buildworkload_controller.go +++ b/kpack-image-builder/controllers/buildworkload_controller.go @@ -37,6 +37,7 @@ import ( buildv1alpha2 "github.com/pivotal/kpack/pkg/apis/build/v1alpha2" corev1alpha1 "github.com/pivotal/kpack/pkg/apis/core/v1alpha1" corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/resource" @@ -164,6 +165,8 @@ func filterBuildWorkloads(object client.Object) bool { //+kubebuilder:rbac:groups=kpack.io,resources=builds,verbs=deletecollection //+kubebuilder:rbac:groups=kpack.io,resources=builders,verbs=get;list;watch;create;patch;update +//+kubebuilder:rbac:groups=storage.k8s.io,resources=storageclasses,verbs=list;watch + //+kubebuilder:rbac:groups="",resources=serviceaccounts;secrets,verbs=get;list;watch;patch //+kubebuilder:rbac:groups="",resources=serviceaccounts/status;secrets/status,verbs=get @@ -651,10 +654,33 @@ func (r *BuildWorkloadReconciler) reconcileKpackImage( cacheSize, err := resource.ParseQuantity(fmt.Sprintf("%dMi", r.controllerConfig.BuildCacheMB)) if err != nil { - log.Error(err, "failed to parse image cache size") + log.Info("failed to parse image cache size", "reason", err) return err } + + recreateImage := false _, err = controllerutil.CreateOrPatch(ctx, r.k8sClient, &desiredKpackImage, func() error { + if desiredKpackImage.Spec.Cache != nil && + desiredKpackImage.Spec.Cache.Volume != nil && + desiredKpackImage.Spec.Cache.Volume.Size != nil && + !desiredKpackImage.Spec.Cache.Volume.Size.Equal(cacheSize) { + // Our new request wants to resize the PV on the existing Image. Fetch the storage class to see if that is allowed. + scName := desiredKpackImage.Spec.Cache.Volume.StorageClassName + scResizable, err2 := r.isResizable(ctx, log, scName) + if err2 != nil { + return err2 + } + if !scResizable { + // Signal to recreate the Image with an error + log.V(1).Info("WARNING: storage class does not support PVC resize. Recreating.", + "storageClassName", scName, + "desiredSize", cacheSize.String(), + "actualSize", desiredKpackImage.Spec.Cache.Volume.Size.String()) + recreateImage = true + return nil + } + } + desiredKpackImage.Labels = map[string]string{ BuildWorkloadLabelKey: buildWorkload.Name, } @@ -703,6 +729,17 @@ func (r *BuildWorkloadReconciler) reconcileKpackImage( return err } + if recreateImage { + // note: we don't need to explicitly requeue because we have a watch on the Image, so deleting it will trigger a requeue of its owner + log.V(1).Info("removing kpack image and re-reconciling", "imageName", desiredKpackImage.Name, "imageNamespace", desiredKpackImage.Namespace) + err = r.k8sClient.Delete(ctx, &desiredKpackImage) + if err != nil { + log.Info("failed to delete kpack image on cache resize", "reason", err) + return err + } + return nil + } + meta.SetStatusCondition(&buildWorkload.Status.Conditions, metav1.Condition{ Type: korifiv1alpha1.SucceededConditionType, Status: metav1.ConditionUnknown, @@ -716,6 +753,26 @@ func (r *BuildWorkloadReconciler) reconcileKpackImage( return nil } +func (r *BuildWorkloadReconciler) isResizable(ctx context.Context, log logr.Logger, scName string) (bool, error) { + scList := storagev1.StorageClassList{} + if listErr := r.k8sClient.List(ctx, &scList); listErr != nil { + log.Info("unable to list storage classes", "reason", listErr) + return false, listErr + } + for _, sc := range scList.Items { + defaultVal, exists := sc.Annotations["storageclass.kubernetes.io/is-default-class"] + if sc.Name == scName || (scName == "" && exists && defaultVal == "true") { + if sc.AllowVolumeExpansion == nil { + return false, nil + } + return *sc.AllowVolumeExpansion, nil + } + } + + log.Info("unable to locate matching or default storage class", "storageClassName", scName) + return false, nil +} + func (r *BuildWorkloadReconciler) generateDropletStatus(ctx context.Context, kpackBuild *buildv1alpha2.Build, imagePullSecrets []corev1.LocalObjectReference) (*korifiv1alpha1.BuildDropletStatus, error) { imageRef := kpackBuild.Status.LatestImage @@ -781,20 +838,20 @@ func (r *BuildWorkloadReconciler) finalize(ctx context.Context, log logr.Logger, lastBuildWorkload, err := r.isLastBuildWorkload(ctx, buildWorkload) if err != nil { - log.Error(err, "failed to check for last build workloads") + log.Info("failed to check for last build workloads", "reason", err) return ctrl.Result{}, err } if lastBuildWorkload { err = r.deleteBuildsForBuildWorkload(ctx, buildWorkload) if err != nil { - log.Error(err, "failed to delete builds for build workload") + log.Info("failed to delete builds for build workload", "reason", err) return ctrl.Result{}, err } hasRemainingBuilds, err := r.hasRemainingBuilds(ctx, buildWorkload) if err != nil { - log.Error(err, "failed to check for remaining builds for build workload") + log.Info("failed to check for remaining builds for build workload", "reason", err) return ctrl.Result{}, err } diff --git a/kpack-image-builder/controllers/buildworkload_controller_test.go b/kpack-image-builder/controllers/buildworkload_controller_test.go index d067abb93..5ad65cf8d 100644 --- a/kpack-image-builder/controllers/buildworkload_controller_test.go +++ b/kpack-image-builder/controllers/buildworkload_controller_test.go @@ -43,9 +43,11 @@ var _ = Describe("BuildWorkloadReconciler", func() { reconcilerName string buildpacks []string imageRepoCreatorCallCount int + expectedCacheVolumeSize string ) BeforeEach(func() { + expectedCacheVolumeSize = "1024Mi" reconcilerName = "kpack-image-builder" namespaceGUID = PrefixedGUID("namespace") Expect(k8sClient.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespaceGUID}})).To(Succeed()) @@ -149,6 +151,8 @@ var _ = Describe("BuildWorkloadReconciler", func() { }) CheckInitialization := func() { + GinkgoHelper() + It("reconciles the kpack.Image", func() { Eventually(func(g Gomega) { kpackImage := new(buildv1alpha2.Image) @@ -161,7 +165,7 @@ var _ = Describe("BuildWorkloadReconciler", func() { g.Expect(kpackImage.Spec.Builder.Kind).To(Equal("ClusterBuilder")) g.Expect(kpackImage.Spec.Builder.Name).To(Equal("cf-kpack-builder")) - g.Expect(kpackImage.Spec.Cache.Volume.Size.Equal(resource.MustParse("1024Mi"))).To(BeTrue()) + g.Expect(kpackImage.Spec.Cache.Volume.Size.Equal(resource.MustParse(expectedCacheVolumeSize))).To(BeTrue()) }).Should(Succeed()) }) @@ -215,6 +219,54 @@ var _ = Describe("BuildWorkloadReconciler", func() { CheckInitialization() }) + When("kpack image exists with a different cache size and the storage class doesn't allow resize", func() { + var originalImageUID types.UID + BeforeEach(func() { + oldSize := resource.MustParse("512Mi") + Expect(k8sClient.Create(ctx, &buildv1alpha2.Image{ + ObjectMeta: metav1.ObjectMeta{ + Name: "app-guid", + Namespace: namespaceGUID, + Labels: map[string]string{ + controllers.BuildWorkloadLabelKey: buildWorkloadGUID, + }, + }, + Spec: buildv1alpha2.ImageSpec{ + Tag: "my-tag-string", + Builder: corev1.ObjectReference{ + Name: "my-builder", + }, + ServiceAccountName: "my-service-account", + Source: corev1alpha1.SourceConfig{ + Registry: &corev1alpha1.Registry{ + Image: "not-an-image", + ImagePullSecrets: nil, + }, + }, + Cache: &buildv1alpha2.ImageCacheConfig{ + Volume: &buildv1alpha2.ImagePersistentVolumeCache{Size: &oldSize, StorageClassName: "non-resizable-class"}, + }, + }, + })).To(Succeed()) + Eventually(func(g Gomega) { + kpackImage := new(buildv1alpha2.Image) + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "app-guid", Namespace: namespaceGUID}, kpackImage)).To(Succeed()) + originalImageUID = kpackImage.UID + g.Expect(originalImageUID).NotTo(BeEmpty()) + }).Should(Succeed()) + + }) + + It("deletes the kpack image and recreates it", func() { + Eventually(func(g Gomega) { + kpackImage := new(buildv1alpha2.Image) + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "app-guid", Namespace: namespaceGUID}, kpackImage)).To(Succeed()) + g.Expect(kpackImage.UID).NotTo(Equal(originalImageUID)) + }).Should(Succeed()) + }) + + CheckInitialization() + }) When("the source image pull secret doesn't exist", func() { var nonExistentSecret string diff --git a/kpack-image-builder/controllers/suite_test.go b/kpack-image-builder/controllers/suite_test.go index 2857c511c..5578bbbbe 100644 --- a/kpack-image-builder/controllers/suite_test.go +++ b/kpack-image-builder/controllers/suite_test.go @@ -22,10 +22,20 @@ import ( "testing" "time" + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/controllers/config" + "code.cloudfoundry.org/korifi/kpack-image-builder/controllers" + "code.cloudfoundry.org/korifi/kpack-image-builder/controllers/fake" + "code.cloudfoundry.org/korifi/kpack-image-builder/controllers/webhooks/finalizer" + "code.cloudfoundry.org/korifi/tools" + "code.cloudfoundry.org/korifi/tools/k8s" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" buildv1alpha2 "github.com/pivotal/kpack/pkg/apis/build/v1alpha2" - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" @@ -33,13 +43,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/envtest" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" - - korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" - "code.cloudfoundry.org/korifi/controllers/config" - "code.cloudfoundry.org/korifi/kpack-image-builder/controllers" - "code.cloudfoundry.org/korifi/kpack-image-builder/controllers/fake" - "code.cloudfoundry.org/korifi/kpack-image-builder/controllers/webhooks/finalizer" - "code.cloudfoundry.org/korifi/tools/k8s" //+kubebuilder:scaffold:imports ) @@ -176,6 +179,15 @@ var _ = BeforeSuite(func() { err = k8sManager.Start(ctx) Expect(err).NotTo(HaveOccurred()) }() + + // create a test storage class that can't be resized + Expect(k8sClient.Create(ctx, &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-resizable-class", + }, + Provisioner: "some-fancy-provisioner", + AllowVolumeExpansion: tools.PtrTo(false), + })).To(Succeed()) }) var _ = AfterSuite(func() {