Skip to content

Commit 4312cb6

Browse files
ushabelgurushabelgur
committed
incorporating review comments
1 parent 3eb6dca commit 4312cb6

File tree

6 files changed

+48
-94
lines changed

6 files changed

+48
-94
lines changed

‎internal/controllers/compute/machine_ephemeralvolume_controller.go‎

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
ctrl "sigs.k8s.io/controller-runtime"
2222
"sigs.k8s.io/controller-runtime/pkg/builder"
2323
"sigs.k8s.io/controller-runtime/pkg/client"
24-
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
2524
"sigs.k8s.io/controller-runtime/pkg/handler"
2625
"sigs.k8s.io/controller-runtime/pkg/predicate"
2726
)
@@ -75,26 +74,16 @@ func (r *MachineEphemeralVolumeReconciler) ephemeralMachineVolumeByName(machine
7574
Spec: ephemeral.VolumeTemplate.Spec.VolumeSpec,
7675
}
7776
annotations.SetDefaultEphemeralManagedBy(volume)
78-
if machineVolume.Ephemeral.VolumeTemplate.Spec.ReclaimPolicy != storagev1alpha1.Retain {
79-
_ = ctrl.SetControllerReference(machine, volume, r.Scheme())
80-
}
77+
_ = ctrl.SetControllerReference(machine, volume, r.Scheme())
8178
res[volumeName] = volume
8279
}
8380
return res
8481
}
8582

