fixed comments and improved error handling

Author: valeryia-hurynovich

controllers/managedcloudprofile_controller.go

@@ -146,7 +146,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 			return ctrl.Result{}, fmt.Errorf("failed to list source versions for garbage collection: %w", err)
 		}
 
-		referencedVersions := r.getReferencedVersions(ctx, mcp.Name, updates.ImageName)
+		referencedVersions, err := r.getReferencedVersions(ctx, mcp.Name, updates.ImageName)
+		if err != nil {
+			if patchErr := r.patchStatusAndCondition(ctx, &mcp, v1alpha1.FailedReconcileStatus, metav1.Condition{
+				Type:               CloudProfileAppliedConditionType,
+				Status:             metav1.ConditionFalse,
+				ObservedGeneration: mcp.Generation,
+				Reason:             "GarbageCollectionFailed",
+				Message:            fmt.Sprintf("failed to determine referenced versions for garbage collection: %s", err),
+			}); patchErr != nil {
+				return ctrl.Result{}, fmt.Errorf("failed to patch ManagedCloudProfile status: %w (original error: %w)", patchErr, err)
+			}
+			return ctrl.Result{}, fmt.Errorf("failed to determine referenced versions for garbage collection: %w", err)
+		}
 
 		cutoff := time.Now().Add(-updates.GarbageCollection.MaxAge.Duration)
 		versionsToDelete := make([]string, 0)
@@ -173,7 +185,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 					Status:             metav1.ConditionFalse,
 					ObservedGeneration: mcp.Generation,
 					Reason:             "GarbageCollectionFailed",
-					Message:            fmt.Sprintf("failed to list source versions for garbage collection: %s", err),
+					Message:            fmt.Sprintf("failed to delete image versions during garbage collection: %s", err),
 				}); patchErr != nil {
 					return ctrl.Result{}, fmt.Errorf("failed to patch ManagedCloudProfile status: %w (original error: %w)", patchErr, err)
 				}
