virt-api, admitter: Refactor & move network validation admitters to the network package #11511

EdDev · 2024-03-14T06:32:51Z

What this PR does

Validation admitters related to network spec are moved under the network package in order to decouple them from the other admitters and allow the sig-network to properly own them.

In addition, work on simplifying the validation code and its tests has been included in this effort.

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

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

NONE

kubevirt-bot · 2024-03-14T06:32:54Z

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-03-14T06:34:13Z

/test pull-kubevirt-build
/test pull-kubevirt-unit-test
/test pull-kubevirt-code-lint
/test pull-kubevirt-generate

EdDev · 2024-03-14T10:29:30Z

/test pull-kubevirt-build
/test pull-kubevirt-unit-test
/test pull-kubevirt-code-lint
/test pull-kubevirt-generate

EdDev · 2024-03-14T11:05:25Z

/test pull-kubevirt-build
/test pull-kubevirt-unit-test
/test pull-kubevirt-code-lint
/test pull-kubevirt-generate

EdDev · 2024-03-14T11:49:49Z

/test pull-kubevirt-build
/test pull-kubevirt-unit-test
/test pull-kubevirt-code-lint
/test pull-kubevirt-generate

EdDev · 2024-03-14T12:06:11Z

Flaky unit test unrelated to this change: https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/11511/pull-kubevirt-unit-test/1768243142961664000

/test pull-kubevirt-unit-test

ormergi

Hi Edy, thanks for the PR

I the network related addmiters code is moved to pkg/network, how about keep it under pkg/virt-api/webhooks/validating-webhook/admitters/ but in new dir for network: pkg/virt-api/webhooks/validating-webhook/admitters/network?

Since the code is tightly related to the webhook and slirp/ iface APIs, I think it make more sense leaving it closer to to the consuming package.
It may also encourage other sigs who doesn't aggregate all related code under pkg/ move their related content in similar why to what I suggest.

pkg/network/admitter/slirp.go

EdDev · 2024-03-14T14:06:46Z

Hi Edy, thanks for the PR

I the network related addmiters code is moved to pkg/network, how about keep it under pkg/virt-api/webhooks/validating-webhook/admitters/ but in new dir for network: pkg/virt-api/webhooks/validating-webhook/admitters/network?

Since the code is tightly related to the webhook and slirp/ iface APIs, I think it make more sense leaving it closer to to the consuming package. It may also encourage other sigs who doesn't aggregate all related code under pkg/ move their related content in similar why to what I suggest.

This decision was already made when the admitter package was introduced under the network root package. What change that needs this to be reexamined?

Most of the current network sub-packages are used by many components in Kubevirt, I see no special thing about the admission ones.

EdDev · 2024-03-14T14:27:58Z

change: Fix a bug detected during the review.

ormergi · 2024-03-14T15:02:22Z

This decision was already made when the admitter package was introduced under the network root package. What change that needs this to be reexamined?

I was under the impression this PR introduce addmiter package, I missed that one.

EdDev · 2024-03-19T05:54:00Z

change: Rebase

pkg/network/admitter/slirp.go

AlonaKaplan · 2024-03-19T09:00:38Z

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

