network: Discontinue (and remove) the passt network core binding #11915

ormergi · 2024-05-15T06:34:46Z

What this PR does

This PR is the next implemention phase of https://github.com/kubevirt/community/blob/main/design-proposals/passt-impl-iface-api-replacment.md

passt [1] network core binding has been marked for deprecation in v1.2 [2] and per the original phase plan it is now discontinued towards v1.3.

Users who would like to continue using the binding are encouraged to use the passt network binding plugin [3].

The passt network core binding did not reach graduation (GA), therefore its removal does not require any special exception nor commitment to preserve the functionality of existing running VMs.
However, in order to ease the upgrade transition for users who are using this network binding, the discontinuation implementation is preserving existing workloads (VMs) running without disruption.

Once the cluster us upgraded, new VMs or migrated VMs will not be able to run with the passt network core binding.

Future releases may drop this soft transition and do not commit to keep such old VMs running.

[1] https://kubevirt.io/user-guide/virtual_machines/interfaces_and_networks/#passt
[2] #10866
[3] https://kubevirt.io/user-guide/virtual_machines/net_binding_plugins/passt

Before this PR:

After this PR:

Fixes #

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

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Notification for the deprecation and removal has been communicated through the mailing list and v1.2 release, where warnings have been posted to users when passt configuration was detected.
This final removal is following with the original plan.

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 'passt' core network binding is discontinued and removed.

kubevirt-bot · 2024-05-15T06:34:48Z

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

ormergi · 2024-05-28T14:45:32Z

You could use a smaller version in this PR if you prefer not to rebase over that one.

Done
Changes: The first commit of #11968 is cherry-picked, the raise warning test when deprecated API is used were changed accordingly.
Please take another look

EdDev

There are a few places that the changes have been missed. Maybe a rebase drop them. See inline comments.

virt-conf, admitter: Register deprecated feature-gate in tests

The raise warning is when deprecated API is used tests depends on a feature-gate
in deprecated state.

Can you please rephrase the commit message?

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

pkg/network/setup/netpod/netpod.go

EdDev · 2024-05-29T05:58:28Z

pkg/network/setup/netpod/netpod_test.go

-		Expect(nmstatestub.spec).To(Equal(
-			nmstate.Spec{
-				Interfaces: []nmstate.Interface{},
-				LinuxStack: nmstate.LinuxStack{IPv4: nmstate.LinuxStackIP4{
-					PingGroupRange:        []int{107, 107},
-					UnprivilegedPortStart: pointer.P(0),
-				}},
-			},
-		))


I see no change.

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

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go

Following passt core binding deprecation example, 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: Or Mergi <ormergi@redhat.com>

ormergi

@EdDev
I fixed all comments, some changes got reveted by rebase it is fixed now, sorry for the mess.
Note that the passt e2e tests is removed and the last commit message is fixed.
Please take another look

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

pkg/network/setup/netpod/netpod.go

ormergi · 2024-05-29T10:02:34Z

pkg/network/setup/netpod/netpod_test.go

-		Expect(nmstatestub.spec).To(Equal(
-			nmstate.Spec{
-				Interfaces: []nmstate.Interface{},
-				LinuxStack: nmstate.LinuxStack{IPv4: nmstate.LinuxStackIP4{
-					PingGroupRange:        []int{107, 107},
-					UnprivilegedPortStart: pointer.P(0),
-				}},
-			},
-		))


Fixed, the relevant production code is removed, following this thread e81b874#r1611582617

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

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go

EdDev · 2024-05-29T13:20:47Z

pkg/network/setup/netpod/netpod.go

-			spec.LinuxStack.IPv4.PingGroupRange = []int{107, 107}
-			spec.LinuxStack.IPv4.UnprivilegedPortStart = pointer.P(0)


This looks wrong. We are supposed to keep the reconcile intact so existing running VMs will not be affected.

From what I have tested with passt we can remove the remaining and its not causing disruptions.
From now I am reverting this change.
Hopefully we can get rid of the remaining in a follow up PR.

Done.

EdDev · 2024-05-29T13:22:25Z

