Skip to content

Commit a6a84a8

Browse files
thardeckthardeck
committed
Track UID of existing bundle deployment to prevent orphaning on failure (#4273)
* Track UID of existing bundle deployment to prevent orphaning on failure * Add integration tests for bundle create failure handling
1 parent 4402cb2 commit a6a84a8

File tree

2 files changed

+325
-0
lines changed

2 files changed

+325
-0
lines changed

‎integrationtests/controller/bundle/bundle_error_handling_test.go‎

Lines changed: 317 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,317 @@
1+
package bundle
2+
3+
import (
4+
"time"
5+
6+
. "github.com/onsi/ginkgo/v2"
7+
. "github.com/onsi/gomega"
8+
9+
"github.com/rancher/fleet/integrationtests/utils"
10+
"github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
11+
12+
corev1 "k8s.io/api/core/v1"
13+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
14+
"k8s.io/apimachinery/pkg/labels"
15+
"k8s.io/apimachinery/pkg/types"
16+
"sigs.k8s.io/controller-runtime/pkg/client"
17+
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
18+
)
19+
20+
const (
21+
testFinalizer = "test.fleet.cattle.io/block-deletion"
22+
testFinalizerNS = "test.fleet.cattle.io/block-ns-deletion"
23+
)
24+
25+
var _ = Describe("Bundle controller error handling", Ordered, func() {
26+
var bundleNS string
27+
28+
createNamespace := func(name string) {
29+
ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: name}}
30+
Expect(k8sClient.Create(ctx, ns)).ToNot(HaveOccurred())
31+
}
32+
33+
createCluster := func(name, namespace, statusNamespace string, labels map[string]string) *v1alpha1.Cluster {
34+
cluster := &v1alpha1.Cluster{
35+
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, Labels: labels},
36+
Spec: v1alpha1.ClusterSpec{Paused: false},
37+
}
38+
Expect(k8sClient.Create(ctx, cluster)).ToNot(HaveOccurred())
39+
40+
Eventually(func(g Gomega) {
41+
g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, cluster)).To(Succeed())
42+
cluster.Status.Namespace = statusNamespace
43+
g.Expect(k8sClient.Status().Update(ctx, cluster)).To(Succeed())
44+
}).Should(Succeed())
45+
46+
return cluster
47+
}
48+
49+
createBundle := func(name, namespace, defaultNS, configMapName string, targets []v1alpha1.BundleTarget) *v1alpha1.Bundle {
50+
bundle := &v1alpha1.Bundle{
51+
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
52+
Spec: v1alpha1.BundleSpec{
53+
BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{DefaultNamespace: defaultNS},
54+
Targets: targets,
55+
Resources: []v1alpha1.BundleResource{{
56+
Name: "test.yaml",
57+
Content: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: " + configMapName + "\ndata:\n key: value",
58+
}},
59+
},
60+
}
61+
Expect(k8sClient.Create(ctx, bundle)).ToNot(HaveOccurred())
62+
return bundle
63+
}
64+
65+
getBundleDeployments := func(bundleName, bundleNS string) *v1alpha1.BundleDeploymentList {
66+
bdList := &v1alpha1.BundleDeploymentList{}
67+
Expect(k8sClient.List(ctx, bdList, client.MatchingLabelsSelector{
68+
Selector: labels.SelectorFromSet(map[string]string{
69+
"fleet.cattle.io/bundle-name": bundleName,
70+
"fleet.cattle.io/bundle-namespace": bundleNS,
71+
}),
72+
})).To(Succeed())
73+
return bdList
74+
}
75+
76+
addFinalizer := func(obj client.Object, finalizer string) {
77+
Eventually(func(g Gomega) {
78+
key := client.ObjectKeyFromObject(obj)
79+
g.Expect(k8sClient.Get(ctx, key, obj)).To(Succeed())
80+
if !controllerutil.ContainsFinalizer(obj, finalizer) {
81+
controllerutil.AddFinalizer(obj, finalizer)
82+
g.Expect(k8sClient.Update(ctx, obj)).To(Succeed())
83+
}
84+
}).Should(Succeed())
85+
}
86+
87+
removeFinalizer := func(obj client.Object, finalizer string) {
88+
Eventually(func(g Gomega) {
89+
key := client.ObjectKeyFromObject(obj)
90+
g.Expect(k8sClient.Get(ctx, key, obj)).To(Succeed())
91+
controllerutil.RemoveFinalizer(obj, finalizer)
92+
g.Expect(k8sClient.Update(ctx, obj)).To(Succeed())
93+
}).Should(Succeed())
94+
}
95+
96+
updateBundleDefaultNamespace := func(bundle *v1alpha1.Bundle, newNS string) {
97+
Eventually(func(g Gomega) {
98+
latestBundle := &v1alpha1.Bundle{}
99+
key := client.ObjectKeyFromObject(bundle)
100+
g.Expect(k8sClient.Get(ctx, key, latestBundle)).To(Succeed())
101+
latestBundle.Spec.BundleDeploymentOptions.DefaultNamespace = newNS
102+
g.Expect(k8sClient.Update(ctx, latestBundle)).To(Succeed())
103+
}).Should(Succeed())
104+
}
105+
106+
generateTestID := func() string {
107+
testID, err := utils.NewNamespaceName()
108+
Expect(err).ToNot(HaveOccurred())
109+
if len(testID) > 5 {
110+
testID = testID[5:]
111+
}
112+
return testID
113+
}
114+
115+
setupClusters := func(cluster1Name, cluster2Name, testID string, clusterLabels []map[string]string) (*v1alpha1.Cluster, *v1alpha1.Cluster) {
116+
cluster1NS := "cluster1-ns-" + testID
117+
cluster2NS := "cluster2-ns-" + testID
118+
119+
createNamespace(cluster1NS)
120+
createNamespace(cluster2NS)
121+
122+
cluster1 := createCluster(cluster1Name, bundleNS, cluster1NS, clusterLabels[0])
123+
cluster2 := createCluster(cluster2Name, bundleNS, cluster2NS, clusterLabels[1])
124+
125+
return cluster1, cluster2
126+
}
127+
128+
cleanupResources := func(bundle *v1alpha1.Bundle, bundleName, testID string, resources ...client.Object) {
129+
_ = k8sClient.Delete(ctx, bundle)
130+
Eventually(func() int {
131+
return len(getBundleDeployments(bundleName, bundleNS).Items)
132+
}).Should(Equal(0))
133+
134+
for _, resource := range resources {
135+
_ = k8sClient.Delete(ctx, resource)
136+
}
137+
138+
_ = k8sClient.Delete(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "cluster1-ns-" + testID}})
139+
_ = k8sClient.Delete(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "cluster2-ns-" + testID}})
140+
}
141+
142+
BeforeAll(func() {
143+
var err error
144+
bundleNS, err = utils.NewNamespaceName()
145+
Expect(err).ToNot(HaveOccurred())
146+
createNamespace(bundleNS)
147+
DeferCleanup(func() {
148+
Expect(k8sClient.Delete(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: bundleNS}})).ToNot(HaveOccurred())
149+
})
150+
})
151+
152+
Context("Issue #4144 - UID tracking prevents incorrect deletion", func() {
153+
var (
154+
bundle *v1alpha1.Bundle
155+
cluster1 *v1alpha1.Cluster
156+
cluster2 *v1alpha1.Cluster
157+
content *v1alpha1.Content
158+
bundleName string
159+
testID string
160+
)
161+
162+
BeforeEach(func() {
163+
bundleName = "test-bundle-uid"
164+
testID = generateTestID()
165+
cluster1, cluster2 = setupClusters("cluster1", "cluster2", testID, []map[string]string{
166+
{"env": "test"},
167+
{"env": "test"},
168+
})
169+
170+
content = &v1alpha1.Content{
171+
ObjectMeta: metav1.ObjectMeta{Name: "test-content-" + testID},
172+
Content: []byte("test content"),
173+
}
174+
Expect(k8sClient.Create(ctx, content)).ToNot(HaveOccurred())
175+
176+
bundle = createBundle(bundleName, bundleNS, "default", "test", []v1alpha1.BundleTarget{{
177+
ClusterSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"env": "test"}},
178+
}})
179+
})
180+
181+
AfterEach(func() {
182+
cleanupResources(bundle, bundleName, testID, content, cluster1, cluster2)
183+
})
184+
185+
It("should not delete existing bundledeployments when bundle deployment creation fails", func() {
186+
By("capturing the original bundledeployment UIDs")
187+
var uid1, uid2 types.UID
188+
Eventually(func(g Gomega) {
189+
bdList := getBundleDeployments(bundleName, bundleNS)
190+
g.Expect(bdList.Items).To(HaveLen(2))
191+
uid1 = bdList.Items[0].UID
192+
uid2 = bdList.Items[1].UID
193+
}).Should(Succeed())
194+
195+
By("putting content resource in deleting state (causes createBundleDeployment to fail)")
196+
addFinalizer(content, testFinalizer)
197+
Expect(k8sClient.Delete(ctx, content)).To(Succeed())
198+
199+
Eventually(func(g Gomega) {
200+
c := &v1alpha1.Content{}
201+
g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: content.Name}, c)).To(Succeed())
202+
g.Expect(c.DeletionTimestamp).NotTo(BeNil())
203+
}).Should(Succeed())
204+
205+
By("triggering bundle reconcile which will fail to update bundledeployments")
206+
updateBundleDefaultNamespace(bundle, "kube-system")
207+
208+
By("verifying bundledeployments keep their original UIDs despite the failure")
209+
Consistently(func(g Gomega) {
210+
bdList := getBundleDeployments(bundleName, bundleNS)
211+
g.Expect(bdList.Items).To(HaveLen(2))
212+
currentUIDs := make(map[types.UID]bool)
213+
for _, bd := range bdList.Items {
214+
currentUIDs[bd.UID] = true
215+
}
216+
g.Expect(currentUIDs).To(HaveKey(uid1))
217+
g.Expect(currentUIDs).To(HaveKey(uid2))
218+
}, 5*time.Second, time.Second).Should(Succeed())
219+
220+
removeFinalizer(content, testFinalizer)
221+
222+
Eventually(func(g Gomega) {
223+
err := k8sClient.Get(ctx, types.NamespacedName{Name: content.Name}, &v1alpha1.Content{})
224+
g.Expect(err).To(HaveOccurred())
225+
}).Should(Succeed())
226+
})
227+
})
228+
229+
Context("Issue #4028 - Continue processing all bundledeployments on error", func() {
230+
var (
231+
bundle *v1alpha1.Bundle
232+
cluster1 *v1alpha1.Cluster
233+
cluster2 *v1alpha1.Cluster
234+
bundleName string
235+
testID string
236+
cluster1NS string
237+
cluster2NS string
238+
)
239+
240+
BeforeEach(func() {
241+
bundleName = "test-bundle-continue"
242+
testID = generateTestID()
243+
cluster1, cluster2 = setupClusters("cluster1-continue", "cluster2-continue", testID, []map[string]string{
244+
{"env": "test", "order": "1"},
245+
{"env": "test", "order": "2"},
246+
})
247+
cluster1NS = cluster1.Status.Namespace
248+
cluster2NS = cluster2.Status.Namespace
249+
250+
bundle = createBundle(bundleName, bundleNS, "default", "test-continue", []v1alpha1.BundleTarget{{
251+
ClusterSelector: &metav1.LabelSelector{
252+
MatchLabels: map[string]string{"env": "test"},
253+
MatchExpressions: []metav1.LabelSelectorRequirement{{
254+
Key: "order",
255+
Operator: metav1.LabelSelectorOpIn,
256+
Values: []string{"1", "2"},
257+
}},
258+
},
259+
}})
260+
})
261+
262+
AfterEach(func() {
263+
cleanupResources(bundle, bundleName, testID, cluster1, cluster2)
264+
})
265+
266+
It("should continue processing second bundledeployment when first fails", func() {
267+
By("capturing the original bundledeployment UIDs")
268+
var bd1UID, bd2UID types.UID
269+
Eventually(func(g Gomega) {
270+
bdList := getBundleDeployments(bundleName, bundleNS)
271+
g.Expect(bdList.Items).To(HaveLen(2))
272+
for _, bd := range bdList.Items {
273+
switch bd.Namespace {
274+
case cluster1NS:
275+
bd1UID = bd.UID
276+
case cluster2NS:
277+
bd2UID = bd.UID
278+
}
279+
}
280+
g.Expect(bd1UID).NotTo(BeEmpty())
281+
g.Expect(bd2UID).NotTo(BeEmpty())
282+
}).Should(Succeed())
283+
284+
By("putting cluster1 namespace in deleting state (causes bundledeployment update to fail)")
285+
cluster1Namespace := &corev1.Namespace{}
286+
Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cluster1NS}, cluster1Namespace)).To(Succeed())
287+
addFinalizer(cluster1Namespace, testFinalizerNS)
288+
Expect(k8sClient.Delete(ctx, cluster1Namespace)).To(Succeed())
289+
290+
Eventually(func(g Gomega) {
291+
g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cluster1NS}, cluster1Namespace)).To(Succeed())
292+
g.Expect(cluster1Namespace.DeletionTimestamp).NotTo(BeNil())
293+
}).Should(Succeed())
294+
295+
By("triggering bundle reconcile which will fail for cluster1 but should continue to cluster2")
296+
updateBundleDefaultNamespace(bundle, "kube-system")
297+
298+
By("verifying cluster2 bundledeployment was updated successfully despite cluster1 failure")
299+
Eventually(func(g Gomega) {
300+
bdList := &v1alpha1.BundleDeploymentList{}
301+
g.Expect(k8sClient.List(ctx, bdList, client.InNamespace(cluster2NS))).To(Succeed())
302+
303+
var bd2 *v1alpha1.BundleDeployment
304+
for _, bd := range bdList.Items {
305+
if bd.Labels["fleet.cattle.io/bundle-name"] == bundleName {
306+
bd2 = &bd
307+
break
308+
}
309+
}
310+
g.Expect(bd2).NotTo(BeNil())
311+
g.Expect(bd2.Spec.Options.DefaultNamespace).To(Equal("kube-system"))
312+
}).Should(Succeed())
313+
314+
removeFinalizer(cluster1Namespace, testFinalizerNS)
315+
})
316+
})
317+
})

‎internal/cmd/controller/reconciler/bundle_controller.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,14 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
358358

359359
helmvalues.ClearOptions(bd)
360360

361+
// If there's already a bundledeployment for this target, track its UID
362+
// before calling createBundleDeployment, which might fail. This prevents
363+
// cleanupOrphanedBundleDeployments from incorrectly removing this bundledeployment
364+
// as "orphaned". See https://github.com/rancher/fleet/issues/4144
365+
if target.Deployment != nil && target.Deployment.UID != "" {
366+
bundleDeploymentUIDs.Insert(target.Deployment.UID)
367+
}
368+
361369
bd, err = r.createBundleDeployment(
362370
ctx,
363371
logger,

0 commit comments

Comments
 (0)