Support `k8s.v1.cni.cncf.io/networks` annotation removal or reset to blank to clean up Pod's secondary interfaces #7119

wenqiq · 2025-04-11T10:54:14Z

This PR introduces support for adding or deleting secondary network configurations for Pods by dynamically reconciling the k8s.v1.cni.cncf.io/networks annotation. Users can now add new secondary network configurations or remove existing ones during a Pod's lifecycle, and the changes will be applied without requiring Pod recreation.

Key Changes:

Annotation lifecycle support:
- Adding the k8s.v1.cni.cncf.io/networks annotation to a Pod triggers creation of all specified secondary network interfaces
- Deleting the annotation triggers removal of all associated secondary network interfaces
Unsupported operations:
- ❌ Modifying existing network entries in the annotation (e.g., adding/removing individual interfaces from the array)
- ❌ Changing interface parameters for existing network definitions

Implementation Details:

Controller Logic: Extended the existing network controller to handle annotation updates and invoke CNI plugins for interface lifecycle operations.
Cleanup Mechanism: Remove orphaned interfaces when annotations are deleted.

TestDone:

Added unit and e2e tests covering:
- Full k8s.v1.cni.cncf.io/networks annotation addition → interface creation
- Full k8s.v1.cni.cncf.io/networks annotation deletion → interface removal

Notes:

Pods must be in a running state for annotation changes to take effect
Interface deletions may cause brief connectivity interruptions
Interface-level modifications are not supported - only complete annotation addition/deletion

Resolves #7065

Depends on #7116

wenqiq · 2025-04-16T14:56:06Z

/test-sriov-secondary-network-e2e

antrea-bot · 2025-04-18T13:13:13Z

Can one of the admins verify this patch?

wenqiq · 2025-04-22T03:42:04Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-04-23T14:52:33Z

/test-sriov-secondary-network-e2e

luolanzone · 2025-04-25T01:47:37Z

@wenqiq please update the description about what's kind of update events have been handled in this PR.

luolanzone · 2025-05-14T09:52:29Z

pkg/agent/cniserver/secondary.go

+func rebindVFToHost(pciAddr string) error {
+	// echo 0000:00:04.0 > /sys/bus/pci/devices/0000:00:04.0/driver/unbind
+	unbindPath := filepath.Join("/sys/bus/pci/devices", pciAddr, "driver/unbind")
+	if err := os.WriteFile(unbindPath, []byte(pciAddr), 0200); err != nil {
+		return fmt.Errorf("unbind driver failed: %v", err)
+	}
+
+	bindPath := filepath.Join("/sys/bus/pci/drivers", vfDriver, "bind")
+	// echo 0000:00:04.0 > /sys/bus/pci/drivers/ixgbevf/bind
+	if err := os.WriteFile(bindPath, []byte(pciAddr), 0200); err != nil {
+		if recoverErr := recoverVFDriver(pciAddr); recoverErr != nil {
+			klog.Errorf("Critical: Failed to recover VF driver: %v", recoverErr)
+		}
+		return fmt.Errorf("bind to driver %s failed: %v", vfDriver, err)
+	}
+
+	klog.Infof("Successfully rebound %s to %s", pciAddr, vfDriver)
+	return nil
+}
+
+func recoverVFDriver(pciAddr string) error {
+	bindPath := filepath.Join("/sys/bus/pci/drivers/vfio-pci/bind")
+	return os.WriteFile(bindPath, []byte(pciAddr), 0200)
+}


I believe rebound is done by sriov-network-device-plugin, why do we need this? can you explain in the comments?

@tnqn if the sriov devices rebound is managed by sriov-network-device-plugin, do we have a possible way to do the annotation update for sriov interfaces? I am not sure if there is a way to trigger cni cmdDel when the annotation is updated but no Pod deletion event?

The sriov-network-device-plugin should only take effect during Pod deletion. In this case, merely updating the Pod's annotations did not trigger the cmdDel operation for Pod cleanup. To remove the interface, we must manually unbind the VFDriver.

