docs, vm: Add network binding plugins guide #742

EdDev · 2023-12-03T14:39:59Z

Describe the network binding plugins infrastructure for users.
Includes detailed explanation with examples.

The following binding plugins are included:

Passt
slirp binding plugin

In follow up PRs, the following subject should be covered:

Domain Attachement
macvtap binding plugin

EdDev · 2023-12-03T17:50:58Z

/cc @AlonaKaplan

docs/virtual_machines/network_binding_plugins.md

AlonaKaplan · 2023-12-04T14:11:51Z

docs/virtual_machines/network_binding_plugins.md

+Registration includes the addition of the binding name with all its parameters
+to the Kubevirt CR.
+
+The following parameters are currently supported (as of v1.1.1):


I would add this section before the deployment part as an introduction to the binding plugin parts.

I would mention that the binding plugin tries to give a pluggable way to implement each part of the network binding.
plugin CNI -> Pod network configuration.
sidecar or core domain attachment -> Domain vNIC configuration.
sidecar -> Services to deliver network details to the guest (optional).

I was thinking that we first need to install (deploy) everything before we can register it for usage. Swapping the order is a bit odd.

I would mention that the binding plugin tries to give a pluggable way to implement each part of the network binding.

The intro about the binding plugin is given in the begging of the document, should we add something that is missing there?

plugin CNI -> Pod network configuration.

This is expressed on line 60, I am confused on what would you want to change.

sidecar or core domain attachment -> Domain vNIC configuration.
sidecar -> Services to deliver network details to the guest (optional).

I am not sure where I need to change something.

I wanted the supported parameters to be also in the Network Binding introduction part. But now I re-read, it is fine to keep it here.
But I would expect a more detailed explanation about each one of the parameters.
For example networkAttachmentDefinition: Pointing to a namespace and object name. - what is this NAD. It is the NAD of what CNI? When this CNI will be invoked? What happen if it is empty? What are the special requirements when setting the NAD here? (multus).

Also, in the introduction part you are talking about the binding parts -

The network binding includes: - Domain vNIC configuration. - Pod network configuration (optional). - Services to deliver network details to the guest (optional).

I would add here to which part each parameter is related.

docs/virtual_machines/network_binding_plugins.md

AlonaKaplan · 2023-12-04T17:29:37Z

docs/virtual_machines/network_binding_plugins.md