pkg/network/setup/netpod/netpod_test.go

-		Expect(nmstatestub.spec).To(Equal(
-			nmstate.Spec{
-				Interfaces: []nmstate.Interface{},
-				LinuxStack: nmstate.LinuxStack{IPv4: nmstate.LinuxStackIP4{
-					PingGroupRange:        []int{107, 107},
-					UnprivilegedPortStart: pointer.P(0),
-				}},
-			},
-		))


I think you should not touch the reconcile loop.
Have we discussed changing the reconcile of the virt-handler anywhere and concluded that it can be changed?

tests/network/vmi_passt.go

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

pkg/virt-config/deprecation/validator_test.go

Processing of passt core binding is removed in a way existing running VM/VMIs are not disrupted. New VM/VMIs which are configured to use the passt 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: Or Mergi <ormergi@redhat.com>

The function is used in one place only, inline the function where it used. Signed-off-by: Or Mergi <ormergi@redhat.com>

Signed-off-by: Or Mergi <ormergi@redhat.com>

A static FG list has been maintained, defining FGs directly into the list upon definition. This approach has several disadvantages: - There is no strict control on adding a new FG. E.g. an addition of two FGs with the same name is possible. - FG have to be defined in a central place and cannot be distributed to the location that is owned by a feature. Therefore, the current implementation is refactored to use a registration function to add FGs in a controlled manner. Duplications is avoided by using a map instead of a list. When `RegisterFeatureGate` is called, the inputted FG is either added to the FG list or overriding an existing FG entry. Signed-off-by: Edward Haas <edwardh@redhat.com>

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

The "raise warning when deprecated API is used" tests depends on feature-gate that is in deprecated state. Since there is no longer a feature-gate in deprecated state we can use in tests, register a fake deprecated feature-gate at the relevant tests. Signed-off-by: Or Mergi <ormergi@redhat.com>

The validator tests depends on feature-gate that is in deprecated state. Since there is no longer a feature-gate in deprecated state we can use in tests, register fake deprecated feature-gate according to each test. Signed-off-by: Or Mergi <ormergi@redhat.com>

EdDev

Thank you.

/approve

@enp0s3 , could you please help out raise the approve for the areas that are not under sig-network?
This change follows the same format as the macvtap removal.

ormergi · 2024-06-03T08:27:42Z

@enp0s3 could you please have a look at the areas that are not part of sig-net? 🙂

enp0s3 · 2024-06-05T10:24:45Z

/approve
@ormergi Thank you!

kubevirt-bot · 2024-06-05T10:24:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EdDev, enp0s3

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~~ [enp0s3]

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-06-05T10:24:55Z

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

dhiller · 2024-06-05T13:08:09Z

/override pull-kubevirt-e2e-k8s-1.29-sig-operator

/hold for virt folks to validate

FYI @acardace @fossedihelm

kubevirt-bot · 2024-06-05T13:08:14Z

@dhiller: Overrode contexts on behalf of dhiller: pull-kubevirt-e2e-k8s-1.29-sig-operator

In response to this:

/override pull-kubevirt-e2e-k8s-1.29-sig-operator

/hold for virt folks to validate

FYI @acardace @fossedihelm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

acardace · 2024-06-05T13:14:53Z

/unhold

Just the usual failures we'll be addressing soon when releasing v1.2.2.

kubevirt-bot requested review from AlonaKaplan, EdDev and RamLavi May 15, 2024 06:35

kubevirt-bot added the sig/buildsystem Denotes an issue or PR that relates to changes in the build system. label May 15, 2024

ormergi force-pushed the discontinue-passt branch 2 times, most recently from 582441e to 1113505 Compare May 15, 2024 07:01

ormergi force-pushed the discontinue-passt branch from 1113505 to 0b4e976 Compare May 22, 2024 16:55

kubevirt-bot added size/L and removed size/XL needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 22, 2024

ormergi force-pushed the discontinue-passt branch 4 times, most recently from 85fe507 to 34f31e7 Compare May 23, 2024 07:54

kubevirt-bot added size/XL and removed size/L labels May 23, 2024

