network, binding: Declare the SLIRP core binding as deprecated and remove it #11701

EdDev · 2024-04-15T05:38:03Z

What this PR does

SLIRP binding has been part of Kubevirt from the early days.
With the introduction of the masquerade binding, SLIRP gradually lost interest due to the performance penalty it comes with.

Nowadays, Passt binding plugin [1] is considered an enhanced alternative to SLIRP and the closest in functionality (user-space connectivity).

The current SLIRP core binding [2] has a backend implementation that uses the SLIRP network binding plugin [3].

We have not encounter actual users who work with SLIRP binding for years now.
In addition, support for the binding is minimum (or nonexistence in practice).

Therefore, it seems reasonable to deprecate and remove SLIRP support.

In case there are users who still require SLIRP, the following mitigation exists:

Existing running VMs are not to be effected and should continue running.
New VMs should use the alternatives: The masquerade binding [4], passt binding plugin [1] or the (less recommended) slirp binding plugin [3].

This change renames slirp fields by prefixing Deprecated to their names but without changing their json naming.
Then, it removed the logic of using the binding plugin backend to define a core SLIRP binding, including the definition of default SLIRP interfaces.
Logic is also removed from virt-launcher in regards to SLIRP binding.

The existing validation admitters and virt-handler SLIRP related logic is preserved to allow existing workloads to continue operating.

The migration scenario is nonexistent for SLIRP core binding because Kubevirt never allowed/supported it.
The migration availability is marked using a condition as not available [5] and therefore migration requests will fail.

[1] https://kubevirt.io/user-guide/virtual_machines/net_binding_plugins/passt/
[2] https://kubevirt.io/user-guide/virtual_machines/interfaces_and_networks/#slirp
[3] https://kubevirt.io/user-guide/virtual_machines/net_binding_plugins/slirp/#slirp-network-binding-plugin
[4] https://kubevirt.io/user-guide/virtual_machines/interfaces_and_networks/#masquerade
[5]

kubevirt/pkg/network/vmispec/interface.go

