Merge pull request vmware-tanzu#1392 from dilyar85/bugfix/group-placement-condition

🐛 Set AlreadyPlaced group member condition correctly

Author: dilyar85

controllers/virtualmachinegroup/virtualmachinegroup_controller.go

@@ -626,8 +626,9 @@ func (r *Reconciler) getVMForPlacement(
 			return
 		}
 
-		// If both error and vm are nil, then the VM is already placed, or will
-		// be placed outside the group. Update PlacementReady condition to True.
+		// Set PlacementReady condition to True if the VM is already placed.
+		// For VMs with zone label override, VMG will receive reconciliation
+		// requests once they're actually placed and update the condition then.
 		if alreadyPlaced {
 			conditions.Set(memberStatus, &metav1.Condition{
 				Type:   vmopv1.VirtualMachineGroupMemberConditionPlacementReady,
@@ -640,8 +641,6 @@ func (r *Reconciler) getVMForPlacement(
 				//
 				Message: alreadyPlacedZone,
 			})
-		} else {
-			conditions.MarkTrue(memberStatus, vmopv1.VirtualMachineGroupMemberConditionPlacementReady)
 		}
 	}()
 
@@ -658,21 +657,6 @@ func (r *Reconciler) getVMForPlacement(
 		return nil, fmt.Errorf("VM %q is assigned to group %q instead of expected %q", vmName, gn, vmGroup.Name)
 	}
 
-	// Check if VM has uniqueID set first to update the AlreadyPlaced reason.
-	if vm.Status.UniqueID != "" {
-
-		alreadyPlaced = true
-		alreadyPlacedZone = vm.Status.Zone
-
-		pkglog.FromContextOrDefault(ctx).V(4).Info(
-			"VM has uniqueID, skipping group placement",
-			"vmName", vmName,
-			"uniqueID", vm.Status.UniqueID,
-			"zone", vm.Status.Zone,
-		)
-		return nil, nil
-	}
-
 	// Skip if the group already has placement condition ready true for this VM.
 	// Need to check the UID in case the VM is recreated with the same name and
 	// without being removed from the group (could have stale placement status).
@@ -688,8 +672,24 @@ func (r *Reconciler) getVMForPlacement(
 		return nil, nil
 	}
 
+	// Group status doesn't have placement condition ready for this VM (UID).
+	// Check if the VM is already placed or will be placed outside the group.
+
 	memberStatus.UID = vm.GetUID()
 
+	if vm.Status.UniqueID != "" {
+		alreadyPlaced = true
+		alreadyPlacedZone = vm.Status.Zone
+
+		pkglog.FromContextOrDefault(ctx).V(4).Info(
+			"VM has uniqueID, skipping group placement",
+			"vmName", vmName,
+			"uniqueID", vm.Status.UniqueID,
+			"zone", vm.Status.Zone,
+		)
+		return nil, nil
+	}
+
 	// If VM has a zone label, skip group placement to respect zone override.
 	if zoneName := vm.Labels[corev1.LabelTopologyZone]; zoneName != "" {
 		pkglog.FromContextOrDefault(ctx).V(4).Info(
@@ -700,6 +700,9 @@ func (r *Reconciler) getVMForPlacement(
 		return nil, nil
 	}
 
+	// VMG doesn't have a PlacementReady condition true for this VM (UID).
+	// VM is not already placed, nor has a zone label override.
+	// Return this VM to get it placed by the group.
 	return vm, nil
 }
 

controllers/virtualmachinegroup/virtualmachinegroup_controller_test.go

@@ -649,10 +649,43 @@ var _ = Describe(
 					setVMZoneLabel(vm3Key, "zone-c")
 				})
 
-				It("should skip placing VM by group and mark PlacementReady true", func() {
-					verifyGroupPlacementReadyCondition(vmGroup1Key, 3, "")
-					verifyGroupPlacementReadyCondition(vmGroup2Key, 1, "")
-					Expect(groupPlacementCallCount.Load()).To(BeZero(), "VM with zone label should not be placed by group")
+				It("should skip group placement and set PlacementReady after VM is placed", func() {
+					By("verifying provider was not called for group placement")
+					Expect(groupPlacementCallCount.Load()).To(BeZero(),
+						"VMs with zone label override should not be placed by group")
+
+					By("verifying PlacementReady is not set on member status")
+					Consistently(func(g Gomega) {
+						group := &vmopv1.VirtualMachineGroup{}
+						g.Expect(ctx.Client.Get(ctx, vmGroup1Key, group)).To(Succeed())
+						g.Expect(group.Status.Members).To(HaveLen(3))
+						for _, ms := range group.Status.Members {
+							if ms.Kind == virtualMachineKind {
+								g.Expect(conditions.Get(&ms, vmopv1.VirtualMachineGroupMemberConditionPlacementReady)).To(BeNil())
+							}
+						}
+					}, "3s").Should(Succeed())
+
+					By("simulating VMs being placed by setting status.uniqueID")
+					setVMUniqueID(vm1Key, "vm-unique-id-1", "zone-a")
+					setVMUniqueID(vm2Key, "vm-unique-id-2", "zone-b")
+					setVMUniqueID(vm3Key, "vm-unique-id-3", "zone-c")
+
+					By("triggering reconciliation of groups")
+					reconcileVMG(vmGroup1Key)
+					reconcileVMG(vmGroup2Key)
+
+					By("verifying group placement call count remains 0")
+					Expect(groupPlacementCallCount.Load()).To(BeZero())
+
+					By("verifying PlacementReady is now true with AlreadyPlaced reason")
+					verifyGroupPlacementReadyCondition(vmGroup1Key, 3, "AlreadyPlaced")
+					verifyGroupPlacementReadyCondition(vmGroup2Key, 1, "AlreadyPlaced")
+
+					By("verifying zone is preserved in PlacementReady condition message")
+					verifyGroupPlacementReadyConditionWithZone(vmGroup1Key, vm1Key, "zone-a")
+					verifyGroupPlacementReadyConditionWithZone(vmGroup2Key, vm2Key, "zone-b")
+					verifyGroupPlacementReadyConditionWithZone(vmGroup1Key, vm3Key, "zone-c")
 				})
 			})