Skip to content

Commit 768d13a

Browse files
fabriziopandinifabriziopandini
committed
Address comments
1 parent 44143fb commit 768d13a

File tree

3 files changed

+34
-43
lines changed

3 files changed

+34
-43
lines changed

‎internal/controllers/machine/machine_controller_noderef_test.go

+5-15
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"sigs.k8s.io/controller-runtime/pkg/reconcile"
3636

3737
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
38+
"sigs.k8s.io/cluster-api/api/v1beta1/index"
3839
"sigs.k8s.io/cluster-api/controllers/remote"
3940
"sigs.k8s.io/cluster-api/internal/test/builder"
4041
"sigs.k8s.io/cluster-api/internal/topology/ownerrefs"
@@ -108,7 +109,7 @@ func TestReconcileNode(t *testing.T) {
108109
},
109110
Status: corev1.NodeStatus{
110111
NodeInfo: corev1.NodeSystemInfo{
111-
MachineID: "fooo",
112+
MachineID: "foo",
112113
},
113114
Addresses: []corev1.NodeAddress{
114115
{
@@ -127,7 +128,9 @@ func TestReconcileNode(t *testing.T) {
127128
expectError: false,
128129
expected: func(g *WithT, m *clusterv1.Machine) {
129130
g.Expect(m.Status.NodeRef).ToNot(BeNil())
131+
g.Expect(m.Status.NodeRef.Name).To(Equal("test-node-1"))
130132
g.Expect(m.Status.NodeInfo).ToNot(BeNil())
133+
g.Expect(m.Status.NodeInfo.MachineID).To(Equal("foo"))
131134
},
132135
},
133136
{
@@ -162,7 +165,7 @@ func TestReconcileNode(t *testing.T) {
162165
t.Run(tc.name, func(t *testing.T) {
163166
g := NewWithT(t)
164167

165-
c := fake.NewClientBuilder().WithObjects(tc.machine).WithIndex(&corev1.Node{}, "spec.providerID", nodeByProviderID).Build()
168+
c := fake.NewClientBuilder().WithObjects(tc.machine).WithIndex(&corev1.Node{}, "spec.providerID", index.NodeByProviderID).Build()
166169
if tc.nodeGetErr {
167170
c = fake.NewClientBuilder().WithObjects(tc.machine).Build() // No Index
168171
}
@@ -193,19 +196,6 @@ func TestReconcileNode(t *testing.T) {
193196
}
194197
}
195198

196-
func nodeByProviderID(o client.Object) []string {
197-
node, ok := o.(*corev1.Node)
198-
if !ok {
199-
panic(fmt.Sprintf("Expected a Node but got a %T", o))
200-
}
201-
202-
if node.Spec.ProviderID == "" {
203-
return nil
204-
}
205-
206-
return []string{node.Spec.ProviderID}
207-
}
208-
209199
func TestGetNode(t *testing.T) {
210200
g := NewWithT(t)
211201

‎internal/controllers/machine/machine_controller_phases.go

+4-3
Original file line numberDiff line numberDiff line change
@@ -170,9 +170,7 @@ func (r *Reconciler) ensureExternalOwnershipAndWatch(ctx context.Context, cluste
170170
return external.ReconcileOutput{}, err
171171
}
172172

173-
// if external ref is paused, return error.
174173
if annotations.IsPaused(cluster, obj) {
175-
log.V(3).Info("External object referenced is paused")
176174
return external.ReconcileOutput{Result: obj, Paused: true}, nil
177175
}
178176
return external.ReconcileOutput{Result: obj}, nil
@@ -265,7 +263,7 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctr
265263
m.Status.FailureReason = ptr.To(capierrors.InvalidConfigurationMachineError)
266264
m.Status.FailureMessage = ptr.To(fmt.Sprintf("Machine infrastructure resource %v with name %q has been deleted after being ready",
267265
m.Spec.InfrastructureRef.GroupVersionKind(), m.Spec.InfrastructureRef.Name))
268-
return ctrl.Result{}, reconcile.TerminalError(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))
269267
}
270268
return ctrl.Result{RequeueAfter: infraReconcileResult.RequeueAfter}, nil
271269
}
@@ -322,6 +320,9 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctr
322320
m.Spec.FailureDomain = ptr.To(failureDomain)
323321
}
324322

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)
325326
m.Spec.ProviderID = ptr.To(providerID)
326327
m.Status.InfrastructureReady = true
327328
return ctrl.Result{}, nil

‎internal/controllers/machine/machine_controller_phases_test.go

+25-25
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ func TestReconcileMachinePhases(t *testing.T) {
135135
return false
136136
}
137137
g.Expect(bootstrapConfig.GetOwnerReferences()).To(HaveLen(1))
138-
g.Expect(bootstrapConfig.GetLabels()[clusterv1.ClusterNameLabel]).To(BeEquivalentTo("test-cluster"))
138+
g.Expect(bootstrapConfig.GetLabels()[clusterv1.ClusterNameLabel]).To(Equal("test-cluster"))
139139
return true
140140
}, 10*time.Second).Should(BeTrue())
141141

@@ -145,7 +145,7 @@ func TestReconcileMachinePhases(t *testing.T) {
145145
return false
146146
}
147147
g.Expect(infraMachine.GetOwnerReferences()).To(HaveLen(1))
148-
g.Expect(infraMachine.GetLabels()[clusterv1.ClusterNameLabel]).To(BeEquivalentTo("test-cluster"))
148+
g.Expect(infraMachine.GetLabels()[clusterv1.ClusterNameLabel]).To(Equal("test-cluster"))
149149
return true
150150
}, 10*time.Second).Should(BeTrue())
151151
})
@@ -726,11 +726,11 @@ func TestReconcileBootstrap(t *testing.T) {
726726
expected: func(g *WithT, m *clusterv1.Machine) {
727727
g.Expect(m.Status.BootstrapReady).To(BeTrue())
728728
g.Expect(m.Spec.Bootstrap.DataSecretName).NotTo(BeNil())
729-
g.Expect(*m.Spec.Bootstrap.DataSecretName).To(ContainSubstring("secret-data"))
729+
g.Expect(*m.Spec.Bootstrap.DataSecretName).To(Equal("secret-data"))
730730
},
731731
},
732732
{
733-
name: "bootstrap config ready but paused, it should reconcile and data should surface on the machine",
733+
name: "bootstrap config ready and paused, it should reconcile and data should surface on the machine",
734734
machine: defaultMachine.DeepCopy(),
735735
bootstrapConfig: map[string]interface{}{
736736
"kind": "GenericBootstrapConfig",
@@ -754,7 +754,7 @@ func TestReconcileBootstrap(t *testing.T) {
754754
expected: func(g *WithT, m *clusterv1.Machine) {
755755
g.Expect(m.Status.BootstrapReady).To(BeTrue())
756756
g.Expect(m.Spec.Bootstrap.DataSecretName).NotTo(BeNil())
757-
g.Expect(*m.Spec.Bootstrap.DataSecretName).To(ContainSubstring("secret-data"))
757+
g.Expect(*m.Spec.Bootstrap.DataSecretName).To(Equal("secret-data"))
758758
},
759759
},
760760
{
@@ -819,7 +819,7 @@ func TestReconcileBootstrap(t *testing.T) {
819819
expectError: false,
820820
expected: func(g *WithT, m *clusterv1.Machine) {
821821
g.Expect(m.Status.BootstrapReady).To(BeTrue())
822-
g.Expect(*m.Spec.Bootstrap.DataSecretName).To(BeEquivalentTo("secret-data"))
822+
g.Expect(*m.Spec.Bootstrap.DataSecretName).To(Equal("secret-data"))
823823
},
824824
},
825825
}
@@ -896,7 +896,7 @@ func TestReconcileInfrastructure(t *testing.T) {
896896
name string
897897
machine *clusterv1.Machine
898898
infraMachine map[string]interface{}
899-
InfraMachineGetError error
899+
infraMachineGetError error
900900
expectResult ctrl.Result
901901
expectError bool
902902
expected func(g *WithT, m *clusterv1.Machine)
@@ -905,15 +905,15 @@ func TestReconcileInfrastructure(t *testing.T) {
905905
name: "err reading infra machine (something different than not found), it should return error",
906906
machine: defaultMachine.DeepCopy(),
907907
infraMachine: nil,
908-
InfraMachineGetError: errors.New("some error"),
908+
infraMachineGetError: errors.New("some error"),
909909
expectResult: ctrl.Result{},
910910
expectError: true,
911911
},
912912
{
913913
name: "infra machine not found and infrastructure not yet ready, it should requeue",
914914
machine: defaultMachine.DeepCopy(),
915915
infraMachine: nil,
916-
InfraMachineGetError: nil,
916+
infraMachineGetError: nil,
917917
expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
918918
expectError: false,
919919
expected: func(g *WithT, m *clusterv1.Machine) {
@@ -949,7 +949,7 @@ func TestReconcileInfrastructure(t *testing.T) {
949949
},
950950
},
951951
},
952-
InfraMachineGetError: nil,
952+
infraMachineGetError: nil,
953953
expectResult: ctrl.Result{},
954954
expectError: false,
955955
expected: func(g *WithT, m *clusterv1.Machine) {
@@ -976,7 +976,7 @@ func TestReconcileInfrastructure(t *testing.T) {
976976
"ready": true,
977977
},
978978
},
979-
InfraMachineGetError: nil,
979+
infraMachineGetError: nil,
980980
expectResult: ctrl.Result{},
981981
expectError: false,
982982
expected: func(g *WithT, m *clusterv1.Machine) {
@@ -1004,7 +1004,7 @@ func TestReconcileInfrastructure(t *testing.T) {
10041004
"ready": true,
10051005
},
10061006
},
1007-
InfraMachineGetError: nil,
1007+
infraMachineGetError: nil,
10081008
expectResult: ctrl.Result{},
10091009
expectError: false,
10101010
expected: func(g *WithT, m *clusterv1.Machine) {
@@ -1041,14 +1041,14 @@ func TestReconcileInfrastructure(t *testing.T) {
10411041
},
10421042
},
10431043
},
1044-
InfraMachineGetError: nil,
1044+
infraMachineGetError: nil,
10451045
expectResult: ctrl.Result{},
10461046
expectError: false,
10471047
expected: func(g *WithT, m *clusterv1.Machine) {
10481048
g.Expect(m.Status.InfrastructureReady).To(BeTrue())
10491049
g.Expect(ptr.Deref(m.Spec.ProviderID, "")).To(Equal("test://id-1"))
10501050
g.Expect(m.Spec.FailureDomain).To(BeNil())
1051-
g.Expect(m.Status.Addresses).ToNot(BeNil())
1051+
g.Expect(m.Status.Addresses).To(HaveLen(2))
10521052
},
10531053
},
10541054
{
@@ -1079,18 +1079,18 @@ func TestReconcileInfrastructure(t *testing.T) {
10791079
},
10801080
},
10811081
},
1082-
InfraMachineGetError: nil,
1082+
infraMachineGetError: nil,
10831083
expectResult: ctrl.Result{},
10841084
expectError: false,
10851085
expected: func(g *WithT, m *clusterv1.Machine) {
10861086
g.Expect(m.Status.InfrastructureReady).To(BeTrue())
10871087
g.Expect(ptr.Deref(m.Spec.ProviderID, "")).To(Equal("test://id-1"))
10881088
g.Expect(ptr.Deref(m.Spec.FailureDomain, "")).To(Equal("foo"))
1089-
g.Expect(m.Status.Addresses).ToNot(BeNil())
1089+
g.Expect(m.Status.Addresses).To(HaveLen(2))
10901090
},
10911091
},
10921092
{
1093-
name: "infra machine ready but paused, it should reconcile but no data should surface on the machine",
1093+
name: "infra machine ready and paused, it should reconcile and no data should surface on the machine",
10941094
machine: defaultMachine.DeepCopy(),
10951095
infraMachine: map[string]interface{}{
10961096
"kind": "GenericInfrastructureMachine",
@@ -1120,14 +1120,14 @@ func TestReconcileInfrastructure(t *testing.T) {
11201120
},
11211121
},
11221122
},
1123-
InfraMachineGetError: nil,
1123+
infraMachineGetError: nil,
11241124
expectResult: ctrl.Result{},
11251125
expectError: false,
11261126
expected: func(g *WithT, m *clusterv1.Machine) {
11271127
g.Expect(m.Status.InfrastructureReady).To(BeTrue())
11281128
g.Expect(ptr.Deref(m.Spec.ProviderID, "")).To(Equal("test://id-1"))
11291129
g.Expect(ptr.Deref(m.Spec.FailureDomain, "")).To(Equal("foo"))
1130-
g.Expect(m.Status.Addresses).ToNot(BeNil())
1130+
g.Expect(m.Status.Addresses).To(HaveLen(2))
11311131
},
11321132
},
11331133
{
@@ -1145,7 +1145,7 @@ func TestReconcileInfrastructure(t *testing.T) {
11451145
"ready": true,
11461146
},
11471147
},
1148-
InfraMachineGetError: nil,
1148+
infraMachineGetError: nil,
11491149
expectResult: ctrl.Result{},
11501150
expectError: true,
11511151
},
@@ -1200,7 +1200,7 @@ func TestReconcileInfrastructure(t *testing.T) {
12001200
},
12011201
},
12021202
},
1203-
InfraMachineGetError: nil,
1203+
infraMachineGetError: nil,
12041204
expectResult: ctrl.Result{},
12051205
expectError: false,
12061206
expected: func(g *WithT, m *clusterv1.Machine) {
@@ -1261,7 +1261,7 @@ func TestReconcileInfrastructure(t *testing.T) {
12611261
},
12621262
},
12631263
},
1264-
InfraMachineGetError: nil,
1264+
infraMachineGetError: nil,
12651265
expectResult: ctrl.Result{},
12661266
expectError: false,
12671267
expected: func(g *WithT, m *clusterv1.Machine) {
@@ -1290,7 +1290,7 @@ func TestReconcileInfrastructure(t *testing.T) {
12901290
},
12911291
},
12921292
infraMachine: nil,
1293-
InfraMachineGetError: errors.New("some error"),
1293+
infraMachineGetError: errors.New("some error"),
12941294
expectResult: ctrl.Result{},
12951295
expectError: true,
12961296
expected: func(g *WithT, m *clusterv1.Machine) {
@@ -1318,7 +1318,7 @@ func TestReconcileInfrastructure(t *testing.T) {
13181318
},
13191319
},
13201320
infraMachine: nil,
1321-
InfraMachineGetError: nil,
1321+
infraMachineGetError: nil,
13221322
expectResult: ctrl.Result{},
13231323
expectError: true,
13241324
expected: func(g *WithT, m *clusterv1.Machine) {
@@ -1340,7 +1340,7 @@ func TestReconcileInfrastructure(t *testing.T) {
13401340
c := fake.NewClientBuilder().
13411341
WithObjects(tc.machine).Build()
13421342

1343-
if tc.InfraMachineGetError == nil {
1343+
if tc.infraMachineGetError == nil {
13441344
g.Expect(c.Create(ctx, builder.GenericInfrastructureMachineCRD.DeepCopy())).To(Succeed())
13451345
}
13461346

0 commit comments

Comments
 (0)