Line 57 in 6fd6313

    
           func VerifyVMIMigratable(vmi *v1.VirtualMachineInstance, bindingPlugins map[string]v1.InterfaceBindingPlugin) error {

Fixes #

Links to places where the discussion took place:
The deprecation and removal of the SLIRP core binding has been discussed and communicated through
the mailing list [5].

[5] https://groups.google.com/g/kubevirt-dev/c/Dp3GVw-Y-K8

Special notes for your reviewer

This change requires wide approver agreement due to its stable API backward incompatibility effect.

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

The SLIRP core binding is deprecated and removed.

kubevirt-bot · 2024-04-15T05:38:05Z

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 · 2024-04-15T05:49:04Z

/test pull-kubevirt-code-lint
/test pull-kubevirt-client-python
/test pull-kubevirt-fossa
/test pull-kubevirt-generate
/test pull-kubevirt-manifest
/test pull-kubevirt-prom-rules-verify
/test pull-kubevirt-unit-test
/test pull-kubevirt-verify-go-mod

EdDev · 2024-04-15T06:34:06Z

/test pull-kubevirt-manifests

EdDev · 2024-04-15T07:23:12Z

/test pull-kubevirt-e2e-k8s-1.29-sig-network

EdDev · 2024-04-15T08:52:27Z

Add the same validation to fail migration on the target (at the virt-controller).

@AlonaKaplan commented that migration with Slirp is not supported anyway, therefore we just need to confirm it is enough to block it.

EdDev · 2024-04-15T20:15:34Z

/test pull-kubevirt-code-lint
/test pull-kubevirt-client-python
/test pull-kubevirt-fossa
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-prom-rules-verify
/test pull-kubevirt-unit-test
/test pull-kubevirt-verify-go-mod

EdDev · 2024-04-15T21:34:08Z

/test pull-kubevirt-unit-test

EdDev · 2024-04-15T21:34:49Z

/test pull-kubevirt-e2e-k8s-1.29-sig-network

EdDev · 2024-04-16T03:36:13Z

/test all

ormergi

Great work! looks good to me overall.

Regarding virt-controller: Add network validation for the VM and VMI controllers commit message these a typo at first line Ad -> 'Add'.

Could you please refer to the following migration scenarios (at least in PR description):

What will happen there is a migration request by the user to slirp VMs?
On post Kubevirt upgrade, what will happen to old slirp VMs?
In both scenarios, will virt-handler / controller will re-enqueuing syncVMI routine due to existing slirp interface?

EdDev · 2024-04-18T19:25:18Z

Great work! looks good to me overall.

Regarding virt-controller: Add network validation for the VM and VMI controllers commit message these a typo at first line Ad -> 'Add'.

Done.

Could you please refer to the following migration scenarios (at least in PR description):

What will happen there is a migration request by the user to slirp VMs?

I'll add to the description the migration scenario. The migration is blocked because SLIRP never supported migration.

On post Kubevirt upgrade, what will happen to old slirp VMs?

I think I did: The existing validation admitters and virt-handler SLIRP related logic is preserved to allow existing workloads to continue operating.

In both scenarios, will virt-handler / controller will re-enqueuing syncVMI routine due to existing slirp interface?

Migration is not supported for SLIRP, it is blocked today.
Existing running VMIs will continue to work as usual, see quote from above.

xpivarc · 2024-05-09T13:23:41Z

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

@@ -195,7 +195,7 @@ func DefaultSlirpNetworkInterface() *Interface {
 	iface := &Interface{
 		Name: "default",
 		InterfaceBindingMethod: InterfaceBindingMethod{
-			Slirp: &InterfaceSlirp{},
+			DeprecatedSlirp: &DeprecatedInterfaceSlirp{},


Should the DefaultSlirpNetworkInterface be removed?

Yes, it should be removed as well. Thanks.

EdDev · 2024-05-09T17:49:45Z

change: Rebase

EdDev · 2024-05-09T18:58:07Z

change: Ran make generate again on the 1st commit.

EdDev · 2024-05-09T19:38:37Z

change: Removed DefaultSlirpNetworkInterface

EdDev · 2024-05-10T03:30:36Z

/test pull-kubevirt-unit-test

AlonaKaplan · 2024-05-10T04:51:32Z

/lgtm

kubevirt-commenter-bot · 2024-05-10T04:51:38Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/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
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

xpivarc · 2024-05-10T07:48:08Z

pkg/virt-config/configuration_test.go

-					NetworkInterface:                  string(v1.SlirpInterface),
-					PermitSlirpInterface:              pointer.P(true),
+					NetworkInterface:                  string(v1.DeprecatedSlirpInterface),
+					DeprecatedPermitSlirpInterface:    pointer.P(true),


Should the slirp be allowed to be default?

On upgrade the KV spec does not change.
There is no restriction imposed to set KV in order not to break existing workloads or the upgrade itself.

New VM/VMI will not accept this configuration and the Admin will need to remove Slirp from the KV spec after the upgrade in order to avoid having Slirp.

On upgrade the KV spec does not change.
There is no restriction imposed to set KV in order not to break existing workloads or the upgrade itself.

I don't think we would break compatibility, the config is internal, it would just drop to default. I am not sure how this works with other bindings....

New VM/VMI will not accept this configuration and the Admin will need to remove Slirp from the KV spec after the upgrade in order to avoid having Slirp.

Does this means that new VM/I will be denied or that they will no use the config?

On upgrade the KV spec does not change.
There is no restriction imposed to set KV in order not to break existing workloads or the upgrade itself.

I don't think we would break compatibility, the config is internal, it would just drop to default. I am not sure how this works with other bindings....

Internal? The Kubevirt CR is not internal.
Anything there will remain as is and it will be reflected when creating new VM/VMI.
If the configuration there is not supported, it will fail validation.

New VM/VMI will not accept this configuration and the Admin will need to remove Slirp from the KV spec after the upgrade in order to avoid having Slirp.

Does this means that new VM/I will be denied or that they will no use the config?

New VM/VMI are denied if the configuration is not supported (i.e. set Slirp interface based on the Kubevirt CR spec). Existing VM/VMI are not affected.
This is no different from defining Slirp explicitly on the VM/VMI.

Internal? The Kubevirt CR is not internal.

Yes but I am commenting about virt-config that is either from configmap or kubevirt cr(the latter lately...). It also has an internal validation on these external sources as these sources can be submitted to the system in invalid form. What I was trying to say in the previous comments is that the validation still allows the slirp. This semantically doesn't make much sense to me.

Anything there will remain as is and it will be reflected when creating new VM/VMI.
If the configuration there is not supported, it will fail validation.

This doesn't sounds better than falling to default binding

Yes but I am commenting about virt-config that is either from configmap or kubevirt cr(the latter lately...). It also has an internal validation on these external sources as these sources can be submitted to the system in invalid form. What I was trying to say in the previous comments is that the validation still allows the slirp. This semantically doesn't make much sense to me.

The validation of VM/VMI to reject Slirp is limited to creation and not updates.
The config is used to calculate defaults also for updates, which should continue passing.

Therefore, I do not think it will be correct to validate the config and reject Slirp at this level.
VM/VMI that are running should not get rejected.

If you still think it is possible to both reject Slirp here and keep existing workloads running (and not fail validation on update), we can improve as a follow up.

Anything there will remain as is and it will be reflected when creating new VM/VMI.
If the configuration there is not supported, it will fail validation.

This doesn't sounds better than falling to default binding

It is not better, it is a limitation due to the requirement to support existing VMs running even when their manifest is updated (and therefore validated).

Existing Go structs and members names are prefixed with `Deprecated` on their names and added documentation comments to mark them as deprecated. The JSON naming has not changed, keeping API fields intact. Signed-off-by: Edward Haas <edwardh@redhat.com>

Given Kubevirt CR configuration to enable SLIRP and allow it to be set by default (in case no network interface is defined), return with an error. The default setting is performed on a mutating admitter webhook for both VM and VMI CRs. In addition, the defaulting is executed when the VM controller prepares a VMI for creation. We would expect the failure to be raised at the admitter, failing immediately the CR creation. However, even if it will fail at the VM controller, the outcome will be somehow similar: VMI will not be created, i.e. the VM will not be able to start. Signed-off-by: Edward Haas <edwardh@redhat.com>

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

Processing of SLIRP core binding is removed with care to preserve existing running VM/VMIs. New VM/VMIs which are configured to use the SLIRP core binding will fail to setup the vNIC. A follow up change will block such a spec at the creation validation admitter to assure early and stable failure for new VM/VMIs. Signed-off-by: Edward Haas <edwardh@redhat.com>

Validate at the validation admitters that new VM/VMI is not created with the SLIRP core binding. It has been added for both the VM and VMI admitters. Limited to the CREATE operation. Signed-off-by: Edward Haas <edwardh@redhat.com>

Add a network validation point at the: - VM controller, before creating a VMI. - VMI controller, before creating a Pod. The network validators, together with the create validator admitters assure no new VM/VMI is created with the SLIRP core binding. The reason to add the validation to the controllers in addition to the existing validation admitters is the async upgrade path of Kubevirt components. There may be a gap between the upgrade of the virt-controller and the virt-api (which holds the admitters). We need to assure that in the gap no new VM/VMI is created with a SLIRP core binding interface. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev · 2024-05-10T09:14:28Z

change: Rebase (again)

ormergi · 2024-05-12T06:27:56Z

/lgtm

kubevirt-commenter-bot · 2024-05-12T06:28:00Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/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
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

EdDev · 2024-05-12T10:55:13Z

/test pull-kubevirt-e2e-k8s-1.29-sig-performance

kubevirt-bot requested review from AlonaKaplan, kbidarkar, ormergi, rthallisey and xpivarc April 15, 2024 05:38

EdDev marked this pull request as ready for review April 17, 2024 00:21

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 17, 2024

kubevirt-bot requested review from enp0s3 and RamLavi April 17, 2024 00:21

ormergi reviewed Apr 18, 2024

View reviewed changes

EdDev force-pushed the remove-slirp branch 2 times, most recently from 8fc69fa to 539b237 Compare April 18, 2024 19:41

xpivarc reviewed May 9, 2024

View reviewed changes

EdDev force-pushed the remove-slirp branch from 5673edb to bd80c17 Compare May 9, 2024 17:48

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

EdDev force-pushed the remove-slirp branch from bd80c17 to 70dd539 Compare May 9, 2024 18:57

EdDev force-pushed the remove-slirp branch from 70dd539 to 9607978 Compare May 9, 2024 19:37

kubevirt-bot assigned AlonaKaplan May 10, 2024

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

xpivarc reviewed May 10, 2024

View reviewed changes

EdDev added 7 commits May 10, 2024 11:36

api, defaults: Remove DefaultSlirpNetworkInterface

2fc8445

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

examples: Remove vmi-slirp example

545c3d2

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

EdDev force-pushed the remove-slirp branch from 9607978 to 41e701a Compare May 10, 2024 09:14

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label May 10, 2024

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

kubevirt-bot merged commit e6207f2 into kubevirt:main May 12, 2024
38 checks passed

EdDev deleted the remove-slirp branch May 12, 2024 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network, binding: Declare the SLIRP core binding as deprecated and remove it #11701

network, binding: Declare the SLIRP core binding as deprecated and remove it #11701

network, binding: Declare the SLIRP core binding as deprecated and remove it #11701

network, binding: Declare the SLIRP core binding as deprecated and remove it #11701

Conversation

What this PR does

Special notes for your reviewer

Checklist

Release note

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