fix(vm): prevent over-creation of vmmac on additional interface updates (#2184)

Fix two issues in VM additional network handling:

1. Prevent over-creation of VirtualMachineMACAddress resources when adding additional interfaces.
2. Fix network interface ID assignment when some IDs are already reserved later in the spec list.

Signed-off-by: Dmitry Lopatin <dmitry.lopatin@flant.com>

Author: LopatinDmitr

images/virtualization-artifact/pkg/common/network/ids.go

@@ -39,12 +39,15 @@ func EnsureNetworkInterfaceIDs(networks []v1alpha2.NetworksSpec) bool {
 	changed := false
 	used := make(map[int]struct{}, len(networks))
 
+	for i := range networks {
+		if networks[i].ID != nil && *networks[i].ID > 0 {
+			used[*networks[i].ID] = struct{}{}
+		}
+	}
+
 	nextID := StartGenericID
 	for i := range networks {
 		if networks[i].ID != nil {
-			if *networks[i].ID > 0 {
-				used[*networks[i].ID] = struct{}{}
-			}
 			continue
 		}
 

images/virtualization-artifact/pkg/common/network/ids_test.go

@@ -61,6 +61,114 @@ var _ = Describe("Network IDs", func() {
 			Expect(*networks[2].ID).To(Equal(2))
 		})
 
+		It("should skip already reserved IDs even if they appear later in list", func() {
+			idMain := 1
+			idMax := 16383
+			idNet := 2
+			networks := []v1alpha2.NetworksSpec{
+				{Type: v1alpha2.NetworksTypeMain, ID: &idMain},
+				{Type: v1alpha2.NetworksTypeClusterNetwork, Name: "cnet-521"},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "net-501", ID: &idMax},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "net-502", ID: &idNet},
+			}
+
+			changed := EnsureNetworkInterfaceIDs(networks)
+
+			Expect(changed).To(BeTrue())
+			Expect(*networks[0].ID).To(Equal(1))
+			Expect(*networks[1].ID).To(Equal(3))
+			Expect(*networks[2].ID).To(Equal(16383))
+			Expect(*networks[3].ID).To(Equal(2))
+		})
+
+		It("should assign minimal free IDs for multiple missing networks", func() {
+			idMain := 1
+			id4 := 4
+			id7 := 7
+			networks := []v1alpha2.NetworksSpec{
+				{Type: v1alpha2.NetworksTypeMain, ID: &idMain},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n-missing-1"},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n-4", ID: &id4},
+				{Type: v1alpha2.NetworksTypeClusterNetwork, Name: "cn-missing-1"},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n-7", ID: &id7},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n-missing-2"},
+			}
+
+			changed := EnsureNetworkInterfaceIDs(networks)
+
+			Expect(changed).To(BeTrue())
+			Expect(*networks[1].ID).To(Equal(2))
+			Expect(*networks[3].ID).To(Equal(3))
+			Expect(*networks[5].ID).To(Equal(5))
+		})
+
+		It("should assign generic IDs starting from 2 when there is no main", func() {
+			id4 := 4
+			networks := []v1alpha2.NetworksSpec{
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n1"},
+				{Type: v1alpha2.NetworksTypeClusterNetwork, Name: "cn1"},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n4", ID: &id4},
+			}
+
+			changed := EnsureNetworkInterfaceIDs(networks)
+
+			Expect(changed).To(BeTrue())
+			Expect(*networks[0].ID).To(Equal(2))
+			Expect(*networks[1].ID).To(Equal(3))
+			Expect(*networks[2].ID).To(Equal(4))
+		})
+
+		It("should stop assignment when generic ID space is exhausted", func() {
+			idMain := 1
+			networks := []v1alpha2.NetworksSpec{{Type: v1alpha2.NetworksTypeMain, ID: &idMain}}
+			for id := StartGenericID; id <= MaxID; id++ {
+				v := id
+				networks = append(networks, v1alpha2.NetworksSpec{Type: v1alpha2.NetworksTypeNetwork, ID: &v})
+			}
+			networks = append(networks, v1alpha2.NetworksSpec{Type: v1alpha2.NetworksTypeClusterNetwork, Name: "overflow"})
+
+			changed := EnsureNetworkInterfaceIDs(networks)
+
+			Expect(changed).To(BeFalse())
+			Expect(networks[len(networks)-1].ID).To(BeNil())
+		})
+
+		It("should keep duplicate id=1 when main has no ID but id=1 is already used", func() {
+			id1 := 1
+			networks := []v1alpha2.NetworksSpec{
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n-with-1", ID: &id1},
+				{Type: v1alpha2.NetworksTypeMain, Name: "main"},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n-missing"},
+			}
+
+			changed := EnsureNetworkInterfaceIDs(networks)
+
+			Expect(changed).To(BeTrue())
+			Expect(*networks[0].ID).To(Equal(1))
+			Expect(*networks[1].ID).To(Equal(1))
+			Expect(*networks[2].ID).To(Equal(2))
+		})
+
+		It("should preserve explicit invalid IDs and assign only missing ones", func() {
+			idZero := 0
+			idNegative := -5
+			idTooHigh := 20000
+			networks := []v1alpha2.NetworksSpec{
+				{Type: v1alpha2.NetworksTypeMain, Name: "main", ID: &idZero},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n-neg", ID: &idNegative},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "n-high", ID: &idTooHigh},
+				{Type: v1alpha2.NetworksTypeClusterNetwork, Name: "cn-missing"},
+			}
+
+			changed := EnsureNetworkInterfaceIDs(networks)
+
+			Expect(changed).To(BeTrue())
+			Expect(*networks[0].ID).To(Equal(0))
+			Expect(*networks[1].ID).To(Equal(-5))
+			Expect(*networks[2].ID).To(Equal(20000))
+			Expect(*networks[3].ID).To(Equal(2))
+		})
+
 		It("should return false when nothing to assign", func() {
 			idMain := 1
 			idNet := 2

images/virtualization-artifact/pkg/controller/vm/internal/mac.go

@@ -115,7 +115,10 @@ func (h *MACHandler) Handle(ctx context.Context, s state.VirtualMachineState) (r
 			}
 		}
 		if createdCount < expectedMACAddresses {
-			macsToCreate := countNetworksWithMACRequest(vm.Spec.Networks, vmmacs) - createdCount
+			macsToCreate := expectedMACAddresses - len(vmmacs) - createdCount
+			if macsToCreate < 0 {
+				macsToCreate = 0
+			}
 			for i := 0; i < macsToCreate; i++ {
 				err = h.macManager.CreateMACAddress(ctx, vm, h.client, "")
 				if err != nil {
@@ -177,25 +180,3 @@ func (h *MACHandler) Handle(ctx context.Context, s state.VirtualMachineState) (r
 func (h *MACHandler) Name() string {
 	return nameMACHandler
 }
-
-func countNetworksWithMACRequest(networkSpec []v1alpha2.NetworksSpec, vmmacs []*v1alpha2.VirtualMachineMACAddress) int {
-	existingMACNames := make(map[string]bool)
-	for _, vmmac := range vmmacs {
-		existingMACNames[vmmac.Name] = true
-	}
-
-	count := 0
-	for _, ns := range networkSpec {
-		if ns.Type == v1alpha2.NetworksTypeMain {
-			continue
-		}
-
-		if ns.VirtualMachineMACAddressName == "" {
-			count++
-		} else if !existingMACNames[ns.VirtualMachineMACAddressName] {
-			count++
-		}
-	}
-
-	return count
-}

images/virtualization-artifact/pkg/controller/vm/internal/mac_test.go

@@ -112,6 +112,22 @@ var _ = Describe("MACHandler", func() {
 	}
 
 	Describe("Condition presence and absence scenarios", func() {
+		It("should create only one additional VMMAC when one additional network is added", func() {
+			vm.Spec.Networks = []v1alpha2.NetworksSpec{
+				{Type: v1alpha2.NetworksTypeMain},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "test-network1"},
+				{Type: v1alpha2.NetworksTypeNetwork, Name: "test-network2"},
+			}
+
+			macAddress1 := newMACAddress("test-mac-address1", "aa:bb:cc:dd:ee:ff", "", "")
+			fakeClient, resource, vmState = setupEnvironment(vm, macAddress1)
+			reconcile()
+
+			vmmacList := &v1alpha2.VirtualMachineMACAddressList{}
+			err := fakeClient.List(ctx, vmmacList, client.InNamespace(namespace))
+			Expect(err).NotTo(HaveOccurred())
+			Expect(vmmacList.Items).To(HaveLen(2))
+		})
 		Describe("NetworkSpec is nil", func() {
 			It("Condition 'MACAddressReady' should have status 'True'", func() {
 				fakeClient, resource, vmState = setupEnvironment(vm)