Merge pull request kubevirt#9779 from EdDev/unplug-iface-from-vmi

vmi, network: Support interface hot-unplug
cnchef · Jun 1, 2023 · e291f5f · e291f5f
2 parents eff942f + 2099544
commit e291f5f
Show file tree

Hide file tree

Showing 54 changed files with 1,312 additions and 174 deletions.
diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json
@@ -11626,6 +11626,57 @@
      }
     ]
    },
+   "/apis/subresources.kubevirt.io/v1/namespaces/{namespace:[a-z0-9][a-z0-9\\-]*}/virtualmachineinstances/{name:[a-z0-9][a-z0-9\\-]*}/removeinterface": {
+    "put": {
+     "description": "Remove a network interface from a running Virtual Machine Instance",
+     "operationId": "v1vmi-removeinterface",
+     "parameters": [
+      {
+       "name": "body",
+       "in": "body",
+       "required": true,
+       "schema": {
+        "$ref": "#/definitions/v1.RemoveInterfaceOptions"
+       }
+      }
+     ],
+     "responses": {
+      "200": {
+       "description": "OK",
+       "schema": {
+        "type": "string"
+       }
+      },
+      "400": {
+       "description": "Bad Request",
+       "schema": {
+        "type": "string"
+       }
+      },
+      "401": {
+       "description": "Unauthorized"
+      }
+     }
+    },
+    "parameters": [
+     {
+      "uniqueItems": true,
+      "type": "string",
+      "description": "Name of the resource",
+      "name": "name",
+      "in": "path",
+      "required": true
+     },
+     {
+      "uniqueItems": true,
+      "type": "string",
+      "description": "Object name and auth scope, such as for teams and projects",
+      "name": "namespace",
+      "in": "path",
+      "required": true
+     }
+    ]
+   },
    "/apis/subresources.kubevirt.io/v1/namespaces/{namespace:[a-z0-9][a-z0-9\\-]*}/virtualmachineinstances/{name:[a-z0-9][a-z0-9\\-]*}/removevolume": {
     "put": {
      "description": "Removes a volume and disk from a running Virtual Machine Instance",
@@ -13132,6 +13183,57 @@
      }
     ]
    },
+   "/apis/subresources.kubevirt.io/v1alpha3/namespaces/{namespace:[a-z0-9][a-z0-9\\-]*}/virtualmachineinstances/{name:[a-z0-9][a-z0-9\\-]*}/removeinterface": {
+    "put": {
+     "description": "Remove a network interface from a running Virtual Machine Instance",
+     "operationId": "v1alpha3vmi-removeinterface",
+     "parameters": [
+      {
+       "name": "body",
+       "in": "body",
+       "required": true,
+       "schema": {
+        "$ref": "#/definitions/v1.RemoveInterfaceOptions"
+       }
+      }
+     ],
+     "responses": {
+      "200": {
+       "description": "OK",
+       "schema": {
+        "type": "string"
+       }
+      },
+      "400": {
+       "description": "Bad Request",
+       "schema": {
+        "type": "string"
+       }
+      },
+      "401": {
+       "description": "Unauthorized"
+      }
+     }
+    },
+    "parameters": [
+     {
+      "uniqueItems": true,
+      "type": "string",
+      "description": "Name of the resource",
+      "name": "name",
+      "in": "path",
+      "required": true
+     },
+     {
+      "uniqueItems": true,
+      "type": "string",
+      "description": "Object name and auth scope, such as for teams and projects",
+      "name": "namespace",
+      "in": "path",
+      "required": true
+     }
+    ]
+   },
    "/apis/subresources.kubevirt.io/v1alpha3/namespaces/{namespace:[a-z0-9][a-z0-9\\-]*}/virtualmachineinstances/{name:[a-z0-9][a-z0-9\\-]*}/removevolume": {
     "put": {
      "description": "Removes a volume and disk from a running Virtual Machine Instance",
@@ -17130,6 +17232,10 @@
      "sriov": {
       "$ref": "#/definitions/v1.InterfaceSRIOV"
      },
+     "state": {
+      "description": "State represents the requested operational state of the interface. The (only) value supported is `absent`, expressing a request to remove the interface.",
+      "type": "string"
+     },
      "tag": {
       "description": "If specified, the virtual network interface address and its tag will be provided to the guest via config drive",
       "type": "string"
@@ -18357,6 +18463,20 @@
      }
     }
    },