cc @tnqn @antoninbas @jianjuns Could you help take a look at this problem?

Without having a great understanding of the issue, this is my hot take:

we should only "undo" what we did ourselves when removing an interface. I believe both binding and unbind are out-of-scope of both Antrea and sriov-network-device-plugin. If we don't bind to vfio-pci in the first place, why should we be repsonsible for unbinding?

my understanding is that sriov-network-device-plugin is acting based on the resource requirements (resources field) in the Pod container spec. These are out-of-scope of Antrea. Even if the annotation is updated to "remove" an SR-IOV secondary network interface, the resource is still assigned to the Pod. I should also be able to undo my change to the annotation (add back the interface) sucessfully.

isn't ixgbevf specific to Intel devices?

More generally, I am not sure what problem we are trying to solve with this code.

Hi @wenqiq any code update regarding the annotation update issue? I believe we don't need to do any specific SR-IOV device operation. As @antoninbas suggested, we should "undo" what we did.

@antoninbas do you think we should support deletion only (when an annotation is deleted, delete the corresponding interface and release the IP)? I am thinking NAD annotation contains lots of different fields including NAD name, interface name, mac address etc. I feel it would be hard to do a comprehensive support.

@luolanzone I commented about this earlier in #7065 (comment)

I don't think it is realistic for now to handle updates to the spec.config of NetworkAttachmentDefinition.

Edit: I see now that your question is specifically about the Pod annotation. It would be good if we could support dynamically adding / removing Pod interfaces by updating the annotation. However, we can prevent mutation of a given interface config (users should delete then recreate). Let me know if this is a good middle ground.

pkg/agent/secondarynetwork/podwatch/controller.go

luolanzone · 2025-05-29T09:10:12Z

pkg/agent/secondarynetwork/podwatch/controller.go

