L1VH CNI first version #2465

paulyufan2 · 2024-01-03T16:15:44Z

Reason for Change:

This implementation is to support swiftv2 L1VH CNI feature:
CNI receives goal state from CNS and create linux/windows pod on Windows VM/Node in swiftv2 L1VH path;
The container pod is using secondary interface with its mac address and NIC type(Delegated NIC or Accelnet) to be created;
We support up to 8 pods to be created by using different interfaces along with different hnsNetwork; CNI supports hnsNetwork creation(when pod is added to the VM) and deletion(when pod is removed from VM) to prevent leakage;

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

Signed-off-by: Paul Yu <129891899+paulyufan2@users.noreply.github.com>

jpayne3506 · 2024-01-22T18:13:10Z

go.mod

@@ -8,7 +8,7 @@ require (
 	github.com/Azure/azure-sdk-for-go/sdk/keyvault/azsecrets v0.12.0
 	github.com/Masterminds/semver v1.5.0
 	github.com/Microsoft/go-winio v0.6.1
-	github.com/Microsoft/hcsshim v0.11.4
+	github.com/Microsoft/hcsshim v0.12.0-rc.2


What caused this update?

we need to fetch this hcsshim version for:
hcnNetwork.Flags = hcn.DisableHostPort

Considering it is a release candidate update. Can you also run CNI Release Test on this PR? Just want to ensure there are not additional changes that are breaking.

we cannot merge with release candidate version.. have to get official version released

This is in rc status as they are making 0.12.X dependent on containerd 2.0, which has not been released yet.

tamilmani1989 · 2024-02-06T21:00:33Z

cni/network/invoker_cns.go

+			// Add secondary interface info from podIPInfo to ipamAddResult
+			info.hostSubnet = response.PodIPInfo[i].HostSecondaryIPInfo.Subnet
+			info.hostPrimaryIP = response.PodIPInfo[i].HostSecondaryIPInfo.PrimaryIP
+			info.hostGateway = response.PodIPInfo[i].HostSecondaryIPInfo.Gateway
+


wont this affect AKS swift 2.0? it was using this before:

hostSubnet: response.PodIPInfo[i].HostPrimaryIPInfo.Subnet, hostPrimaryIP: response.PodIPInfo[i].HostPrimaryIPInfo.PrimaryIP,

it gets secondary interface info from HostSecondaryIPInfo

tamilmani1989 · 2024-02-08T23:46:14Z

cni/network/network.go

@@ -45,6 +45,9 @@ const (
 	ipv6FullMask          = 128
 )

+// Secondary interface NIC types
+var secondaryInterfaceNICType = []string{cns.BackendNICNC, string(cns.DelegatedVMNIC)}


why typecasting for delegatedvmnic but not for backendnicnc

I think it's because they are defined in different types on CNS codes, I can update CNS code

tamilmani1989 · 2024-02-09T00:00:45Z

cni/network/network.go

+		cniResult := cniTypesCurr.Result{}
+		// Add secondaryInterface info to result if it exists; otherwise add default primary interface info to result
+		if hasSecondaryInterface(ipamAddResult) {
+			cniResult = *convertInterfaceInfoToCniResult(ipamAddResult.secondaryInterfacesInfo[0], args.IfName)


can it be cniResult = convertInterfaceInfoToCniResult(ipamAddResult.secondaryInterfacesInfo[0], args.IfName)

if required , define a new variable.. but lets not make code convoluted

tamilmani1989 · 2024-02-09T21:28:42Z

cni/network/invoker_cns.go

@@ -450,6 +455,13 @@ func configureSecondaryAddResult(info *IPResultInfo, addResult *IPAMAddResult, p
 		return errors.Wrap(err, "Invalid mac address")
 	}

+	_, hostIPNet, err := net.ParseCIDR(info.hostSubnet)


why this is required?

because we do need "hostSubnetPrefix" to find master interface if you check findMasterInterface()

we should check based on macAddress instead?

tamilmani1989 · 2024-02-09T21:29:47Z

cni/network/network.go

@@ -45,6 +45,9 @@ const (
 	ipv6FullMask          = 128
 )

+// Secondary interface NIC types
+var secondaryInterfaceNICType = []string{cns.BackendNICNC, string(cns.DelegatedVMNIC)}


we should have enum for nic types.. we should resuse from cns package

tamilmani1989 · 2024-02-09T21:31:51Z

cni/network/network.go

@@ -362,12 +386,18 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {
 		telemetry.SendCNIMetric(&cniMetric, plugin.tb)

 		// Add Interfaces to result.
-		defaultCniResult := convertInterfaceInfoToCniResult(ipamAddResult.defaultInterfaceInfo, args.IfName)
+		cniResult := cniTypesCurr.Result{}
+		// Add secondaryInterface info to result if it exists; otherwise add default primary interface info to result


can it be other way.. add secondary interface to result if default interface does not exist

in design, we have both default and secondary interface info in ipamAddResult;

tamilmani1989 · 2024-02-09T21:33:37Z

cni/network/network.go

@@ -460,6 +494,14 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {
 				zap.Any("results", ipamAddResult))
 			return plugin.Errorf(errMsg)
 		}
+	} else if !nwCfg.MultiTenancy && nwCfg.IPAM.Type == network.AzureCNS {


wanted to understand why we need this change

because for swiftv2 path, the nwconfig multitenancy must not be set to true which was confused before; and the nwcfg ipam type should be azureCNS

tamilmani1989 · 2024-02-12T21:29:00Z

cni/network/invoker_cns.go

@@ -450,6 +455,13 @@ func configureSecondaryAddResult(info *IPResultInfo, addResult *IPAMAddResult, p
 		return errors.Wrap(err, "Invalid mac address")
 	}

+	_, hostIPNet, err := net.ParseCIDR(info.hostSubnet)


we should check based on macAddress instead?

tamilmani1989 · 2024-02-12T21:32:57Z

cni/network/network.go

@@ -78,6 +81,14 @@ type NetPlugin struct {
 	multitenancyClient MultitenancyClient
 }

+// save secondary interface usage with ipNet and macAddress properties
+var SecondaryInterfaces map[string]SecondaryInterfaceProperty


what's key here? Add a comment on it

tamilmani1989 · 2024-02-12T21:37:13Z

cni/network/network.go

+func hasSecondaryInterface(ipamAddResult IPAMAddResult) bool {
+	if ipamAddResult.secondaryInterfacesInfo != nil {
+		for _, interfaceNICType := range secondaryInterfaceNICType {
+			if string(ipamAddResult.secondaryInterfacesInfo[0].NICType) == interfaceNICType {
+				return true
+			}
+		}
+	}
+	return false
+}


we should just check `len(secondaryInterfacesInfo)>0``

tamilmani1989 · 2024-02-12T21:46:29Z

cni/network/network.go

+	interfaceInfo := network.InterfaceInfo{}
+	// find ipnet from secondaryInterfaceInfo if it exists;
+	if hasSecondaryInterface(ipamAddResult) {
+		interfaceInfo = ipamAddResult.secondaryInterfacesInfo[0]


lets iterate instead of assuming index 0 will be expected nic info

tamilmani1989 · 2024-02-12T21:54:28Z

cni/network/network.go

+		macAddress = ipamAddResult.secondaryInterfacesInfo[0].MacAddress.String()
+	}
+
+	masterIfName := plugin.findMasterInterface(ipamAddConfig.nwCfg, &ipnet)


find master interface by macAddress if hostSubnetPrefix is not provided or interface couldn't find

tamilmani1989 · 2024-02-12T22:16:41Z

cni/network/network.go

+	// Get secondary interface info from CNI state file and save them to ipamAddResult in CNI Delete call
+	secondaryInterfaceInfo, err := plugin.nm.GetNetworkSecondaryInterfaceInfo(endpointID)
+	if err != nil {
+		logger.Error("Failed to get secondaryInterfaceInfo", zap.Error(err))


change level to info
log endpointID

tamilmani1989 · 2024-02-12T22:18:05Z

cni/network/network.go

+		logger.Error("Failed to get secondaryInterfaceInfo", zap.Error(err))
+	}
+
+	ipamAddResult.secondaryInterfacesInfo = []network.InterfaceInfo{}


if err == nil {
then set this
}

tamilmani1989 · 2024-02-12T22:21:20Z

cni/network/network.go

+	endpointID := plugin.nm.GetEndpointID(args.ContainerID, args.IfName)
+
+	// Get secondary interface info from CNI state file and save them to ipamAddResult in CNI Delete call
+	secondaryInterfaceInfo, err := plugin.nm.GetNetworkSecondaryInterfaceInfo(endpointID)


should not be specfic call for each interface, instead it should fetch all interface associated with that containerID

will address in stateless cni

tamilmani1989 · 2024-02-12T22:23:51Z

cni/network/network.go

@@ -1032,6 +1120,16 @@ func (plugin *NetPlugin) Delete(args *cniSkel.CmdArgs) error {
 			err = plugin.Errorf("Failed to extract network name from network config. error: %v", err)
 			return err
 		}
+
+		// cleanup interfaces usage map for swiftv2 L1VH and delete hnsNetwork associated with this networkID
+		if &ipamAddResult != nil && ipamAddResult.secondaryInterfacesInfo != nil {


change ipamAddResult to pointer otherwise

tamilmani1989 · 2024-02-12T22:25:46Z

cni/network/network.go

@@ -1049,7 +1147,6 @@ func (plugin *NetPlugin) Delete(args *cniSkel.CmdArgs) error {
 			}
 		}



revert this change

this line is on line 1082, just moved to the above

cni/api/api.go

paulyufan2 · 2024-02-13T22:06:46Z

cni/network/network.go

@@ -1049,7 +1147,6 @@ func (plugin *NetPlugin) Delete(args *cniSkel.CmdArgs) error {
 			}
 		}



this line is on line 1082, just moved to the above

tamilmani1989 · 2024-02-14T17:18:01Z

cni/network/network.go

+// save secondary interface property
+// key is secondary interface name and value is its macAddress
+var SecondaryInterfaces map[string]SecondaryInterfaceProperty
+
+type SecondaryInterfaceProperty struct {
+	macAddress string
+}


if its not required, lets remove

tamilmani1989 · 2024-02-14T17:23:09Z

cni/network/network.go

@@ -217,6 +226,14 @@ func (plugin *NetPlugin) findMasterInterface(nwCfg *cni.NetworkConfig, subnetPre
 	subnetPrefixString := subnetPrefix.String()
 	interfaces, _ := net.Interfaces()
 	for _, iface := range interfaces {
+		if subnetPrefixString == "" {


add a new function findMasterInterfaceByMac

how would be branch to this vs the default function used by other paths?

tamilmani1989 · 2024-02-14T17:24:23Z

cni/network/network.go

@@ -362,12 +387,17 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {
 		telemetry.SendCNIMetric(&cniMetric, plugin.tb)

 		// Add Interfaces to result.
-		defaultCniResult := convertInterfaceInfoToCniResult(ipamAddResult.defaultInterfaceInfo, args.IfName)
+		cniResult := &cniTypesCurr.Result{}
+		cniResult = convertInterfaceInfoToCniResult(ipamAddResult.defaultInterfaceInfo, args.IfName)


can you add UT for this?

tamilmani1989 · 2024-02-14T17:25:14Z

cni/network/network.go

+		cniResult := &cniTypesCurr.Result{}
+		cniResult = convertInterfaceInfoToCniResult(ipamAddResult.defaultInterfaceInfo, args.IfName)
+		// if defaultInterfaceInfo does not exist, then try to use secondaryInterfaceInfo
+		if cniResult == nil && hasSecondaryInterface(ipamAddResult) {


should append to result.Interface since its an array and not overwrite it

I checked with Kshitija and we only add one NC at a time and the secondary interface is always the first element of the secondaryInterfacesInfo

tamilmani1989 · 2024-02-14T17:27:04Z

cni/network/network.go


+	options := make(map[string]any)
+	ipamAddConfig := IPAMAddConfig{nwCfg: nwCfg, args: args, options: options}


if not required, lets remove it

github-actions · 2024-03-13T00:01:20Z

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

github-actions · 2024-03-21T00:01:08Z

Pull request closed due to inactivity.

paulyufan2 and others added 14 commits January 3, 2024 11:14

L1VH CNI

a50182b

fix issues

0677f69

fix issues

180715c

fix deletion

af02703

current code

1e7c325

fix cni deletion call

2ea66a2

fix issues

6692b24

fix issues

bfb63c9

fix issues

aae1984

fix hnsNetwork deletion issue

c005303

fix logs

f3c5b91

fix logs

36932fd

add windows UT

a27ecc4

Merge branch 'master' into CNIL1VHBranch

d2e6624

Signed-off-by: Paul Yu <129891899+paulyufan2@users.noreply.github.com>

paulyufan2 marked this pull request as ready for review January 19, 2024 22:02

paulyufan2 requested review from a team as code owners January 19, 2024 22:02

paulyufan2 requested review from jpayne3506 and jaer-tsun January 19, 2024 22:02

paulyufan2 added the cni Related to CNI. label Jan 19, 2024

fix test issues

62d0805

jpayne3506 reviewed Jan 22, 2024

View reviewed changes

add UTs

851592b

paulyufan2 requested a review from tamilmani1989 January 22, 2024 21:34

paulyufan2 and others added 5 commits January 22, 2024 16:56

add UTs

7d11d01

add UTs

fe01226

add UT

017fe33

Merge branch 'master' into CNIL1VHBranch

9e28b3b

fix UTs

12383d3

tamilmani1989 requested changes Feb 9, 2024

View reviewed changes

Merge branch 'master' into CNIL1VHBranch

eb0f53c

tamilmani1989 requested changes Feb 12, 2024

View reviewed changes

paulyufan2 commented Feb 13, 2024

View reviewed changes

fix comments

bf20cb7

tamilmani1989 requested changes Feb 14, 2024

View reviewed changes

paulyufan2 and others added 19 commits February 15, 2024 16:29

Merge branch 'master' into CNIL1VHBranch

b328e06

fix main comments

8c1ab69

Merge branch 'master' into CNIL1VHBranch

4d8a0d8

Merge branch 'master' into CNIL1VHBranch

c8ccc07

L1VH CNI

b078d8d

fix deletion

49748c3

fix cni deletion call

34b688a

fix hnsNetwork deletion issue

fcc3538

add comments

1bb97ef

fix comments

b3fe948

fix TM's comments

f90accd

fix the linter issue to assign a vairable

88157f2

fix issues'

fded358

fix the UT

7ea8c68

fix the linter issue

50826f7

fix an linter issue

96b2363

fix issues

0da0f39

fix a variable definition

94c3800

fix the cnsconfig changes

1fe4c7d

github-actions bot added the stale Stale due to inactivity. label Mar 13, 2024

github-actions bot closed this Mar 21, 2024

github-actions bot deleted the CNIL1VHBranch branch March 21, 2024 00:01

		@@ -1049,7 +1147,6 @@ func (plugin NetPlugin) Delete(args cniSkel.CmdArgs) error {
		}
		}


		options := make(map[string]any)
		ipamAddConfig := IPAMAddConfig{nwCfg: nwCfg, args: args, options: options}

L1VH CNI first version #2465

L1VH CNI first version #2465

Conversation

paulyufan2 commented Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpayne3506 Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 21, 2024

paulyufan2 commented Jan 3, 2024 •

edited

Loading

jpayne3506 Jan 23, 2024 •

edited

Loading