+   "v1.RemoveInterfaceOptions": {
+    "description": "RemoveInterfaceOptions is provided when dynamically hot unplugging a network interface",
+    "type": "object",
+    "required": [
+     "name"
+    ],
+    "properties": {
+     "name": {
+      "description": "Name indicates the logical name of the interface.",
+      "type": "string",
+      "default": ""
+     }
+    }
+   },
    "v1.RemoveVolumeOptions": {
     "description": "RemoveVolumeOptions is provided when dynamically hot unplugging volume and disk",
     "type": "object",
@@ -20000,6 +20120,10 @@
      "addInterfaceOptions": {
       "description": "AddInterfaceOptions when set indicates a network interface should be added. The details within this field specify how to add the interface",
       "$ref": "#/definitions/v1.AddInterfaceOptions"
+     },
+     "removeInterfaceOptions": {
+      "description": "RemoveInterfaceOptions when set indicates a network interface should be removed. The details within this field specify how to remove the interface",
+      "$ref": "#/definitions/v1.RemoveInterfaceOptions"
      }
     }
    },

diff --git a/manifests/generated/operator-csv.yaml.in b/manifests/generated/operator-csv.yaml.in
@@ -652,6 +652,7 @@ spec:
           - virtualmachineinstances/unfreeze
           - virtualmachineinstances/softreboot
           - virtualmachineinstances/addinterface
+          - virtualmachineinstances/removeinterface
           verbs:
           - update
         - apiGroups:

diff --git a/manifests/generated/rbac-operator.authorization.k8s.yaml.in b/manifests/generated/rbac-operator.authorization.k8s.yaml.in
@@ -580,6 +580,7 @@ rules:
   - virtualmachineinstances/unfreeze
   - virtualmachineinstances/softreboot
   - virtualmachineinstances/addinterface
+  - virtualmachineinstances/removeinterface
   verbs:
   - update
 - apiGroups:

