Binding plugin API #10284

AlonaKaplan · 2023-08-14T10:20:44Z

What this PR does / why we need it:
This PR introduces network bindings API as plugins external to KubeVirt

Network bindings are registered on the cluster by specifying on the Kubevirt CR their name and configuration (e.g sidecar image).

example:

piVersion: kubevirt.io/v1
kind: KubeVirt
metadata:
  name: kubevirt
  namespace: kubevirt
spec:
  configuration:
    network:
      binding:
        bridge:
          sidecarImage: quay.io/kubevirt/network-bridge-binding

On the VM/VMI specification, the binding is referenced by name.

example:

apiVersion: kubevirt.io/v1
kind: VirtualMachine
metadata:
  name: vmi-1-bridge
spec:
  template:
    spec:
      domain:
        devices:
          interfaces:
          - name: blue
            mac: 12:34:56:78:9a:bc
            binding:
              name: bridge
      networks:
      - name: blue
        pod: {}

The feature is behind "NetworkBindingPlugins" gate.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
The API is not final (v1alphav1) and may be extended as we add more plugins.

Release note:

Introduce an API for network binding plugins. The feature is behind "NetworkBindingPlugins" gate.

kubevirt-bot · 2023-08-14T10:20:46Z

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

EdDev

I reviewed all except the net binding plugin: Add binding sidecars to virt-launcher commit.

I think we also need a feature gate.

Please also specify in the release notes that this is an alphav1 release of this API.

EdDev · 2023-08-15T10:01:59Z

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

+	// Image that runs on virt-launcher pod.
+	//It run services and configures the libvirt domain.


// SidecarImage references a container image that runs in the virt-launcher pod. // The sidecar handles (libvirt) domain configuration and optional services.

EdDev · 2023-08-15T11:36:24Z

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