@@ -213,78 +225,61 @@ func (r *Reconciler) deleteVersion(ctx context.Context, cloudProfileName, imageN
 	}
 	if cp.Spec.ProviderConfig != nil {
 		var cfg providercfg.CloudProfileConfig
-		if err := json.Unmarshal(cp.Spec.ProviderConfig.Raw, &cfg); err == nil {
-			for i := range cfg.MachineImages {
-				if cfg.MachineImages[i].Name != imageName {
-					continue
-				}
-				filtered := make([]providercfg.MachineImageVersion, 0, len(cfg.MachineImages[i].Versions))
-				for _, mv := range cfg.MachineImages[i].Versions {
-					found := false
-					for _, version := range versions {
-						if strings.HasSuffix(mv.Image, ":"+version) {
-							found = true
-							break
-						}
-					}
-					if !found {
-						filtered = append(filtered, mv)
+		if err := json.Unmarshal(cp.Spec.ProviderConfig.Raw, &cfg); err != nil {
+			return fmt.Errorf("failed to unmarshal ProviderConfig: %w", err)
+		}
+		for i := range cfg.MachineImages {
+			if cfg.MachineImages[i].Name != imageName {
+				continue
+			}
+			filtered := make([]providercfg.MachineImageVersion, 0, len(cfg.MachineImages[i].Versions))
+			for _, mv := range cfg.MachineImages[i].Versions {
+				found := false
+				for _, version := range versions {
+					if strings.HasSuffix(mv.Image, ":"+version) {
+						found = true
+						break
 					}
 				}
-				cfg.MachineImages[i].Versions = filtered
-			}
-			raw, err := json.Marshal(cfg)
-			if err == nil {
-				cp.Spec.ProviderConfig.Raw = raw
+				if !found {
+					filtered = append(filtered, mv)
+				}
 			}
+			cfg.MachineImages[i].Versions = filtered
+		}
+		raw, err := json.Marshal(cfg)
+		if err != nil {
+			return fmt.Errorf("failed to marshal ProviderConfig: %w", err)
 		}
+		cp.Spec.ProviderConfig.Raw = raw
 	}
 
 	return r.Update(ctx, &cp)
 }
 
-func (r *Reconciler) getReferencedVersions(ctx context.Context, cloudProfileName, imageName string) map[string]bool {
+func (r *Reconciler) getReferencedVersions(ctx context.Context, cloudProfileName, imageName string) (map[string]bool, error) {
 	referenced := make(map[string]bool)
 
-	var cp gardenerv1beta1.CloudProfile
-	if err := r.Get(ctx, types.NamespacedName{Name: cloudProfileName}, &cp); err == nil {
-		if cp.Spec.ProviderConfig != nil {
-			var cfg providercfg.CloudProfileConfig
-			if err := json.Unmarshal(cp.Spec.ProviderConfig.Raw, &cfg); err == nil {
-				for _, img := range cfg.MachineImages {
-					if img.Name != imageName {
-						continue
-					}
-					for _, v := range img.Versions {
-						if idx := strings.LastIndex(v.Image, ":"); idx != -1 {
-							version := v.Image[idx+1:]
-							referenced[version] = true
-						}
-					}
-				}
-			}
-		}
+	shootList := &gardenerv1beta1.ShootList{}
+	if err := r.List(ctx, shootList, client.InNamespace(metav1.NamespaceAll)); err != nil {
+		return nil, fmt.Errorf("failed to list Shoots: %w", err)
 	}
+	for _, shoot := range shootList.Items {
+		if shoot.Spec.CloudProfile == nil || shoot.Spec.CloudProfile.Name != cloudProfileName {
+			continue
+		}
 
-	shootList := &gardenerv1beta1.ShootList{}
-	if err := r.List(ctx, shootList, client.InNamespace(metav1.NamespaceAll)); err == nil {
-		for _, shoot := range shootList.Items {
-			if shoot.Spec.CloudProfile == nil || shoot.Spec.CloudProfile.Name != cloudProfileName {
+		for _, worker := range shoot.Spec.Provider.Workers {
+			if worker.Machine.Image == nil || worker.Machine.Image.Name != imageName {
 				continue
 			}
-
-			for _, worker := range shoot.Spec.Provider.Workers {
-				if worker.Machine.Image == nil || worker.Machine.Image.Name != imageName {
-					continue
-				}
-				if worker.Machine.Image.Version != nil {
-					referenced[*worker.Machine.Image.Version] = true
-				}
+			if worker.Machine.Image.Version != nil {
+				referenced[*worker.Machine.Image.Version] = true
 			}
 		}
 	}
 
-	return referenced
+	return referenced, nil
 }
 
 func (r *Reconciler) updateMachineImages(ctx context.Context, log logr.Logger, update v1alpha1.MachineImageUpdate, cpSpec *gardenerv1beta1.CloudProfileSpec) error {

controllers/managedcloudprofile_controller_test.go

@@ -338,6 +338,24 @@ var _ = Describe("The ManagedCloudProfile reconciler", func() {
 		cloudProfile.Spec.ProviderConfig = &runtime.RawExtension{Raw: raw}
 		Expect(k8sClient.Create(ctx, &cloudProfile)).To(Succeed())
 
+		// Create a Shoot that references version "1.0.0"
+		var shoot gardenerv1beta1.Shoot
+		shoot.Name = "test-shoot-preserve"
+		shoot.Namespace = metav1.NamespaceDefault
+		shoot.Spec.CloudProfile = &gardenerv1beta1.CloudProfileReference{Name: cloudProfile.Name}
+		shoot.Spec.Provider.Workers = []gardenerv1beta1.Worker{
+			{
+				Name: "worker1",
+				Machine: gardenerv1beta1.Machine{
+					Image: &gardenerv1beta1.ShootMachineImage{
+						Name:    "preserve-image",
+						Version: ptr.To("1.0.0"),
+					},
+				},
+			},
+		}
+		Expect(k8sClient.Create(ctx, &shoot)).To(Succeed())
+
 		var mcp v1alpha1.ManagedCloudProfile
 		mcp.Name = "test-gc-preserve"
 		mcp.Spec.CloudProfile = v1alpha1.CloudProfileSpec{
@@ -391,6 +409,7 @@ var _ = Describe("The ManagedCloudProfile reconciler", func() {
 
 		Expect(k8sClient.Delete(ctx, &mcp)).To(Succeed())
 		Expect(k8sClient.Delete(ctx, &cloudProfile)).To(Succeed())
+		Expect(k8sClient.Delete(ctx, &shoot)).To(Succeed())
 	})
 
 	It("preserves machine image versions referenced by Shoot workers", func(ctx SpecContext) {