Deviceinfo downwardAPI #11618

AlonaKaplan · 2024-03-28T07:27:58Z

What this PR does

In some cases the multus status annotation contains device-info.
This info may be valuable for the binding plugin sidecar. For example SRIOV and VDPA plugins need this info.
As the multus status annotation is not there yet on virt-laucher pod creation, the suggested solution is to use downwardAPI volume and mount it to the sidecar.
The downwardAPI volume will track a new virt-launcher pod annotation kubevirt.io/network-info that will contain a list of interfaces with their device-info.
Very similar to the existing SRIOV downwardAPI solution which is mapping the networkName to the pci-address of the device and mounting the downwardAPI volume to the virt-launcher pod.

To enable the downwardAPI for a network binding plugin, it should have downwardAPI: device-info attribute.

apiVersion: kubevirt.io/v1
kind: KubeVirt
metadata:
  name: kubevirt
  namespace: kubevirt
spec:
  configuration:
    network:
      binding:
        vdpa:
          sidecarImage: quay.io/kubevirt/network-vdpa-binding
          downwardAPI: device-info

Before this PR:

Interfaces device info was exposed to the virt-launcher compute container via environment variables by the device plugin.
The compute container could use those env var to construct the libvirt domain xml.
A downward API is used to expose network to PCI map for SRIOV interfaces.

After this PR:

A downward API is used to expose interfaces device info to virt-launcher hook sidecars.

Follow-up - use the new network to device info map for SRIOV interfaces instead of the network to PCI map,

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:
alternatives -

Instead of configuring in the kubevirt CR whether deviceInfo should be mounted to the sidecar or not we can always mount it in case it exists in the status.
Share via the downwardAPI only the deviceInfo of binding that request it. It can be controlled by the binding config in the kubivirt CR (e.g “deviceInfo: true/false” attribute).
Passing PCIDEVICE_<RESOURCE_NAME>_NET_INFO env var from the virt-launcher to the sidecar by adding it as an annotation to the vmi object passed to the onDefineDomain method. The problem with this approach is that it will reintroduce the same bug SRIOV that the SRIOV downward API solved.
Sharing CNIDeviceInfo file with the sidecar. It is a very similar approach to the downwardAPI option, the * disadvantage here is that the user would have to specify the file path in the NAD.
More alternatives to a similar issue are listed in the SRIOV mapping design doc.

Links to places where the discussion took place:
design proposal

TBD: covert the to design to markdown and add it to the kubevirt/community design proposals.

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

Extend network binding plugin to support device-info DownwardAPI.

kubevirt-bot · 2024-03-28T07:28:00Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

AlonaKaplan · 2024-03-28T08:19:17Z

/test pull-kubevirt-build
/test pull-kubevirt-build-arm64
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-unit-test
/test pull-kubevirt-e2e-k8s-1.27-sig-network

ormergi · 2024-03-28T10:08:25Z

pkg/network/deviceinfo/deviceinfo.go

