-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Design Proposal: Provide way to customize generated Multus pod annotation #275
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @chomatdam. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
Signed-off-by: Damien Chomat <damien.chomat@gmail.com>
fedd91f
to
01e897a
Compare
/cc |
/sig network |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this proposal, I like its direction to integrate into the main flow with minimal API change.
My ideal solution would be to have the webhook developed and deployed independently of the core project, but this can be negotiated.
`k8s.v1.cni.cncf.io/networks` | ||
2. Pod `virt-launcher` inherits from all the VMI annotations (an already existing behavior) | ||
3. A new mutating webhook is observing `virt-launcher` pod creation and updates, and merges `k8s.v1.cni.cncf.io/networks-customization` into | ||
`k8s.v1.cni.cncf.io/networks` based on the network's `name` and `namespace` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that two interfaces can point to the same network (i.e NAD) and it is a valid configuration.
This will be a 1:N mapping.
Is that desired?
- Altering the current behavior, the current state covers 90% of use cases. Only enhancing the Multus support by | ||
providing a way to customize it, if needed, without needing KubeVirt maintainers to actively maintain it | ||
|
||
# Design |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a few words about the different components introduced?
(e.g. webhook mutation admitter)
-
The webhook could be introduced from "outside" the core kubevirt, behaving as an add-on.
It will be less intrusive for most of the users who do not need it and empower the author of it to maintain it himself. If it will prove useful for more community members, we can then integrate it as a hook point into the core kubevirt.- Another option is to make it pluggable at build time. There may be other options to explore.
Note: The option of having this as part of the kubevirt/kubevirt code-base (the webhook itself) should come with a Feature Gate.
It would also be nice to describe in more detail and formally how the "merge" algorithm works, between the annotation customized data and the multus network one.
**NOTE:** Existing Multus fields managed by the `virt-controller` are immutable. Any change to `interface` or `mac` is | ||
rejected by the webhook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a maintenance point of view, this is a burden to track. While today these are reserved, in the future others may be used by the core.
As such, I think it will be better to only warn in the documentation about this but not restrict/validate in-code.
/cc @AlonaKaplan |
/sig api |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. /close |
@kubevirt-bot: Closed this PR. In response to this:
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. |
What this PR does / why we need it:
When
virt-controller
is generating the Multus pod annotation, several properties can be set:name
namespace
interface
mac
(optional)This proposal enables the customization of the Multus annotation.
Implementation
Documentation
Which issue(s) this PR fixes:
Fixes #4564
Checklist
Release note: