Skip to content

Commit 9cc5762

Browse files
zhanggbjzhanggbj
committed
Fix UT failure
Signed-off-by: Gong Zhang <gong.zhang@broadcom.com>
1 parent dc441f6 commit 9cc5762

File tree

2 files changed

+52
-87
lines changed

2 files changed

+52
-87
lines changed

‎controllers/vmware/virtualmachinegroup_reconciler.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ func isMemberUpdateAllowed(ctx context.Context, kubeClient client.Client, target
359359
}
360360

361361
// If FailureDomain is NOT set. Requires placement or placement Annotation. Fall through to full VMG Annotation check.
362-
logger.V(5).Info("New member's CAPI Machine lacks FailureDomain. Falling through to full VMG Ready and Annotation check.", "MachineName", machineOwnerName)
362+
logger.V(5).Info("New member's CAPI Machine lacks FailureDomain. Falling through to VMG Annotation check.", "MachineName", machineOwnerName)
363363

364364
// If no Placement Annotations, skip member update and wait for it.
365365
annotations := vmg.GetAnnotations()
@@ -368,6 +368,9 @@ func isMemberUpdateAllowed(ctx context.Context, kubeClient client.Client, target
368368
}
369369

370370
mdLabelName := vsphereMachine.Labels[clusterv1.MachineDeploymentNameLabel]
371+
if mdLabelName == "" {
372+
return false, errors.Wrapf(nil, "VSphereMachine doesn't have MachineDeployment name label %s", klog.KObj(vsphereMachine))
373+
}
371374

372375
annotationKey := fmt.Sprintf("%s/%s", ZoneAnnotationPrefix, mdLabelName)
373376

‎controllers/vmware/virtualmachinegroup_reconciler_test.go‎

Lines changed: 48 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -41,20 +41,22 @@ import (
4141
)
4242

4343
const (
44-
clusterName = "test-cluster"
45-
otherClusterName = "other-cluster"
46-
clusterNamespace = "test-ns"
47-
mdName1 = "md-worker-a"
48-
mdName2 = "md-worker-b"
49-
mdNameStale = "md-stale-c"
50-
zoneA = "zone-a"
51-
zoneB = "zone-b"
52-
vmgName = "test-vmg"
53-
vmgNamespace = "test-vmg-ns"
54-
memberName1 = "vm-01"
55-
memberName2 = "vm-02"
56-
memberKind = "VirtualMachine"
57-
failureDomainA = "zone-1"
44+
clusterName = "test-cluster"
45+
otherClusterName = "other-cluster"
46+
clusterNamespace = "test-ns"
47+
mdName1 = "md-worker-a"
48+
mdName2 = "md-worker-b"
49+
mdNameStale = "md-stale-c"
50+
zoneA = "zone-a"
51+
zoneB = "zone-b"
52+
vmgName = "test-vmg"
53+
vmgNamespace = "test-vmg-ns"
54+
memberName1 = "vm-01"
55+
memberName2 = "vm-02"
56+
ownerMachineName1 = "m-01"
57+
ownerMachineName2 = "m-02"
58+
memberKind = "VirtualMachine"
59+
failureDomainA = "zone-a"
5860
)
5961

6062
func TestIsMemberUpdateAllowed(t *testing.T) {
@@ -95,7 +97,7 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
9597
UID: types.UID(ownerMachineName + "-uid"),
9698
},
9799
},
98-
Labels: map[string]string{fmt.Sprintf("%s/%s", ZoneAnnotationPrefix, mdName): "zone-1"},
100+
Labels: map[string]string{clusterv1.MachineDeploymentNameLabel: mdName},
99101
},
100102
}
101103
}
@@ -183,17 +185,8 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
183185
{
184186
name: "Skip member update if new member VSphereMachine Not Found",
185187
targetMember: []vmoprv1.GroupMember{member(memberName1), member(memberName2)}, // vm-02 is new
186-
vmgInput: func() *vmoprv1.VirtualMachineGroup {
187-
v := baseVMG.DeepCopy()
188-
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
189-
{
190-
Name: memberName1,
191-
Kind: memberKind,
192-
},
193-
}}}
194-
return v
195-
}(),
196-
mdNames: []string{mdName1},
188+
vmgInput: baseVMG.DeepCopy(),
189+
mdNames: []string{mdName1},
197190
existingObjects: func() []runtime.Object {
198191
v := baseVMG.DeepCopy()
199192
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
@@ -203,25 +196,16 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
203196
},
204197
}}}
205198
// vm-02 VSphereMachine is missing
206-
return []runtime.Object{v, makeVSphereMachineOwned(memberName1, vmgNamespace, "m-01", mdName1), makeCAPIMachine("m-01", vmgNamespace, ptr.To(failureDomainA))}
199+
return []runtime.Object{v, makeVSphereMachineOwned(memberName1, vmgNamespace, ownerMachineName1, mdName1), makeCAPIMachine(ownerMachineName1, vmgNamespace, ptr.To(failureDomainA))}
207200
}(),
208201
wantAllowed: false,
209202
wantErr: false,
210203
},
211204
{
212205
name: "Skip member update if VSphereMachine found but CAPI Machine missing",
213206
targetMember: []vmoprv1.GroupMember{member(memberName1), member(memberName2)}, // vm-02 is new
214-
vmgInput: func() *vmoprv1.VirtualMachineGroup {
215-
v := baseVMG.DeepCopy()
216-
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
217-
{
218-
Name: memberName1,
219-
Kind: memberKind,
220-
},
221-
}}}
222-
return v
223-
}(),
224-
mdNames: []string{mdName1},
207+
vmgInput: baseVMG.DeepCopy(),
208+
mdNames: []string{mdName1},
225209
existingObjects: func() []runtime.Object {
226210
v := baseVMG.DeepCopy()
227211
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
@@ -231,25 +215,16 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
231215
},
232216
}}}
233217
// vm-02 VSphereMachine exists but has no owner ref
234-
return []runtime.Object{v, makeVSphereMachineOwned(memberName1, vmgNamespace, "m-01", mdName1), makeCAPIMachine("m-01", vmgNamespace, ptr.To(failureDomainA)), makeVSphereMachineNoOwner(memberName2, vmgNamespace)}
218+
return []runtime.Object{v, makeVSphereMachineOwned(memberName1, vmgNamespace, "ownerMachineName1", mdName1), makeCAPIMachine("ownerMachineName1", vmgNamespace, ptr.To(failureDomainA)), makeVSphereMachineNoOwner(memberName2, vmgNamespace)}
235219
}(),
236220
wantAllowed: false,
237221
wantErr: false,
238222
},
239223
{
240224
name: "Allow member update if all new members have Machine FailureDomain specified",
241225
targetMember: []vmoprv1.GroupMember{member(memberName1), member(memberName2)}, // vm-02 is new
242-
vmgInput: func() *vmoprv1.VirtualMachineGroup {
243-
v := baseVMG.DeepCopy()
244-
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
245-
{
246-
Name: memberName1,
247-
Kind: memberKind,
248-
},
249-
}}}
250-
return v
251-
}(),
252-
mdNames: []string{mdName1},
226+
vmgInput: baseVMG.DeepCopy(),
227+
mdNames: []string{mdName1},
253228
existingObjects: func() []runtime.Object {
254229
v := baseVMG.DeepCopy()
255230
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
@@ -258,11 +233,11 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
258233
Kind: memberKind,
259234
},
260235
}}}
261-
// m-02 (owner of vm-02) has FailureDomain set -> Allowed
236+
// m-02 (owner of vownerMachineName2) has FailureDomain set
262237
return []runtime.Object{
263238
v,
264-
makeVSphereMachineOwned(memberName1, vmgNamespace, "m-01", mdName1), makeCAPIMachine("m-01", vmgNamespace, nil),
265-
makeVSphereMachineOwned(memberName2, vmgNamespace, "m-02", mdName2), makeCAPIMachine("m-02", vmgNamespace, ptr.To(failureDomainA)),
239+
makeVSphereMachineOwned(memberName1, vmgNamespace, "ownerMachineName1", mdName1), makeCAPIMachine("ownerMachineName1", vmgNamespace, nil),
240+
makeVSphereMachineOwned(memberName2, vmgNamespace, "ownerMachineName2", mdName2), makeCAPIMachine("ownerMachineName2", vmgNamespace, ptr.To(failureDomainA)),
266241
}
267242
}(),
268243
wantAllowed: true, // Allowed because new members don't require VMO placement
@@ -290,17 +265,8 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
290265
{
291266
name: "Skip member update if new member Machine requires placement annotation",
292267
targetMember: []vmoprv1.GroupMember{member(memberName1), member(memberName2)}, // vm-02 is new and requires placement
293-
vmgInput: func() *vmoprv1.VirtualMachineGroup {
294-
v := baseVMG.DeepCopy()
295-
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
296-
{
297-
Name: memberName1,
298-
Kind: memberKind,
299-
},
300-
}}}
301-
return v
302-
}(),
303-
mdNames: []string{mdName1},
268+
vmgInput: baseVMG.DeepCopy(),
269+
mdNames: []string{mdName1},
304270
existingObjects: func() []runtime.Object {
305271
v := baseVMG.DeepCopy()
306272
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
@@ -312,8 +278,8 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
312278
// m-02 lacks FailureDomain and new Member requires placement annotation
313279
return []runtime.Object{
314280
v,
315-
makeVSphereMachineOwned(memberName1, vmgNamespace, "m-01", mdName1), makeCAPIMachine("m-01", vmgNamespace, ptr.To(failureDomainA)),
316-
makeVSphereMachineOwned(memberName2, vmgNamespace, "m-02", mdName2), makeUnplacedCAPIMachine("m-02", vmgNamespace),
281+
makeVSphereMachineOwned(memberName1, vmgNamespace, "ownerMachineName1", mdName1), makeCAPIMachine("ownerMachineName1", vmgNamespace, ptr.To(failureDomainA)),
282+
makeVSphereMachineOwned(memberName2, vmgNamespace, "ownerMachineName2", mdName2), makeUnplacedCAPIMachine("ownerMachineName2", vmgNamespace),
317283
}
318284
}(),
319285
wantAllowed: false,
@@ -322,20 +288,14 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
322288
{
323289
name: "Allow new member Machine since required placement annotation exists",
324290
targetMember: []vmoprv1.GroupMember{member(memberName1), member(memberName2)}, // vm-02 is new and requires placement
325-
vmgInput: func() *vmoprv1.VirtualMachineGroup {
326-
v := baseVMG.DeepCopy()
327-
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
328-
{
329-
Name: memberName1,
330-
Kind: memberKind,
331-
},
332-
}}}
333-
return v
334-
}(),
335-
mdNames: []string{mdName1},
291+
vmgInput: baseVMG.DeepCopy(),
292+
mdNames: []string{mdName1},
336293
existingObjects: func() []runtime.Object {
337294
v := baseVMG.DeepCopy()
338-
v.Annotations = map[string]string{fmt.Sprintf("%s/%s", ZoneAnnotationPrefix, mdName1): zoneA}
295+
v.Annotations = map[string]string{
296+
fmt.Sprintf("%s/%s", ZoneAnnotationPrefix, mdName1): zoneA,
297+
fmt.Sprintf("%s/%s", ZoneAnnotationPrefix, mdName2): zoneB,
298+
}
339299
v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{{Members: []vmoprv1.GroupMember{
340300
{
341301
Name: memberName1,
@@ -344,8 +304,8 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
344304
}}}
345305
return []runtime.Object{
346306
v,
347-
makeVSphereMachineOwned(memberName1, vmgNamespace, "m-01", mdName1), makeCAPIMachine("m-01", vmgNamespace, ptr.To(failureDomainA)),
348-
makeVSphereMachineOwned(memberName2, vmgNamespace, "m-02", mdName2), makeUnplacedCAPIMachine("m-02", vmgNamespace),
307+
makeVSphereMachineOwned(memberName1, vmgNamespace, "ownerMachineName1", mdName1), makeCAPIMachine("ownerMachineName1", vmgNamespace, nil),
308+
makeVSphereMachineOwned(memberName2, vmgNamespace, "ownerMachineName2", mdName2), makeUnplacedCAPIMachine("ownerMachineName2", vmgNamespace),
349309
}
350310
}(),
351311
wantAllowed: true,
@@ -354,19 +314,21 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
354314
}
355315

356316
for _, tt := range tests {
317+
// Looks odd, but need to reinitialize test variable
318+
tt := tt
357319
t.Run(tt.name, func(t *testing.T) {
320+
g := NewWithT(t)
358321
kubeClient := fake.NewClientBuilder().WithRuntimeObjects(tt.existingObjects...).Build()
359322

360323
vmgInput := tt.vmgInput.DeepCopy()
361324

362325
gotAllowed, err := isMemberUpdateAllowed(ctx, kubeClient, tt.targetMember, vmgInput)
363326

364-
if (err != nil) != tt.wantErr {
365-
t.Fatalf("isMemberUpdateAllowed() error = %v, wantErr %v", err, tt.wantErr)
366-
}
367-
368-
if gotAllowed != tt.wantAllowed {
369-
t.Errorf("isMemberUpdateAllowed() gotAllowed = %t, wantAllowed %t", gotAllowed, tt.wantAllowed)
327+
if tt.wantErr {
328+
g.Expect(err).To(HaveOccurred())
329+
} else {
330+
g.Expect(err).NotTo(HaveOccurred())
331+
g.Expect(gotAllowed).To(Equal(true))
370332
}
371333
})
372334
}

0 commit comments

Comments
 (0)