test(vmnetcfg): add complex test case for sync handler

Signed-off-by: Zespre Chang <zespre.chang@suse.com>
(cherry picked from commit 0efc388)

Author: starbops

pkg/controller/vm/controller.go

@@ -106,6 +106,11 @@ func (h *Handler) OnChange(key string, vm *kubevirtv1.VirtualMachine) (*kubevirt
 	vmNetCfgCpy := oldVmNetCfg.DeepCopy()
 	vmNetCfgCpy.Spec.NetworkConfigs = vmNetCfg.Spec.NetworkConfigs
 
+	// The following block is a two-step process. Ideally,
+	// 1. if the network config of the VirtualMachine has been changed, update the status of the VirtualMachineNetworkConfig
+	//   to out-of-sync so that the vmnetcfg-controller can handle it accordingly, and
+	// 2. since the spec of the VirtualMachineNetworkConfig hasn't been changed, update it to reflect the new network config.
+	// This is to throttle the vmnetcfg-controller and to avoid allocate-before-deallocate from happening.
 	if !reflect.DeepEqual(vmNetCfgCpy.Spec.NetworkConfigs, oldVmNetCfg.Spec.NetworkConfigs) {
 		if networkv1.InSynced.IsFalse(oldVmNetCfg) {
 			logrus.Infof("(vm.OnChange) vmnetcfg %s/%s is deemed out-of-sync, updating it", vmNetCfgCpy.Namespace, vmNetCfgCpy.Name)

pkg/controller/vmnetcfg/controller.go

@@ -228,10 +228,6 @@ func (h *Handler) Sync(vmNetCfg *networkv1.VirtualMachineNetworkConfig, status n
 		return status, fmt.Errorf("vmnetcfg %s/%s was administratively disabled", vmNetCfg.Namespace, vmNetCfg.Name)
 	}
 
-	if len(vmNetCfg.Spec.NetworkConfigs) == 0 {
-		return status, fmt.Errorf("vmnetcfg %s/%s has no network configs", vmNetCfg.Namespace, vmNetCfg.Name)
-	}
-
 	// Nothing to do if the VirtualMachineNetworkConfig is already in-sync
 	if networkv1.InSynced.IsTrue(vmNetCfg) {
 		logrus.Debugf("(vmnetcfg.InSynced) vmnetcfg %s/%s is in-sync", vmNetCfg.Namespace, vmNetCfg.Name)
@@ -246,25 +242,23 @@ func (h *Handler) Sync(vmNetCfg *networkv1.VirtualMachineNetworkConfig, status n
 		macAddressSet[nc.MACAddress] = struct{}{}
 	}
 
-	// Mark the NetworkConfigStatus as stale if the MAC address is not in the Spec
+	// Mark the NetworkConfigStatus as stale if the MAC address is not in
+	// the Spec; otherwise, add it to the non-stale list.
+	var nonStaleNetworkConfigs []networkv1.NetworkConfigStatus
 	for i, ncStatus := range status.NetworkConfigs {
 		if _, ok := macAddressSet[ncStatus.MACAddress]; !ok {
 			status.NetworkConfigs[i].State = networkv1.StaleState
+			continue
 		}
+		nonStaleNetworkConfigs = append(nonStaleNetworkConfigs, ncStatus)
 	}
 
 	// Cleanup the stale records
 	if err := h.cleanup(vmNetCfg, true); err != nil {
 		return status, err
 	}
 
-	// Remove the stale NetworkConfigStatus from the status
-	var nonStaleNetworkConfigs []networkv1.NetworkConfigStatus
-	for _, ncStatus := range status.NetworkConfigs {
-		if ncStatus.State != networkv1.StaleState {
-			nonStaleNetworkConfigs = append(nonStaleNetworkConfigs, ncStatus)
-		}
-	}
+	// Update the VirtualMachineNetworkConfig status after cleanup
 	status.NetworkConfigs = nonStaleNetworkConfigs
 
 	return status, nil

pkg/controller/vmnetcfg/controller_test.go

@@ -37,9 +37,11 @@ const (
 	testIPAddress1  = "192.168.0.111"
 	testIPAddress2  = "192.168.0.177"
 	testIPAddress3  = "192.168.0.189"
+	testIPAddress4  = "192.168.0.199"
 	testMACAddress1 = "11:22:33:44:55:66"
 	testMACAddress2 = "22:33:44:55:66:77"
 	testMACAddress3 = "33:44:55:66:77:88"
+	testMACAddress4 = "44:55:66:77:88:99"
 )
 
 func newTestVmNetCfgBuilder() *vmNetCfgBuilder {
@@ -655,7 +657,7 @@ func TestHandler_Sync(t *testing.T) {
 		assert.Equal(t, expectedCacheAllocator, handler.cacheAllocator)
 	})
 
-	t.Run("sync new vmnetcfg with empty network config should error", func(t *testing.T) {
+	t.Run("sync new vmnetcfg with empty network config should succeed", func(t *testing.T) {
 		givenVmNetCfg := newTestVmNetCfgBuilder().Build()
 		givenIPPool := newTestIPPoolBuilder().
 			ServerIP(testServerIP).
@@ -691,7 +693,7 @@ func TestHandler_Sync(t *testing.T) {
 		}
 
 		status, err := handler.Sync(givenVmNetCfg, givenVmNetCfg.Status)
-		assert.Equal(t, fmt.Sprintf("vmnetcfg %s/%s has no network configs", testVmNetCfgNamespace, testVmNetCfgName), err.Error())
+		assert.Nil(t, err)
 
 		SanitizeStatus(&expectedStatus)
 		SanitizeStatus(&status)
@@ -1006,4 +1008,94 @@ func TestHandler_Sync(t *testing.T) {
 		assert.Equal(t, expectedIPAllocator, handler.ipAllocator)
 		assert.Equal(t, expectedCacheAllocator, handler.cacheAllocator)
 	})
+
+	t.Run("sync out-of-sync vmnetcfg with additional, modified, and missing network configs should succeed and stale records should be removed", func(t *testing.T) {
+		givenVmNetCfg := newTestVmNetCfgBuilder().
+			WithNetworkConfig(testIPAddress1, testMACAddress1, testNetworkName).
+			WithNetworkConfig(testIPAddress4, testMACAddress4, testNetworkName).
+			WithNetworkConfigStatus(testIPAddress1, testMACAddress1, testNetworkName, networkv1.AllocatedState).
+			WithNetworkConfigStatus(testIPAddress2, testMACAddress2, testNetworkName, networkv1.AllocatedState).
+			WithNetworkConfigStatus(testIPAddress3, testMACAddress3, testNetworkName, networkv1.AllocatedState).Build()
+		givenIPPool := newTestIPPoolBuilder().
+			ServerIP(testServerIP).
+			CIDR(testCIDR).
+			PoolRange(testStartIP, testEndIP).
+			NetworkName(testNetworkName).
+			Allocated(testIPAddress1, testMACAddress1).
+			Allocated(testIPAddress2, testMACAddress2).
+			Allocated(testIPAddress3, testMACAddress3).
+			CacheReadyCondition(corev1.ConditionTrue, "", "").Build()
+		givenCacheAllocator := newTestCacheAllocatorBuilder().
+			MACSet(testNetworkName).
+			Add(testNetworkName, testMACAddress1, testIPAddress1).
+			Add(testNetworkName, testMACAddress2, testIPAddress2).
+			Add(testNetworkName, testMACAddress3, testIPAddress3).Build()
+		givenIPAllocator := newTestIPAllocatorBuilder().
+			IPSubnet(testNetworkName, testCIDR, testStartIP, testEndIP).
+			Allocate(testNetworkName, testIPAddress1, testIPAddress2, testIPAddress3).Build()
+		givenNAD := newTestNetworkAttachmentDefinitionBuilder().
+			Label(util.IPPoolNamespaceLabelKey, testIPPoolNamespace).
+			Label(util.IPPoolNameLabelKey, testIPPoolName).Build()
+
+		expectedStatus := newTestVmNetCfgStatusBuilder().
+			WithNetworkConfigStatus(testIPAddress1, testMACAddress1, testNetworkName, networkv1.AllocatedState).Build()
+		expectedIPPool := newTestIPPoolBuilder().
+			ServerIP(testServerIP).
+			CIDR(testCIDR).
+			PoolRange(testStartIP, testEndIP).
+			NetworkName(testNetworkName).
+			Allocated(testIPAddress1, testMACAddress1).
+			CacheReadyCondition(corev1.ConditionTrue, "", "").Build()
+		expectedCacheAllocator := newTestCacheAllocatorBuilder().
+			MACSet(testNetworkName).
+			Add(testNetworkName, testMACAddress1, testIPAddress1).Build()
+		expectedIPAllocator := newTestIPAllocatorBuilder().
+			IPSubnet(testNetworkName, testCIDR, testStartIP, testEndIP).
+			Allocate(testNetworkName, testIPAddress1).Build()
+
+		nadGVR := schema.GroupVersionResource{
+			Group:    "k8s.cni.cncf.io",
+			Version:  "v1",
+			Resource: "network-attachment-definitions",
+		}
+
+		clientset := fake.NewSimpleClientset()
+		err := clientset.Tracker().Create(nadGVR, givenNAD, givenNAD.Namespace)
+		assert.Nil(t, err, "mock resource should add into fake controller tracker")
+
+		err = clientset.Tracker().Add(givenVmNetCfg)
+		if err != nil {
+			t.Fatal(err)
+		}
+		err = clientset.Tracker().Add(givenIPPool)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		handler := Handler{
+			cacheAllocator:   givenCacheAllocator,
+			ipAllocator:      givenIPAllocator,
+			metricsAllocator: metrics.New(),
+			ippoolClient:     fakeclient.IPPoolClient(clientset.NetworkV1alpha1().IPPools),
+			ippoolCache:      fakeclient.IPPoolCache(clientset.NetworkV1alpha1().IPPools),
+			nadCache:         fakeclient.NetworkAttachmentDefinitionCache(clientset.K8sCniCncfIoV1().NetworkAttachmentDefinitions),
+		}
+
+		status, err := handler.Sync(givenVmNetCfg, givenVmNetCfg.Status)
+		assert.Nil(t, err)
+
+		SanitizeStatus(&expectedStatus)
+		SanitizeStatus(&status)
+		assert.Equal(t, expectedStatus, status)
+
+		ipPool, err := handler.ippoolClient.Get(testIPPoolNamespace, testIPPoolName, metav1.GetOptions{})
+		assert.Nil(t, err)
+
+		ippool.SanitizeStatus(&expectedIPPool.Status)
+		ippool.SanitizeStatus(&ipPool.Status)
+		assert.Equal(t, expectedIPPool, ipPool)
+
+		assert.Equal(t, expectedIPAllocator, handler.ipAllocator)
+		assert.Equal(t, expectedCacheAllocator, handler.cacheAllocator)
+	})
 }