+            "binding": {
+                "passt": {
+                    "networkAttachmentDefinition": "default/netbindingpasst",
+                    "sidecarImage": "registry:5000/kubevirt/network-passt-binding:devel"


same here, please use a place holder. "registry:5000" feels wrong here.

AlonaKaplan · 2023-12-04T17:32:02Z

docs/virtual_machines/network_binding_plugins.md

+      terminationGracePeriodSeconds: 0
+      volumes:
+      - containerDisk:
+          image: registry:5000/kubevirt/fedora-with-test-tooling-container-disk:devel


Please don't use registry:5000. Maybe docker.io/kubevirt/fedora-cloud-container-disk-demo:latestas used in the main interface doc.

AlonaKaplan · 2023-12-04T17:32:48Z

docs/virtual_machines/network_binding_plugins.md

+      terminationGracePeriodSeconds: 0
+      volumes:
+      - containerDisk:
+          image: registry:5000/kubevirt/fedora-with-test-tooling-container-disk:devel


Please remove the registry:5000.

AlonaKaplan · 2023-12-04T17:33:43Z

docs/virtual_machines/network_binding_plugins.md

+            "binding": {
+                "passt": {
+                    "networkAttachmentDefinition": "default/netbindingpasst",
+                    "sidecarImage": "registry:5000/kubevirt/network-passt-binding:devel"


Please remove the registry:5000.

AlonaKaplan · 2023-12-06T10:41:55Z

docs/virtual_machines/network_binding_plugins.md

+Registration includes the addition of the binding name with all its parameters
+to the Kubevirt CR.
+
+The following parameters are currently supported (as of v1.1.1):


I wanted the supported parameters to be also in the Network Binding introduction part. But now I re-read, it is fine to keep it here.
But I would expect a more detailed explanation about each one of the parameters.
For example networkAttachmentDefinition: Pointing to a namespace and object name. - what is this NAD. It is the NAD of what CNI? When this CNI will be invoked? What happen if it is empty? What are the special requirements when setting the NAD here? (multus).

Also, in the introduction part you are talking about the binding parts -

The network binding includes: - Domain vNIC configuration. - Pod network configuration (optional). - Services to deliver network details to the guest (optional).

I would add here to which part each parameter is related.

AlonaKaplan · 2023-12-06T11:02:59Z

docs/virtual_machines/network_binding_plugins.md

+          - name: slirpnet
+            binding:
+              name: slirp
+            ports:


I would removed the ports from all the examples.

EdDev · 2023-12-07T11:51:46Z

docs/virtual_machines/network_binding_plugins.md

+Registration includes the addition of the binding name with all its parameters
+to the Kubevirt CR.
+
+The following parameters are currently supported (as of v1.1.1):


EdDev · 2023-12-07T11:54:12Z

docs/virtual_machines/network_binding_plugins.md

+          - name: slirpnet
+            binding:
+              name: slirp
+            ports:


EdDev · 2023-12-07T11:55:02Z

docs/virtual_machines/network_binding_plugins.md

+        }}]'
+```
+
+### VM Passt Network Interface


EdDev · 2023-12-07T11:56:28Z

change: Answered review comments.

AlonaKaplan · 2023-12-07T13:12:41Z

docs/virtual_machines/network_binding_plugins.md

+  that defines the CNI plugin and the configuration the binding plugin uses.
+  Used when the binding plugin needs to change the pod network namespace.
+- sidecarImage: Specify a container image in a registry.
+  Used when the binding plugin needs to configure the domain vNIC configuration


configure/modify

I meant both should be used "configure/modify".
In no domain attachment is used then the sidecars configures the domain vnic.
If it is used, then it modifies.

docs/virtual_machines/network_binding_plugins.md

AlonaKaplan · 2023-12-07T13:16:39Z

docs/virtual_machines/network_binding_plugins.md

+          - name: passtnet
+            binding:
+              name: passt
+            ports:


please remove the ports.

This is the passt specific example, I think we want to express it is supported. No?

If we want to express it, we need a separate section that explains it.

I do not think it exists at the moment.
I suggest to add it, perhaps for other supported fields in a follow up.

AlonaKaplan · 2023-12-07T13:18:07Z

docs/virtual_machines/network_binding_plugins.md

+to the Kubevirt CR.
+
+The following (optional) parameters are currently supported (as of v1.1.1):
+- networkAttachmentDefinition: Use the <namespace/object-name> format to specify


Please mention it requires multus.

I have added it to the deployment section where it is more relevant.

EdDev · 2023-12-10T07:57:28Z

change: Answered review comments.

AlonaKaplan · 2023-12-10T10:31:38Z

docs/virtual_machines/network_binding_plugins.md

+
+## Passt network binding plugin
+
+[Plug A Simple Socket Transport](https://passt.top/passt/about/) is an enhanced


Just wondering, if this is replacing the original passt impl. Does it replace the original doc as well?
If so, we need to copy its content here. e.g comparison between binding, restrictions, etc.

EdDev · 2023-12-10T10:42:20Z

TODO:

Copy passt explanation from the current interfaces-and-networks section.
Structure files (file for binding plugins, and individual ones for each plugin).
Mention passt deprecation.

EdDev · 2023-12-13T16:16:38Z

change: Completed the action items per the previous review.

* Copy `passt` explanation from the current interfaces-and-networks section.

* Structure files (file for binding plugins, and individual ones for each plugin).

* Mention passt deprecation.

phoracek · 2023-12-13T16:24:48Z

docs/virtual_machines/.pages

@@ -10,6 +10,7 @@ nav:
  - numa.md
  - disks_and_volumes.md
  - interfaces_and_networks.md
+  - Network Binding Plugins: net_binding_plugins


Please move it to the end of the menu. It is now breaking the otherwise flat menu in half:

I think that passt and slirp should be in the tree.

phoracek · 2023-12-13T16:25:00Z

docs/virtual_machines/net_binding_plugins/.pages

@@ -0,0 +1,2 @@
+nav:
+  - network_binding_plugins.md


Add the two plugins to the list so they appear in the menu

They are accessible from the main one. Exposing the list from the main menu is a bit "heavy" IMO.
But if you think it looks better, I do not mind.

I think it would be better for visibility. But if you'd rather keep them hidden from the menu, then we should change the configuration so "Network Binding Plugins" does not appear as its own section. If you decide for the latter, then I think you would need to go back to the previous version without .pages in the subdirectory and use net_binding_plugins/passt.md as the link (can't find a doc reference to confirm that that works ATM)

then I think you would need to go back to the previous version without .pages in the subdirectory and use net_binding_plugins/passt.md as the link (can't find a doc reference to confirm that that works ATM)

That is what I had and it did not work.

The one I saw you had it set to

have you tried setting it without the leading ./?

I tried that, it did not work.
I ended up with the main file outside and all the specific plugins in the folder.
Hope this is ok with you.

Done.

phoracek · 2023-12-13T16:27:45Z

docs/virtual_machines/net_binding_plugins/network_binding_plugins.md

+needs.
+
+These layers include:
+- Host connectivity: Network provider.


You must add an empty line above each list. Check the rendered page to make sure that everything shows up as expected

Ahh... it is not like the markdown rendering.

Describe the network binding plugins infrastructure for users. Includes detailed explanation with examples. Signed-off-by: Edward Haas <edwardh@redhat.com>

Signed-off-by: Edward Haas <edwardh@redhat.com>

AlonaKaplan · 2023-12-14T12:18:29Z

/approve

Although I think that passt and slirp bindings should be in the tree and not just links in the main doc.

kubevirt-bot · 2023-12-14T12:18:34Z

[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

AlonaKaplan · 2023-12-17T11:08:10Z

/lgtm
all Petr's comment were fixed.

aburdenthehand · 2024-05-02T09:24:00Z

docs/virtual_machines/net_binding_plugins/passt.md

+- Passt CNI plugin.
+- Sidecar image.
+
+As described in the [definition & flow](#definition--flow) section,


@EdDev What is this meant to link to? I can't seem to find a related heading in any of our docs

It is here: https://github.com/kubevirt/user-guide/pull/742/files#diff-954edce67fbabb116f75540df31fbd8ceeb7de6a02cebd186d92880c33563a69R49

I'll send a fix, thanks.

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Dec 3, 2023

kubevirt-bot requested review from cwilkers and davidvossel December 3, 2023 14:40

kubevirt-bot added the size/L label Dec 3, 2023

EdDev force-pushed the network-binding-plugin branch from 584be84 to e84a5d5 Compare December 3, 2023 14:43

kubevirt-bot requested a review from AlonaKaplan December 3, 2023 17:51

AlonaKaplan reviewed Dec 4, 2023

View reviewed changes

EdDev force-pushed the network-binding-plugin branch from e86b49e to 9f5fc78 Compare December 5, 2023 14:29

kubevirt-bot added size/XL and removed size/L labels Dec 5, 2023

AlonaKaplan reviewed Dec 6, 2023

View reviewed changes

EdDev commented Dec 7, 2023

View reviewed changes

EdDev force-pushed the network-binding-plugin branch from 9f5fc78 to de863d6 Compare December 7, 2023 11:55

AlonaKaplan reviewed Dec 7, 2023

View reviewed changes

EdDev force-pushed the network-binding-plugin branch from de863d6 to eaedc97 Compare December 10, 2023 07:56

AlonaKaplan reviewed Dec 10, 2023

View reviewed changes

EdDev force-pushed the network-binding-plugin branch 2 times, most recently from 9152fa2 to 283236b Compare December 13, 2023 16:14

phoracek reviewed Dec 13, 2023

View reviewed changes

EdDev force-pushed the network-binding-plugin branch 3 times, most recently from 666e16e to fbd664d Compare December 13, 2023 18:57

EdDev force-pushed the network-binding-plugin branch from fbd664d to 3df9154 Compare December 13, 2023 19:02

EdDev added 3 commits December 13, 2023 21:07

docs, vm: Add network binding plugins guide

0aa2880

Describe the network binding plugins infrastructure for users. Includes detailed explanation with examples. Signed-off-by: Edward Haas <edwardh@redhat.com>

docs, vm, network-binding-plugins: Add passt network binding

5ac2d31

Signed-off-by: Edward Haas <edwardh@redhat.com>

docs, vm, network: Adjust Slirp to appear in the binding plugin file

2dac2a6

Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev force-pushed the network-binding-plugin branch from 3df9154 to 2dac2a6 Compare December 13, 2023 19:11

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

kubevirt-bot assigned AlonaKaplan Dec 17, 2023

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

kubevirt-bot merged commit 2d5d6cf into kubevirt:main Dec 17, 2023
6 checks passed

aburdenthehand reviewed May 2, 2024

View reviewed changes


		## Passt network binding plugin

		[Plug A Simple Socket Transport](https://passt.top/passt/about/) is an enhanced

docs, vm: Add network binding plugins guide #742

docs, vm: Add network binding plugins guide #742

Conversation

EdDev commented Dec 3, 2023 • edited Loading

EdDev commented Dec 3, 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

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 commented Dec 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev commented Dec 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev commented Dec 10, 2023

EdDev commented Dec 13, 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

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 Dec 14, 2023

kubevirt-bot commented Dec 14, 2023

AlonaKaplan commented Dec 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev commented Dec 3, 2023 •

edited

Loading