Skip to content

Commit 15cdd85

Browse files
darkowlzzdarkowlzz
committed
controllers: Allow deletion of suspended objects
Reorders the object suspended check in all the reconcilers to allow deletion of objects when they are suspended. Objects used to get stuck on delete because the finalizers were not getting removed due to the suspended state. Adds a generic test for all the reconcilers to check if a suspended source object can be delete. Signed-off-by: Sunny <[email protected]>
1 parent 15b4f96 commit 15cdd85

13 files changed

+239
-147
lines changed

‎controllers/bucket_controller.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -258,12 +258,6 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
258258
// Record suspended status metric
259259
r.RecordSuspend(ctx, obj, obj.Spec.Suspend)
260260

261-
// Return early if the object is suspended
262-
if obj.Spec.Suspend {
263-
log.Info("reconciliation is suspended for this object")
264-
return ctrl.Result{}, nil
265-
}
266-
267261
// Initialize the patch helper with the current version of the object.
268262
patchHelper, err := patch.NewHelper(obj, r.Client)
269263
if err != nil {
@@ -309,6 +303,13 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
309303
return
310304
}
311305

306+
// Return if the object is suspended.
307+
if obj.Spec.Suspend {
308+
log.Info("reconciliation is suspended for this object")
309+
recResult, retErr = sreconcile.ResultEmpty, nil
310+
return
311+
}
312+
312313
// Reconcile actual object
313314
reconcilers := []bucketReconcileFunc{
314315
r.reconcileStorage,

‎controllers/bucket_controller_test.go

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import (
3030

3131
. "github.com/onsi/gomega"
3232
corev1 "k8s.io/api/core/v1"
33-
apierrors "k8s.io/apimachinery/pkg/api/errors"
3433
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3534
"k8s.io/client-go/tools/record"
3635
kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status"
@@ -85,7 +84,7 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
8584
g.Expect(testEnv.Create(ctx, secret)).To(Succeed())
8685
defer testEnv.Delete(ctx, secret)
8786

88-
obj := &sourcev1.Bucket{
87+
origObj := &sourcev1.Bucket{
8988
ObjectMeta: metav1.ObjectMeta{
9089
GenerateName: "bucket-reconcile-",
9190
Namespace: "default",
@@ -102,6 +101,7 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
102101
},
103102
},
104103
}
104+
obj := origObj.DeepCopy()
105105
g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
106106

107107
key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
@@ -115,17 +115,7 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
115115
}, timeout).Should(BeTrue())
116116

117117
// Wait for Bucket to be Ready
118-
g.Eventually(func() bool {
119-
if err := testEnv.Get(ctx, key, obj); err != nil {
120-
return false
121-
}
122-
if !conditions.IsReady(obj) || obj.Status.Artifact == nil {
123-
return false
124-
}
125-
readyCondition := conditions.Get(obj, meta.ReadyCondition)
126-
return obj.Generation == readyCondition.ObservedGeneration &&
127-
obj.Generation == obj.Status.ObservedGeneration
128-
}, timeout).Should(BeTrue())
118+
waitForSourceReadyWithArtifact(ctx, g, obj)
129119

130120
// Check if the object status is valid.
131121
condns := &conditionscheck.Conditions{NegativePolarity: bucketReadyCondition.NegativePolarity}
@@ -157,12 +147,11 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
157147
g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
158148

159149
// Wait for Bucket to be deleted
160-
g.Eventually(func() bool {
161-
if err := testEnv.Get(ctx, key, obj); err != nil {
162-
return apierrors.IsNotFound(err)
163-
}
164-
return false
165-
}, timeout).Should(BeTrue())
150+
waitForSourceDeletion(ctx, g, obj)
151+
152+
// Check if a suspended object gets deleted.
153+
obj = origObj.DeepCopy()
154+
testSuspendedObjectDeleteWithArtifact(ctx, g, obj)
166155
}
167156

