Add DeviceInfo Data to NetworkStatus Annotation #577

Billy99 · 2020-11-04T14:10:45Z

Update the NetworkStatus with DeviceInfo per the approved device-info-spec, which can be found:
https://github.com/k8snetworkplumbingwg/device-info-spec

This allows DevicePlugins and CNIs to add additional data to the NetworkStatus annotation, which can be used by a container to add in deployment of the container workload.

This PR is based on npwg_poc2 by Adrian Moreno

Billy99 · 2020-11-04T14:12:28Z

/hold

This PR requires k8snetworkplumbingwg/network-attachment-definition-client#29 to be merged first. Once that is merged, go.mod will be updated to point to those changes,

Billy99 · 2020-11-04T16:08:45Z

multus/multus.go

@@ -558,9 +585,16 @@ func cmdAdd(args *skel.CmdArgs, exec invoke.Exec, kubeClient *k8s.ClientInfo) (c
 	cniArgs := os.Getenv("CNI_ARGS")
 	for idx, delegate := range n.Delegates {
 		ifName := getIfname(delegate, args.IfName, idx)
+		rt, cniDeviceInfoPath := types.CreateCNIRuntimeConf(args, k8sArgs, ifName, n.RuntimeConfig, delegate)
+		if cniDeviceInfoPath != "" {
+			err = nadutils.CopyDeviceInfoForCNIFromDP(cniDeviceInfoPath, delegate.ResourceName, delegate.DeviceID)


@amorenoz There are several places in the PR where I ignore the errors coming back from utility calls for the DP and CNI files. Is there anyway to know for sure if a file should exist and take extra action? Or leave as is and handle file if it exists and move on otherwise.

The thing is, everything is optional for now right?
From Multul's point of view:

If DP file does not exist, we may want to log, but that 's not an error

If CNI does not create a file, we may want to log, but is not an error either.

If DP does exist, we do the copy and then it is magically removed after the delegation-chain call (i.e: when we write the NetworkStatus Annotation), then yes, that's an error.

dougbtv · 2020-11-04T18:58:46Z

types/conf.go

-func MergeCNIRuntimeConfig(runtimeConfig *RuntimeConfig, delegate *DelegateNetConf) *RuntimeConfig {
-	logging.Debugf("MergeCNIRuntimeConfig: %v %v", runtimeConfig, delegate)
+// mergeCNIRuntimeConfig creates CNI runtimeconfig from delegate
+func mergeCNIRuntimeConfig(runtimeConfig *RuntimeConfig, delegate *DelegateNetConf) *RuntimeConfig {


Just making sure I understand this, but, you're making this a private function in favor of the types.CreateCNIRuntimeConf where it's called from multus.go?

Yes making private. All cases in multus.go that mergeCNIRuntimeConfig was called, it was followed by a call to CreateCNIRuntimeConf. So mergeCNIRuntimeConfig is now called from CreateCNIRuntimeConf and no longer called from multus.go.

coveralls · 2020-11-17T19:51:00Z

Pull Request Test Coverage Report for Build 373163174

70 of 96 (72.92%) changed or added relevant lines in 3 files are covered.
6 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.4%) to 70.544%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
multus/multus.go	40	45	88.89%
types/conf.go	27	48	56.25%

Files with Coverage Reduction	New Missed Lines	%
multus/multus.go	1	69.16%
types/conf.go	5	73.74%

Totals
Change from base Build 372597014:	-0.4%
Covered Lines:	1116
Relevant Lines:	1582

💛 - Coveralls

multus/multus.go

amorenoz · 2020-11-18T13:23:16Z

types/conf.go

@@ -164,18 +168,26 @@ func mergeCNIRuntimeConfig(runtimeConfig *RuntimeConfig, delegate *DelegateNetCo
 		if delegate.DeviceID != "" {
 			runtimeConfig.DeviceID = delegate.DeviceID
 		}
+		logging.Debugf("mergeCNIRuntimeConfig: add runtimeConfig for net-attach-def: %v", runtimeConfig)
 	}

 	return runtimeConfig
 }

 // CreateCNIRuntimeConf create CNI RuntimeConf for a delegate


This function has changed, maybe update the comment?

Tweaked the comment. Let me know if you think there should be more.

amorenoz · 2020-11-18T13:32:25Z

types/types.go

@@ -111,6 +112,8 @@ type DelegateNetConf struct {
 	ConfListPlugin bool `json:"-"`
 	// DeviceID is only used internal housekeeping
 	DeviceID string `json:"deviceID,omitempty"`
+	// ResourceName is only used internal housekeeping
+	ResourceName string `json:"resourceName,omitempty"`



Why do we need this for?
It may be there from my old poc code in which I used the ResourceName to derive the CNIDeviceInfoFile.
But now we're using the RuntimeConfig to store the entire CNIDeviceInfoFile so this might not be needed

It looks like Billy needs to pass this between two places and he's stashed it here in the DelegateNetConf.

I'm personally OK with this. However, if you have a recommendation for a way to extract it otherwise, I'm definitely all ears.

ResourceName is used in multus.go in cmdAdd() to call nadutils.CopyDeviceInfoForCNIFromDP(), used to determine the DP file name. There may be a better way to get the data from k8sclient.go in getKubernetesDelegate() where it is pulled from the annotation from NetworkAttachmentDefinition, but I haven't found it.

Billy99 · 2020-11-18T15:16:21Z

/hold cancel

k8sclient/k8sclient.go

Move Multus to use latest network-attachment-definition-client repo which contains the device-info-spec changes. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

RuntimeConfig depends on the delegate configuration. The Netconf runtime information should be merged with the delegate config and the result added to each command that is sent to the delegates. That was being done for all commands except for delPlugins. Do not export MergeCNIRuntimeConfig and call it from CreateCNIRuntimeConf that now accepts a delegate ptr. Call it from delPlugins for each delegate Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

DPDeviceFile is used by Device Plugins to pass device data to CNIs, as defined in the device-info-spec. The name of the DPDeviceFile is defined by the device-info-spec as: <ResourceName>-<DeviceID>-device.json If the DPDeviceFile exists, the NPWG implementation makes a copy of the file and passes the name of the file to the delegate CNI via capabilityArgs as CNIDeviceFile. If the DPDeviceFile does not exist, the filename is still passed to the CNI. The CNI can create the file and popluate it if a device is created within the CNI. The name of the CNIDeviceFile is not defined by device-info-spec, but to ensure the name does not clash it is formed by the following unique triplet: [networkName, PodUUID, ifName] k8snetworkplumbingwg/network-attachment-definition-client repo has utility functions to abstract some of this functionality so it can be reused across Device Plugins, NPWG implementations and CNIs. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

NetworkStatus was moved to network-attachment-definition-client in previous merges and the Multus version of the structure was left behind, and not being used. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

zshi-redhat · 2020-11-23T06:44:53Z

types/conf.go

@@ -202,6 +212,9 @@ func CreateCNIRuntimeConf(args *skel.CmdArgs, k8sArgs *K8sArgs, ifName string, r
 		if len(delegateRc.InfinibandGUID) != 0 {
 			capabilityArgs["infinibandGUID"] = delegateRc.InfinibandGUID
 		}
+		if delegateRc.DeviceID != "" {


Does this mean we need to add DeviceID capability explicitly when creating net-attach-def?

It's not required. You would only need it if the CNI supports deviceIDs not coming from DevicePlugin, e.g: host-device

zshi-redhat · 2020-11-23T06:47:49Z

types/conf.go

@@ -122,6 +122,13 @@ func LoadDelegateNetConf(bytes []byte, net *NetworkSelectionElement, deviceID st
 		if net.InfinibandGUIDRequest != "" {
 			delegateConf.InfinibandGUIDRequest = net.InfinibandGUIDRequest
 		}
+		if net.DeviceID != "" {
+			if deviceID != "" {
+				logging.Debugf("Warning: Both RuntimeConfig and ResourceMap provide deviceID. Ignoring RuntimeConfig")


Do you see RuntimeConfig.DeviceID is to replace ResourceMap.DeviceID as default in the future?

@zshi-redhat AFAIK, I don't think so. RuntimeConfig.DeviceID is useful for cases in which you don't have Device Plugin (hence, no ResourceMap.DeviceID)

dougbtv

LGTM! Thanks Billy -- I realize this was a long road. Thanks for all the attention between both the spec, as well as this PR, and all the due dilligence.

s1061123 · 2020-11-30T17:26:49Z

types/conf.go

@@ -55,7 +56,7 @@ func LoadDelegateNetConfList(bytes []byte, delegateConf *DelegateNetConf) error
 }

 // LoadDelegateNetConf converts raw CNI JSON into a DelegateNetConf structure
-func LoadDelegateNetConf(bytes []byte, net *NetworkSelectionElement, deviceID string) (*DelegateNetConf, error) {
+func LoadDelegateNetConf(bytes []byte, net *NetworkSelectionElement, deviceID string, resourceName string) (*DelegateNetConf, error) {


Is there any testcase that contains LoadDelegateNetConf() with resourceName? As far as I looked your code, all test cases uses LoadDelegateNetConf() with empty resource name.

Billy99 force-pushed the billy99-device-info branch from 2f86cde to ae33f06 Compare November 4, 2020 16:01

Billy99 commented Nov 4, 2020

View reviewed changes

dougbtv reviewed Nov 4, 2020

View reviewed changes

Billy99 force-pushed the billy99-device-info branch 2 times, most recently from c58f060 to c86a4fc Compare November 17, 2020 19:47

amorenoz reviewed Nov 18, 2020

View reviewed changes

Billy99 changed the title ~~[WIP] Add DeviceInfo Data to NetworkStatus Annotation~~ Add DeviceInfo Data to NetworkStatus Annotation Nov 18, 2020

dougbtv reviewed Nov 19, 2020

View reviewed changes

k8sclient/k8sclient.go Show resolved Hide resolved

Billy99 added 2 commits November 19, 2020 16:06

Point to latest network-attachment-definition-client

e064679

Move Multus to use latest network-attachment-definition-client repo which contains the device-info-spec changes. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Update vendor for network-attachment-definition-client update

2cd91b6

Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Billy99 force-pushed the billy99-device-info branch from c86a4fc to 1127411 Compare November 19, 2020 21:11

Billy99 added 5 commits November 19, 2020 16:34

Add deviceID support in runtimeConfig

fd3f485

Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Add DeviceInfo Data to Network Status Annotation

f826461

Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Remove Multus version of NetworkStatus

ae79780

NetworkStatus was moved to network-attachment-definition-client in previous merges and the Multus version of the structure was left behind, and not being used. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Billy99 force-pushed the billy99-device-info branch from 1127411 to ae79780 Compare November 19, 2020 21:39

zshi-redhat reviewed Nov 23, 2020

View reviewed changes

dougbtv approved these changes Dec 1, 2020

View reviewed changes

dougbtv merged commit e924318 into k8snetworkplumbingwg:master Dec 1, 2020

s1061123 reviewed Dec 3, 2020

View reviewed changes

Billy99 deleted the billy99-device-info branch January 15, 2021 18:32

ormergi mentioned this pull request Apr 1, 2024

network binding plugin: DownwardAPI for device info kubevirt/community#281

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DeviceInfo Data to NetworkStatus Annotation #577

Add DeviceInfo Data to NetworkStatus Annotation #577

Billy99 commented Nov 4, 2020

Billy99 commented Nov 4, 2020

Billy99 Nov 4, 2020

amorenoz Nov 18, 2020

dougbtv Nov 4, 2020

Billy99 Nov 4, 2020

coveralls commented Nov 17, 2020 •

edited

Loading

amorenoz Nov 18, 2020

Billy99 Nov 19, 2020

amorenoz Nov 18, 2020

dougbtv Nov 19, 2020

Billy99 Nov 19, 2020

Billy99 commented Nov 18, 2020

zshi-redhat Nov 23, 2020

amorenoz Dec 1, 2020 •

edited

Loading

zshi-redhat Nov 23, 2020

amorenoz Dec 1, 2020

dougbtv left a comment

s1061123 Nov 30, 2020

Add DeviceInfo Data to NetworkStatus Annotation #577

Add DeviceInfo Data to NetworkStatus Annotation #577

Conversation

Billy99 commented Nov 4, 2020

Billy99 commented Nov 4, 2020

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

coveralls commented Nov 17, 2020 • edited Loading

Pull Request Test Coverage Report for Build 373163174

💛 - Coveralls

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

Billy99 commented Nov 18, 2020

Choose a reason for hiding this comment

amorenoz Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbtv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 17, 2020 •

edited

Loading

amorenoz Dec 1, 2020 •

edited

Loading