Implement device-info-spec in app-netutil by Billy99 · Pull Request #24 · openshift/app-netutil

Billy99 · 2020-11-16T21:53:53Z

Add support to app-netutil to implement the device-info-spec. Primarily, it consists of pull additional data from the DeviceInfo field in the NetworkStatus.

As part of supporting the device-info-spec, migrate app-netutil to use network-attachment-definition-client data structs. app-netutil redefined some of the data structs from Multus and Userspace CNI. Since network-attachment-definition-client is now the location for these data structs, migrate to app-netutil to use these instead of defining it's own.

Billy99 · 2020-11-16T21:54:43Z

/hold

Need to get k8snetworkplumbingwg/network-attachment-definition-client#30 merged first.

Billy99 · 2020-11-18T15:22:38Z

/hold cancel

zshi-redhat

Billy, I added a few comments to see if I understand the logic correctly.

lib/v1alpha/network.go

zshi-redhat · 2020-11-19T01:41:11Z

lib/v1alpha/network.go

+				glog.Infof("%s is the \"default\" interface, mark as \"%s\"",
+					ifaceData.NetworkStatus.Interface, ifaceData.DeviceType)
+			} else if pciIndex < len(pciAddressSlice) {
+				if ifaceData.NetworkStatus.DeviceInfo == nil {


If it's a device set in PCIDEVICE_ env var, but DeviceInfo doesn't exist, mark it as SR-IOV type

Yes, because I think only SR-IOV is using that env var. This would be the case where a newer app-netutil is running with older SR-IOV DP and older Multus.

zshi-redhat · 2020-11-19T01:42:09Z

lib/v1alpha/network.go

+						ifaceData.NetworkStatus.Interface, ifaceData.DeviceType)
 				} else {
-					glog.Warningf("More SR-IOV interfaces detected than PCI addresses - %s", interfaceData.IfName)
+					glog.Warningf("%s was \"unknown\", but DeviceInfo exists with type \"%s\"",


If it's a device set in PCIDEVICE_ env var, but doesn't match with DeviceInfo PCI address, leave the device type as unknown.

This should not happen. It's really the case where the type is "unknown", but DeviceInfo struct was created. This is for debugging to catch invalid config or errors in the code where the NetworkStatus annotation was parsed.

zshi-redhat · 2020-11-19T01:44:25Z

lib/v1alpha/network.go

+						ifaceData.NetworkStatus.Interface, ifaceData.NetworkStatus.DeviceInfo.Type)
 				}