ormergi changed the title ~~[WIP] network: Discontinue (and remove) the passt network core binding~~ network: Discontinue (and remove) the passt network core binding May 23, 2024

ormergi marked this pull request as ready for review May 23, 2024 08:03

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 May 23, 2024

kubevirt-bot requested a review from mhenriks May 23, 2024 08:03

ormergi force-pushed the discontinue-passt branch from d7add73 to b72684b Compare May 28, 2024 14:42

ormergi mentioned this pull request May 28, 2024

virt-config, feature-gates: Control the addition of FGs to the FG list #11968

Open

8 tasks

EdDev suggested changes May 29, 2024

View reviewed changes

ormergi force-pushed the discontinue-passt branch from b72684b to 632b83b Compare May 29, 2024 10:54

ormergi commented May 29, 2024

View reviewed changes

EdDev suggested changes May 29, 2024

View reviewed changes

ormergi force-pushed the discontinue-passt branch from 8392ff4 to 0f06329 Compare May 29, 2024 14:17

ormergi and others added 4 commits May 29, 2024 18:13

pkg/util: Remove IsPasstVMI

e1db193

The function is used in one place only, inline the function where it used. Signed-off-by: Or Mergi <ormergi@redhat.com>

virt-config, featue-gates: Discontinue passt FG

01a447d

Signed-off-by: Or Mergi <ormergi@redhat.com>

ormergi force-pushed the discontinue-passt branch from 0f06329 to 053f8c3 Compare May 29, 2024 15:15

EdDev reviewed May 29, 2024

View reviewed changes

pkg/virt-api/webhooks/validating-webhook/admitters/admitters_suite_test.go Outdated Show resolved Hide resolved

ormergi added 2 commits May 29, 2024 18:54

ormergi force-pushed the discontinue-passt branch from 053f8c3 to 359aef3 Compare May 29, 2024 15:54

EdDev approved these changes May 29, 2024

View reviewed changes

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

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

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2024

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: Discontinue (and remove) the passt network core binding #11915

network: Discontinue (and remove) the passt network core binding #11915

ormergi commented May 15, 2024 •

edited

kubevirt-bot commented May 15, 2024

ormergi commented May 28, 2024

EdDev left a comment

EdDev May 29, 2024

ormergi left a comment

ormergi May 29, 2024

EdDev May 29, 2024

ormergi May 29, 2024

EdDev May 29, 2024

EdDev left a comment

ormergi commented Jun 3, 2024

enp0s3 commented Jun 5, 2024

kubevirt-bot commented Jun 5, 2024

kubevirt-commenter-bot commented Jun 5, 2024

dhiller commented Jun 5, 2024

kubevirt-bot commented Jun 5, 2024

acardace commented Jun 5, 2024

		spec.LinuxStack.IPv4.PingGroupRange = []int{107, 107}
		spec.LinuxStack.IPv4.UnprivilegedPortStart = pointer.P(0)

network: Discontinue (and remove) the passt network core binding #11915

Are you sure you want to change the base?

network: Discontinue (and remove) the passt network core binding #11915

Conversation

ormergi commented May 15, 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 May 15, 2024

ormergi commented May 28, 2024

EdDev left a comment

Choose a reason for hiding this comment

EdDev May 29, 2024

Choose a reason for hiding this comment

ormergi left a comment

Choose a reason for hiding this comment

ormergi May 29, 2024

Choose a reason for hiding this comment

EdDev May 29, 2024

Choose a reason for hiding this comment

ormergi May 29, 2024

Choose a reason for hiding this comment

EdDev May 29, 2024

Choose a reason for hiding this comment

EdDev left a comment

Choose a reason for hiding this comment

ormergi commented Jun 3, 2024

enp0s3 commented Jun 5, 2024

kubevirt-bot commented Jun 5, 2024

kubevirt-commenter-bot commented Jun 5, 2024

dhiller commented Jun 5, 2024

kubevirt-bot commented Jun 5, 2024

acardace commented Jun 5, 2024

ormergi commented May 15, 2024 •

edited