diff --git a/pkg/network/namescheme/networknamescheme.go b/pkg/network/namescheme/networknamescheme.go
@@ -57,7 +57,7 @@ func CreateHashedNetworkNameScheme(vmiNetworks []v1.Network) map[string]string {
 
 func HashedPodInterfaceName(network v1.Network) string {
 	if vmispec.IsSecondaryMultusNetwork(network) {
-		return generateHashedInterfaceName(network.Name)
+		return GenerateHashedInterfaceName(network.Name)
 	}
 
 	return PrimaryPodInterfaceName
@@ -66,12 +66,12 @@ func HashedPodInterfaceName(network v1.Network) string {
 func mapMultusNonDefaultNetworksToPodInterfaceName(networks []v1.Network) map[string]string {
 	networkNameSchemeMap := map[string]string{}
 	for _, network := range vmispec.FilterMultusNonDefaultNetworks(networks) {
-		networkNameSchemeMap[network.Name] = generateHashedInterfaceName(network.Name)
+		networkNameSchemeMap[network.Name] = GenerateHashedInterfaceName(network.Name)
 	}
 	return networkNameSchemeMap
 }
 
-func generateHashedInterfaceName(networkName string) string {
+func GenerateHashedInterfaceName(networkName string) string {
 	hash := sha256.New()
 	_, _ = io.WriteString(hash, networkName)
 	hashedName := fmt.Sprintf("%x", hash.Sum(nil))[:maxIfaceNameLen]
@@ -135,17 +135,17 @@ func CreateNetworkNameSchemeByPodNetworkStatus(networks []v1.Network, networkSta
 // PodHasOrdinalInterfaceName check if the given pod network status has at least one pod interface with ordinal name
 func PodHasOrdinalInterfaceName(podNetworkStatus map[string]networkv1.NetworkStatus) bool {
 	for _, networkStatus := range podNetworkStatus {
-		if ordinalSecondaryInterfaceName(networkStatus.Interface) {
+		if OrdinalSecondaryInterfaceName(networkStatus.Interface) {
 			return true
 		}
 	}
 	return false
 }
 
-// ordinalSecondaryInterfaceName check if the given name is in form of the ordinal
+// OrdinalSecondaryInterfaceName check if the given name is in form of the ordinal
 // name scheme (e.g.: net1, net2..).
 // Primary iface name (eth0) is treated as non-ordinal interface name.
-func ordinalSecondaryInterfaceName(name string) bool {
+func OrdinalSecondaryInterfaceName(name string) bool {
 	const ordinalIfaceNameRegex = `^net\d+$`
 	match, err := regexp.MatchString(ordinalIfaceNameRegex, name)
 	if err != nil {

diff --git a/pkg/network/setup/configstate.go b/pkg/network/setup/configstate.go
@@ -43,12 +43,13 @@ func NewConfigState(configStateCache configStateCacheReaderWriter, ns NSExecutor
 }
 
 // Run passes through the state machine flow, executing the following steps:
+// - PreRun processes the nics and potentially updates and filters them (e.g. filter-out networks marked for removal).
 // - Discover the current pod network configuration status and persist some of it for future use.
 // - Configure the pod network.
 //
 // The discovery step can be executed repeatedly with no limitation.
 // The configuration step is allowed to run only once. Any attempt to run it again will cause a critical error.
-func (c ConfigState) Run(nics []podNIC, discoverFunc func(*podNIC) error, configFunc func(*podNIC) error) error {
+func (c ConfigState) Run(nics []podNIC, preRunFunc func([]podNIC) ([]podNIC, error), discoverFunc func(*podNIC) error, configFunc func(*podNIC) error) error {
 	var pendingNICs []podNIC
 	for _, nic := range nics {
 		state, err := c.cache.Read(nic.vmiSpecNetwork.Name)
@@ -72,6 +73,11 @@ func (c ConfigState) Run(nics []podNIC, discoverFunc func(*podNIC) error, config
 	}
 
 	err := c.ns.Do(func() error {
+		var preErr error
+		nics, preErr = preRunFunc(nics)
+		if preErr != nil {
+			return preErr
+		}
 		return c.plug(nics, discoverFunc, configFunc)
 	})
 	return err

diff --git a/pkg/network/setup/configstate_test.go b/pkg/network/setup/configstate_test.go
@@ -60,7 +60,7 @@ var _ = Describe("config state", func() {
 	It("runs with no current state (cache is empty)", func() {
 		discover, config := &funcStub{}, &funcStub{}
 
-		Expect(configState.Run(nics, discover.f, config.f)).To(Succeed())
+		Expect(configState.Run(nics, noopCSPreRun, discover.f, config.f)).To(Succeed())
 
 		Expect(discover.executedNetworks).To(Equal([]string{testNet0}), "the discover step should execute")
 		Expect(config.executedNetworks).To(Equal([]string{testNet0}), "the config step should execute")
@@ -74,7 +74,7 @@ var _ = Describe("config state", func() {
 		Expect(configStateCache.Write(testNet0, cache.PodIfaceNetworkPreparationPending)).To(Succeed())
 		discover, config := &funcStub{}, &funcStub{}
 
-		Expect(configState.Run(nics, discover.f, config.f)).To(Succeed())
+		Expect(configState.Run(nics, noopCSPreRun, discover.f, config.f)).To(Succeed())
 
 		Expect(discover.executedNetworks).To(Equal([]string{testNet0}), "the discover step should execute")
 		Expect(config.executedNetworks).To(Equal([]string{testNet0}), "the config step should execute")
@@ -89,7 +89,7 @@ var _ = Describe("config state", func() {
 		discover, config := &funcStub{}, &funcStub{}
 
 		ns.shouldNotBeExecuted = true
-		err := configState.Run(nics, discover.f, config.f)
+		err := configState.Run(nics, noopCSPreRun, discover.f, config.f)
 		Expect(err).To(HaveOccurred())
 		var criticalNetErr *neterrors.CriticalNetworkError
 		Expect(errors.As(err, &criticalNetErr)).To(BeTrue())
@@ -107,7 +107,7 @@ var _ = Describe("config state", func() {
 		discover, config := &funcStub{}, &funcStub{}
 
 		ns.shouldNotBeExecuted = true
-		Expect(configState.Run(nics, discover.f, config.f)).To(Succeed())
+		Expect(configState.Run(nics, noopCSPreRun, discover.f, config.f)).To(Succeed())
 
 		Expect(discover.executedNetworks).To(BeEmpty(), "the discover step should not be execute")
 		Expect(config.executedNetworks).To(BeEmpty(), "the config step should not be execute")
@@ -121,7 +121,7 @@ var _ = Describe("config state", func() {
 		injectedErr := fmt.Errorf("fail discovery")
 		discover, config := &funcStub{errRun: injectedErr}, &funcStub{}
 
-		Expect(configState.Run(nics, discover.f, config.f)).To(MatchError(injectedErr))
+		Expect(configState.Run(nics, noopCSPreRun, discover.f, config.f)).To(MatchError(injectedErr))
 
 		Expect(discover.executedNetworks).To(Equal([]string{testNet0}), "the discover step should execute")
 		Expect(config.executedNetworks).To(BeEmpty(), "the config step should not execute")
@@ -135,7 +135,7 @@ var _ = Describe("config state", func() {
 		injectedErr := fmt.Errorf("fail config")
 		discover, config := &funcStub{}, &funcStub{errRun: injectedErr}
 
-		Expect(configState.Run(nics, discover.f, config.f)).To(MatchError(injectedErr))
+		Expect(configState.Run(nics, noopCSPreRun, discover.f, config.f)).To(MatchError(injectedErr))
 
 		Expect(discover.executedNetworks).To(Equal([]string{testNet0}), "the discover step should execute")
 		Expect(config.executedNetworks).To(Equal([]string{testNet0}), "the config step should execute")
@@ -153,7 +153,7 @@ var _ = Describe("config state", func() {
 		discover, config := &funcStub{}, &funcStub{}
 
 		ns.shouldNotBeExecuted = true
-		Expect(configState.Run(nics, discover.f, config.f)).To(MatchError(injectedErr))
+		Expect(configState.Run(nics, noopCSPreRun, discover.f, config.f)).To(MatchError(injectedErr))
 
 		Expect(discover.executedNetworks).To(BeEmpty(), "the discover step shouldn't execute")
 		Expect(config.executedNetworks).To(BeEmpty(), "the config step shouldn't execute")
@@ -166,7 +166,7 @@ var _ = Describe("config state", func() {
 
 		discover, config := &funcStub{}, &funcStub{}
 
-		Expect(configState.Run(nics, discover.f, config.f)).To(MatchError(injectedErr))
+		Expect(configState.Run(nics, noopCSPreRun, discover.f, config.f)).To(MatchError(injectedErr))
 
 		Expect(discover.executedNetworks).To(Equal([]string{testNet0}), "the discover step should execute")
 		Expect(config.executedNetworks).To(BeEmpty(), "the config step shouldn't execute")
@@ -183,7 +183,7 @@ var _ = Describe("config state", func() {
 		It("runs with no current state (cache is empty)", func() {
 			discover, config := &funcStub{}, &funcStub{}
 
-			Expect(configState.Run(nics, discover.f, config.f)).To(Succeed())
+			Expect(configState.Run(nics, noopCSPreRun, discover.f, config.f)).To(Succeed())
 
 			Expect(discover.executedNetworks).To(Equal([]string{testNet0, testNet1, testNet2}))
 			Expect(config.executedNetworks).To(Equal([]string{testNet0, testNet1, testNet2}))
@@ -199,7 +199,7 @@ var _ = Describe("config state", func() {
 			injectedErr := fmt.Errorf("fail config")
 			discover, config := &funcStub{}, &funcStub{errRun: injectedErr, errRunForPodIfaceName: testNet1}
 
-			Expect(configState.Run(nics, discover.f, config.f)).To(MatchError(injectedErr))
+			Expect(configState.Run(nics, noopCSPreRun, discover.f, config.f)).To(MatchError(injectedErr))
 
 			Expect(discover.executedNetworks).To(Equal([]string{testNet0, testNet1, testNet2}))
 			Expect(config.executedNetworks).To(Equal([]string{testNet0, testNet1}))
@@ -211,6 +211,16 @@ var _ = Describe("config state", func() {
 			}
 		})
 
+		It("runs with filter that removes all networks", func() {
+			discover, config := &funcStub{}, &funcStub{}
+			preRunFilterOutAll := func(_ []podNIC) ([]podNIC, error) {
+				return nil, nil
+			}
+			Expect(configState.Run(nics, preRunFilterOutAll, discover.f, config.f)).To(Succeed())
+
+			Expect(discover.executedNetworks).To(BeEmpty())
+			Expect(config.executedNetworks).To(BeEmpty())
+		})
 	})
 })
 
@@ -231,3 +241,7 @@ func (f *funcStub) f(nic *podNIC) error {
 	}
 	return err
 }
+
+func noopCSPreRun(nics []podNIC) ([]podNIC, error) {
+	return nics, nil
+}