kubevirt
kubevirt copied to clipboard
kubevirt
multus, annotations, Move methods to new package
What this PR does / why we need it:
This PR creates a new package multus under pkg/network.
Since network names given in k8s.v1.cni.cncf.io/networks annotation by virt-controller are given by index, and later used by other places in the code, there is a need to keep the naming logic consistent.
We do this by moving methods related to multus annotations to a separate package.
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:
Release note:
NONE
Thank you for the PR @RamLavi. Is it possible to please make the change gradually over more commits? I personally find the change too big for me to fully understand.
@orelmisan I'ved broken it up
Thank you for the PR @RamLavi. Is it possible to please make the change gradually over more commits? I personally find the change too big for me to fully understand.
@orelmisan I'ved broken it up
Thank you for the change, I find it helpful in reducing the cognitive load.
First commit message:
This commit creates a new Multus package for annotation related methods, and only moves filterMultusNonDefaultNetworks function to it.Can you please reason why only this function was moved and not the whole
pkg/virt-controller/services/multus_annotations.gofile?
The reason it was moved is because it was needed to be used in other places. The other functions there could go into "multus/annotations.go". We should do it in a separate PR, wdyt?
/test pull-kubevirt-e2e-k8s-1.22-sig-network /test pull-kubevirt-e2e-k8s-1.23-sig-network /test pull-kubevirt-e2e-k8s-1.24-sig-network
First commit message:
This commit creates a new Multus package for annotation related methods, and only moves filterMultusNonDefaultNetworks function to it.Can you please reason why only this function was moved and not the whole
pkg/virt-controller/services/multus_annotations.gofile?The reason it was moved is because it was needed to be used in other places. The other functions there could go into "multus/annotations.go". We should do it in a separate PR, wdyt?
I there's no reason to use those funcs from outside the virt-controller template package I see no reason to change their scope or move them around.
/test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-unit-test
@RamLavi what you think about using the VMI spec.device.interfaces names instead of the current scheme (net1, net2 ..)?
(by specifying them in the k8s.v1.cni.cncf.io/networks annotation)
First commit message:
This commit creates a new Multus package for annotation related methods, and only moves filterMultusNonDefaultNetworks function to it.Can you please reason why only this function was moved and not the whole
pkg/virt-controller/services/multus_annotations.gofile?The reason it was moved is because it was needed to be used in other places. The other functions there could go into "multus/annotations.go". We should do it in a separate PR, wdyt?
I there's no reason to use those funcs from outside the virt-controller template package I see no reason to change their scope or move them around.
But after this change we will have two packages in two different directories with roughly the same subject, thus spreading the logic to more places.
As the commit message states: This commit creates a new Multus package for annotation related methods, which is what pkg/virt-controller/services/multus_annotations.go is doing at the moment.
@RamLavi what you think about using the VMI
spec.device.interfacesnames instead of the current scheme (net1, net2 ..)? (by specifying them in thek8s.v1.cni.cncf.io/networksannotation)
@ormergi AFAIU these names are supposed to be the interface names inside the virt-launcher Pod.
IIRC they are limited in length.
Besides, I think it is a change that may break backwards compatibility because other code is relying on that scheme.
@ormergi fixed and addressed comments
/test pull-kubevirt-unit-test /test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-e2e-k8s-1.24-sig-network
First commit message:
This commit creates a new Multus package for annotation related methods, and only moves filterMultusNonDefaultNetworks function to it.Can you please reason why only this function was moved and not the whole
pkg/virt-controller/services/multus_annotations.gofile?The reason it was moved is because it was needed to be used in other places. The other functions there could go into "multus/annotations.go". We should do it in a separate PR, wdyt?
I there's no reason to use those funcs from outside the virt-controller template package I see no reason to change their scope or move them around.
But after this change we will have two packages in two different directories with roughly the same subject, thus spreading the logic to more places. As the commit message states:
This commit creates a new Multus package for annotation related methods, which is whatpkg/virt-controller/services/multus_annotations.gois doing at the moment.
I tend to agree with @orelmisan , just for the sake of keeping things together in the same place. Having said that - again I think this is out of the scope of this PR, so I would suggest we move this suggestion (and it's important discussion over it) to another PR.
/test pull-kubevirt-unit-test /test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-e2e-k8s-1.24-sig-network
@ormergi AFAIU these names are supposed to be the interface names inside the
virt-launcherPod. IIRC they are limited in length.
Correct, the CNI will set the interfaces with these names (net1, net2..), the limitation is actually a limitation of the Linux kernel (15 character max IIRC). I am not saying it won't require a change to Kubevirt admission webhook (on the VMI interfaces length)..
Besides, I think it is a change that may break backwards compatibility because other code is relying on that scheme.
How is it different from introducing any other feature that requires changing more than one component?
@ormergi AFAIU these names are supposed to be the interface names inside the
virt-launcherPod. IIRC they are limited in length.Correct, the CNI will set the interfaces with these names (net1, net2..), the limitation is actually a limitation of the Linux kernel (15 character max IIRC). I am not saying it won't require a change to Kubevirt admission webhook (on the VMI interfaces length)..
Besides, I think it is a change that may break backwards compatibility because other code is relying on that scheme.
How is it different from introducing any other feature that requires changing more than one component?
I think it would make things easier, but I don't think it should be done as part of the design. This kind of change deserves a design of its own.
/test pull-kubevirt-unit-test /test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-e2e-k8s-1.24-sig-network
/test pull-kubevirt-unit-test /test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-e2e-k8s-1.24-sig-network
/test pull-kubevirt-unit-test /test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-e2e-k8s-1.24-sig-network
/test pull-kubevirt-unit-test /test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-e2e-k8s-1.24-sig-network
/test pull-kubevirt-unit-test /test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-e2e-k8s-1.24-sig-network
/test pull-kubevirt-unit-test /test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-e2e-k8s-1.24-sig-network
/test pull-kubevirt-unit-test /test pull-kubevirt-generate /test pull-kubevirt-build /test pull-kubevirt-e2e-k8s-1.24-sig-network
[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
- ~~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: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-kubevirt-fossa | 764ae8d1cacc580f2037ab46d8ce9a4ec2826504 | link | false | /test pull-kubevirt-fossa |
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/test-infra repository. I understand the commands that are listed here.
/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.
@RamLavi: #8263 failed to apply on top of branch "release-0.56":
Applying: network, vmispec: Move multus network filtering helper
Using index info to reconstruct a base tree...
M pkg/network/vmispec/network.go
M pkg/virt-controller/services/BUILD.bazel
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/services/BUILD.bazel
CONFLICT (content): Merge conflict in pkg/virt-controller/services/BUILD.bazel
Auto-merging pkg/network/vmispec/network.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 network, vmispec: Move multus network filtering helper
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
In response to this:
/cherry-pick release-0.56
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/test-infra repository.
failed on release-0.56, but doesn't hurt to try on release-0.55 :)
/cherry-pick release-0.55
@RamLavi: #8263 failed to apply on top of branch "release-0.55":
Applying: network, vmispec: Move multus network filtering helper
Using index info to reconstruct a base tree...
M pkg/network/vmispec/network.go
M pkg/virt-controller/services/BUILD.bazel
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/services/BUILD.bazel
CONFLICT (content): Merge conflict in pkg/virt-controller/services/BUILD.bazel
Auto-merging pkg/network/vmispec/network.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 network, vmispec: Move multus network filtering helper
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
In response to this:
failed on release-0.56, but doesn't hurt to try on release-0.55 :)
/cherry-pick release-0.55
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/test-infra repository.