Skip to content

Commit 0a10f78

Browse files
committed
fix: remove blocking waits during deletion
Replace blocking wait.PollUntilContextTimeout with non-blocking check-and-requeuepattern in Deleter reconciler. This reduces PowerMonitor CR deletion time from5+ minutes to under 30 seconds by allowing Kubernetes GC to run in parallelinstead of sequentially polling for each resource deletion. Changes: - Refactor Deleter to issue non blocking deletes and return Continue - Remove WaitTimeout field from Deleter - Update Deleter tests for non blocking deletes - Remove 2 minute namespace deletion wait in PMI cleanup Signed-off-by: vprashar2929 <vibhu.sharma2929@gmail.com>
1 parent 329e01d commit 0a10f78

4 files changed

Lines changed: 73 additions & 55 deletions

File tree

internal/controller/power_monitor.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,8 @@ func (r PowerMonitorReconciler) reconcilersForPowerMonitor(pm *v1alpha1.PowerMon
202202
detail = components.Full
203203
}
204204

205+
// Deleter proceeds without waiting, owner references guarantee
206+
// PowerMonitorInternal cleanup via Kubernetes GC
205207
rs := []reconciler.Reconciler{
206208
op(newPowerMonitorInternal(detail, pm)),
207209
reconciler.Finalizer{

internal/controller/power_monitor_internal.go

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,7 @@ package controller
66
import (
77
"context"
88
"fmt"
9-
109
"slices"
11-
"time"
1210

1311
"github.com/go-logr/logr"
1412
secv1 "github.com/openshift/api/security/v1"
@@ -569,15 +567,14 @@ func (r PowerMonitorInternalReconciler) reconcilersForPowerMonitor(pmi *v1alpha1
569567
rs = append(rs, exporterReconcilers...)
570568

571569
if cleanup {
570+
// Deleter proceeds without waiting, owner references guarantee
571+
// namespace cleanup via Kubernetes GC
572572
rs = append(rs, reconciler.Deleter{
573-
OnError: reconciler.Requeue,
574-
Resource: components.NewNamespace(pmi.Namespace()),
575-
WaitTimeout: 2 * time.Minute,
573+
OnError: reconciler.Requeue,
574+
Resource: components.NewNamespace(pmi.Namespace()),
576575
})
577576
}
578577

579-
// WARN: only run finalizer if theren't any errors
580-
// this bug 🐛 must be FIXED
581578
rs = append(rs, reconciler.Finalizer{
582579
Resource: pmi,
583580
Finalizer: Finalizer,

pkg/reconciler/deleter.go

Lines changed: 6 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -6,47 +6,29 @@ package reconciler
66
import (
77
"context"
88
"fmt"
9-
"time"
109

1110
"github.com/sustainable.computing.io/kepler-operator/pkg/utils/k8s"
12-
"k8s.io/apimachinery/pkg/api/errors"
1311
"k8s.io/apimachinery/pkg/runtime"
14-
"k8s.io/apimachinery/pkg/util/wait"
1512
"sigs.k8s.io/controller-runtime/pkg/client"
1613
)
1714

15+
// Deleter issues a non-blocking delete and returns Continue, allowing
16+
// multiple resources to be deleted in a single reconciliation pass.
17+
// Guaranteed cleanup is provided by owner references and Kubernetes GC.
1818
type Deleter struct {
19-
Resource client.Object
20-
OnError Action
21-
WaitTimeout time.Duration
19+
Resource client.Object
20+
OnError Action
2221
}
2322

2423
func (r Deleter) Reconcile(ctx context.Context, c client.Client, scheme *runtime.Scheme) Result {
25-
objKey := client.ObjectKeyFromObject(r.Resource)
26-
2724
if err := c.Delete(ctx, r.Resource); client.IgnoreNotFound(err) != nil {
2825
return Result{
2926
Error: r.error("failed to delete", err),
3027
Action: r.OnError,
3128
}
3229
}
3330

34-
dup := r.Resource.DeepCopyObject().(client.Object)
35-
36-
timeout := max(r.WaitTimeout, 60*time.Second)
37-
err := wait.PollUntilContextTimeout(ctx, 5*time.Second, timeout, true, func(ctx context.Context) (bool, error) {
38-
err := c.Get(ctx, objKey, dup)
39-
// repeat until object is not found
40-
return errors.IsNotFound(err), nil
41-
})
42-
43-
if err != nil {
44-
return Result{
45-
Error: r.error("timed out waiting for deletion", err),
46-
Action: r.OnError,
47-
}
48-
}
49-
return Result{}
31+
return Result{Action: Continue}
5032
}
5133

5234
func (r Deleter) error(msg string, err error) error {

pkg/reconciler/deleter_test.go

Lines changed: 61 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818

1919
"sigs.k8s.io/controller-runtime/pkg/client"
2020
"sigs.k8s.io/controller-runtime/pkg/client/fake"
21+
"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
2122
)
2223

2324
func TestDeleterReconcile(t *testing.T) {
@@ -27,28 +28,64 @@ func TestDeleterReconcile(t *testing.T) {
2728
require.NoError(t, corev1.AddToScheme(testScheme))
2829
require.NoError(t, appsv1.AddToScheme(testScheme))
2930

30-
dep := k8s.Deployment("ns", "name").Build()
31-
c := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(dep).Build()
32-
33-
tt := []struct {
34-
scenario string
35-
resource client.Object
36-
}{
37-
{"deletes existing resources", dep},
38-
{"deletes non-existent resources", k8s.Deployment("ns", "non-existent").Build()},
39-
}
40-
41-
for _, tc := range tt {
42-
tc := tc
43-
t.Run(tc.scenario, func(t *testing.T) {
44-
deleter := Deleter{Resource: tc.resource}
45-
result := deleter.Reconcile(context.TODO(), c, testScheme)
46-
assert.Exactly(t, Continue, result.Action)
47-
assert.NoError(t, result.Error)
48-
49-
dummy := tc.resource.DeepCopyObject().(client.Object)
50-
err := c.Get(context.TODO(), client.ObjectKeyFromObject(tc.resource), dummy)
51-
assert.ErrorContains(t, err, fmt.Sprintf(`"%s" not found`, tc.resource.GetName()))
52-
})
53-
}
31+
t.Run("returns Continue after deleting existing resource", func(t *testing.T) {
32+
dep := k8s.Deployment("ns", "existing").Build()
33+
c := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(dep).Build()
34+
35+
deleter := Deleter{Resource: dep}
36+
result := deleter.Reconcile(context.TODO(), c, testScheme)
37+
38+
// Non-blocking: returns Continue to allow parallel deletion
39+
assert.Exactly(t, Continue, result.Action)
40+
assert.NoError(t, result.Error)
41+
42+
// Resource should be deleted (fake client deletes immediately)
43+
dummy := dep.DeepCopyObject().(client.Object)
44+
err := c.Get(context.TODO(), client.ObjectKeyFromObject(dep), dummy)
45+
assert.ErrorContains(t, err, fmt.Sprintf(`"%s" not found`, dep.GetName()))
46+
})
47+
48+
t.Run("returns Continue when resource already deleted", func(t *testing.T) {
49+
nonExistent := k8s.Deployment("ns", "non-existent").Build()
50+
c := fake.NewClientBuilder().WithScheme(testScheme).Build()
51+
52+
deleter := Deleter{Resource: nonExistent}
53+
result := deleter.Reconcile(context.TODO(), c, testScheme)
54+
55+
// Resource already gone, no requeue needed
56+
assert.Exactly(t, Continue, result.Action)
57+
assert.NoError(t, result.Error)
58+
})
59+
60+
t.Run("multiple deleters execute in single pass", func(t *testing.T) {
61+
dep1 := k8s.Deployment("ns", "dep1").Build()
62+
dep2 := k8s.Deployment("ns", "dep2").Build()
63+
c := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(dep1, dep2).Build()
64+
65+
// Both deleters return Continue, enabling parallel deletion
66+
deleter1 := Deleter{Resource: dep1}
67+
result := deleter1.Reconcile(context.TODO(), c, testScheme)
68+
assert.Exactly(t, Continue, result.Action)
69+
assert.NoError(t, result.Error)
70+
71+
deleter2 := Deleter{Resource: dep2}
72+
result = deleter2.Reconcile(context.TODO(), c, testScheme)
73+
assert.Exactly(t, Continue, result.Action)
74+
assert.NoError(t, result.Error)
75+
})
76+
77+
t.Run("returns error when Delete fails", func(t *testing.T) {
78+
dep := k8s.Deployment("ns", "del-err").Build()
79+
c := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(dep).WithInterceptorFuncs(interceptor.Funcs{
80+
Delete: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteOption) error {
81+
return fmt.Errorf("injected delete error")
82+
},
83+
}).Build()
84+
85+
deleter := Deleter{Resource: dep, OnError: Requeue}
86+
result := deleter.Reconcile(context.TODO(), c, testScheme)
87+
88+
assert.Exactly(t, Requeue, result.Action)
89+
assert.ErrorContains(t, result.Error, "failed to delete")
90+
})
5491
}

0 commit comments

Comments
 (0)