+	for _, inf := range storedInterfaces {
+		exists := false
+		for _, network := range networkList {
+			if inf.IFDev == network.InterfaceRequest {


I don't think this is correct way to check stale interfaces. Considering a case that the interface name might not be changed when the other fields are updated in one annotation, e.g: NAD name.

I would consider @antoninbas 's suggestion to disallow fields update #7119 (comment), but only supporting deletion and add back.
cc @tnqn

The problem is InterfaceRequest is optional. If it is not set for an existing interface, then the interface will always be deleted, even there is no change of interface configuration.

Guess we need to be smarter to correlate a network in the annotation to an existing interface; when InterfaceRequest is not present, we need to check the network name and even configuration?

pkg/agent/secondarynetwork/podwatch/controller.go

jianjuns · 2025-05-29T22:35:07Z

pkg/agent/secondarynetwork/podwatch/controller.go

+	for _, inf := range storedInterfaces {
+		exists := false
+		for _, network := range networkList {
+			if inf.IFDev == network.InterfaceRequest {


The problem is InterfaceRequest is optional. If it is not set for an existing interface, then the interface will always be deleted, even there is no change of interface configuration.

Guess we need to be smarter to correlate a network in the annotation to an existing interface; when InterfaceRequest is not present, we need to check the network name and even configuration?

luolanzone · 2025-06-05T09:53:07Z

@wenqiq please re-base to the latest codes in #7116

wenqiq · 2025-06-10T16:32:38Z

/test-sriov-secondary-network-e2e

luolanzone · 2025-07-04T08:16:09Z

@wenqiq please resolve code conflicts.

luolanzone

LGTM overall, @jianjuns @tnqn could you take a look?

wenqiq · 2025-07-04T17:01:48Z

/test-sriov-secondary-network-e2e

luolanzone · 2025-07-07T06:45:51Z

@wenqiq please update the title to ensure it's precise.
Probably "Support k8s.v1.cni.cncf.io/networks annotation removal or reset to blank to clean up Pod's secondary interfaces"

pkg/agent/secondarynetwork/podwatch/controller.go

jianjuns · 2025-07-07T19:24:08Z

pkg/agent/secondarynetwork/podwatch/controller.go

+// So when networkList is empty, it indicates that existing interfaces should be deleted
+// Otherwise, it means a network is being added from an empty annotation, and no deletion is needed
+// TODO: We can enhance this function to support more complex annotation operations.
+func (pc *PodController) filterStaleInterfaces(networkList []*netdefv1.NetworkSelectionElement, storedInterfaces []*interfacestore.InterfaceConfig) (staleInfs []*interfacestore.InterfaceConfig) {


Should be removed?

Could it cause any issues in handling it this way?

After reading the code again, I may get what you intended to do (still not very sure). But do you think better to go a more straightforward way, e.g.:

if len(storedSecondaryInterfaces) > 0 { if len(networkList) > 0 { // We do not support secondary network update at the moment. Return as long as one // secondary interface has been created for the Pod. klog.V(1).InfoS("Secondary network already configured on this Pod. Changes to secondary network configuration are not supported, skipping update", "Pod", klog.KObj(pod)) return nil } if err := pc.removeInterfaces(pc.filterStaleInterfaces(networkList, storedSecondaryInterfaces)); err != nil { return err } } var netStatus []netdefv1.NetworkStatus if len(networkList) > 0 { var err error netStatus, err = pc.configurePodSecondaryNetwork(pod, networkList, podCNIInfo) if err != nil { return err } } return pc.updatePodNetworkStatusAnnotation(netStatus, pod)

jianjuns · 2025-07-07T19:25:10Z

pkg/agent/secondarynetwork/podwatch/controller.go

 	}

-	return pc.configurePodSecondaryNetwork(pod, networklist, podCNIInfo)
+	return pc.configurePodSecondaryNetwork(pod, networkList, podCNIInfo)


I feel better to move the annotation update code to handleAddUpdatePod, so it is clearer when we need to update annotation.

pkg/agent/secondarynetwork/podwatch/controller.go

jianjuns · 2025-07-08T18:42:55Z

pkg/agent/secondarynetwork/podwatch/controller.go

+	}
+
+	// Update the Pod's network status annotation
+	if err := netdefutils.SetNetworkStatus(pc.kubeClient, pod, netStatus); err != nil {


When no secondary network is configured, should we remove the networkStatus annotation?

jianjuns · 2025-07-08T18:56:33Z

pkg/agent/secondarynetwork/podwatch/controller.go

+// So when networkList is empty, it indicates that existing interfaces should be deleted
+// Otherwise, it means a network is being added from an empty annotation, and no deletion is needed
+// TODO: We can enhance this function to support more complex annotation operations.
+func (pc *PodController) filterStaleInterfaces(networkList []*netdefv1.NetworkSelectionElement, storedInterfaces []*interfacestore.InterfaceConfig) (staleInfs []*interfacestore.InterfaceConfig) {


After reading the code again, I may get what you intended to do (still not very sure). But do you think better to go a more straightforward way, e.g.:

if len(storedSecondaryInterfaces) > 0 { if len(networkList) > 0 { // We do not support secondary network update at the moment. Return as long as one // secondary interface has been created for the Pod. klog.V(1).InfoS("Secondary network already configured on this Pod. Changes to secondary network configuration are not supported, skipping update", "Pod", klog.KObj(pod)) return nil } if err := pc.removeInterfaces(pc.filterStaleInterfaces(networkList, storedSecondaryInterfaces)); err != nil { return err } } var netStatus []netdefv1.NetworkStatus if len(networkList) > 0 { var err error netStatus, err = pc.configurePodSecondaryNetwork(pod, networkList, podCNIInfo) if err != nil { return err } } return pc.updatePodNetworkStatusAnnotation(netStatus, pod)

jianjuns · 2025-07-08T18:58:57Z

pkg/agent/secondarynetwork/podwatch/controller.go

 	}

-	return pc.configurePodSecondaryNetwork(pod, networklist, podCNIInfo)
+	return pc.updatePodNetworkStatusAnnotation(netStatus, pod)


Returning an error seems not help. Probably just ignore it. Maybe good to add comments to explain why.

pkg/agent/secondarynetwork/podwatch/controller.go

Signed-off-by: Wenqi Qiu <[email protected]>

wenqiq · 2025-07-09T03:31:33Z

/test-sriov-secondary-network-e2e

jianjuns

LGTM. Nits.

jianjuns · 2025-07-09T04:32:13Z

pkg/agent/secondarynetwork/podwatch/controller.go

+	}
+
+	var netStatus []netdefv1.NetworkStatus
+	var err error


Nit: err can be moved into the if len(networkList) > 0 { block.

pkg/agent/secondarynetwork/podwatch/controller.go

jianjuns · 2025-07-09T04:36:20Z

pkg/agent/secondarynetwork/podwatch/controller.go

+}
+
+var (
+	netdefutilsSetNetworkStatus = netdefutils.SetNetworkStatus


Probably just name the func variable: setNetworkStatus

Signed-off-by: Wenqi Qiu <[email protected]>

jianjuns · 2025-07-09T16:00:17Z

pkg/agent/secondarynetwork/podwatch/controller.go

+	if netStatus != nil {
+		// Intentionally ignore errors from updating the Pod's network status annotation here.
+		// Failure to update the annotation does not affect the actual network setup for the Pod.
+		// The annotation is mainly used for status reporting and is not critical for Pod networking functionality.


Actually, it is also used for restoring SR-IOV interface information after agent restarts.

Could you give a more accurate comment here？

"The annotation is mainly used for status reporting, as well as restoring SR-IOV interface information after agent restarts."?

Updated. I think error handling is omitted here primarily due to configurePodSecondaryNetwork not being idempotent? However, combined with the retry mechanism in the SetNetworkStatus, the likelihood of updatePodNetworkStatusAnnotation returning errors is low, and the impact of ignoring them is negligible.

Yes, we do not handle secondary network changes and do not check annotation should be updated either.
I think there can still be failures due to network failure, etc.

Signed-off-by: Wenqi Qiu <[email protected]>

jianjuns · 2025-07-09T19:18:22Z

/test-all

jianjuns · 2025-07-09T22:56:50Z

@luolanzone @wenqiq : next time please trim the commit description to be <= 72 chars, and also update the commit message before merging.

The commit was merged with the following commit message:

Support secondarynetwork update

It does not match the change.

wenqiq · 2025-07-09T23:36:00Z

@luolanzone @wenqiq : next time please trim the commit description to be <= 72 chars, and also update the commit message before merging.

Got it. Will pay attention to editing each commit message before pushing.

wenqiq force-pushed the support-secondarynetwork-update branch 2 times, most recently from 69d1de2 to 33e9c45 Compare April 16, 2025 10:56

wenqiq marked this pull request as ready for review April 17, 2025 15:36

wenqiq force-pushed the support-secondarynetwork-update branch from 8b49a8d to 4153418 Compare April 17, 2025 15:52

wenqiq changed the title ~~[WIP]Support secondarynetwork update~~ Support secondarynetwork update Apr 17, 2025

wenqiq force-pushed the support-secondarynetwork-update branch 2 times, most recently from f268415 to 91350bf Compare April 23, 2025 13:17

wenqiq force-pushed the support-secondarynetwork-update branch from 91350bf to 570bde4 Compare April 23, 2025 15:33

wenqiq requested review from antoninbas, tnqn, jianjuns and luolanzone April 24, 2025 00:52

luolanzone reviewed May 14, 2025

View reviewed changes

wenqiq force-pushed the support-secondarynetwork-update branch 3 times, most recently from 08c9e56 to bee7d62 Compare May 28, 2025 15:35

luolanzone added this to the Antrea v2.4 release milestone May 29, 2025

luolanzone reviewed May 29, 2025

View reviewed changes

jianjuns reviewed May 29, 2025

View reviewed changes

wenqiq force-pushed the support-secondarynetwork-update branch 4 times, most recently from a243a9b to cb0cb72 Compare June 10, 2025 07:51

wenqiq force-pushed the support-secondarynetwork-update branch 3 times, most recently from 5f84c5e to d92ca8a Compare July 4, 2025 13:43

luolanzone reviewed Jul 4, 2025

View reviewed changes

luolanzone mentioned this pull request Jul 7, 2025

Release 2.4.0 #7250

Merged

wenqiq changed the title ~~Support secondarynetwork update~~ Support k8s.v1.cni.cncf.io/networks annotation removal or reset to blank to clean up Pod's secondary interfaces Jul 7, 2025

jianjuns reviewed Jul 7, 2025

View reviewed changes

wenqiq force-pushed the support-secondarynetwork-update branch 2 times, most recently from f6f5d4f to ca0f56e Compare July 8, 2025 02:40

jianjuns reviewed Jul 8, 2025

View reviewed changes

pkg/agent/secondarynetwork/podwatch/controller.go Outdated Show resolved Hide resolved

wenqiq force-pushed the support-secondarynetwork-update branch 2 times, most recently from 177a2c5 to 1979ab6 Compare July 8, 2025 08:51

jianjuns reviewed Jul 8, 2025

View reviewed changes

wenqiq force-pushed the support-secondarynetwork-update branch from 1979ab6 to 4abf224 Compare July 9, 2025 01:52

Support secondarynetwork update

f201839

Signed-off-by: Wenqi Qiu <[email protected]>

wenqiq force-pushed the support-secondarynetwork-update branch from 4abf224 to f201839 Compare July 9, 2025 02:46

jianjuns previously approved these changes Jul 9, 2025

View reviewed changes

Address comments

35cc073

Signed-off-by: Wenqi Qiu <[email protected]>

wenqiq dismissed jianjuns’s stale review via 35cc073 July 9, 2025 04:59

jianjuns reviewed Jul 9, 2025

View reviewed changes

update

a05865b

Signed-off-by: Wenqi Qiu <[email protected]>

jianjuns approved these changes Jul 9, 2025

View reviewed changes

luolanzone merged commit 16367d5 into antrea-io:main Jul 9, 2025
95 of 103 checks passed

Support k8s.v1.cni.cncf.io/networks annotation removal or reset to blank to clean up Pod's secondary interfaces #7119

Support k8s.v1.cni.cncf.io/networks annotation removal or reset to blank to clean up Pod's secondary interfaces #7119

Uh oh!

Conversation

wenqiq commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Changes:

Implementation Details:

TestDone:

Uh oh!

wenqiq commented Apr 16, 2025

Uh oh!

antrea-bot commented Apr 18, 2025

Uh oh!

wenqiq commented Apr 22, 2025

Uh oh!

wenqiq commented Apr 23, 2025

Uh oh!

luolanzone commented Apr 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luolanzone May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoninbas May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luolanzone commented Jun 5, 2025

Uh oh!

wenqiq commented Jun 10, 2025

Uh oh!

luolanzone commented Jul 4, 2025

Uh oh!

luolanzone left a comment

Choose a reason for hiding this comment

Uh oh!

wenqiq commented Jul 4, 2025

Uh oh!

luolanzone commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Support `k8s.v1.cni.cncf.io/networks` annotation removal or reset to blank to clean up Pod's secondary interfaces #7119

Support `k8s.v1.cni.cncf.io/networks` annotation removal or reset to blank to clean up Pod's secondary interfaces #7119

wenqiq commented Apr 11, 2025 •

edited

Loading

luolanzone May 14, 2025 •

edited

Loading

antoninbas May 27, 2025 •

edited

Loading

luolanzone commented Jul 7, 2025 •

edited

Loading

jianjuns Jul 9, 2025 •

edited

Loading