-
Notifications
You must be signed in to change notification settings - Fork 4
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
UDN, IPAM: Use v1.multus-cni.io/default-network #69
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 |
Before OVN changes i get this error
should i add to the new NSE entry Additional question that might be important: |
Is this the first error it shows ? Trying to ensure it didn't fail for something else first, then also failed to cleanup the pod's sandbox, leaving a
Your NSE plumbs the default network interface into the pod, right ? And multus will not invoke the cluster default network attachment. The secondary network controller for the UDN should still kick in, compute the synthetic network selection element for the UDN, and append that to the list of NSEs that will be processed. Here is where you need to adjust the code: the IPAMClaim reference should be set in the synthetic network selection element, not in the default cluster network NSE. |
according describe on the pod, yes
|
if i understood correctly (if i am wrong we can continue offline as we spoke, thanks) ipam-ext should add this only OVN should take |
Actually, to the primary interface that |
You'll need to understand what on earth is creating
|
the NSE that i added i think (we have 2 here, primary-udn-kubevirt-binding and the new one i added wrongly) but iiuc it will be dropped due to the discussion above, so it wont be interesting |
possible thanks, |
WIP - OVN part ovn-kubernetes/ovn-kubernetes#4732 |
basically it seems to work will raise soon some little points to discuss once we decide to implement it i will inject it to OVN to run all tests so we see if any problems exists |
Two questions:
Thanks for the effort. |
Both suggestions involve adding a NSE, that NSE must not overlap with an interface that we are already creating, Note: i still didnt finish to make all tests pass with the current method |
another note, if we decide to implement it, we need to consider adding the NAD that appear on the PR desc to CNAO (that creates the other NAD) |
updated PRs |
all tests passed (failures are just known flakes, rebased the ref PR because a fix that should solve them was merged) let me know please if there is a green light to implement it |
Cool, really good.
Yeah, if you don't mind, start working on that. There is a slight chance of it not landing in the product though - I will discuss this next-week with the maintainers. |
In order to specify ipam-claim-reference for the primary network, use v1.multus-cni.io/default-network instead k8s.ovn.org/primary-udn-ipamclaim. See kubevirt/ipam-extensions#69 Signed-off-by: Or Shoval <[email protected]>
In order to specify ipam-claim-reference for the primary network, use v1.multus-cni.io/default-network instead k8s.ovn.org/primary-udn-ipamclaim. See kubevirt/ipam-extensions#69 Signed-off-by: Or Shoval <[email protected]>
Side quest idea if desired: Allow make |
The failure now is the one fixed at #70 |
both PRs basically ready for review please (as product, not poc) |
/hold until ovn-kubernetes/ovn-kubernetes#4732 is lgtm and ready to be merged |
In order to specify ipam-claim-reference for the primary network, use v1.multus-cni.io/default-network instead k8s.ovn.org/primary-udn-ipamclaim. Signed-off-by: Or Shoval <[email protected]>
rebased Quique raised some concerns offline that we should discuss (i.e about who creates the NAD, and that it should do it dynamically according the default config file) We also discussed a potential alternative if desired, that default-network will have some indication |
Yes, having to create the NAD is one clear downside of this approach. You raise a good point: who should be responsible for creating this. Let's wait to what the maintainers think. |
should we raise it on the OVN PR or ask OVN maintainers offline please ? |
Point Jaime to the OVN-K PR. I'll add this to the weekly sync we have THU. |
|
@maiqueb |
for now it is on hold due to the reasons above |
What this PR does / why we need it:
In order to specify ipam-claim-reference for the primary network,
use
v1.multus-cni.io/default-network
insteadk8s.ovn.org/primary-udn-ipamclaim
.Webhook adds
v1.multus-cni.io/default-network: '[{"namespace":"default","name":"ovn-kubernetes","ipam-claim-reference":"vm-a.passtnet"}]'
(
ipam-claim-reference
value is dynamic of course)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:
OVN K8s PR
ovn-kubernetes/ovn-kubernetes#4732
Both PRs need to be merged else the tests (that atm exists only on OVN) will break.
We should not merge it until the above PR is merge candid.
Need to create this NAD (because default-network points to a NAD):
based on
/etc/cni/net.d/10-ovn-kubernetes.conf
Release note: