Skip to content

Commit e678f88

Browse files
fabriziopandinifabriziopandini
committed
Improve unit tests for Machine controller
1 parent 9569cd6 commit e678f88

File tree

3 files changed

+620
-243
lines changed

3 files changed

+620
-243
lines changed

‎internal/controllers/machine/machine_controller_noderef_test.go

+155
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
corev1 "k8s.io/api/core/v1"
2727
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2828
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
29+
"k8s.io/client-go/tools/record"
2930
"k8s.io/utils/ptr"
3031
ctrl "sigs.k8s.io/controller-runtime"
3132
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -34,13 +35,167 @@ import (
3435
"sigs.k8s.io/controller-runtime/pkg/reconcile"
3536

3637
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
38+
"sigs.k8s.io/cluster-api/api/v1beta1/index"
3739
"sigs.k8s.io/cluster-api/controllers/remote"
3840
"sigs.k8s.io/cluster-api/internal/test/builder"
3941
"sigs.k8s.io/cluster-api/internal/topology/ownerrefs"
4042
"sigs.k8s.io/cluster-api/util"
4143
"sigs.k8s.io/cluster-api/util/kubeconfig"
4244
)
4345

46+
func TestReconcileNode(t *testing.T) {
47+
defaultMachine := clusterv1.Machine{
48+
ObjectMeta: metav1.ObjectMeta{
49+
Name: "machine-test",
50+
Namespace: metav1.NamespaceDefault,
51+
Labels: map[string]string{
52+
clusterv1.ClusterNameLabel: "test-cluster",
53+
},
54+
},
55+
Spec: clusterv1.MachineSpec{
56+
ProviderID: ptr.To("aws://us-east-1/test-node-1"),
57+
},
58+
}
59+
60+
defaultCluster := &clusterv1.Cluster{
61+
ObjectMeta: metav1.ObjectMeta{
62+
Name: "test-cluster",
63+
Namespace: metav1.NamespaceDefault,
64+
},
65+
}
66+
67+
testCases := []struct {
68+
name string
69+
machine *clusterv1.Machine
70+
node *corev1.Node
71+
nodeGetErr bool
72+
expectResult ctrl.Result
73+
expectError bool
74+
expected func(g *WithT, m *clusterv1.Machine)
75+
}{
76+
{
77+
name: "No op if provider ID is not set",
78+
machine: &clusterv1.Machine{},
79+
node: nil,
80+
nodeGetErr: false,
81+
expectResult: ctrl.Result{},
82+
expectError: false,
83+
},
84+
{
85+
name: "err reading node (something different than not found), it should return error",
86+
machine: defaultMachine.DeepCopy(),
87+
node: nil,
88+
nodeGetErr: true,
89+
expectResult: ctrl.Result{},
90+
expectError: true,
91+
},
92+
{
93+
name: "waiting for the node to exist, no op",
94+
machine: defaultMachine.DeepCopy(),
95+
node: nil,
96+
nodeGetErr: false,
97+
expectResult: ctrl.Result{},
98+
expectError: false,
99+
},
100+
{
101+
name: "node found, should surface info",
102+
machine: defaultMachine.DeepCopy(),
103+
node: &corev1.Node{
104+
ObjectMeta: metav1.ObjectMeta{
105+
Name: "test-node-1",
106+
},
107+
Spec: corev1.NodeSpec{
108+
ProviderID: "aws://us-east-1/test-node-1",
109+
},
110+
Status: corev1.NodeStatus{
111+
NodeInfo: corev1.NodeSystemInfo{
112+
MachineID: "foo",
113+
},
114+
Addresses: []corev1.NodeAddress{
115+
{
116+
Type: corev1.NodeInternalIP,
117+
Address: "1.1.1.1",
118+
},
119+
{
120+
Type: corev1.NodeInternalIP,
121+
Address: "2.2.2.2",
122+
},
123+
},
124+
},
125+
},
126+
nodeGetErr: false,
127+
expectResult: ctrl.Result{},
128+
expectError: false,
129+
expected: func(g *WithT, m *clusterv1.Machine) {
130+
g.Expect(m.Status.NodeRef).ToNot(BeNil())
131+
g.Expect(m.Status.NodeRef.Name).To(Equal("test-node-1"))
132+
g.Expect(m.Status.NodeInfo).ToNot(BeNil())
133+
g.Expect(m.Status.NodeInfo.MachineID).To(Equal("foo"))
134+
},
135+
},
136+
{
137+
name: "node not found when already seen, should error",
138+
machine: &clusterv1.Machine{
139+
ObjectMeta: metav1.ObjectMeta{
140+
Name: "machine-test",
141+
Namespace: metav1.NamespaceDefault,
142+
Labels: map[string]string{
143+
clusterv1.ClusterNameLabel: "test-cluster",
144+
},
145+
},
146+
Spec: clusterv1.MachineSpec{
147+
ProviderID: ptr.To("aws://us-east-1/test-node-1"),
148+
},
149+
Status: clusterv1.MachineStatus{
150+
NodeRef: &corev1.ObjectReference{
151+
Kind: "Node",
152+
Name: "test-node-1",
153+
APIVersion: "v1",
154+
},
155+
},
156+
},
157+
node: nil,
158+
nodeGetErr: false,
159+
expectResult: ctrl.Result{},
160+
expectError: true,
161+
},
162+
}
163+
164+
for _, tc := range testCases {
165+
t.Run(tc.name, func(t *testing.T) {
166+
g := NewWithT(t)
167+
168+
c := fake.NewClientBuilder().WithObjects(tc.machine).WithIndex(&corev1.Node{}, "spec.providerID", index.NodeByProviderID).Build()
169+
if tc.nodeGetErr {
170+
c = fake.NewClientBuilder().WithObjects(tc.machine).Build() // No Index
171+
}
172+
173+
if tc.node != nil {
174+
g.Expect(c.Create(ctx, tc.node)).To(Succeed())
175+
defer func() { _ = c.Delete(ctx, tc.node) }()
176+
}
177+
178+
r := &Reconciler{
179+
Tracker: remote.NewTestClusterCacheTracker(ctrl.Log, c, c, fakeScheme, client.ObjectKeyFromObject(defaultCluster)),
180+
Client: c,
181+
recorder: record.NewFakeRecorder(10),
182+
}
183+
s := &scope{cluster: defaultCluster, machine: tc.machine}
184+
result, err := r.reconcileNode(ctx, s)
185+
g.Expect(result).To(BeComparableTo(tc.expectResult))
186+
if tc.expectError {
187+
g.Expect(err).To(HaveOccurred())
188+
} else {
189+
g.Expect(err).ToNot(HaveOccurred())
190+
}
191+
192+
if tc.expected != nil {
193+
tc.expected(g, tc.machine)
194+
}
195+
})
196+
}
197+
}
198+
44199
func TestGetNode(t *testing.T) {
45200
g := NewWithT(t)
46201

‎internal/controllers/machine/machine_controller_phases.go

+13-21
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import (
3232
ctrl "sigs.k8s.io/controller-runtime"
3333
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
3434
"sigs.k8s.io/controller-runtime/pkg/handler"
35+
"sigs.k8s.io/controller-runtime/pkg/reconcile"
3536

3637
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
3738
"sigs.k8s.io/cluster-api/controllers/external"
@@ -95,10 +96,9 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
9596
if err != nil {
9697
return external.ReconcileOutput{}, err
9798
}
98-
if result.RequeueAfter > 0 || result.Paused {
99+
if result.RequeueAfter > 0 {
99100
return result, nil
100101
}
101-
102102
obj := result.Result
103103

104104
// Set failure reason and message, if any.
@@ -139,12 +139,6 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste
139139
return external.ReconcileOutput{}, err
140140
}
141141

142-
// if external ref is paused, return error.
143-
if annotations.IsPaused(cluster, obj) {
144-
log.V(3).Info("External object referenced is paused")
145-
return external.ReconcileOutput{Paused: true}, nil
146-
}
147-
148142
// Initialize the patch helper.
149143
patchHelper, err := patch.NewHelper(obj, r.Client)
150144
if err != nil {
@@ -176,6 +170,9 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste
176170
return external.ReconcileOutput{}, err
177171
}
178172

173+
if annotations.IsPaused(cluster, obj) {
174+
return external.ReconcileOutput{Result: obj, Paused: true}, nil
175+
}
179176
return external.ReconcileOutput{Result: obj}, nil
180177
}
181178

@@ -197,11 +194,6 @@ func (r *Reconciler) reconcileBootstrap(ctx context.Context, s *scope) (ctrl.Res
197194
}
198195
s.bootstrapConfig = externalResult.Result
199196

200-
// If the external object is paused return.
201-
if externalResult.Paused {
202-
return ctrl.Result{}, nil
203-
}
204-
205197
if externalResult.RequeueAfter > 0 {
206198
return ctrl.Result{RequeueAfter: externalResult.RequeueAfter}, nil
207199
}
@@ -271,14 +263,11 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctr
271263
m.Status.FailureReason = ptr.To(capierrors.InvalidConfigurationMachineError)
272264
m.Status.FailureMessage = ptr.To(fmt.Sprintf("Machine infrastructure resource %v with name %q has been deleted after being ready",
273265
m.Spec.InfrastructureRef.GroupVersionKind(), m.Spec.InfrastructureRef.Name))
274-
return ctrl.Result{}, errors.Errorf("could not find %v %q for Machine %q in namespace %q, requeuing", m.Spec.InfrastructureRef.GroupVersionKind().String(), m.Spec.InfrastructureRef.Name, m.Name, m.Namespace)
266+
return ctrl.Result{}, reconcile.TerminalError(errors.Errorf("could not find %v %q for Machine %q in namespace %q", m.Spec.InfrastructureRef.GroupVersionKind().String(), m.Spec.InfrastructureRef.Name, m.Name, m.Namespace))
275267
}
276268
return ctrl.Result{RequeueAfter: infraReconcileResult.RequeueAfter}, nil
277269
}
278-
// if the external object is paused, return without any further processing
279-
if infraReconcileResult.Paused {
280-
return ctrl.Result{}, nil
281-
}
270+
282271
infraConfig := infraReconcileResult.Result
283272