+			} else {
+				ifaceData.DeviceType = types.INTERFACE_TYPE_HOST


If device doesn't exist in PCIDEVICE_ && device doesn't have a deviceInfo, mark it as a HOST type device.

Yes. We don't know what it is. I guess we could leave as unknown and let the consumer of the API decide?

zshi-redhat · 2020-11-19T01:45:22Z

lib/v1alpha/network.go

+
+	// PCI Address found that did not match an existing interface in the
+	// NetworkStatus annotation so add to list.
+	if pciIndex < len(pciAddressSlice) {


If a device set in PCIDEVICE_ env var, but still has unknown type, mark it as SR-IOV type.

There are PCI Addresses in the PCIDEVICE_ env var but no associated NetworkStatus annotation. The only time I hit this was working a POC that wasn't using Multus.

zshi-redhat · 2020-11-19T01:56:24Z

lib/v1alpha/network.go

 	glog.Infof("RESPONSE:")
-	for _, interfaceData := range response.Interface {
-		glog.Infof("%v", interfaceData)
+	for _, ifaceData := range response.Interface {


I think there might still be devices that have empty type at this point, right? it would be the device that has a deviceInfo but DeviceInfo.Type is not configured.

Yes, what should I do with those? Invalid config.

nothing need to change, I just make sure understand it correctly.

zshi-redhat · 2020-11-19T01:57:19Z

btw, is there any device plugin patch required to make this work?

Billy99 · 2020-11-19T21:51:16Z

btw, is there any device plugin patch required to make this work?

k8snetworkplumbingwg/sriov-network-device-plugin#287

Move repository to point to the latest network-attachment-definition-client. This required an update to userspace-cni-network-plugin, so also move to latest version of that repository as well. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

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

The multusplugin pkg was used to pull in the NetworkStatus map from the multus-cni code base and parse the NetworkStatus annotation. Turns out that map was outdated and the map that was being pushed in annotations was from the network-attachment-definition-client library. So moved to pull from there and remove references to Multus. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Multus deprecated networks-status annotation in favor of network-status. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

To support SR-IOV Device Plugin, volume mount for directory DPDeviceInfoFile is created in needs to be added to the pod.spec for SR-IOV DP. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Add support to app-netutil to implement the device-info-spec. Primarily, it consists of pull additional data fron the DeviceInfo field in the NetworkStatus. As part of supporting the device-info-spec, migrate app-netutil to use network-attachment-definition-client data structs. app-netutil redefined some of the data structs from Multus and Userspace CNI. Since network-attachment-definition-client is now the location for these data structs, migrate to app-netutil to use these instead of defining it's own. Since app-netutil is not GA yet, the data structure returned by GetInterfaces() changed. Once this package is moved to GA, the API will need to be backwards compatible. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Billy99 · 2020-11-23T14:07:29Z

@zshi-redhat Sorry, had some comments "pending" that I thought I submitted early last week. Let me know if you have any more comments. I pushed a small change up last week to count the number of "unknown" interfaces and take that into consideration when applying the PCIDEVICE_ interfaces.

zshi-redhat · 2020-11-24T11:05:46Z

@zshi-redhat Sorry, had some comments "pending" that I thought I submitted early last week. Let me know if you have any more comments. I pushed a small change up last week to count the number of "unknown" interfaces and take that into consideration when applying the PCIDEVICE_ interfaces.

@Billy99, I don't have further comments, shall I merge it now or wait until DP/Multus PRs be merged?

Billy99 · 2020-11-24T13:19:09Z

@zshi-redhat Sorry, had some comments "pending" that I thought I submitted early last week. Let me know if you have any more comments. I pushed a small change up last week to count the number of "unknown" interfaces and take that into consideration when applying the PCIDEVICE_ interfaces.

@Billy99, I don't have further comments, shall I merge it now or wait until DP/Multus PRs be merged?

Yes. I have other changes coming for exposing hugepages via DownwardAPI. So let's get this in and build on it.

openshift-ci-robot requested review from fepan and pliurh November 16, 2020 21:54

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2020

Billy99 force-pushed the billy99-device-info-spec branch from 6360b08 to 6fd56fa Compare November 17, 2020 19:30

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 18, 2020

zshi-redhat reviewed Nov 19, 2020

View reviewed changes

Billy99 added 6 commits November 19, 2020 16:59

Update /vendor for move to network-attachment-definition-client

b012a48

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

Move to use network-status instead of networks-status

36f0b3c

Multus deprecated networks-status annotation in favor of network-status. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Update SR-IOV example to work with SR-IOV Device Plugin

20f12f2

To support SR-IOV Device Plugin, volume mount for directory DPDeviceInfoFile is created in needs to be added to the pod.spec for SR-IOV DP. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

Billy99 force-pushed the billy99-device-info-spec branch from 6fd56fa to 72333ff Compare November 19, 2020 23:03

zshi-redhat merged commit b734995 into openshift:master Nov 24, 2020

Billy99 deleted the billy99-device-info-spec branch December 4, 2020 19:12

Comments

Conversation

Billy99 commented Nov 16, 2020

Uh oh!

Billy99 commented Nov 16, 2020

Uh oh!

Billy99 commented Nov 18, 2020

Uh oh!

zshi-redhat left a comment

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

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!

Choose a reason for hiding this comment

Uh oh!

zshi-redhat Nov 19, 2020 • 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!

zshi-redhat commented Nov 19, 2020

Uh oh!

Billy99 commented Nov 19, 2020

Uh oh!

Billy99 commented Nov 23, 2020

Uh oh!

zshi-redhat commented Nov 24, 2020

Uh oh!

Billy99 commented Nov 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zshi-redhat Nov 19, 2020 •

edited

Loading