From 4312cb6f16bd69fc8c9b4afc479f05308ca558ec Mon Sep 17 00:00:00 2001 From: ushabelgur Date: Fri, 20 Dec 2024 10:27:13 +0530 Subject: [PATCH] incorporating review comments --- .../machine_ephemeralvolume_controller.go | 27 ++------ ...machine_ephemeralvolume_controller_test.go | 68 ------------------- .../controllers/machine_controller.go | 5 ++ .../controllers/machine_controller_test.go | 15 +++- .../controllers/machine_controller_volume.go | 23 +++++++ utils/annotations/annotations.go | 4 -- 6 files changed, 48 insertions(+), 94 deletions(-) diff --git a/internal/controllers/compute/machine_ephemeralvolume_controller.go b/internal/controllers/compute/machine_ephemeralvolume_controller.go index 5bcf2ee4f..e91a2244f 100644 --- a/internal/controllers/compute/machine_ephemeralvolume_controller.go +++ b/internal/controllers/compute/machine_ephemeralvolume_controller.go @@ -21,7 +21,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -75,26 +74,16 @@ func (r *MachineEphemeralVolumeReconciler) ephemeralMachineVolumeByName(machine Spec: ephemeral.VolumeTemplate.Spec.VolumeSpec, } annotations.SetDefaultEphemeralManagedBy(volume) - if machineVolume.Ephemeral.VolumeTemplate.Spec.ReclaimPolicy != storagev1alpha1.Retain { - _ = ctrl.SetControllerReference(machine, volume, r.Scheme()) - } + _ = ctrl.SetControllerReference(machine, volume, r.Scheme()) res[volumeName] = volume } return res } -func (r *MachineEphemeralVolumeReconciler) handleExistingVolume(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine, shouldManage bool, volume *storagev1alpha1.Volume, ephemVolume *storagev1alpha1.Volume) error { - log.WithValues("Volume", klog.KObj(volume), "ShouldManage", shouldManage) - if annotations.IsDefaultEphemeralOrControlledBy(volume, machine) { +func (r *MachineEphemeralVolumeReconciler) handleExistingVolume(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine, shouldManage bool, volume *storagev1alpha1.Volume) error { + if annotations.IsDefaultEphemeralControlledBy(volume, machine) { if shouldManage { - log.V(1).Info("Ephemeral volume is present") - if controllerutil.HasControllerReference(ephemVolume) && !controllerutil.HasControllerReference(volume) { - baseVol := volume.DeepCopy() - _ = ctrl.SetControllerReference(machine, volume, r.Scheme()) - if err := r.Patch(ctx, volume, client.StrategicMergeFrom(baseVol, client.MergeFromWithOptimisticLock{})); err != nil { - return fmt.Errorf("error patching volume: %w", err) - } - } + log.V(1).Info("Ephemeral volume is present and controlled by machine") return nil } @@ -134,9 +123,7 @@ func (r *MachineEphemeralVolumeReconciler) handleCreateVolume(ctx context.Contex } // Treat a retrieved volume as an existing we should manage. - ephemVolumeByName := r.ephemeralMachineVolumeByName(machine) - ephemVolume, shouldManage := ephemVolumeByName[volume.Name] - return r.handleExistingVolume(ctx, log, machine, shouldManage, volume, ephemVolume) + return r.handleExistingVolume(ctx, log, machine, true, volume) } func (r *MachineEphemeralVolumeReconciler) reconcile(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine) (ctrl.Result, error) { @@ -156,10 +143,10 @@ func (r *MachineEphemeralVolumeReconciler) reconcile(ctx context.Context, log lo ) for _, volume := range volumeList.Items { volumeName := volume.Name - ephemVolume, shouldManage := ephemVolumeByName[volumeName] + _, shouldManage := ephemVolumeByName[volumeName] delete(ephemVolumeByName, volumeName) log := log.WithValues("Volume", klog.KObj(&volume), "ShouldManage", shouldManage) - if err := r.handleExistingVolume(ctx, log, machine, shouldManage, &volume, ephemVolume); err != nil { + if err := r.handleExistingVolume(ctx, log, machine, shouldManage, &volume); err != nil { errs = append(errs, err) } } diff --git a/internal/controllers/compute/machine_ephemeralvolume_controller_test.go b/internal/controllers/compute/machine_ephemeralvolume_controller_test.go index f977313f8..ae8732c21 100644 --- a/internal/controllers/compute/machine_ephemeralvolume_controller_test.go +++ b/internal/controllers/compute/machine_ephemeralvolume_controller_test.go @@ -10,12 +10,10 @@ import ( . "github.com/ironcore-dev/ironcore/utils/testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - . "github.com/onsi/gomega/gstruct" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" ) @@ -84,13 +82,6 @@ var _ = Describe("MachineEphemeralVolumeController", func() { }, } Eventually(Get(ephemVolume)).Should(Succeed()) - By("Verifying OwnerRef is updated for ephemeral volume") - Expect(ephemVolume).Should(HaveField("ObjectMeta.OwnerReferences", ConsistOf(MatchFields(IgnoreExtras, Fields{ - "APIVersion": Equal(computev1alpha1.SchemeGroupVersion.String()), - "Kind": Equal("Machine"), - "Name": Equal(machine.Name), - })), - )) By("asserting the referenced volume still exists") Consistently(Get(refVolume)).Should(Succeed()) @@ -133,63 +124,4 @@ var _ = Describe("MachineEphemeralVolumeController", func() { By("asserting that the external volume is not being deleted") Consistently(Object(externalVolume)).Should(HaveField("DeletionTimestamp", BeNil())) }) - - It("verify ownerRef is set for ephemeral volumes based on reclaim policy", func(ctx SpecContext) { - By("creating a machine") - machine := &computev1alpha1.Machine{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: ns.Name, - GenerateName: "machine-", - }, - Spec: computev1alpha1.MachineSpec{ - MachineClassRef: corev1.LocalObjectReference{Name: machineClass.Name}, - Volumes: []computev1alpha1.Volume{ - { - Name: "ephem-volume", - VolumeSource: computev1alpha1.VolumeSource{ - Ephemeral: &computev1alpha1.EphemeralVolumeSource{ - VolumeTemplate: &storagev1alpha1.VolumeTemplateSpec{ - Spec: storagev1alpha1.EphemeralVolumeSpec{ - ReclaimPolicy: storagev1alpha1.Retain, - }, - }, - }, - }, - }, - }, - }, - } - Expect(k8sClient.Create(ctx, machine)).To(Succeed()) - - By("waiting for the ephemeral volume to exist") - ephemVolume := &storagev1alpha1.Volume{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: ns.Name, - Name: computev1alpha1.MachineEphemeralVolumeName(machine.Name, "ephem-volume"), - }, - } - Eventually(Get(ephemVolume)).Should(Succeed()) - By("Verifying OwnerRef is not set for ephemeral volume when reclaim policy is retain") - Eventually(Object(ephemVolume)).Should(HaveField("ObjectMeta.OwnerReferences", BeEmpty())) - - By("Updating reclaim policy to delete") - baseMachine := machine.DeepCopy() - machineVolumes := machine.Spec.Volumes - for _, machineVolume := range machineVolumes { - if machineVolume.Ephemeral == nil { - continue - } - machineVolume.Ephemeral.VolumeTemplate.Spec.ReclaimPolicy = storagev1alpha1.Delete - } - machine.Spec.Volumes = machineVolumes - Expect(k8sClient.Patch(ctx, machine, client.MergeFrom(baseMachine))).To(Succeed()) - By("Verifying OwnerRef is updated for ephemeral volume") - Eventually(Object(ephemVolume)).Should(HaveField("ObjectMeta.OwnerReferences", ConsistOf(MatchFields(IgnoreExtras, Fields{ - "APIVersion": Equal(computev1alpha1.SchemeGroupVersion.String()), - "Kind": Equal("Machine"), - "Name": Equal(machine.Name), - })), - )) - - }) }) diff --git a/poollet/machinepoollet/controllers/machine_controller.go b/poollet/machinepoollet/controllers/machine_controller.go index fbb9a8f0c..26c9c7d2b 100644 --- a/poollet/machinepoollet/controllers/machine_controller.go +++ b/poollet/machinepoollet/controllers/machine_controller.go @@ -195,6 +195,11 @@ func (r *MachineReconciler) deleteGone(ctx context.Context, log logr.Logger, mac func (r *MachineReconciler) reconcileExists(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine) (ctrl.Result, error) { if !machine.DeletionTimestamp.IsZero() { + log.V(1).Info("Machine is deleting, handle ephemeral volumes") + if err := r.handleEphemeralVolume(ctx, machine); err != nil { + return ctrl.Result{}, err + } + return r.delete(ctx, log, machine) } return r.reconcile(ctx, log, machine) diff --git a/poollet/machinepoollet/controllers/machine_controller_test.go b/poollet/machinepoollet/controllers/machine_controller_test.go index 50af90708..6f0bd8683 100644 --- a/poollet/machinepoollet/controllers/machine_controller_test.go +++ b/poollet/machinepoollet/controllers/machine_controller_test.go @@ -427,7 +427,9 @@ var _ = Describe("MachineController", func() { VolumeSource: computev1alpha1.VolumeSource{ Ephemeral: &computev1alpha1.EphemeralVolumeSource{ VolumeTemplate: &storagev1alpha1.VolumeTemplateSpec{ - Spec: storagev1alpha1.EphemeralVolumeSpec{}, + Spec: storagev1alpha1.EphemeralVolumeSpec{ + ReclaimPolicy: storagev1alpha1.Retain, + }, }, }, }, @@ -496,8 +498,17 @@ var _ = Describe("MachineController", func() { "State": Equal(computev1alpha1.VolumeStatePending), "VolumeRef": Equal(corev1.LocalObjectReference{Name: ephimeralVolume.Name}), })))) - }) + Expect(k8sClient.Delete(ctx, machine)).To(Succeed()) + + Eventually(Object(ephimeralVolume)).Should(SatisfyAll( + HaveField("ObjectMeta.OwnerReferences", BeEmpty()), + HaveField("DeletionTimestamp", BeZero()))) + + Consistently(Object(ephimeralVolume)).Should(SatisfyAll( + HaveField("DeletionTimestamp", BeZero()))) + + }) }) func GetSingleMapEntry[K comparable, V any](m map[K]V) (K, V) { diff --git a/poollet/machinepoollet/controllers/machine_controller_volume.go b/poollet/machinepoollet/controllers/machine_controller_volume.go index 42b559bc2..b46c17197 100644 --- a/poollet/machinepoollet/controllers/machine_controller_volume.go +++ b/poollet/machinepoollet/controllers/machine_controller_volume.go @@ -24,6 +24,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) type volumeClaimStrategy struct { @@ -407,3 +408,25 @@ func (r *MachineReconciler) addVolumeStatusValues(now metav1.Time, existing, new existing.Handle = newValues.Handle existing.VolumeRef = newValues.VolumeRef } + +func (r *MachineReconciler) handleEphemeralVolume(ctx context.Context, machine *computev1alpha1.Machine) error { + for _, machineVolume := range machine.Spec.Volumes { + ephemeral := machineVolume.Ephemeral + if ephemeral == nil { + continue + } + if machineVolume.Ephemeral.VolumeTemplate.Spec.ReclaimPolicy == storagev1alpha1.Retain { + volumeName := computev1alpha1.MachineEphemeralVolumeName(machine.Name, machineVolume.Name) + volume := &storagev1alpha1.Volume{} + if err := r.Get(ctx, client.ObjectKey{Namespace: machine.GetNamespace(), Name: volumeName}, volume); err != nil { + return fmt.Errorf("error getting volume %s: %w", volume.Name, err) + } + baseVol := volume.DeepCopy() + _ = controllerutil.RemoveControllerReference(machine, volume, r.Scheme()) + if err := r.Patch(ctx, volume, client.StrategicMergeFrom(baseVol, client.MergeFromWithOptimisticLock{})); err != nil { + return fmt.Errorf("error patching volume: %w", err) + } + } + } + return nil +} diff --git a/utils/annotations/annotations.go b/utils/annotations/annotations.go index 37a4bd3eb..b92eda05b 100644 --- a/utils/annotations/annotations.go +++ b/utils/annotations/annotations.go @@ -36,10 +36,6 @@ func IsDefaultEphemeralControlledBy(o metav1.Object, owner metav1.Object) bool { return metav1.IsControlledBy(o, owner) && IsEphemeralManagedBy(o, commonv1alpha1.DefaultEphemeralManager) } -func IsDefaultEphemeralOrControlledBy(o metav1.Object, owner metav1.Object) bool { - return metav1.IsControlledBy(o, owner) || IsEphemeralManagedBy(o, commonv1alpha1.DefaultEphemeralManager) -} - func SetDefaultEphemeralManagedBy(o metav1.Object) { metautils.SetAnnotation(o, commonv1alpha1.EphemeralManagedByAnnotation, commonv1alpha1.DefaultEphemeralManager) }