@@ -1201,6 +1201,9 @@ type Interface struct {
 	// BindingMethod specifies the method which will be used to connect the interface to the guest.
 	// Defaults to Bridge.
 	InterfaceBindingMethod `json:",inline"`
+	// Binding specifies the binding plugin that will be used to to connect the interface to the guest.


typo: double "to".

EdDev · 2023-08-15T11:50:33Z

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

@@ -1310,6 +1313,12 @@ type InterfaceMacvtap struct{}
 // InterfacePasst connects to a given network.
 type InterfacePasst struct{}

+// PluggedBinding represents a binding implemented in a plugin.
+type PluggedBinding struct {


Perhaps Plugin instead of Plugged?

How about BindingPlugin?

EdDev · 2023-08-15T11:53:36Z

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

@@ -1310,6 +1313,12 @@ type InterfaceMacvtap struct{}
 // InterfacePasst connects to a given network.
 type InterfacePasst struct{}

+// PluggedBinding represents a binding implemented in a plugin.
+type PluggedBinding struct {
+	// Reference to the binding name as denined in the kubevirt CR.


// Name references to the binding name ....

EdDev · 2023-08-15T11:56:13Z

pkg/virt-api/webhooks/validating-webhook/admitters/network.go

@@ -58,3 +58,24 @@ func validateInterfaceStateValue(field *k8sfield.Path, spec *v1.VirtualMachineIn
 	}
 	return causes
 }
+
+func validateInterfaceBinding(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) []metav1.StatusCause {


Someone needs to call it :)

Unit tests will be nice too.

oops:) done.

EdDev · 2023-08-15T11:58:36Z

pkg/virt-api/webhooks/validating-webhook/admitters/network.go

+			if iface.InterfaceBindingMethod.Bridge != nil ||
+				iface.InterfaceBindingMethod.Slirp != nil ||
+				iface.InterfaceBindingMethod.Masquerade != nil ||
+				iface.InterfaceBindingMethod.SRIOV != nil ||
+				iface.InterfaceBindingMethod.Macvtap != nil ||
+				iface.InterfaceBindingMethod.Passt != nil {


Can you extract it to a small helper that accepts this InterfaceBindingMethod struct and checks if one of them exists? It will make this code clearer.

Thanks, you can resolve it now.

EdDev · 2023-08-15T12:22:46Z

pkg/virt-controller/services/net_binding.go

@@ -0,0 +1,30 @@
+package services


Please add the header.

EdDev · 2023-08-15T12:24:14Z

pkg/virt-controller/services/net_binding.go

+	"kubevirt.io/kubevirt/pkg/hooks"
+)
+
+func BindingPluginSidecarList(vmi *v1.VirtualMachineInstance, config *v1.KubeVirtConfiguration) (hooks.HookSidecarList, error) {


We need a context for this being network related. Perhaps just add Net as a prefix.

EdDev · 2023-08-15T12:26:26Z

pkg/virt-controller/services/net_binding.go

+			}
+
+			if !exist {
+				return nil, fmt.Errorf("couldn't find configuration for bindining: %s", iface.Binding.Name)


"binding" >> "network binding".

EdDev · 2023-08-15T12:28:25Z

pkg/virt-controller/services/net_binding_test.go

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


nit: This import is not in the same block as the above.

EdDev · 2023-08-15T12:28:50Z

pkg/virt-controller/services/net_binding_test.go

@@ -0,0 +1,85 @@
+package services_test


Please add a header.

EdDev · 2023-08-15T12:30:02Z

pkg/virt-controller/services/template.go

@@ -359,6 +359,12 @@ func (t *templateService) renderLaunchManifest(vmi *v1.VirtualMachineInstance, i
 		return nil, err
 	}

+	bindingSidecars, err := BindingPluginSidecarList(vmi, t.clusterConfig.GetConfig())
+	requestedHookSidecarList = append(requestedHookSidecarList, bindingSidecars...)


This should come after the error check.

Good catch!

EdDev · 2023-08-15T12:31:41Z

pkg/virt-controller/services/net_binding_test.go

+		shouldSucceed = true
+	)
+
+	BeforeEach(func() {


EdDev · 2023-08-15T12:33:20Z

pkg/virt-controller/services/net_binding_test.go

+				},
+			}
+			sidecars, err := services.BindingPluginSidecarList(vmi, config)
+			if shouldSucceed {


This usually is a hint that two different test tables are a better fit.

I don't like this pattern because it makes tests a bit harder to follow, I think its worth adding another DescribeTable.

EdDev · 2023-08-15T12:36:16Z

pkg/virt-controller/services/net_binding_test.go

+				hooks.HookSidecarList{{Image: testSidecarImage1}},
+				shouldSucceed),
+			Entry("VMI has multiple plugin bindings",
+				libvmi.New(libvmi.WithInterface(v1.Interface{Name: testNetworkName1, Binding: &v1.PluggedBinding{Name: testBindingName1}}),


Please add another interface with the same plugin binding as an existing one.
We still need to see one sidecare even more than one interface ref it.

done, thanks for discovering a bug:)

ormergi · 2023-08-15T11:34:30Z

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

@@ -1201,6 +1201,9 @@ type Interface struct {
 	// BindingMethod specifies the method which will be used to connect the interface to the guest.
 	// Defaults to Bridge.
 	InterfaceBindingMethod `json:",inline"`
+	// Binding specifies the binding plugin that will be used to to connect the interface to the guest.
+	// It provides an alternative to InterfaceBindingMethod.
+	Binding *PluggedBinding `json:"binding,omitempty"`


Does it make sense/possible to change InterfaceBindingMethodattribute to be non mandatory, and make the validation webhook to force having binding-method or binding-plugin?

It could make it easier to work with (e.g: check if iface has both binding-plugin and binding-method: if iface.binding != nil && iface.bindingMethod != nil).

ormergi · 2023-08-15T13:07:17Z

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

@@ -1310,6 +1313,12 @@ type InterfaceMacvtap struct{}
 // InterfacePasst connects to a given network.
 type InterfacePasst struct{}

+// PluggedBinding represents a binding implemented in a plugin.
+type PluggedBinding struct {


How about BindingPlugin?

ormergi · 2023-08-15T13:08:07Z

pkg/virt-api/webhooks/validating-webhook/admitters/network.go

@@ -58,3 +58,24 @@ func validateInterfaceStateValue(field *k8sfield.Path, spec *v1.VirtualMachineIn
 	}
 	return causes
 }
+
+func validateInterfaceBinding(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) []metav1.StatusCause {


nit: How about validateInterfaceBindingPlugin?

It will be incorrect. It checks all binding configuration, even the old one.

ormergi · 2023-08-15T13:11:44Z

pkg/virt-controller/services/net_binding_test.go

+				},
+			}
+			sidecars, err := services.BindingPluginSidecarList(vmi, config)
+			if shouldSucceed {


I don't like this pattern because it makes tests a bit harder to follow, I think its worth adding another DescribeTable.

ormergi · 2023-08-15T13:13:26Z

pkg/virt-controller/services/net_binding_test.go

+			}
+		},
+			Entry("VMI has binding plugin but config doesn't exist",
+				libvmi.New(libvmi.WithInterface(v1.Interface{Name: testNetworkName1, Binding: &v1.PluggedBinding{Name: testBindingName1}}),


I think it was raised before, we should not use e2e tests helpers in unit tests, isn't it? (talking about libvmi)

ormergi · 2023-08-15T13:30:37Z

pkg/virt-controller/services/net_binding.go

+				pluginInfo, exist = config.NetworkConfiguration.Binding[iface.Binding.Name]
+			}
+
+			if !exist {


Please consider adding validation for adding binding plugin to Kubevirt CR, at least that the image is not empty.
Running pods with containers who has empty image should fail.

It may also reduce some error checking like this one if its validated at the API level.

That is not recomended, we are not validating the KV CR and we should not add such dependencies in the webhook for inter components dependencies.

ormergi · 2023-08-15T13:32:30Z

pkg/virt-controller/services/template.go

@@ -359,6 +359,12 @@ func (t *templateService) renderLaunchManifest(vmi *v1.VirtualMachineInstance, i
 		return nil, err
 	}

+	bindingSidecars, err := BindingPluginSidecarList(vmi, t.clusterConfig.GetConfig())
+	requestedHookSidecarList = append(requestedHookSidecarList, bindingSidecars...)


Good catch!

EdDev

Very nice, thanks!

EdDev · 2023-08-16T09:58:31Z

pkg/virt-controller/services/net_binding.go

+				pluginInfo, exist = config.NetworkConfiguration.Binding[iface.Binding.Name]
+			}
+
+			if !exist {


That is not recomended, we are not validating the KV CR and we should not add such dependencies in the webhook for inter components dependencies.

EdDev · 2023-08-16T10:09:04Z

pkg/network/setup/network.go

-		// SR-IOV devices are not part of the phases
-		if iface.SRIOV != nil {
+		// Binding plugin and SR-IOV devices are not part of the phases
+		if iface.Binding != nil || iface.SRIOV != nil {


This may not work well when we'll need to work with tap devices (or other future exceptions).
But these thoughts are futuristic, for now this seem fine.

xpivarc · 2023-08-16T13:54:28Z

FYI @kvaps

EdDev · 2023-08-16T13:59:27Z

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

@@ -1201,6 +1201,9 @@ type Interface struct {
 	// BindingMethod specifies the method which will be used to connect the interface to the guest.
 	// Defaults to Bridge.
 	InterfaceBindingMethod `json:",inline"`
+	// Binding specifies the binding plugin that will be used to connect the interface to the guest.
+	// It provides an alternative to InterfaceBindingMethod.


Lets try to do our best and declare all the new fields as v1alphav1 in order to warn about using them.

EdDev · 2023-08-16T13:59:34Z

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

@@ -1310,6 +1313,12 @@ type InterfaceMacvtap struct{}
 // InterfacePasst connects to a given network.
 type InterfacePasst struct{}

+// PluginBinding represents a binding implemented in a plugin.
+type PluginBinding struct {
+	// Name references to the binding name as denined in the kubevirt CR.


Lets try to do our best and declare all the new fields as v1alphav1 in order to warn about using them.

EdDev · 2023-08-16T13:59:52Z

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

+
+type InterfaceBindingPlugin struct {
+	// SidecarImage references a container image that runs in the virt-launcher pod.
+	// The sidecar handles (libvirt) domain configuration and optional services.


Lets try to do our best and declare all the new fields as v1alphav1 in order to warn about using them.

EdDev · 2023-08-16T14:00:31Z

/hold

EdDev · 2023-08-16T14:35:33Z

pkg/virt-config/feature-gates.go

@@ -72,6 +72,8 @@ const (
 	VMLiveUpdateFeaturesGate = "VMLiveUpdateFeatures"
 	// When BochsDisplayForEFIGuests is enabled, EFI guests will be started with Bochs display instead of VGA
 	BochsDisplayForEFIGuests = "BochsDisplayForEFIGuests"
+	// BindingPlugingsGate enables using a plugin to bind the pod and the VM network
+	BindingPlugingsGate = "BindingPlugins"


Can you add a Network prefix? It is not clear what binding is referred here.

EdDev · 2023-08-16T14:36:01Z

/hold cancel

EdDev · 2023-08-16T14:43:31Z

Can you also add the FG info in the release notes and the PR description?

The kubevirt CR will contain the sidecar image per each binding. example: piVersion: kubevirt.io/v1 kind: KubeVirt metadata: name: kubevirt namespace: kubevirt spec: configuration: network: binding: bridge: sidecarImage: quay.io/kubevirt/network-bridge-binding Signed-off-by: Alona Paz <alkaplan@redhat.com>

The binding will represent the binding plugin. Signed-off-by: Alona Paz <alkaplan@redhat.com>

A VMI interface can have only one binding method. In case binding plugin is define, no other binding method can exist on this interface. Signed-off-by: Alona Paz <alkaplan@redhat.com>

Signed-off-by: Alona Paz <alkaplan@redhat.com>

Since those parts are done by the plugin (cni or sidecar) it should be skipped in the core code. Signed-off-by: Alona Paz <alkaplan@redhat.com> Signed-off-by: Or Mergi <ormergi@redhat.com>

PreferredInterfaceMasquerade shouldn't be applied on an interface with binding plugin. Signed-off-by: Alona Paz <alkaplan@redhat.com>

Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan · 2023-08-16T17:06:52Z

2 lgtms, raising to approve
/approve

kubevirt-bot · 2023-08-16T17:07:15Z

[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 · 2023-08-16T23:14:59Z

/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 dhiller and iholder101 August 14, 2023 10:20

kubevirt-bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 14, 2023

AlonaKaplan force-pushed the binding_plugin_api branch from e8cec21 to 274224f Compare August 14, 2023 12:27

kubevirt-bot added sig/network release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 14, 2023

AlonaKaplan force-pushed the binding_plugin_api branch from e749d60 to 3ab2e63 Compare August 14, 2023 14:28

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Aug 14, 2023

ormergi mentioned this pull request Aug 14, 2023

Integrate net bind api and slirp hook sidecar #10289

Closed

AlonaKaplan marked this pull request as ready for review August 15, 2023 07:12

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 Aug 15, 2023

kubevirt-bot requested review from 0xFelix and rhrazdil August 15, 2023 07:12

AlonaKaplan force-pushed the binding_plugin_api branch from 3ab2e63 to 14e7fe7 Compare August 15, 2023 09:20

EdDev suggested changes Aug 15, 2023

View reviewed changes

kubevirt-bot assigned EdDev Aug 15, 2023

EdDev reviewed Aug 15, 2023

View reviewed changes

ormergi reviewed Aug 15, 2023

View reviewed changes

AlonaKaplan force-pushed the binding_plugin_api branch from 14e7fe7 to cb7ff78 Compare August 15, 2023 18:43

kubevirt-bot added size/XL and removed size/L labels Aug 15, 2023

AlonaKaplan force-pushed the binding_plugin_api branch from cb7ff78 to aa33891 Compare August 16, 2023 07:48

EdDev approved these changes Aug 16, 2023

View reviewed changes

EdDev reviewed Aug 16, 2023

View reviewed changes

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2023

AlonaKaplan force-pushed the binding_plugin_api branch from 0998c93 to fb1063e Compare August 16, 2023 14:24

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2023

EdDev reviewed Aug 16, 2023

View reviewed changes

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2023

ormergi mentioned this pull request Aug 16, 2023

Slirp hook sidecar #10272

Merged

AlonaKaplan added 2 commits August 16, 2023 18:30

net binding plugin: add "Binding" to VMI interface

b4c7ff6

The binding will represent the binding plugin. Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan force-pushed the binding_plugin_api branch from fb1063e to aef0356 Compare August 16, 2023 15:36

AlonaKaplan added 5 commits August 16, 2023 18:36

net binding plugin: block plugin and embedded binding on the same iface

234467a

A VMI interface can have only one binding method. In case binding plugin is define, no other binding method can exist on this interface. Signed-off-by: Alona Paz <alkaplan@redhat.com>

net binding plugin: Add binding sidecars to virt-launcher

6068a78

Signed-off-by: Alona Paz <alkaplan@redhat.com>

net binding plugin: phase1,2 and domain creation should be skipped

a79137b

Since those parts are done by the plugin (cni or sidecar) it should be skipped in the core code. Signed-off-by: Alona Paz <alkaplan@redhat.com> Signed-off-by: Or Mergi <ormergi@redhat.com>

net binding plugin, instancetype: iface with plugin binding is not unset

82793ae

PreferredInterfaceMasquerade shouldn't be applied on an interface with binding plugin. Signed-off-by: Alona Paz <alkaplan@redhat.com>

net binding plugin: add a feature gate

ba7bbb9

Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan force-pushed the binding_plugin_api branch from aef0356 to ba7bbb9 Compare August 16, 2023 15:36

EdDev approved these changes Aug 16, 2023

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2023

ormergi approved these changes Aug 16, 2023

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2023

kubevirt-bot merged commit 9c14b3e into kubevirt:main Aug 16, 2023
35 of 36 checks passed

alicefr mentioned this pull request Sep 7, 2023

vHostUser interface Design Proposal kubevirt/community#218

Open

ormergi mentioned this pull request Apr 4, 2024

Deviceinfo downwardAPI #11618

Merged

8 tasks

		// Image that runs on virt-launcher pod.
		//It run services and configures the libvirt domain.

Binding plugin API #10284

Binding plugin API #10284

Conversation

AlonaKaplan commented Aug 14, 2023 • edited

kubevirt-bot commented Aug 14, 2023

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

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

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

xpivarc commented Aug 16, 2023

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 commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev commented Aug 16, 2023

EdDev commented Aug 16, 2023

AlonaKaplan commented Aug 16, 2023

kubevirt-bot commented Aug 16, 2023

kubevirt-commenter-bot commented Aug 16, 2023

AlonaKaplan commented Aug 14, 2023 •

edited