Skip to content

Commit d892f42

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #417 from shapeblue/requeue-instead-of-logging-error
Requeue instead of throwing an error when onwer of kind can't be found
2 parents aa3252d + 5a66366 commit d892f42

File tree

4 files changed

+252
-8
lines changed

4 files changed

+252
-8
lines changed

‎controllers/utils/base_reconciler.go

+3
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,9 @@ func (r *ReconcilerBase) InitFromMgr(mgr ctrl.Manager, client cloud.Client) {
464464
func (r *ReconciliationRunner) GetParent(child client.Object, parent client.Object) CloudStackReconcilerMethod {
465465
return func() (ctrl.Result, error) {
466466
err := GetOwnerOfKind(r.RequestCtx, r.K8sClient, child, parent)
467+
if err != nil && strings.Contains(err.Error(), "couldn't find owner of kind") {
468+
return r.RequeueWithMessage(err.Error())
469+
}
467470
return ctrl.Result{}, err
468471
}
469472
}

‎controllers/utils/base_reconciler_test.go

+209
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
/*
2+
Copyright 2022 The Kubernetes 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 utils_test
18+
19+
import (
20+
"context"
21+
22+
"github.com/go-logr/logr"
23+
"github.com/golang/mock/gomock"
24+
. "github.com/onsi/ginkgo/v2"
25+
. "github.com/onsi/gomega"
26+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
27+
"k8s.io/apimachinery/pkg/runtime"
28+
"k8s.io/client-go/tools/record"
29+
infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3"
30+
"sigs.k8s.io/cluster-api-provider-cloudstack/controllers/utils"
31+
dummies "sigs.k8s.io/cluster-api-provider-cloudstack/test/dummies/v1beta3"
32+
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
33+
ctrl "sigs.k8s.io/controller-runtime"
34+
"sigs.k8s.io/controller-runtime/pkg/client"
35+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
36+
)
37+
38+
// mockConcreteRunner is a minimal implementation of the runner interface
39+
type mockConcreteRunner struct{}
40+
41+
func (m *mockConcreteRunner) ReconcileDelete() (ctrl.Result, error) {
42+
return ctrl.Result{}, nil
43+
}
44+
45+
func (m *mockConcreteRunner) Reconcile() (ctrl.Result, error) {
46+
return ctrl.Result{}, nil
47+
}
48+
49+
var _ = Describe("ReconciliationRunner", func() {
50+
var (
51+
mockCtrl *gomock.Controller
52+
k8sClient client.Client
53+
scheme *runtime.Scheme
54+
baseRunner *utils.ReconciliationRunner
55+
ctx context.Context
56+
mockRunner *mockConcreteRunner
57+
)
58+
59+
BeforeEach(func() {
60+
mockCtrl = gomock.NewController(GinkgoT())
61+
scheme = runtime.NewScheme()
62+
Expect(infrav1.AddToScheme(scheme)).To(Succeed())
63+
Expect(clusterv1.AddToScheme(scheme)).To(Succeed())
64+
65+
k8sClient = fake.NewClientBuilder().WithScheme(scheme).Build()
66+
ctx = context.Background()
67+
mockRunner = &mockConcreteRunner{}
68+
69+
// Create the base reconciler
70+
base := utils.ReconcilerBase{
71+
K8sClient: k8sClient,
72+
Scheme: scheme,
73+
BaseLogger: logr.Discard(),
74+
Recorder: record.NewFakeRecorder(10),
75+
}
76+
77+
// Create a reconciliation runner with our mock concrete runner
78+
baseRunner = utils.NewRunner(mockRunner, &infrav1.CloudStackMachine{}, "TestController")
79+
baseRunner.UsingBaseReconciler(base)
80+
baseRunner.WithRequestCtx(ctx)
81+
82+
// Setup a fake Request
83+
baseRunner.ForRequest(ctrl.Request{
84+
NamespacedName: client.ObjectKey{
85+
Namespace: "default",
86+
Name: "test-machine",
87+
},
88+
})
89+
})
90+
91+
AfterEach(func() {
92+
mockCtrl.Finish()
93+
})
94+
95+
Describe("GetParent", func() {
96+
var (
97+
child *infrav1.CloudStackMachine
98+
)
99+
100+
BeforeEach(func() {
101+
dummies.SetDummyVars()
102+
103+
// Set up child object
104+
child = &infrav1.CloudStackMachine{
105+
ObjectMeta: metav1.ObjectMeta{
106+
Name: "test-child",
107+
Namespace: "default",
108+
},
109+
}
110+
})
111+
112+
Context("when parent exists and is correctly referenced", func() {
113+
var parent *clusterv1.Machine
114+
BeforeEach(func() {
115+
parent = &clusterv1.Machine{
116+
ObjectMeta: metav1.ObjectMeta{
117+
Name: "test-parent",
118+
Namespace: "default",
119+
},
120+
}
121+
// Set up owner reference
122+
child.OwnerReferences = []metav1.OwnerReference{
123+
{
124+
APIVersion: clusterv1.GroupVersion.String(),
125+
Kind: "Machine",
126+
Name: parent.Name,
127+
UID: "test-uid",
128+
},
129+
}
130+
131+
// Create the objects in the fake client
132+
Expect(k8sClient.Create(ctx, parent)).To(Succeed())
133+
Expect(k8sClient.Create(ctx, child)).To(Succeed())
134+
})
135+
136+
It("should find the parent successfully", func() {
137+
// Create an empty parent object to be filled
138+
parentToFind := &clusterv1.Machine{}
139+
140+
// Call GetParent
141+
result, err := baseRunner.GetParent(child, parentToFind)()
142+
143+
// Check results
144+
Expect(err).NotTo(HaveOccurred())
145+
Expect(result).To(Equal(ctrl.Result{}))
146+
Expect(parentToFind.Name).To(Equal(parent.Name))
147+
Expect(parentToFind.Namespace).To(Equal(parent.Namespace))
148+
})
149+
})
150+
151+
Context("when parent doesn't exist", func() {
152+
BeforeEach(func() {
153+
// Set up owner reference to non-existent parent
154+
child.OwnerReferences = []metav1.OwnerReference{
155+
{
156+
APIVersion: clusterv1.GroupVersion.String(),
157+
Kind: "Machine",
158+
Name: "non-existent-parent",
159+
UID: "test-uid",
160+
},
161+
}
162+
163+
// Create only the child in the fake client
164+
Expect(k8sClient.Create(ctx, child)).To(Succeed())
165+
})
166+
167+
It("should return an error", func() {
168+
// Create an empty parent object to be filled
169+
parentToFind := &clusterv1.Machine{}
170+
171+
// Call GetParent
172+
_, err := baseRunner.GetParent(child, parentToFind)()
173+
174+
// Check results
175+
Expect(err).To(HaveOccurred())
176+
Expect(err.Error()).To(ContainSubstring("not found"))
177+
})
178+
})
179+
180+
Context("when no owner reference of requested kind exists", func() {
181+
BeforeEach(func() {
182+
// Set up owner reference to different kind
183+
child.OwnerReferences = []metav1.OwnerReference{
184+
{
185+
APIVersion: "different.api/v1",
186+
Kind: "DifferentKind",
187+
Name: "different-name",
188+
UID: "test-uid",
189+
},
190+
}
191+
192+
// Create only the child in the fake client
193+
Expect(k8sClient.Create(ctx, child)).To(Succeed())
194+
})
195+
196+
It("should requeue with error message", func() {
197+
// Create an empty parent object to be filled
198+
parentToFind := &clusterv1.Machine{}
199+
200+
// Call GetParent
201+
result, err := baseRunner.GetParent(child, parentToFind)()
202+
203+
// Check results
204+
Expect(err).NotTo(HaveOccurred())
205+
Expect(result.RequeueAfter).To(Equal(utils.RequeueTimeout))
206+
})
207+
})
208+
})
209+
})

‎controllers/utils/utils_suite_test.go

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/*
2+
Copyright 2022 The Kubernetes 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 utils_test
18+
19+
import (
20+
"testing"
21+
22+
. "github.com/onsi/ginkgo/v2"
23+
. "github.com/onsi/gomega"
24+
)
25+
26+
func TestUtils(t *testing.T) {
27+
RegisterFailHandler(Fail)
28+
RunSpecs(t, "Utils Suite")
29+
}

‎test/e2e/common.go

+11-8
Original file line numberDiff line numberDiff line change
@@ -361,18 +361,21 @@ func CheckAffinityGroupInProject(client *cloudstack.CloudStackClient, clusterNam
361361

362362
for _, affinity := range vm.Affinitygroup {
363363
affinityIds = append(affinityIds, affinity.Id)
364-
affinity, _, _ := client.AffinityGroup.GetAffinityGroupByID(affinity.Id, cloudstack.WithProject(project))
364+
// Store the ID before potential nil issues
365+
affinityID := affinity.Id
366+
// Rename the variable to avoid shadowing
367+
affinityGroup, _, err := client.AffinityGroup.GetAffinityGroupByID(affinityID, cloudstack.WithProject(project))
365368
if err != nil {
366-
Fail("Failed to get affinity group for " + affinity.Id + " : " + err.Error())
369+
Fail("Failed to get affinity group for " + affinityID + " : " + err.Error())
367370
}
368-
if !strings.Contains(affinity.Name, affinityTypeString) {
369-
Fail(affinity.Name + " does not contain " + affinityTypeString)
371+
if !strings.Contains(affinityGroup.Name, affinityTypeString) {
372+
Fail(affinityGroup.Name + " does not contain " + affinityTypeString)
370373
}
371-
if affinityType == "pro" && affinity.Type != "host affinity" {
372-
Fail(affinity.Type + " does not match " + affinityType)
374+
if affinityType == "pro" && affinityGroup.Type != "host affinity" {
375+
Fail(affinityGroup.Type + " does not match " + affinityType)
373376
}
374-
if affinityType == "anti" && affinity.Type != "host anti-affinity" {
375-
Fail(affinity.Type + " does not match " + affinityType)
377+
if affinityType == "anti" && affinityGroup.Type != "host anti-affinity" {
378+
Fail(affinityGroup.Type + " does not match " + affinityType)
376379
}
377380
}
378381
}

0 commit comments

Comments
 (0)