+)
+
+const (
+	MountPath = "/etc/podinfo"


I would store the new device-info annotation in different dir then the sriov-network-pci one.
IMO it will be easier to debug that way, also avoid potential overwrites, if tomorrow someone changes the downward API volumes render code.

It is under the deviceinfo dir. What place do you find better than this?

The path will be /etc/podinfo/deviceinfo?

I though about saving it in its own dir /etc/net-device-info

pkg/network/deviceinfo/deviceinfo_suite_test.go

ormergi · 2024-03-28T10:09:57Z

pkg/network/deviceinfo/deviceinfo_test.go

+		booOrdinalIfaceName = "net2"
+	)
+
+	networkStatusWithMixedNetworksFmt := `


nit: adding some indentations could make it easier to read.

I don't think it will work with indentations. Can you please give an example of what you suggest?

See the following examples, if the assertions fails you can use MatchJSON matcher:

networkStatusWithMixedNetworksFmt := `[ { "name": "kindnet", "interface": "eth0", "ips": [ "10.244.1.9" ], "mac": "3a:7e:42:fa:37:c6", "default": true, "dns": {} }, { "name": "default/nad1", "interface": "%s", "mac": "8a:37:d9:e7:0f:18", "dns": {} }, { "name": "default/nad2", "interface": "%s", "dns": {}, "device-info": { "type": "pci", "version": "1.0.0", "pci": { "pci-address": "0000:65:00.2" } } } ]` networkStatusWithPrimaryInterfaceOnly := `[{ "name": "kindnet", "interface": "eth0", "ips": [ "10.244.2.131" ], "mac": "82:cf:7c:98:43:7e", "default": true, "dns": {} }]`

pkg/network/deviceinfo/sriov_test.go

ormergi · 2024-03-28T10:29:00Z

pkg/network/deviceinfo/deviceinfo.go

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Copyright 2022 Red Hat, Inc.


Recently I got review comment that it should be The Kubevirt Authors instead of Red Hat, Inc.
Nevertheless, some files in the code base have the first and other has the latter, I guess it should be aligned soon to The Kubevirt Authors.

ormergi · 2024-03-28T16:08:39Z

pkg/network/deviceinfo/deviceinfo_test.go

+		),
+	)
+
+	DescribeTable("should prepare non empty network device info annotation",


It seems that only the expected annotation varies between the tests in this table, specifically the pod interface names, hashed vs. ordinal.
Please consider simplifying it by passing only the expected annotation.

pkg/network/deviceinfo/sriov.go

ormergi · 2024-03-28T16:13:22Z

pkg/network/deviceinfo/deviceinfo.go

@@ -31,7 +31,8 @@ import (
 )

 const (
-	MountPath = "/etc/podinfo"
+	NetworkDeviceInfoMapAnnot = "kubevirt.io/network-device-info-map"


Please consider removing map prefix, IMO it doesn't add value.
Also if tomorrow we decide to change the annotation value format (to not be map) the name would no longer be valid.

If we change the annotation value not to be a map it is API change. virt-launcher containers assume it is a map.

Here is a nice argue about having info about the value of a variable in its name
kubevirt/community#281 (comment)

pkg/virt-controller/network/pod_annotations_test.go

ormergi · 2024-03-28T16:22:03Z

pkg/virt-controller/network/pod_annotations_test.go

+]`
+
+	Context("Generate pod network annotations", func() {
+		It("should have empty device info annotation when there are no networks", func() {


Please consider using the "equal" matcher instead of (haveLen + haveKeyWithValue), it can provide better visibility for expected results.

In general it can be easier to maintain: in case there it require adding another annotation, you could change the expected annotation slice instead adding new assert and incrementing the haveLen assert.

AlonaKaplan · 2024-03-31T09:25:34Z

/test pull-kubevirt-build
/test pull-kubevirt-build-arm64
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-unit-test
/test pull-kubevirt-e2e-k8s-1.27-sig-network

AlonaKaplan · 2024-03-31T10:27:14Z

/test pull-kubevirt-build
/test pull-kubevirt-build-arm64
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-unit-test
/test pull-kubevirt-e2e-k8s-1.27-sig-network

ormergi

Thanks for the changes, second round

ormergi · 2024-04-01T13:20:01Z

staging/src/kubevirt.io/api/core/v1/types.go

@@ -2777,6 +2777,10 @@ type InterfaceBindingPlugin struct {
 	// Migration means the VM using the plugin can be safely migrated
 	// version: 1alphav1
 	Migration *InterfaceBindingMigration `json:"migration,omitempty"`
+	// DownwardAPI specifies what kind of data should be exposed to the binding plugin sidecar.


Suggested change

// DownwardAPI specifies what kind of data should be exposed to the binding plugin sidecar.

// DownwardAPI specifies what kind of data should be exposed to the binding plugin sidecar using Kubernetes Downward API

https://kubernetes.io/docs/concepts/workloads/pods/downward-api/

Why should I mention here what method is used to implement it. It is an implementation detail.

Well this impl. detail is well exposed by the downwardAPI field name :)

The name downward API means we want to expose to the container some of the pod fields.
It doesn't expose how we implement it and whether we use a k8s volume or some other method (e.g env vars).

Reference - https://kubernetes.io/docs/concepts/workloads/pods/downward-api/

Maybe we need to think of better name then DownwardAPI because this name imply k8s downward API feature is being used, which is an impl. detail according to you.

I dont have better name to suggest though.

I do not think the the name is a trade-mark.
It sounds a reasonable name to me, taking API data "down".

If there is no better name to suggest, continuing this thread is not useful.

ormergi · 2024-04-01T13:20:44Z

staging/src/kubevirt.io/api/core/v1/types.go

@@ -2777,6 +2777,10 @@ type InterfaceBindingPlugin struct {
 	// Migration means the VM using the plugin can be safely migrated
 	// version: 1alphav1
 	Migration *InterfaceBindingMigration `json:"migration,omitempty"`
+	// DownwardAPI specifies what kind of data should be exposed to the binding plugin sidecar.
+	// Supported values: "DeviceInfo"


Should be device-info?
If so, please note there are other places it should be changed.

ormergi · 2024-04-01T13:32:30Z

pkg/network/deviceinfo/deviceinfo.go

+)
+
+const (
+	MountPath = "/etc/podinfo"


The path will be /etc/podinfo/deviceinfo?

I though about saving it in its own dir /etc/net-device-info

ormergi · 2024-04-01T13:39:38Z

pkg/network/deviceinfo/deviceinfo_test.go

+		booOrdinalIfaceName = "net2"
+	)
+
+	networkStatusWithMixedNetworksFmt := `


See the following examples, if the assertions fails you can use MatchJSON matcher:

networkStatusWithMixedNetworksFmt := `[ { "name": "kindnet", "interface": "eth0", "ips": [ "10.244.1.9" ], "mac": "3a:7e:42:fa:37:c6", "default": true, "dns": {} }, { "name": "default/nad1", "interface": "%s", "mac": "8a:37:d9:e7:0f:18", "dns": {} }, { "name": "default/nad2", "interface": "%s", "dns": {}, "device-info": { "type": "pci", "version": "1.0.0", "pci": { "pci-address": "0000:65:00.2" } } } ]` networkStatusWithPrimaryInterfaceOnly := `[{ "name": "kindnet", "interface": "eth0", "ips": [ "10.244.2.131" ], "mac": "82:cf:7c:98:43:7e", "default": true, "dns": {} }]`

ormergi · 2024-04-02T06:52:24Z

pkg/network/deviceinfo/deviceinfo.go

+
+func CreateNetworkDeviceInfoAnnotationValue(networks []v1.Network, interfaces []v1.Interface, networkStatusAnnotationValue string, bindingPlugins map[string]v1.InterfaceBindingPlugin) string {
+	networkDeviceInfoMap, err := mapNetworkNameToDeviceInfo(networks, networkStatusAnnotationValue, func() []v1.Interface {
+		return filterDeviceInfoBindingPluginEntries(interfaces, bindingPlugins)


Please consider simplifying mapNetworkNameToDeviceInfo by passing interfaces slice instead of a lambda.
Also we can use vmispec.FilterInterfacesSpec for filtering:

vmispec.FilterInterfacesSpec(interfaces, func(iface v1.Interface) bool { if iface.Binding != nil { plugin, exist := bindingPlugins[iface.Binding.Name] return exist && binding.DownwardAPI == v1.DeviceInfo } })

ormergi · 2024-04-02T07:11:12Z

pkg/network/deviceinfo/deviceinfo.go

+	if err != nil {
+		log.Log.Warningf("failed to create network-device-info-map: %v", err)
+		networkDeviceInfoMap = map[string]*networkv1.DeviceInfo{}
+	}
+
+	networkDeviceInfoMapBytes, err := json.Marshal(networkDeviceInfoMap)
+	if err != nil {
+		log.Log.Warningf("failed to marshal network-device-info-map: %v", err)
+		return ""
+	}


I would not add the annotation at all if it has empty value, having it with empty value is equivalent to no exist at all.

ormergi · 2024-04-02T07:44:47Z

pkg/network/deviceinfo/deviceinfo.go

@@ -31,7 +31,8 @@ import (
 )

 const (
-	MountPath = "/etc/podinfo"
+	NetworkDeviceInfoMapAnnot = "kubevirt.io/network-device-info-map"


Here is a nice argue about having info about the value of a variable in its name
kubevirt/community#281 (comment)

ormergi · 2024-04-02T07:51:52Z

pkg/network/vmispec/interface.go

@@ -171,3 +171,14 @@ func FilterInterfacesByNetworks(interfaces []v1.Interface, networks []v1.Network
 	}
 	return ifaces
 }
+
+func BindingPluginNetworkWithDeviceInfoExist(ifaces []v1.Interface, bindingPlugins map[string]v1.InterfaceBindingPlugin) bool {


nit: I would call it NetworkBindingPluginRequireDeviceInfo

I find the original name more explicit.

The BindingPluginNetwork confusing it should be NetworkBindingPlugin as the feature name IMO.

The method is mot checking whether there is NetworkBindingPlugin with device info (in the kubevirt CR). It checks whether a VM has a network using this kind of BindingPlugin. Therefore, I find the current name as more suitable.

Now I understand what it means, maybe mentioning interface fit better a because the function receives interfaces slice.
Although, we already know the API suffer from the network/interface naming semantics, so maybe network can still work.

Still a nit, not a must.

ormergi · 2024-04-02T07:52:25Z

pkg/network/vmispec/interface_test.go

+			nonDeviceInfoPlugin: {},
+		}
+
+		It("returns false when there is no network with device onfo plugin", func() {


s/onfo/info/g

ormergi · 2024-04-02T07:55:04Z

pkg/virt-controller/services/template.go

@@ -28,6 +28,8 @@ import (
 	"strconv"
 	"strings"

+	"kubevirt.io/kubevirt/pkg/network/deviceinfo"


It seems to remain at the same place

EdDev

Partial review.

EdDev · 2024-04-04T09:55:55Z

staging/src/kubevirt.io/api/core/v1/types.go

@@ -2777,6 +2777,10 @@ type InterfaceBindingPlugin struct {
 	// Migration means the VM using the plugin can be safely migrated
 	// version: 1alphav1
 	Migration *InterfaceBindingMigration `json:"migration,omitempty"`
+	// DownwardAPI specifies what kind of data should be exposed to the binding plugin sidecar.
+	// Supported values: "device-info"
+	// version: 1alphav1


I think it should be v1alphav1.

Done. We need a separate PR to change all the occurrences.

staging/src/kubevirt.io/api/core/v1/types.go

pkg/network/downwardapi/types.go

pkg/network/deviceinfo/deviceinfo.go

EdDev · 2024-04-04T10:34:32Z

pkg/network/deviceinfo/deviceinfo.go

+
+func mapNetworkNameToDeviceInfo(networks []v1.Network,
+	networkStatusAnnotationValue string, filterMethod func() []v1.Interface) (map[string]*networkv1.DeviceInfo, error) {
+	multusInterfaceNameToNetworkStatusMap, err := mapMultusInterfaceNameToNetworkStatus(networkStatusAnnotationValue)


nit: You can drop the Map, it is ugly. Same for networkDeviceInfoMap.

EdDev · 2024-04-04T10:58:34Z

pkg/network/deviceinfo/deviceinfo.go

+	return mapNetworkNameToDeviceInfo(networks, networkStatusAnnotationValue, func() []v1.Interface {
+		return filterDeviceInfoBindingPluginEntries(interfaces, bindingPlugins)
+	})


I am not sure if it is something I missed, but this seems over complicated.
Why not pass the filtered interface instead of the function?

Will this work?

return mapNetworkNameToDeviceInfo(networks, networkStatusAnnotationValue, vmispec.FilterInterfacesSpec(interfaces, func(iface v1.Interface) bool { if iface.Binding != nil { if binding, exist := bindingPlugins[iface.Binding.Name]; exist && binding.DownwardAPI == v1.DeviceInfo { return true } } return false })

You can even consider making this an object, as a large number of data is passed around.

EdDev · 2024-04-04T11:02:46Z

pkg/network/deviceinfo/deviceinfo_suite_test.go

 	"kubevirt.io/client-go/testutils"
 )

 func TestDeviceInfo(t *testing.T) {
 	testutils.KubeVirtTestSuiteSetup(t)
 }
+
+func newBridgeInterface(name string) virtv1.Interface {


Can you use libvmi for this one and all the rest?

done, much nicer:)

Define the API for passing network info to virt-launcher containers using DownwardAPI volume. Signed-off-by: Alona Paz <alkaplan@redhat.com>

EdDev

Reviewed the 1st 5 commits.

pkg/network/deviceinfo/deviceinfo.go

EdDev · 2024-04-04T12:58:51Z

pkg/network/deviceinfo/deviceinfo.go

+func MapBindingPluginNetworkNameToDeviceInfo(networks []v1.Network, interfaces []v1.Interface,
+	networkStatusAnnotationValue string, bindingPlugins map[string]v1.InterfaceBindingPlugin) (map[string]*networkv1.DeviceInfo, error) {
+	return mapNetworkNameToDeviceInfo(networks, networkStatusAnnotationValue,
+		filterDeviceInfoBindingPluginEntries(interfaces, bindingPlugins))


Any reason you have not used vmispec.FilterInterfacesSpec as suggested previously?

EdDev · 2024-04-04T13:02:06Z

pkg/network/deviceinfo/deviceinfo.go

+		networkStatusEntry, exist := multusInterfaceNameToNetworkStatus[multusInterfaceName]
+		if !exist {
+			continue // The interface is not plugged yet
+		}
+		networkDeviceInfo[iface.Name] = networkStatusEntry.DeviceInfo


I find continue messy.
Perhaps you will agree to a direct approach:

if networkStatusEntry, exist := multusInterfaceNameToNetworkStatus[multusInterfaceName]; exist { networkDeviceInfo[iface.Name] = networkStatusEntry.DeviceInfo }

EdDev · 2024-04-04T13:04:12Z

pkg/network/deviceinfo/deviceinfo_test.go

+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	virtv1 "kubevirt.io/api/core/v1"


nit: This is "our" API, I think it can be a simple v1.

EdDev · 2024-04-04T13:16:17Z

pkg/network/deviceinfo/deviceinfo_test.go

+			networkDeviceInfoMap, err :=
+				deviceinfo.MapBindingPluginNetworkNameToDeviceInfo(networkList, interfaceList, networkStatusAnnotationValue, bindingPlugins)


I am not familiar with this style, usually we do:

Expect(deviceinfo.MapBindingPluginNetworkNameToDeviceInfo( networkList, interfaceList, networkStatusAnnotationValue, bindingPlugins, )).To(BeEmpty())

The error is implicitly checked.

I would still use additional variable in between (networkDeviceInfoMap), just not break the line right at the assignment.

EdDev · 2024-04-04T13:50:20Z

pkg/network/downwardapi/networkinfo.go

+		log.Log.Warningf("failed to marshal network-info: %v", err)
+		return ""


Do we need to ignore the error so deep here? Why not let the caller decide if to ignore it or not?

If you can explain what is the reason for this in the commit message, it will be great.

This behavior was copied from SRIOV to keep consistency.
Updated the commit message.

EdDev · 2024-04-04T13:51:20Z

pkg/network/downwardapi/networkinfo.go

+	networkInfo := NetworkInfo{Interfaces: downwardAPIInterfaces}
+	return networkInfo


nit: return NetworkInfo{Interfaces: downwardAPIInterfaces}

EdDev · 2024-04-04T13:53:26Z

pkg/network/downwardapi/networkinfo.go

+}
+
+func generateNetworkInfo(networkDeviceInfoMap map[string]*networkv1.DeviceInfo) NetworkInfo {
+	downwardAPIInterfaces := []Interface{}


This will return an empty list and not a nil one.
Any reason it is not a simple uninitialized slice? var downwardAPIInterfaces []Interface

EdDev · 2024-04-04T13:54:03Z

pkg/network/downwardapi/networkinfo_test.go

+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"


nit: I can you keep these separated from the above one?

EdDev · 2024-04-04T13:55:44Z

pkg/network/downwardapi/networkinfo_test.go

+		expectedNetworkInfo := `{"interfaces":[{"network":"boo"},{"network":"foo","deviceInfo":{"type":"fooType"}}]}`
+		Expect(downwardapi.CreateNetworkInfoAnnotationValue(networkDeviceInfoMap)).To(Equal(expectedNetworkInfo))
+	})
+


Please consider:

An empty list one.

One with an error in the marshaling (if possible).

An empty list one.

Done.

One with an error in the marshaling.

I don't know how to simulate it.

Marshaling errors results in empty annotation value.

ormergi · 2024-04-04T17:12:02Z

pkg/virt-controller/watch/vmi.go

-				vmi.Spec.Networks, vmi.Spec.Domain.Devices.Interfaces, pod.Annotations[networkv1.NetworkStatusAnnot],
-			)
-			newAnnotations := map[string]string{deviceinfo.NetworkPCIMapAnnot: networkPCIMapAnnotationValue}
+		newAnnotations := network.GeneratePodAnnotations(vmi.Spec.Networks, vmi.Spec.Domain.Devices.Interfaces, pod.Annotations[networkv1.NetworkStatusAnnot], c.clusterConfig.GetNetworkBindings())


Please note line 1282, there is already code that update multus network annotation (c.updateMultus, it also calls syncAnnotations) and patches the pod.
Is it possible to consolidate these two pieces of code so there will be one patch operation?

It is not in the scope of this big enough PR.

ormergi · 2024-04-04T17:16:22Z

pkg/virt-controller/network/pod_annotations_test.go

+func newMultusNetwork(name, networkName string) virtv1.Network {
+	return virtv1.Network{
+		Name: name,
+		NetworkSource: virtv1.NetworkSource{
+			Multus: &virtv1.MultusNetwork{
+				NetworkName: networkName,
+			},
+		},
+	}
+}
+
+func newInterface(name string) virtv1.Interface {
+	return virtv1.Interface{
+		Name: name,
+	}
+}
+
+func newSRIOVInterface(name string) virtv1.Interface {
+	return virtv1.Interface{
+		Name:                   name,
+		InterfaceBindingMethod: virtv1.InterfaceBindingMethod{SRIOV: &virtv1.InterfaceSRIOV{}},
+	}
+}
+
+func newBindingPluginInterface(name, bindingPlugin string) virtv1.Interface {
+	return virtv1.Interface{
+		Name:    name,
+		Binding: &virtv1.PluginBinding{Name: bindingPlugin},
+	}
+}


Myabe you can replace those with libvmi as well.

ormergi · 2024-04-04T18:02:43Z

pkg/network/deviceinfo/deviceinfo_test.go

+		),
+	)
+
+	It("should prepare network device info annotation with multiple networks", func() {


Suggested change

It("should prepare network device info annotation with multiple networks", func() {

It("should prepare network to device-info mapping given multiple networks", func() {

I think it is fine as is, the suggestion is not better than the existing. And it is a super nit.

yep not a must, sorry for not mentioning it a nit and not meaningful

ormergi · 2024-04-04T18:12:52Z

pkg/network/deviceinfo/deviceinfo_test.go

+			newBindingPluginInterface("foo", deviceInfoPlugin),
+			newBindingPluginInterface("doo", deviceInfoPlugin),
+		}
+		expectedMap :=


nit:

Suggested change

expectedMap :=

expectedMap := map[string]*networkv1.DeviceInfo{

"foo": {Type: "pci", Version: "1.0.0", Pci: &networkv1.PciDevice{PciAddress: "0000:65:00.2"}}

}

pkg/network/deviceinfo/sriov.go

ormergi · 2024-04-04T18:25:59Z

pkg/network/vmispec/interface.go

@@ -171,3 +171,14 @@ func FilterInterfacesByNetworks(interfaces []v1.Interface, networks []v1.Network
 	}
 	return ifaces
 }
+
+func BindingPluginNetworkWithDeviceInfoExist(ifaces []v1.Interface, bindingPlugins map[string]v1.InterfaceBindingPlugin) bool {


The BindingPluginNetwork confusing it should be NetworkBindingPlugin as the feature name IMO.

ormergi · 2024-04-04T18:27:31Z

pkg/network/vmispec/interface_test.go

+			ifaces := []v1.Interface{interfaceWithBindingPlugin("net1", nonDeviceInfoPlugin)}
+			Expect(netvmispec.BindingPluginNetworkWithDeviceInfoExist(ifaces, bindingPlugins)).To(BeFalse())
+		})
+		It("returns true when there is no network with device info plugin", func() {


Suggested change

It("returns true when there is no network with device info plugin", func() {

It("returns true when there is at least one network with device-info plugin", func() {

done, thanks.

ormergi · 2024-04-04T18:30:01Z

pkg/virt-controller/network/pod_annotations_test.go

+		networkStatus := `
+[
+{
+ "name": "default/no-device-info",
+ "interface": "pod6446d58d6df",
+ "mac": "8a:37:d9:e7:0f:18",
+ "dns": {}
+},
+{
+ "name": "default/with-device-info",
+ "interface": "pod2c26b46b68f",
+ "dns": {},
+ "device-info": {
+   "type": "pci",
+   "version": "1.0.0",
+   "pci": {
+     "pci-address": "0000:65:00.2"
+   }
+ }
+},
+{
+ "name": "default/sriov",
+ "interface": "pod778c553efa0",
+ "dns": {},
+ "device-info": {
+   "type": "pci",
+   "version": "1.0.0",
+   "pci": {
+     "pci-address": "0000:65:00.3"
+   }
+ }
+}
+]`


nit: indentation would be nice, also it seems it can defined as const

networkStatus := `[ { "name": "default/no-device-info", "interface": "pod6446d58d6df", "mac": "8a:37:d9:e7:0f:18", "dns": {} }, { "name": "default/with-device-info", "interface": "pod2c26b46b68f", "dns": {}, "device-info": { "type": "pci", "version": "1.0.0", "pci": { "pci-address": "0000:65:00.2" } } }, { "name": "default/sriov", "interface": "pod778c553efa0", "dns": {}, "device-info": { "type": "pci", "version": "1.0.0", "pci": { "pci-address": "0000:65:00.3" } } } ]`

ormergi · 2024-04-04T18:33:55Z

pkg/virt-controller/network/pod_annotations_test.go

+			Expect(podAnnotationMap).To(HaveLen(1))
+			Expect(podAnnotationMap).To(HaveKeyWithValue(downwardapi.NetworkInfoAnnot, `{"interfaces":[{"network":"foo","deviceInfo":{"type":"pci","version":"1.0.0","pci":{"pci-address":"0000:65:00.2"}}}]}`))
+		})
+		It("should have both network-pci-map, network-info entries when there is SRIOV interface and binding plugin interface with device-info", func() {
+			networks := []virtv1.Network{
+				newMultusNetwork("foo", "default/with-device-info"),
+				newMultusNetwork("doo", "default/sriov"),
+			}
+
+			interfaces := []virtv1.Interface{
+				newBindingPluginInterface("foo", deviceInfoPlugin),
+				newSRIOVInterface("doo"),
+			}
+			podAnnotationMap := network.GeneratePodAnnotations(networks, interfaces, networkStatus, bindingPlugins)
+			Expect(podAnnotationMap).To(HaveLen(2))
+			Expect(podAnnotationMap).To(HaveKeyWithValue(downwardapi.NetworkInfoAnnot, `{"interfaces":[{"network":"foo","deviceInfo":{"type":"pci","version":"1.0.0","pci":{"pci-address":"0000:65:00.2"}}}]}`))
+			Expect(podAnnotationMap).To(HaveKeyWithValue(deviceinfo.NetworkPCIMapAnnot, `{"doo":"0000:65:00.3"}`))


nit: Using equal matcher would improve visibility of expected results.

EdDev · 2024-04-05T04:51:47Z

staging/src/kubevirt.io/api/core/v1/types.go

@@ -2777,6 +2777,10 @@ type InterfaceBindingPlugin struct {
 	// Migration means the VM using the plugin can be safely migrated
 	// version: 1alphav1
 	Migration *InterfaceBindingMigration `json:"migration,omitempty"`
+	// DownwardAPI specifies what kind of data should be exposed to the binding plugin sidecar.


I do not think the the name is a trade-mark.
It sounds a reasonable name to me, taking API data "down".

If there is no better name to suggest, continuing this thread is not useful.

EdDev · 2024-04-05T08:50:09Z

pkg/network/deviceinfo/deviceinfo_test.go

+		),
+	)
+
+	It("should prepare network device info annotation with multiple networks", func() {


I think it is fine as is, the suggestion is not better than the existing. And it is a super nit.

EdDev · 2024-04-05T08:52:51Z

pkg/network/deviceinfo/deviceinfo_test.go

+		actualMap, err := deviceinfo.MapBindingPluginNetworkNameToDeviceInfo(networks, interfaces, networkStatusWithMixedNetworks, bindingPlugins)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(actualMap).To(Equal(expectedMap))


nit:

Expect(deviceinfo.MapBindingPluginNetworkNameToDeviceInfo(networks, interfaces, networkStatusWithMixedNetworks, bindingPlugins)).To(Equal(expectedMap))

The error is implicitly checked.

pkg/network/deviceinfo/sriov.go

EdDev · 2024-04-05T09:00:53Z

pkg/network/deviceinfo/sriov.go

 	for _, sriovIface := range vmispec.FilterSRIOVInterfaces(interfaces) {
-		multusInterfaceName := networkNameScheme[sriovIface.Name]
-		networkStatusEntry, exist := multusInterfaceNameToNetworkStatusMap[multusInterfaceName]
+		deviceInfo, exist := networkNameToDeviceInfo[sriovIface.Name]


In continuation or Or's comment a few lines before.
I think you can do this now:

for netName, deviceInfo := range networkNameToDeviceInfo { ... }

Wherever sriovIface.Name appears, netName can be used.

EdDev

6th commit review

pkg/virt-controller/network/pod_annotations.go

EdDev · 2024-04-07T06:11:41Z

pkg/virt-controller/network/pod_annotations.go

+		newAnnotations[deviceinfo.NetworkPCIMapAnnot] = networkPCIMapAnnotationValue
+	}
+	if vmispec.BindingPluginNetworkWithDeviceInfoExist(interfaces, bindingPlugins) {
+		networkDeviceInfoMap, err := deviceinfo.MapBindingPluginNetworkNameToDeviceInfo(networks, interfaces, multusStatusAnnotation, bindingPlugins)


I think this is the only production usage of this function, therefore it may be a good idea to reconsider its signature.
There is duplication with BindingPluginNetworkWithDeviceInfoExist and I hope we can avoid adding that other function.

How about this: The bindingPlugins is only needed for the filtering, both here and in the prev one.
So we can pass in an already filtered interfaces slice.

ifacesWithDeviceInfo := vmispec.FilterInterfacesSpec(interfaces, func(iface v1.Interface) bool { if iface.Binding != nil { if binding, exist := bindingPlugins[iface.Binding.Name]; exist && binding.DownwardAPI == v1.DeviceInfo { return true } } return false }) if len(ifacesWithDeviceInfo) > 0 { networkDeviceInfoMap, err := deviceinfo.MapBindingPluginNetworkNameToDeviceInfo(networks, ifacesWithDeviceInfo, multusStatusAnnotation) if err != nil { ... } ... } return newAnnotations

The method has one more usage in the next commit. Also, I find it much more readable and telling the story of the method the way it is.

IMO the only readability problem is with binding, exist := bindingPlugins[iface.Binding.Name]; exist && binding.DownwardAPI == v1.DeviceInfo , which you could extract to a function if you want.

If the problem is re-use, you can also just move the filter to vmispec and name it.

vmispec.FilterInterfacesSpec(interfaces, func(iface v1.Interface) bool { if iface.Binding != nil { if binding, exist := bindingPlugins[iface.Binding.Name]; exist && binding.DownwardAPI == v1.DeviceInfo { return true } } return false })

We will benefit here from the filtered interfaces slice.

I find the current helper in vmispec not elegant.

But if you do not like it, I won't insist further.

EdDev · 2024-04-07T06:25:35Z

pkg/virt-controller/services/template.go

@@ -679,6 +680,9 @@ func newSidecarContainerRenderer(sidecarName string, vmiSpec *v1.VirtualMachineI

 	var mounts []k8sv1.VolumeMount
 	mounts = append(mounts, sidecarVolumeMount())
+	if requestedHookSidecar.DownwardAPI == v1.DeviceInfo {
+		mounts = append(mounts, mountPath(downwardapi.NetworkInfoVolumeName, downwardapi.MountPath))


nit: Exposing the consts, it may have been better to just return from downwardapi package the k8sv1.VolumeMount. The advantage would have been that no one will misuse the consts.
WDYT?

I think the current option is more readable and the consts (at least downwardapi.NetworkInfoVolumeName) are used on volume creation as well.

It is unsafe to use consts like this, because they may be used in the wrong place.
Returning the object itself would have been safer over exposing the consts.

I will not insist on this.

EdDev · 2024-04-08T03:56:18Z

pkg/hooks/hooks.go

+	Args            []string                         `json:"args,omitempty"`
+	ConfigMap       *ConfigMap                       `json:"configMap,omitempty"`
+	PVC             *PVC                             `json:"pvc,omitempty"`
+	DownwardAPI     v1.NetworkBindingDownwardAPIType `json:"downwardAPI,omitempty"`


I think that when you add it here, it is exposed to the sidecar-hook annotation [1].

[1]

kubevirt/pkg/hooks/hooks_test.go

Lines 49 to 60 in 25b3733

hooks.HookSidecarListAnnotationName: `

[

{

"image": "some-image:v1",

"imagePullPolicy": "IfNotPresent"

},

{

"image": "another-image:v1",

"imagePullPolicy": "Always"

}

]

`,

Changed to json:"-" so the DownwardAPI value won't be unmarshaled from the annotation.

Create a map between networks and their device info using the mutlus status annotation. Since a similar map between the network to PCI address is already created for the SRIOV use case, the SRIOV code was changed to reuse the device info general methods. Signed-off-by: Alona Paz <alkaplan@redhat.com>

The VMI controller can use this method when generating the pod annotations. The method doesn't retrun an error in case of marshaling failure, so the VMI controller using it will just get an empty annotation. If evetually the annotation won't be created, a follow-up step will fail, there is no need to corrupt the vmi controller. Signed-off-by: Alona Paz <alkaplan@redhat.com>

The annotation should appear only on pods with binding plugin network that has DeviceInfo downwardAPI. Signed-off-by: Alona Paz <alkaplan@redhat.com>

Expose network-info annotation using downward API dir volume. Mount this volume to the hook sidecars. Signed-off-by: Alona Paz <alkaplan@redhat.com>

EdDev

Thank you!

I don't like the addition added to vmispec, but it is not worth blocking this great work.

We should wait for the design to be merged before taking this one in.
Hope we can resolve the current discussion there soon.

EdDev · 2024-04-09T15:50:41Z

pkg/network/downwardapi/networkinfo.go

+	networkInfo := NetworkInfo{Interfaces: downwardAPIInterfaces}
+	return networkInfo


EdDev · 2024-04-09T16:08:56Z

pkg/virt-controller/network/pod_annotations.go

+		newAnnotations[deviceinfo.NetworkPCIMapAnnot] = networkPCIMapAnnotationValue
+	}
+	if vmispec.BindingPluginNetworkWithDeviceInfoExist(interfaces, bindingPlugins) {
+		networkDeviceInfoMap, err := deviceinfo.MapBindingPluginNetworkNameToDeviceInfo(networks, interfaces, multusStatusAnnotation, bindingPlugins)


IMO the only readability problem is with binding, exist := bindingPlugins[iface.Binding.Name]; exist && binding.DownwardAPI == v1.DeviceInfo , which you could extract to a function if you want.

If the problem is re-use, you can also just move the filter to vmispec and name it.

vmispec.FilterInterfacesSpec(interfaces, func(iface v1.Interface) bool { if iface.Binding != nil { if binding, exist := bindingPlugins[iface.Binding.Name]; exist && binding.DownwardAPI == v1.DeviceInfo { return true } } return false })

We will benefit here from the filtered interfaces slice.

I find the current helper in vmispec not elegant.

But if you do not like it, I won't insist further.

EdDev · 2024-04-09T16:11:30Z

pkg/virt-controller/services/template.go

@@ -679,6 +680,9 @@ func newSidecarContainerRenderer(sidecarName string, vmiSpec *v1.VirtualMachineI

 	var mounts []k8sv1.VolumeMount
 	mounts = append(mounts, sidecarVolumeMount())
+	if requestedHookSidecar.DownwardAPI == v1.DeviceInfo {
+		mounts = append(mounts, mountPath(downwardapi.NetworkInfoVolumeName, downwardapi.MountPath))


It is unsafe to use consts like this, because they may be used in the wrong place.
Returning the object itself would have been safer over exposing the consts.

I will not insist on this.

ormergi

Thank you, awesome work

lmilleri · 2024-04-10T11:30:05Z

I have tested this PR content in my vdpa environment and it works as expected, thank you!
This is an example of the file mounted in the vdpa sidecar plugin:

{ "interfaces": [ { "network": "nic1", "deviceInfo": { "type": "vdpa", "version": "1.1.0", "vdpa": { "parent-device": "vdpa:0000:65:00.2", "driver": "vhost", "path": "/dev/vhost-vdpa-0", "pci-address": "0000:65:00.2" } } } ] }

AlonaKaplan · 2024-04-17T10:46:45Z

Raising 2 lgtms to approve.

/approve

kubevirt-bot · 2024-04-17T10:46:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlonaKaplan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [AlonaKaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-commenter-bot · 2024-04-17T10:46:54Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

kubevirt-commenter-bot · 2024-04-17T23:08:54Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot requested review from EdDev, mhenriks and ormergi March 28, 2024 07:28

kubevirt-bot added sig/buildsystem Denotes an issue or PR that relates to changes in the build system. sig/network labels Mar 28, 2024

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from 38a493b to cd49eab Compare March 28, 2024 12:26

ormergi reviewed Mar 28, 2024

View reviewed changes

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from cd49eab to 4803c83 Compare March 31, 2024 08:56

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from 4803c83 to fde2363 Compare March 31, 2024 10:16

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from fde2363 to d188b9a Compare March 31, 2024 16:32

ormergi reviewed Apr 2, 2024

View reviewed changes

AlonaKaplan force-pushed the deviceinfo_downwardapi branch 2 times, most recently from cbf267a to b5e666a Compare April 3, 2024 15:15

AlonaKaplan marked this pull request as ready for review April 4, 2024 07:24

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2024

kubevirt-bot requested review from ormergi and RamLavi April 4, 2024 07:24

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from b5e666a to 802f483 Compare April 4, 2024 09:32

EdDev reviewed Apr 4, 2024

View reviewed changes

network: Define the API for DownwardAPI.NetwrokInfo

93af3bd

Define the API for passing network info to virt-launcher containers using DownwardAPI volume. Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from 802f483 to 4363b69 Compare April 4, 2024 12:19

EdDev suggested changes Apr 4, 2024

View reviewed changes

kubevirt-bot assigned EdDev Apr 4, 2024

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from 4363b69 to cf736fa Compare April 4, 2024 17:51

ormergi reviewed Apr 4, 2024

View reviewed changes

EdDev reviewed Apr 5, 2024

View reviewed changes

EdDev reviewed Apr 7, 2024

View reviewed changes

EdDev reviewed Apr 8, 2024

View reviewed changes

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from cf736fa to 91a27af Compare April 8, 2024 18:14

kubevirt-bot added size/XXL and removed size/XL labels Apr 8, 2024

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from 91a27af to eda85c5 Compare April 9, 2024 08:14

AlonaKaplan added 3 commits April 9, 2024 11:27

virt-controller, vmi: Patch network-info annotation

e9efce8

The annotation should appear only on pods with binding plugin network that has DeviceInfo downwardAPI. Signed-off-by: Alona Paz <alkaplan@redhat.com>

virt-controller, Expose network-info with DownwardAPI

c9a80b5

Expose network-info annotation using downward API dir volume. Mount this volume to the hook sidecars. Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan force-pushed the deviceinfo_downwardapi branch from eda85c5 to c9a80b5 Compare April 9, 2024 08:27

EdDev approved these changes Apr 9, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2024

ormergi approved these changes Apr 9, 2024

View reviewed changes

kubevirt-bot assigned ormergi Apr 9, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2024

kubevirt-bot merged commit 895075a into kubevirt:main Apr 18, 2024
38 checks passed

ormergi mentioned this pull request Apr 25, 2024

SR-IOV binding: Use network-info to map between the network and PCI address #11788

Merged

8 tasks

	// DownwardAPI specifies what kind of data should be exposed to the binding plugin sidecar.
	// DownwardAPI specifies what kind of data should be exposed to the binding plugin sidecar using Kubernetes Downward API
	https://kubernetes.io/docs/concepts/workloads/pods/downward-api/

		networkDeviceInfoMap, err :=
		deviceinfo.MapBindingPluginNetworkNameToDeviceInfo(networkList, interfaceList, networkStatusAnnotationValue, bindingPlugins)

		log.Log.Warningf("failed to marshal network-info: %v", err)
		return ""

		networkInfo := NetworkInfo{Interfaces: downwardAPIInterfaces}
		return networkInfo

	It("should prepare network device info annotation with multiple networks", func() {
	It("should prepare network to device-info mapping given multiple networks", func() {

-		expectedMap :=
+		expectedMap := map[string]*networkv1.DeviceInfo{
+		    "foo": {Type: "pci", Version: "1.0.0", Pci: &networkv1.PciDevice{PciAddress: "0000:65:00.2"}}
+		}

	It("returns true when there is no network with device info plugin", func() {
	It("returns true when there is at least one network with device-info plugin", func() {

	hooks.HookSidecarListAnnotationName: `
	[
	{
	"image": "some-image:v1",
	"imagePullPolicy": "IfNotPresent"
	},
	{
	"image": "another-image:v1",
	"imagePullPolicy": "Always"
	}
	]
	`,

Deviceinfo downwardAPI #11618

Deviceinfo downwardAPI #11618

Conversation

AlonaKaplan commented Mar 28, 2024 • edited

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

kubevirt-bot commented Mar 28, 2024

AlonaKaplan commented Mar 28, 2024

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

AlonaKaplan commented Mar 31, 2024

AlonaKaplan commented Mar 31, 2024

ormergi left a 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

EdDev left a 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

EdDev Apr 4, 2024 • edited

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

EdDev left a 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

AlonaKaplan commented Mar 28, 2024 •

edited

EdDev Apr 4, 2024 •

edited