86-
func (r *MachineEphemeralVolumeReconciler) handleExistingVolume(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine, shouldManage bool, volume *storagev1alpha1.Volume, ephemVolume *storagev1alpha1.Volume) error {
87-
log.WithValues("Volume", klog.KObj(volume), "ShouldManage", shouldManage)
88-
if annotations.IsDefaultEphemeralOrControlledBy(volume, machine) {
83+
func (r *MachineEphemeralVolumeReconciler) handleExistingVolume(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine, shouldManage bool, volume *storagev1alpha1.Volume) error {
84+
if annotations.IsDefaultEphemeralControlledBy(volume, machine) {
8985
if shouldManage {
90-
log.V(1).Info("Ephemeral volume is present")
91-
if controllerutil.HasControllerReference(ephemVolume) && !controllerutil.HasControllerReference(volume) {
92-
baseVol := volume.DeepCopy()
93-
_ = ctrl.SetControllerReference(machine, volume, r.Scheme())
94-
if err := r.Patch(ctx, volume, client.StrategicMergeFrom(baseVol, client.MergeFromWithOptimisticLock{})); err != nil {
95-
return fmt.Errorf("error patching volume: %w", err)
96-
}
97-
}
86+
log.V(1).Info("Ephemeral volume is present and controlled by machine")
9887
return nil
9988
}
10089

@@ -134,9 +123,7 @@ func (r *MachineEphemeralVolumeReconciler) handleCreateVolume(ctx context.Contex
134123
}
135124

136125
// Treat a retrieved volume as an existing we should manage.
137-
ephemVolumeByName := r.ephemeralMachineVolumeByName(machine)
138-
ephemVolume, shouldManage := ephemVolumeByName[volume.Name]
139-
return r.handleExistingVolume(ctx, log, machine, shouldManage, volume, ephemVolume)
126+
return r.handleExistingVolume(ctx, log, machine, true, volume)
140127
}
141128

142129
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
156143
)
157144
for _, volume := range volumeList.Items {
158145
volumeName := volume.Name
159-
ephemVolume, shouldManage := ephemVolumeByName[volumeName]
146+
_, shouldManage := ephemVolumeByName[volumeName]
160147
delete(ephemVolumeByName, volumeName)
161148
log := log.WithValues("Volume", klog.KObj(&volume), "ShouldManage", shouldManage)
162-
if err := r.handleExistingVolume(ctx, log, machine, shouldManage, &volume, ephemVolume); err != nil {
149+
if err := r.handleExistingVolume(ctx, log, machine, shouldManage, &volume); err != nil {
163150
errs = append(errs, err)
164151
}
165152
}

‎internal/controllers/compute/machine_ephemeralvolume_controller_test.go‎

Lines changed: 0 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,10 @@ import (
1010
. "github.com/ironcore-dev/ironcore/utils/testing"
1111
. "github.com/onsi/ginkgo/v2"
1212
. "github.com/onsi/gomega"
13-
. "github.com/onsi/gomega/gstruct"
1413
corev1 "k8s.io/api/core/v1"
1514
apierrors "k8s.io/apimachinery/pkg/api/errors"
1615
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1716
ctrl "sigs.k8s.io/controller-runtime"
18-
"sigs.k8s.io/controller-runtime/pkg/client"
1917
. "sigs.k8s.io/controller-runtime/pkg/envtest/komega"
2018
)
2119

@@ -84,13 +82,6 @@ var _ = Describe("MachineEphemeralVolumeController", func() {
8482
},
8583
}
8684
Eventually(Get(ephemVolume)).Should(Succeed())
87-
By("Verifying OwnerRef is updated for ephemeral volume")
88-
Expect(ephemVolume).Should(HaveField("ObjectMeta.OwnerReferences", ConsistOf(MatchFields(IgnoreExtras, Fields{
89-
"APIVersion": Equal(computev1alpha1.SchemeGroupVersion.String()),
90-
"Kind": Equal("Machine"),
91-
"Name": Equal(machine.Name),
92-
})),
93-
))
9485

9586
By("asserting the referenced volume still exists")
9687
Consistently(Get(refVolume)).Should(Succeed())
@@ -133,63 +124,4 @@ var _ = Describe("MachineEphemeralVolumeController", func() {
133124
By("asserting that the external volume is not being deleted")
134125
Consistently(Object(externalVolume)).Should(HaveField("DeletionTimestamp", BeNil()))
135126
})
136-
137-
It("verify ownerRef is set for ephemeral volumes based on reclaim policy", func(ctx SpecContext) {
138-
By("creating a machine")
139-
machine := &computev1alpha1.Machine{
140-
ObjectMeta: metav1.ObjectMeta{
141-
Namespace: ns.Name,
142-
GenerateName: "machine-",
143-
},
144-
Spec: computev1alpha1.MachineSpec{
145-
MachineClassRef: corev1.LocalObjectReference{Name: machineClass.Name},
146-
Volumes: []computev1alpha1.Volume{
147-
{
148-
Name: "ephem-volume",
149-
VolumeSource: computev1alpha1.VolumeSource{
150-
Ephemeral: &computev1alpha1.EphemeralVolumeSource{
151-
VolumeTemplate: &storagev1alpha1.VolumeTemplateSpec{
152-
Spec: storagev1alpha1.EphemeralVolumeSpec{
153-
ReclaimPolicy: storagev1alpha1.Retain,
154-
},
155-
},
156-
},
157-
},
158-
},
159-
},
160-
},
161-
}
162-
Expect(k8sClient.Create(ctx, machine)).To(Succeed())
163-
164-
By("waiting for the ephemeral volume to exist")
165-
ephemVolume := &storagev1alpha1.Volume{
166-
ObjectMeta: metav1.ObjectMeta{
167-
Namespace: ns.Name,
168-
Name: computev1alpha1.MachineEphemeralVolumeName(machine.Name, "ephem-volume"),
169-
},
170-
}
171-
Eventually(Get(ephemVolume)).Should(Succeed())
172-
By("Verifying OwnerRef is not set for ephemeral volume when reclaim policy is retain")
173-
Eventually(Object(ephemVolume)).Should(HaveField("ObjectMeta.OwnerReferences", BeEmpty()))
174-
175-
By("Updating reclaim policy to delete")
176-
baseMachine := machine.DeepCopy()
177-
machineVolumes := machine.Spec.Volumes
178-
for _, machineVolume := range machineVolumes {
179-
if machineVolume.Ephemeral == nil {
180-
continue
181-
}
182-
machineVolume.Ephemeral.VolumeTemplate.Spec.ReclaimPolicy = storagev1alpha1.Delete
183-
}
184-
machine.Spec.Volumes = machineVolumes
185-
Expect(k8sClient.Patch(ctx, machine, client.MergeFrom(baseMachine))).To(Succeed())
186-
By("Verifying OwnerRef is updated for ephemeral volume")
187-
Eventually(Object(ephemVolume)).Should(HaveField("ObjectMeta.OwnerReferences", ConsistOf(MatchFields(IgnoreExtras, Fields{
188-
"APIVersion": Equal(computev1alpha1.SchemeGroupVersion.String()),
189-
"Kind": Equal("Machine"),
190-
"Name": Equal(machine.Name),
191-
})),
192-
))
193-
194-
})
195127
})

‎poollet/machinepoollet/controllers/machine_controller.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,11 @@ func (r *MachineReconciler) deleteGone(ctx context.Context, log logr.Logger, mac
195195

196196
func (r *MachineReconciler) reconcileExists(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine) (ctrl.Result, error) {
197197
if !machine.DeletionTimestamp.IsZero() {
198+
log.V(1).Info("Machine is deleting, handle ephemeral volumes")
199+
if err := r.handleEphemeralVolume(ctx, machine); err != nil {
200+
return ctrl.Result{}, err
201+
}
202+
198203
return r.delete(ctx, log, machine)
199204
}
200205
return r.reconcile(ctx, log, machine)

‎poollet/machinepoollet/controllers/machine_controller_test.go‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,9 @@ var _ = Describe("MachineController", func() {
427427
VolumeSource: computev1alpha1.VolumeSource{
428428
Ephemeral: &computev1alpha1.EphemeralVolumeSource{
429429
VolumeTemplate: &storagev1alpha1.VolumeTemplateSpec{
430-
Spec: storagev1alpha1.EphemeralVolumeSpec{},
430+
Spec: storagev1alpha1.EphemeralVolumeSpec{
431+
ReclaimPolicy: storagev1alpha1.Retain,
432+
},
431433
},
432434
},
433435
},
@@ -496,8 +498,17 @@ var _ = Describe("MachineController", func() {
496498
"State": Equal(computev1alpha1.VolumeStatePending),
497499
"VolumeRef": Equal(corev1.LocalObjectReference{Name: ephimeralVolume.Name}),
498500
}))))
499-
})
500501