168157
func TestBucketReconciler_reconcileStorage(t *testing.T) {

‎controllers/common_test.go

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
/*
2+
Copyright 2022 The Flux authors
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package controllers
18+
19+
import (
20+
"context"
21+
22+
. "github.com/onsi/gomega"
23+
apierrors "k8s.io/apimachinery/pkg/api/errors"
24+
"sigs.k8s.io/controller-runtime/pkg/client"
25+
26+
"github.com/fluxcd/pkg/apis/meta"
27+
"github.com/fluxcd/pkg/runtime/conditions"
28+
"github.com/fluxcd/pkg/runtime/patch"
29+
30+
"github.com/fluxcd/source-controller/internal/object"
31+
)
32+
33+
// waitForSourceDeletion is a generic test helper to wait for object deletion of
34+
// any source kind.
35+
func waitForSourceDeletion(ctx context.Context, g *WithT, obj conditions.Setter) {
36+
g.THelper()
37+
38+
key := client.ObjectKeyFromObject(obj)
39+
g.Eventually(func() bool {
40+
if err := testEnv.Get(ctx, key, obj); err != nil {
41+
return apierrors.IsNotFound(err)
42+
}
43+
return false
44+
}, timeout).Should(BeTrue())
45+
}
46+
47+
// waitForSuspended is a generic test helper to wait for object to be suspended
48+
// of any source kind.
49+
func waitForSuspended(ctx context.Context, g *WithT, obj conditions.Setter) {
50+
g.THelper()
51+
52+
key := client.ObjectKeyFromObject(obj)
53+
g.Eventually(func() bool {
54+
if err := testEnv.Get(ctx, key, obj); err != nil {
55+
return false
56+
}
57+
suspended, err := object.GetSuspend(obj)
58+
if err != nil {
59+
return false
60+
}
61+
return suspended == true
62+
}, timeout).Should(BeTrue())
63+
}
64+
65+
// waitForSourceReadyWithArtifact is a generic test helper to wait for an object
66+
// to be ready of any source kind that have artifact in status when ready.
67+
func waitForSourceReadyWithArtifact(ctx context.Context, g *WithT, obj conditions.Setter) {
68+
g.THelper()
69+
waitForSourceReady(ctx, g, obj, true)
70+
}
71+
72+
// waitForSourceReadyWithoutArtifact is a generic test helper to wait for an object
73+
// to be ready of any source kind that don't have artifact in status when ready.
74+
func waitForSourceReadyWithoutArtifact(ctx context.Context, g *WithT, obj conditions.Setter) {
75+
g.THelper()
76+
waitForSourceReady(ctx, g, obj, false)
77+
}
78+
79+
// waitForSourceReady is a generic test helper to wait for an object to be
80+
// ready of any source kind.
81+
func waitForSourceReady(ctx context.Context, g *WithT, obj conditions.Setter, withArtifact bool) {
82+
g.THelper()
83+
84+
key := client.ObjectKeyFromObject(obj)
85+
g.Eventually(func() bool {
86+
if err := testEnv.Get(ctx, key, obj); err != nil {
87+
return false
88+
}
89+
if withArtifact {
90+
artifact, err := object.GetArtifact(obj)
91+
if err != nil {
92+
return false
93+
}
94+
if artifact == nil {
95+
return false
96+
}
97+
}
98+
if !conditions.IsReady(obj) {
99+
return false
100+
}
101+
readyCondition := conditions.Get(obj, meta.ReadyCondition)
102+
statusObservedGen, err := object.GetStatusObservedGeneration(obj)
103+
if err != nil {
104+
return false
105+
}
106+
return obj.GetGeneration() == readyCondition.ObservedGeneration &&
107+
obj.GetGeneration() == statusObservedGen
108+
}, timeout).Should(BeTrue())
109+
}
110+
111+
// testSuspendedObjectDeleteWithArtifact is a generic test helper to test if a
112+
// suspended object can be deleted for objects that have artifact in status when
113+
// ready.
114+
func testSuspendedObjectDeleteWithArtifact(ctx context.Context, g *WithT, obj conditions.Setter) {
115+
g.THelper()
116+
testSuspendedObjectDelete(ctx, g, obj, true)
117+
}
118+
119+
// testSuspendedObjectDeleteWithoutArtifact is a generic test helper to test if
120+
// a suspended object can be deleted for objects that don't have artifact in
121+
// status when ready.
122+
func testSuspendedObjectDeleteWithoutArtifact(ctx context.Context, g *WithT, obj conditions.Setter) {
123+
g.THelper()
124+
testSuspendedObjectDelete(ctx, g, obj, false)
125+
}
126+
127+
// testSuspendedObjectDelete is a generic test helper to test if a suspended
128+
// object can be deleted.
129+
func testSuspendedObjectDelete(ctx context.Context, g *WithT, obj conditions.Setter, withArtifact bool) {
130+
g.THelper()
131+
132+
// Create the object and wait for it to be ready.
133+
g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
134+
waitForSourceReady(ctx, g, obj, withArtifact)
135+
136+
// Suspend the object.
137+
patchHelper, err := patch.NewHelper(obj, testEnv.Client)
138+
g.Expect(err).ToNot(HaveOccurred())
139+
g.Expect(object.SetSuspend(obj, true)).ToNot(HaveOccurred())
140+
g.Expect(patchHelper.Patch(ctx, obj)).ToNot(HaveOccurred())
141+
waitForSuspended(ctx, g, obj)
142+
143+
// Delete the object.
144+
g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
145+
waitForSourceDeletion(ctx, g, obj)
146+
}

‎controllers/gitrepository_controller.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -172,12 +172,6 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
172172
// Record suspended status metric
173173
r.RecordSuspend(ctx, obj, obj.Spec.Suspend)
174174

175-
// Return early if the object is suspended
176-
if obj.Spec.Suspend {
177-
log.Info("reconciliation is suspended for this object")
178-
return ctrl.Result{}, nil
179-
}
180-
181175
// Initialize the patch helper with the current version of the object.
182176
patchHelper, err := patch.NewHelper(obj, r.Client)
183177
if err != nil {
@@ -225,6 +219,13 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
225219
return
226220
}
227221

222+
// Return if the object is suspended.
223+
if obj.Spec.Suspend {
224+
log.Info("reconciliation is suspended for this object")
225+
recResult, retErr = sreconcile.ResultEmpty, nil
226+
return
227+
}
228+
228229
// Reconcile actual object
229230
reconcilers := []gitRepositoryReconcileFunc{
230231
r.reconcileStorage,

‎controllers/gitrepository_controller_test.go

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ import (
3737
. "github.com/onsi/gomega"
3838
sshtestdata "golang.org/x/crypto/ssh/testdata"
3939
corev1 "k8s.io/api/core/v1"
40-
apierrors "k8s.io/apimachinery/pkg/api/errors"
4140
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4241
"k8s.io/apimachinery/pkg/runtime"
4342
"k8s.io/client-go/tools/record"
@@ -168,7 +167,7 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
168167
_, err = initGitRepo(server, "testdata/git/repository", git.DefaultBranch, repoPath)
169168
g.Expect(err).NotTo(HaveOccurred())
170169

171-
obj := &sourcev1.GitRepository{
170+
origObj := &sourcev1.GitRepository{
172171
ObjectMeta: metav1.ObjectMeta{
173172
GenerateName: "gitrepository-reconcile-",
174173
Namespace: "default",
@@ -178,6 +177,7 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
178177
URL: server.HTTPAddress() + repoPath,
179178
},
180179
}
180+
obj := origObj.DeepCopy()
181181
g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
182182

183183
key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
@@ -191,17 +191,7 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
191191
}, timeout).Should(BeTrue())
192192

193193
// Wait for GitRepository to be Ready
194-
g.Eventually(func() bool {
195-
if err := testEnv.Get(ctx, key, obj); err != nil {
196-
return false
197-
}
198-
if !conditions.IsReady(obj) || obj.Status.Artifact == nil {
199-
return false
200-
}
201-
readyCondition := conditions.Get(obj, meta.ReadyCondition)
202-
return obj.Generation == readyCondition.ObservedGeneration &&
203-
obj.Generation == obj.Status.ObservedGeneration
204-
}, timeout).Should(BeTrue())
194+
waitForSourceReadyWithArtifact(ctx, g, obj)
205195

206196
// Check if the object status is valid.
207197
condns := &conditionscheck.Conditions{NegativePolarity: gitRepositoryReadyCondition.NegativePolarity}
@@ -233,12 +223,11 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
233223
g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
234224

235225
// Wait for GitRepository to be deleted
236-
g.Eventually(func() bool {
237-
if err := testEnv.Get(ctx, key, obj); err != nil {
238-
return apierrors.IsNotFound(err)
239-
}
240-
return false
241-
}, timeout).Should(BeTrue())
226+
waitForSourceDeletion(ctx, g, obj)
227+
228+
// Check if a suspended object gets deleted.
229+
obj = origObj.DeepCopy()
230+
testSuspendedObjectDeleteWithArtifact(ctx, g, obj)
242231
}
243232

244233
func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {

‎controllers/helmchart_controller.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -194,12 +194,6 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
194194
// Record suspended status metric
195195
r.RecordSuspend(ctx, obj, obj.Spec.Suspend)
196196

197-
// Return early if the object is suspended
198-
if obj.Spec.Suspend {
199-
log.Info("Reconciliation is suspended for this object")
200-
return ctrl.Result{}, nil
201-
}
202-
203197
// Initialize the patch helper with the current version of the object.
204198
patchHelper, err := patch.NewHelper(obj, r.Client)
205199
if err != nil {
@@ -246,6 +240,13 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
246240
return
247241
}
248242

243+
// Return if the object is suspended.
244+
if obj.Spec.Suspend {
245+
log.Info("Reconciliation is suspended for this object")
246+
recResult, retErr = sreconcile.ResultEmpty, nil
247+
return
248+
}
249+
249250
// Reconcile actual object
250251
reconcilers := []helmChartReconcileFunc{
251252
r.reconcileStorage,

0 commit comments

Comments
 (0)