@@ -284,6 +284,7 @@ func validateNetworksMatchInterfaces(field *k8sfield.Path, spec *v1.VirtualMachi

 		networkData, networkExists := networkNameMap[iface.Name]

+		causes = append(causes, netadmitter.ValidateSlirpBinding(field, spec, config)...)


The method should be outside of the for loop.

AlonaKaplan · 2024-03-19T09:07:07Z

pkg/network/admitter/slirp.go

+		}
+
+		if net.Pod == nil {
+			causes = append(causes, metav1.StatusCause{


Shouldn't it also support multus network with default true?
Well, if it wasn't supported till now I believe we shouldn't start supporting it now the feature is going to be deprecated.

It was not supported per what I understand.

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

EdDev · 2024-03-25T06:40:50Z

change: Answered a review comment to move out the slirp validation outside the for-loop.

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

Relocate the slirp admitter validations to the network/admitter package. Simplify the implementation and provide clear unit tests to the various checks. Signed-off-by: Edward Haas <edwardh@redhat.com>

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

Use interfaces on the validation function to ease its testing. By using a focused interface, the tests now can use a simple stub instead of a complex fake. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev · 2024-03-25T12:02:35Z

change: Rebase

AlonaKaplan · 2024-03-25T14:21:44Z

/approve

kubevirt-bot · 2024-03-25T14:21: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

RamLavi · 2024-03-26T11:20:50Z

/sig code-quality

RamLavi

/lgtm

RamLavi · 2024-03-26T11:31:21Z

pkg/network/admitter/BUILD.bazel

@@ -15,11 +15,15 @@ go_library(

 go_test(
    name = "go_default_test",
-    srcs = ["admit_test.go"],
+    srcs = [


nit: please consider moving this commit to before the test is moved.

kubevirt-commenter-bot · 2024-03-26T11:46:38Z

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

EdDev · 2024-03-26T12:19:05Z

/test pull-kubevirt-e2e-k8s-1.29-sig-compute-migrations

kubevirt-bot requested review from AlonaKaplan and enp0s3 March 14, 2024 06:33

EdDev force-pushed the admitters-net-move branch from 77154c0 to d01e238 Compare March 14, 2024 10:29

EdDev force-pushed the admitters-net-move branch from d01e238 to 8a458c6 Compare March 14, 2024 11:04

EdDev marked this pull request as ready for review March 14, 2024 12:16

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 Mar 14, 2024

kubevirt-bot requested review from lyarwood and RamLavi March 14, 2024 12:16

ormergi reviewed Mar 14, 2024

View reviewed changes

pkg/network/admitter/slirp.go Outdated Show resolved Hide resolved

EdDev force-pushed the admitters-net-move branch from ac9d8b9 to 99a546d Compare March 14, 2024 14:27

ormergi approved these changes Mar 14, 2024

View reviewed changes

kubevirt-bot assigned ormergi Mar 14, 2024

kubevirt-bot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 14, 2024

EdDev force-pushed the admitters-net-move branch from 99a546d to e24ff7a Compare March 19, 2024 05:53

kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 19, 2024

AlonaKaplan reviewed Mar 19, 2024

View reviewed changes

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

EdDev mentioned this pull request Mar 19, 2024

net, admitter: Move network related admiter validations to the network/admitter package #11544

Merged

8 tasks

EdDev force-pushed the admitters-net-move branch from e24ff7a to 108a131 Compare March 25, 2024 06:39

EdDev added 6 commits March 25, 2024 14:02

virt-api, vmi-admitter: Refactor single pod network validation

e421829

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

virt-api, admitter: Move single pod net check

96f681f

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

net, admitter: Add test suite to run the unit tests

3c34ca4

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

virt-api, admitter: Move out slirp binding validation

89a1d2c

Relocate the slirp admitter validations to the network/admitter package. Simplify the implementation and provide clear unit tests to the various checks. Signed-off-by: Edward Haas <edwardh@redhat.com>

makefile: Add pkg/network/admitter to the linter coverage

9262d59

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

net, admitter, slirp: Make the validations easier to test

9d3867f

Use interfaces on the validation function to ease its testing. By using a focused interface, the tests now can use a simple stub instead of a complex fake. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev force-pushed the admitters-net-move branch from 108a131 to 9d3867f Compare March 25, 2024 12:02

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

kubevirt-bot added the sig/code-quality label Mar 26, 2024

RamLavi approved these changes Mar 26, 2024

View reviewed changes

kubevirt-bot assigned RamLavi Mar 26, 2024

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

kubevirt-bot merged commit b1d9476 into kubevirt:main Mar 27, 2024
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

virt-api, admitter: Refactor & move network validation admitters to the network package #11511

virt-api, admitter: Refactor & move network validation admitters to the network package #11511

EdDev commented Mar 14, 2024

kubevirt-bot commented Mar 14, 2024

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

ormergi left a comment

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

ormergi commented Mar 14, 2024

EdDev commented Mar 19, 2024

AlonaKaplan Mar 19, 2024

EdDev Mar 25, 2024

AlonaKaplan Mar 19, 2024

EdDev Mar 19, 2024

EdDev commented Mar 25, 2024

EdDev commented Mar 25, 2024

AlonaKaplan commented Mar 25, 2024

kubevirt-bot commented Mar 25, 2024

RamLavi commented Mar 26, 2024

RamLavi left a comment

RamLavi Mar 26, 2024

kubevirt-commenter-bot commented Mar 26, 2024

EdDev commented Mar 26, 2024

		@@ -284,6 +284,7 @@ func validateNetworksMatchInterfaces(field k8sfield.Path, spec v1.VirtualMachi

		networkData, networkExists := networkNameMap[iface.Name]

		causes = append(causes, netadmitter.ValidateSlirpBinding(field, spec, config)...)

virt-api, admitter: Refactor & move network validation admitters to the network package #11511

virt-api, admitter: Refactor & move network validation admitters to the network package #11511

Conversation

EdDev commented Mar 14, 2024

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

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

ormergi left a comment

Choose a reason for hiding this comment

EdDev commented Mar 14, 2024

EdDev commented Mar 14, 2024

ormergi commented Mar 14, 2024

EdDev commented Mar 19, 2024

AlonaKaplan Mar 19, 2024

Choose a reason for hiding this comment

EdDev Mar 25, 2024

Choose a reason for hiding this comment

AlonaKaplan Mar 19, 2024

Choose a reason for hiding this comment

EdDev Mar 19, 2024

Choose a reason for hiding this comment

EdDev commented Mar 25, 2024

EdDev commented Mar 25, 2024

AlonaKaplan commented Mar 25, 2024

kubevirt-bot commented Mar 25, 2024

RamLavi commented Mar 26, 2024

RamLavi left a comment

Choose a reason for hiding this comment

RamLavi Mar 26, 2024

Choose a reason for hiding this comment

kubevirt-commenter-bot commented Mar 26, 2024

EdDev commented Mar 26, 2024