502+
Expect(k8sClient.Delete(ctx, machine)).To(Succeed())
503+
504+
Eventually(Object(ephimeralVolume)).Should(SatisfyAll(
505+
HaveField("ObjectMeta.OwnerReferences", BeEmpty()),
506+
HaveField("DeletionTimestamp", BeZero())))
507+
508+
Consistently(Object(ephimeralVolume)).Should(SatisfyAll(
509+
HaveField("DeletionTimestamp", BeZero())))
510+
511+
})
501512
})
502513

503514
func GetSingleMapEntry[K comparable, V any](m map[K]V) (K, V) {

‎poollet/machinepoollet/controllers/machine_controller_volume.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2525
"k8s.io/apimachinery/pkg/util/sets"
2626
"sigs.k8s.io/controller-runtime/pkg/client"
27+
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
2728
)
2829

2930
type volumeClaimStrategy struct {
@@ -407,3 +408,25 @@ func (r *MachineReconciler) addVolumeStatusValues(now metav1.Time, existing, new
407408
existing.Handle = newValues.Handle
408409
existing.VolumeRef = newValues.VolumeRef
409410
}
411+
412+
func (r *MachineReconciler) handleEphemeralVolume(ctx context.Context, machine *computev1alpha1.Machine) error {
413+
for _, machineVolume := range machine.Spec.Volumes {
414+
ephemeral := machineVolume.Ephemeral
415+
if ephemeral == nil {
416+
continue
417+
}
418+
if machineVolume.Ephemeral.VolumeTemplate.Spec.ReclaimPolicy == storagev1alpha1.Retain {
419+
volumeName := computev1alpha1.MachineEphemeralVolumeName(machine.Name, machineVolume.Name)
420+
volume := &storagev1alpha1.Volume{}
421+
if err := r.Get(ctx, client.ObjectKey{Namespace: machine.GetNamespace(), Name: volumeName}, volume); err != nil {
422+
return fmt.Errorf("error getting volume %s: %w", volume.Name, err)
423+
}
424+
baseVol := volume.DeepCopy()
425+
_ = controllerutil.RemoveControllerReference(machine, volume, r.Scheme())
426+
if err := r.Patch(ctx, volume, client.StrategicMergeFrom(baseVol, client.MergeFromWithOptimisticLock{})); err != nil {
427+
return fmt.Errorf("error patching volume: %w", err)
428+
}
429+
}
430+
}
431+
return nil
432+
}

‎utils/annotations/annotations.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,6 @@ func IsDefaultEphemeralControlledBy(o metav1.Object, owner metav1.Object) bool {
3636
return metav1.IsControlledBy(o, owner) && IsEphemeralManagedBy(o, commonv1alpha1.DefaultEphemeralManager)
3737
}
3838

39-
func IsDefaultEphemeralOrControlledBy(o metav1.Object, owner metav1.Object) bool {
40-
return metav1.IsControlledBy(o, owner) || IsEphemeralManagedBy(o, commonv1alpha1.DefaultEphemeralManager)
41-
}
42-
4339
func SetDefaultEphemeralManagedBy(o metav1.Object) {
4440
metautils.SetAnnotation(o, commonv1alpha1.EphemeralManagedByAnnotation, commonv1alpha1.DefaultEphemeralManager)
4541
}

0 commit comments

Comments
 (0)