Merge pull request kubevirt#9979 from EdDev/net-do-not-set-absent-on-…

…old-vm

virt-controller, vm: Avoid unplugging interfaces on older running VMs

pkg/controller/controller.go

@@ -390,17 +390,26 @@ func AttachmentPods(ownerPod *k8sv1.Pod, podInformer cache.SharedIndexInformer)
 func ApplyNetworkInterfaceRequestOnVMISpec(vmiSpec *v1.VirtualMachineInstanceSpec, request *v1.VirtualMachineInterfaceRequest) *v1.VirtualMachineInstanceSpec {
 	switch {
 	case request.AddInterfaceOptions != nil:
-		if iface := vmispec.LookupInterfaceByName(vmiSpec.Domain.Devices.Interfaces, request.AddInterfaceOptions.Name); iface == nil {
-			newNetwork, newIface := newNetworkInterface(request.AddInterfaceOptions.Name, request.AddInterfaceOptions.NetworkAttachmentDefinitionName)
-			vmiSpec.Networks = append(vmiSpec.Networks, newNetwork)
-			vmiSpec.Domain.Devices.Interfaces = append(vmiSpec.Domain.Devices.Interfaces, newIface)
-		}
+		vmiSpec = ApplyNetworkInterfaceAddRequest(vmiSpec, request.AddInterfaceOptions)
 	case request.RemoveInterfaceOptions != nil:
-		if iface := vmispec.LookupInterfaceByName(vmiSpec.Domain.Devices.Interfaces, request.RemoveInterfaceOptions.Name); iface != nil {
-			iface.State = v1.InterfaceStateAbsent
-		}
+		vmiSpec = ApplyNetworkInterfaceRemoveRequest(vmiSpec, request.RemoveInterfaceOptions)
 	}
+	return vmiSpec
+}
 
+func ApplyNetworkInterfaceAddRequest(vmiSpec *v1.VirtualMachineInstanceSpec, options *v1.AddInterfaceOptions) *v1.VirtualMachineInstanceSpec {
+	if iface := vmispec.LookupInterfaceByName(vmiSpec.Domain.Devices.Interfaces, options.Name); iface == nil {
+		newNetwork, newIface := newNetworkInterface(options.Name, options.NetworkAttachmentDefinitionName)
+		vmiSpec.Networks = append(vmiSpec.Networks, newNetwork)
+		vmiSpec.Domain.Devices.Interfaces = append(vmiSpec.Domain.Devices.Interfaces, newIface)
+	}
+	return vmiSpec
+}
+
+func ApplyNetworkInterfaceRemoveRequest(vmiSpec *v1.VirtualMachineInstanceSpec, options *v1.RemoveInterfaceOptions) *v1.VirtualMachineInstanceSpec {
+	if iface := vmispec.LookupInterfaceByName(vmiSpec.Domain.Devices.Interfaces, options.Name); iface != nil {
+		iface.State = v1.InterfaceStateAbsent
+	}
 	return vmiSpec
 }
 

pkg/virt-controller/watch/application.go