284273
if !infraConfig.GetDeletionTimestamp().IsZero() {
@@ -293,16 +282,15 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctr
293282
if ready && !m.Status.InfrastructureReady {
294283
log.Info("Infrastructure provider has completed machine infrastructure provisioning and reports status.ready", infraConfig.GetKind(), klog.KObj(infraConfig))
295284
}
296-
m.Status.InfrastructureReady = ready
297285

298286
// Report a summary of current status of the infrastructure object defined for this machine.
299287
conditions.SetMirror(m, clusterv1.InfrastructureReadyCondition,
300288
conditions.UnstructuredGetter(infraConfig),
301289
conditions.WithFallbackValue(ready, clusterv1.WaitingForInfrastructureFallbackReason, clusterv1.ConditionSeverityInfo, ""),
302290
)
303291

304-
// If the infrastructure provider is not ready, return early.
305-
if !ready {
292+
// If the infrastructure provider is not ready (and it wasn't ready before), return early.
293+
if !ready && !m.Status.InfrastructureReady {
306294
log.Info("Waiting for infrastructure provider to create machine infrastructure and report status.ready", infraConfig.GetKind(), klog.KObj(infraConfig))
307295
return ctrl.Result{}, nil
308296
}
@@ -332,7 +320,11 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctr
332320
m.Spec.FailureDomain = ptr.To(failureDomain)
333321
}
334322

323+
// When we hit this point provider id is set, and either:
324+
// - the infra machine is reporting ready for the first time
325+
// - the infra machine already reported ready (and thus m.Status.InfrastructureReady is already true and it should not flip back)
335326
m.Spec.ProviderID = ptr.To(providerID)
327+
m.Status.InfrastructureReady = true
336328
return ctrl.Result{}, nil
337329
}
338330

0 commit comments

Comments
 (0)