@@ -690,6 +690,7 @@ func (vca *VirtControllerApp) initVirtualMachines() {
 		vca.dataVolumeInformer,
 		vca.persistentVolumeClaimInformer,
 		vca.controllerRevisionInformer,
+		vca.kvPodInformer,
 		instancetypeMethods,
 		recorder,
 		vca.clientSet,

pkg/virt-controller/watch/application_test.go

@@ -143,6 +143,7 @@ var _ = Describe("Application", func() {
 			dataVolumeInformer,
 			pvcInformer,
 			crInformer,
+			podInformer,
 			instancetypeMethods,
 			recorder,
 			virtClient,

pkg/virt-controller/watch/network.go

@@ -19,28 +19,17 @@
 package watch
 
 import (
-	k8sv1 "k8s.io/api/core/v1"
-
 	v1 "kubevirt.io/api/core/v1"
 
-	"kubevirt.io/client-go/log"
-
-	"kubevirt.io/kubevirt/pkg/network/namescheme"
 	"kubevirt.io/kubevirt/pkg/network/vmispec"
-	"kubevirt.io/kubevirt/pkg/virt-controller/services"
 )
 
-func calculateDynamicInterfaces(vmi *v1.VirtualMachineInstance, pod *k8sv1.Pod) ([]v1.Interface, []v1.Network, bool) {
+func calculateDynamicInterfaces(vmi *v1.VirtualMachineInstance) ([]v1.Interface, []v1.Network, bool) {
 	vmiSpecIfaces := vmispec.FilterInterfacesSpec(vmi.Spec.Domain.Devices.Interfaces, func(iface v1.Interface) bool {
 		return iface.State != v1.InterfaceStateAbsent
 	})
 	ifacesToHotUnplugExist := len(vmi.Spec.Domain.Devices.Interfaces) > len(vmiSpecIfaces)
 
-	if ifacesToHotUnplugExist && namescheme.PodHasOrdinalInterfaceName(services.NonDefaultMultusNetworksIndexedByIfaceName(pod)) {
-		vmiSpecIfaces = vmi.Spec.Domain.Devices.Interfaces
-		ifacesToHotUnplugExist = false
-		log.Log.Object(vmi).Error("hot-unplug is not supported on old VMIs with ordered pod interface names")
-	}
 	vmiSpecNets := vmi.Spec.Networks
 	if ifacesToHotUnplugExist {
 		vmiSpecNets = vmispec.FilterNetworksByInterfaces(vmi.Spec.Networks, vmiSpecIfaces)

pkg/virt-controller/watch/network_test.go

@@ -45,7 +45,7 @@ var _ = Describe("Network interface hot{un}plug", func() {
 	DescribeTable("calculate if changes are required",
 
 		func(vmi *v1.VirtualMachineInstance, pod *k8sv1.Pod, expIfaces []v1.Interface, expNets []v1.Network, expToChange bool) {
-			ifaces, nets, exists := calculateDynamicInterfaces(vmi, pod)
+			ifaces, nets, exists := calculateDynamicInterfaces(vmi)
 			Expect(ifaces).To(Equal(expIfaces))
 			Expect(nets).To(Equal(expNets))
 			Expect(exists).To(Equal(expToChange))
@@ -105,46 +105,6 @@ var _ = Describe("Network interface hot{un}plug", func() {
 			[]v1.Network{{Name: testNetworkName1}},
 			expectToChange,
 		),
-		Entry("when a vmi interface has state set to `absent`, but pod iface name is ordinal",
-			libvmi.New(
-				libvmi.WithInterface(v1.Interface{Name: testNetworkName1}),
-				libvmi.WithInterface(v1.Interface{Name: testNetworkName2, State: v1.InterfaceStateAbsent}),
-				libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
-				libvmi.WithNetwork(&v1.Network{Name: testNetworkName2}),
-				withInterfaceStatus(v1.VirtualMachineInstanceNetworkInterface{Name: testNetworkName1}),
-				withInterfaceStatus(v1.VirtualMachineInstanceNetworkInterface{Name: testNetworkName2}),
-			),
-			&k8sv1.Pod{
-				ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
-					networkv1.NetworkStatusAnnot: `[
-						{"interface":"net1", "name":"red-net", "namespace": "default"},
-						{"interface":"net2", "name":"blue-net", "namespace": "default"}
-					]`,
-				}},
-			},
-			nil,
-			nil,
-			expectNoChange,
-		),
-		Entry("when vmi interfaces have an interface to hotplug and one to hot-unplug, given ordinal names",
-			libvmi.New(
-				libvmi.WithInterface(v1.Interface{Name: testNetworkName1, State: v1.InterfaceStateAbsent}),
-				libvmi.WithInterface(v1.Interface{Name: testNetworkName2}),
-				libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
-				libvmi.WithNetwork(&v1.Network{Name: testNetworkName2}),
-				withInterfaceStatus(v1.VirtualMachineInstanceNetworkInterface{Name: testNetworkName1}),
-			),
-			&k8sv1.Pod{
-				ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
-					networkv1.NetworkStatusAnnot: `[
-						{"interface":"net1", "name":"red-net", "namespace": "default"}
-					]`,
-				}},
-			},
-			[]v1.Interface{{Name: testNetworkName1, State: v1.InterfaceStateAbsent}, {Name: testNetworkName2}},
-			[]v1.Network{{Name: testNetworkName1}, {Name: testNetworkName2}},
-			expectToChange,
-		),
 		Entry("when vmi interfaces have an interface to hotplug and one to hot-unplug, given hashed names",
 			libvmi.New(
 				libvmi.WithInterface(v1.Interface{Name: testNetworkName1, State: v1.InterfaceStateAbsent}),

pkg/virt-controller/watch/vm.go

@@ -30,6 +30,9 @@ import (
 	"strings"
 	"time"
 
+	"kubevirt.io/kubevirt/pkg/network/namescheme"
+	"kubevirt.io/kubevirt/pkg/virt-controller/services"
+
 	"kubevirt.io/kubevirt/pkg/util/hardware"
 
 	"kubevirt.io/kubevirt/pkg/virt-api/webhooks"
@@ -117,6 +120,7 @@ func NewVMController(vmiInformer cache.SharedIndexInformer,
 	dataVolumeInformer cache.SharedIndexInformer,
 	pvcInformer cache.SharedIndexInformer,
 	crInformer cache.SharedIndexInformer,
+	podInformer cache.SharedIndexInformer,
 	instancetypeMethods instancetype.Methods,
 	recorder record.EventRecorder,
 	clientset kubecli.KubevirtClient,
@@ -131,6 +135,7 @@ func NewVMController(vmiInformer cache.SharedIndexInformer,
 		dataVolumeInformer:     dataVolumeInformer,
 		pvcInformer:            pvcInformer,
 		crInformer:             crInformer,
+		podInformer:            podInformer,
 		instancetypeMethods:    instancetypeMethods,
 		recorder:               recorder,
 		clientset:              clientset,
@@ -189,6 +194,7 @@ type VMController struct {
 	dataVolumeInformer     cache.SharedIndexInformer
 	pvcInformer            cache.SharedIndexInformer
 	crInformer             cache.SharedIndexInformer
+	podInformer            cache.SharedIndexInformer
 	instancetypeMethods    instancetype.Methods
 	recorder               record.EventRecorder
 	expectations           *controller.UIDTrackingControllerExpectations
@@ -204,7 +210,7 @@ func (c *VMController) Run(threadiness int, stopCh <-chan struct{}) {
 	log.Log.Info("Starting VirtualMachine controller.")
 
 	// Wait for cache sync before we start the controller
-	cache.WaitForCacheSync(stopCh, c.vmiInformer.HasSynced, c.vmInformer.HasSynced, c.dataVolumeInformer.HasSynced)
+	cache.WaitForCacheSync(stopCh, c.vmiInformer.HasSynced, c.vmInformer.HasSynced, c.dataVolumeInformer.HasSynced, c.podInformer.HasSynced)
 
 	// Start the actual work
 	for i := 0; i < threadiness; i++ {
@@ -2717,24 +2723,47 @@ func (c *VMController) handleDynamicInterfaceRequests(vm *virtv1.VirtualMachine,
 	for i := range vm.Status.InterfaceRequests {
 		vmTemplateCopy = controller.ApplyNetworkInterfaceRequestOnVMISpec(
 			vmTemplateCopy, &vm.Status.InterfaceRequests[i])
+	}
+	vm.Spec.Template.Spec = *vmTemplateCopy
 
-		if vmi == nil || vmi.DeletionTimestamp != nil {
-			continue
+	if vmi != nil && vmi.DeletionTimestamp == nil {
+		hasOrdinalIfaces, err := c.hasOrdinalNetworkInterfaces(vmi)
+		if err != nil {
+			return err
 		}
 
 		vmiSpecCopy := vmi.Spec.DeepCopy()
-		vmiSpecCopy = controller.ApplyNetworkInterfaceRequestOnVMISpec(
-			vmiSpecCopy, &vm.Status.InterfaceRequests[i])
-
+		for i := range vm.Status.InterfaceRequests {
+			request := &vm.Status.InterfaceRequests[i]
+			switch {
+			case request.AddInterfaceOptions != nil:
+				vmiSpecCopy = controller.ApplyNetworkInterfaceAddRequest(vmiSpecCopy, request.AddInterfaceOptions)
+			case !hasOrdinalIfaces && request.RemoveInterfaceOptions != nil:
+				vmiSpecCopy = controller.ApplyNetworkInterfaceRemoveRequest(vmiSpecCopy, request.RemoveInterfaceOptions)
+			}
+		}
 		if err := c.vmiInterfacesPatch(vmiSpecCopy, vmi); err != nil {
 			return err
 		}
 	}
 
-	vm.Spec.Template.Spec = *vmTemplateCopy
 	return nil
 }
 
+func (c *VMController) hasOrdinalNetworkInterfaces(vmi *virtv1.VirtualMachineInstance) (bool, error) {
+	pod, err := controller.CurrentVMIPod(vmi, c.podInformer)
+	if err != nil {
+		log.Log.Object(vmi).Reason(err).Error("Failed to fetch pod from cache.")
+		return false, err
+	}
+	if pod == nil {
+		log.Log.Object(vmi).Reason(err).Error("Failed to find VMI pod in cache.")
+		return false, err
+	}
+	hasOrdinalIfaces := namescheme.PodHasOrdinalInterfaceName(services.NonDefaultMultusNetworksIndexedByIfaceName(pod))
+	return hasOrdinalIfaces, nil
+}
+
 func (c *VMController) vmiInterfacesPatch(newVmiSpec *virtv1.VirtualMachineInstanceSpec, vmi *virtv1.VirtualMachineInstance) error {
 	if equality.Semantic.DeepEqual(vmi.Spec.Domain.Devices.Interfaces, newVmiSpec.Domain.Devices.Interfaces) {
 		return nil

pkg/virt-controller/watch/vm_test.go

@@ -68,6 +68,7 @@ var _ = Describe("VirtualMachine", func() {
 		var dataVolumeSource *framework.FakeControllerSource
 		var pvcInformer cache.SharedIndexInformer
 		var crInformer cache.SharedIndexInformer
+		var podInformer cache.SharedIndexInformer
 		var instancetypeMethods *testutils.MockInstancetypeMethods
 		var stop chan struct{}
 		var controller *VMController
@@ -119,6 +120,7 @@ var _ = Describe("VirtualMachine", func() {
 					return nil, nil
 				},
 			})
+			podInformer, _ = testutils.NewFakeInformerFor(&k8sv1.Pod{})
 
 			instancetypeMethods = testutils.NewMockInstancetypeMethods()
 
@@ -132,6 +134,7 @@ var _ = Describe("VirtualMachine", func() {
 				dataVolumeInformer,
 				pvcInformer,
 				crInformer,
+				podInformer,
 				instancetypeMethods,
 				recorder,
 				virtClient,

pkg/virt-controller/watch/vmi.go

@@ -1267,7 +1267,7 @@ func (c *VMIController) sync(vmi *virtv1.VirtualMachineInstance, pod *k8sv1.Pod,
 			}
 		}
 
-		if vmiSpecIfaces, vmiSpecNets, dynamicIfacesExist := calculateDynamicInterfaces(vmi, pod); dynamicIfacesExist {
+		if vmiSpecIfaces, vmiSpecNets, dynamicIfacesExist := calculateDynamicInterfaces(vmi); dynamicIfacesExist {
 			if err := c.handleDynamicInterfaceRequests(vmi.Namespace, vmiSpecIfaces, vmiSpecNets, pod); err != nil {
 				return &syncErrorImpl{
 					err:    fmt.Errorf("failed to hot{un}plug network interfaces for vmi [%s/%s]: %w", vmi.GetNamespace(), vmi.GetName(), err),