Routed ingress primary udn with static ips #1793
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 |
|
/cc @kyrtapz |
| A new CRD - named `IPPool`, or `DHCPLeaseConfig` (or the like) - will be | ||
| created, and is associated to a UDN (both UDN, and C-UDN). This CRD holds the | ||
| association of MAC address to IPs for a UDN. When importing the VM into | ||
| OpenShift Virt, MTV will provision / update this object with this information. |
There was a problem hiding this comment.
Can also be a modificable field at the UDN/CUDN, since implicitly is affecting the network
There was a problem hiding this comment.
it surely can, but I think:
- that'll cause marshal/unmarshal of the (c)UDN object to cost more
- there's value in separating the concepts
- (c) UDN is an OVN-K entity. I don't see why this needs to be an OVN-K entity - OVN-K doesn't need write / read access to it. This is for the admin (or MTV, or any other introspection tool) to configure, and for the ipam-extensions to read / act upon its data
| The `ipam-extensions` mutating webhook will kick in whenever a virt launcher | ||
| pod is created - it will identify when the VM has a primary UDN attachment | ||
| (already happens today), and will also identify when the pod network attachment | ||
| has a MAC address configuration request. |
There was a problem hiding this comment.
btw we have see that qemu do also get configured with macAddress field on some envs, we should tackle that
There was a problem hiding this comment.
IIUC you're saying there's value outside of this enhancement (preserve IP / MAC / GW net config of imported VMs to L2 overlays w/ IPAM) in allowing the MAC address of the primary UDN attachment to be configurable.
There was a problem hiding this comment.
I mean that configuring macAddress at the VMI interface has consequences, that we should be aware of.
| OVN-Kubernetes will then act upon this information, by configuring the | ||
| requested MAC and IPs in the pod. If the allocation of the IP is successful, | ||
| said IPs will be persisted in the corresponding `IPAMClaim` CR (which already | ||
| happens today). If it fails (e.g. that IP address is already in use in the | ||
| subnet), the CNI will fail, crash-looping the pod. The error condition will be | ||
| reported in the associated `IPAMClaim` CR, and an event logged in the pod. | ||
|
|
||
| This flow is described in the following sequence diagram: | ||
| ```mermaid |
There was a problem hiding this comment.
Also for live migration/restart we are going to have both NSE address and the ipamclaims address, we should take care of that, maybe just checking that both are the same.
There was a problem hiding this comment.
that's a good point. First thing that comes to mind is we could report an error condition in the IPAMClaim + event on the pod when they differ.
There was a problem hiding this comment.
This will be handled in detail in the upstream OVN-Kubernetes OKEP.
| A new CRD - named `IPPool`, or `DHCPLeaseConfig` (or the like) - will be | ||
| created, and is associated to a UDN (both UDN, and C-UDN). This CRD holds the | ||
| association of MAC address to IPs for a UDN. When importing the VM into | ||
| OpenShift Virt, MTV will provision / update this object with this information. |
There was a problem hiding this comment.
it surely can, but I think:
- that'll cause marshal/unmarshal of the (c)UDN object to cost more
- there's value in separating the concepts
- (c) UDN is an OVN-K entity. I don't see why this needs to be an OVN-K entity - OVN-K doesn't need write / read access to it. This is for the admin (or MTV, or any other introspection tool) to configure, and for the ipam-extensions to read / act upon its data
| The `ipam-extensions` mutating webhook will kick in whenever a virt launcher | ||
| pod is created - it will identify when the VM has a primary UDN attachment | ||
| (already happens today), and will also identify when the pod network attachment | ||
| has a MAC address configuration request. |
There was a problem hiding this comment.
IIUC you're saying there's value outside of this enhancement (preserve IP / MAC / GW net config of imported VMs to L2 overlays w/ IPAM) in allowing the MAC address of the primary UDN attachment to be configurable.
| OVN-Kubernetes will then act upon this information, by configuring the | ||
| requested MAC and IPs in the pod. If the allocation of the IP is successful, | ||
| said IPs will be persisted in the corresponding `IPAMClaim` CR (which already | ||
| happens today). If it fails (e.g. that IP address is already in use in the | ||
| subnet), the CNI will fail, crash-looping the pod. The error condition will be | ||
| reported in the associated `IPAMClaim` CR, and an event logged in the pod. | ||
|
|
||
| This flow is described in the following sequence diagram: | ||
| ```mermaid |
There was a problem hiding this comment.
that's a good point. First thing that comes to mind is we could report an error condition in the IPAMClaim + event on the pod when they differ.
| pod is created - it will identify when the VM has a primary UDN attachment | ||
| (already happens today), and will also identify when the pod network attachment | ||
| has a MAC address configuration request. | ||
| It will then access the `IPPool` (or `DHCPLeaseConfig` for the UDN) to extract |
There was a problem hiding this comment.
btw this CRD should be at the same "namespace" as IPAMClaim it should not be a OVN thing
There was a problem hiding this comment.
hm, not 100% sure yet.
How will it work for C-UDNs ? We need the MAC:IPs association per network, which is a non-namespaced object.
Furthermore, I envision associating this pool to the network using the NAD, enabling this feature to work outside OVN-K (i.e. making it a part of k8snetworkplumbingwg, as IPAMClaims are).
There was a problem hiding this comment.
Sorry I mean the kind namespace, not the resource namespace like muiltus.io/IPAMClaim or the like.
There was a problem hiding this comment.
OK, then I agree. I don't see a reason for this CRD to be added on OVN-K. I think we can get it on k8snetworkplumbing - as IPAMClaim is.
maiqueb
force-pushed
the
routed-ingress-primary-udn-with-static-ips
branch
from
4a7aa05 to
3b854af
Compare
| solution, and OpenShift Virtualization currently supports these features on its | ||
| primary UserDefinedNetworks (UDNs). | ||
|
|
||
| These users have additional requirements, like routed ingress into their VMs, |
There was a problem hiding this comment.
reachability to the VM is what we are after when you say "routed" ?
routed ingress here and everywhere would be good to clarify if its simple external to pod or something else
There was a problem hiding this comment.
reachability to the VM without NAT, yes.
|
|
||
| ## Motivation | ||
|
|
||
| Some users are running VMs in virtualization platforms having a managed IP |
There was a problem hiding this comment.
same here their own managed IP configuration rather than the CNI doing it
There was a problem hiding this comment.
I read the words, but I'm afraid I don't follow what the sentence means ...
Could you clarify ?
| address (the one assgined to the VM). | ||
| - As the owner of a VM running in a traditional virtualization platform, I want | ||
| to import said VM into Kubernetes, attaching it to an overlay network. I want to | ||
| have managed IPs for my VMs, since that feature was already available in my |
There was a problem hiding this comment.
I imagine this would be the IPAM disable mode? cause once migraton is done we don't let them do any IPAM management, new ones will get random IPs from IPAM
TBD to be revisited as spoken with @maiqueb on slack
There was a problem hiding this comment.
No, not at all.
IPAM disabled mode is OVN-K is 100% unaware of what IPs are in the workloads.
IP spoofing protection is off, network policies will only allow IPBlock selectors, and so forth.
| association of MAC address to IPs for a UDN. When importing the VM into | ||
| OpenShift Virt, MTV will provision / update this object with this information. | ||
| This object is providing to the admin user a single place to check the IP | ||
| address MAC to IPs mapping. On an first implementation phase, we can have the |
There was a problem hiding this comment.
hmm i don't think manual population like that will be acceptable will it? for ppl bringing in 5000 vms is this even possible?
There was a problem hiding this comment.
a later implementation can focus on that. E.g. introspect the src cluster (check out ARP requests / replies on the switch) and provision this on behalf of the user.
... or something like that. That should be covered on an MTV enhancement.
| required MAC address. We would need MTV to somehow also figure out what IP | ||
| addresses are on the aforementioned interfaces. | ||
|
|
||
| A new CRD - named `IPPool`, or `DHCPLeaseConfig` (or the like) - will be |
There was a problem hiding this comment.
hmm why not make IPAMClaim's v2alpha2 API AddressClaims so enhance IPAMClaim ? :D
There was a problem hiding this comment.
- that's not a centralized place with the MAC => IPs association. There's value in that. Plus, it's closer to what existing platforms provide.
- I feel we'd do that because "there's a box we're opening already. Let's put this disjoint information into the same box, and retrieve both". I.e. we're bending what we have so we reach the goal we think we have faster.
Having said that, re-purposing IPAMClaims to something more generic is an option. I just feel we're not doing it for the right reasons. I'm afraid we might be doing it because it fits the time-frame rather than the use case.
FWIW, the issue of "importing 5000" VMs is still there if we take this route. How will the 5000 AddressClaims be provisioned ? By whom ? With what data ?
maiqueb
force-pushed
the
routed-ingress-primary-udn-with-static-ips
branch
2 times, most recently
from
067607d to
cc333a0
Compare
| to import said VM - whose IPs were statically configured - into Kubernetes, | ||
| attaching it to an overlay network. I want to ingress/egress using the same IP | ||
| address (the one assgined to the VM). | ||
| - As the owner of a VM running in a traditional virtualization platform, I want |
There was a problem hiding this comment.
No - "As the owner of a VM running in a traditional virtualization platform, I want to import said VM into Kubernetes, attaching it to an overlay network"
There was a problem hiding this comment.
But is said
I want to
have managed IPs for my VMs
There was a problem hiding this comment.
Yes, but I still don't see the implication.
If it helps, this is about importing a VM into a layer2 overlay with IPAM.
|
|
||
| - Preserve the original VM's MAC address | ||
| - Preserve the original VM's IP address | ||
| - Specify the gateway IP address of the imported VM so it can keep the same |
There was a problem hiding this comment.
Do we know if the ip address is at the same subnet ?
There was a problem hiding this comment.
Yeah, we'll only take care of that scenario.
I'll add it to the list of non-goals.
There was a problem hiding this comment.
Also put at the non goals adding non default gw routes.
| be able to consume Kubernetes features like network policies, and services, to | ||
| benefit from the Kubernetes experience. | ||
|
|
||
| ### Goals |
There was a problem hiding this comment.
Do we also need to preserve the other DHCP stuff like DNS servers and MTU ? like imaging that we configure DHCPOPtions with bad MTU, after the lease we may break stuff ?
There was a problem hiding this comment.
I don't understand what you mean, but I think we need to keep advertising MTU / DNS / other properties as we are already doing for primary UDN.
There was a problem hiding this comment.
Ahh after migration virtual machine get restarted so it will take the new MTU for new system also the new DNS servers, nah all good here.
|
|
||
| ### Non-Goals | ||
|
|
||
| Handle importing VMs without a managed IP experience - i.e. IPs were defined |
There was a problem hiding this comment.
The enhacement is about static IPs but we talk all over about managed IPs, is quite confusing for me, static IPs does not feel like managed IPs.
There was a problem hiding this comment.
they're not. Managed IPs are about having IPAM in the UDN, while static IPs are about having the ability of specifying which IPs is your workload going to have.
I'll add a disclaimer / explanation to a nomenclature section.
| OpenShift Virt, MTV (or a separate, dedicated component) will provision / | ||
| update this object with the required information (MAC to IPs association). | ||
| This object is providing to the admin user a single place to check the IP | ||
| address MAC to IPs mapping. On an first implementation phase, we can have the |
There was a problem hiding this comment.
This is missing the step about MTV creating the VirtualMachine with macAddress field so ipam extensions can map it to IPs.
| This approach requires the `IPAMClaim` CRD to be updated, specifically its | ||
| status sub-resource - we need to introduce `Conditions` so we can report errors | ||
| when allocating IPs which were requested by the user - what if the address is | ||
| already in use within the UDN ? |
There was a problem hiding this comment.
This is useful always I think, for centralized and not centralized.
There was a problem hiding this comment.
I'm mentioning it somewhere else that this is needed for both options, as you're pointing out.
| OVN-Kubernetes would read the CR, attempt to reserve the requested MAC and IP, | ||
| and then persist that information in the IPAMClaim status - reporting a | ||
| successful sync in the `IPAMClaim` status - or a failure otherwise. | ||
|
|
There was a problem hiding this comment.
I am missing the mapping mechanism between VM and IPAMClaim
There was a problem hiding this comment.
This will be elaborated on the implementation details section. Does it make sense to split it like this ?
There was a problem hiding this comment.
ahh ok, implementation details is the place.
|
|
||
| type IPPoolSpec struct { | ||
| NetworkName string `json:"network-name"` | ||
| Entries map[net.HardwareAddr][]net.IP `json:"entries"` |
There was a problem hiding this comment.
Do we need this to be a CIDR ?
| Entries map[net.HardwareAddr][]net.IP `json:"entries"` | |
| Entries map[net.HardwareAddr][]net.IPNet `json:"entries"` |
The NSE ip request is a cidr
annotations:
k8s.v1.cni.cncf.io/networks: '[
{
"name": "macvlan1-config",
"ips": [ "10.1.1.11/24" ],
"interface": "net1"
}
]'Maybe is dangerous to do so.
|
|
||
| type IPPoolStatus struct { | ||
| Conditions []Condition | ||
| AssociatedNADs []NADInfo |
There was a problem hiding this comment.
That means we will need to reconcile on nad creation even if i's not attached to any VM, not sure it's worth it, I would just not include that and add if only if needed.
There was a problem hiding this comment.
I think it is good information to have and present to the admin.
Still, I might flag it for an optional future improvement - since the feature itself does not require it.
Does that work for you @qinqon ?
There was a problem hiding this comment.
Yep, if it's not really needed, let's mark it as opìonal, we have already a lot of stuff at our plate.
| The `IPPool` CRD will have at least the following conditions: | ||
| - DuplicateMACAddresses: will indicate to the admin that a MAC address appears | ||
| multiple times in the `Entries` list | ||
| - DuplicateIPAddresses: will indicate to the admin that an IP address appears | ||
| multiple times associated to different MAC addresses in the `Entries` list | ||
| - Success: the data present in the spec is valid (no duplicate MACs or IPs) |
There was a problem hiding this comment.
Show yaml with them to see typoe, reason and message.
| We plan on reporting in the `IPPool` the name of the NADs which are holding the | ||
| configuration for the network which this pool stores the MAC <=> IPs | ||
| associations. |
There was a problem hiding this comment.
As said this is a not needed extra work that need a new reconcile logic to keep this up to date.
There was a problem hiding this comment.
Listed these as optional improvements for improving the UX.
| aligned with existing user expectations? Will it be a significant maintenance | ||
| burden? Is it likely to be superceded by something else in the near future? | ||
|
|
||
| ## Alternatives (Not Implemented) |
There was a problem hiding this comment.
Maybe we can put here the alternative of chaning kubevirt VMI api ? stating the problems with ip pool depletion and stuff
There was a problem hiding this comment.
Good point. I'll refer that alternative. FWIW, I assume you're mentioning adding an IP / IPRequests attribute to the KubeVirt Interface definition.
|
|
||
| - Preserve the original VM's MAC address | ||
| - Preserve the original VM's IP address | ||
| - Specify the gateway IP address of the imported VM so it can keep the same |
| OVN-Kubernetes would read the CR, attempt to reserve the requested MAC and IP, | ||
| and then persist that information in the IPAMClaim status - reporting a | ||
| successful sync in the `IPAMClaim` status - or a failure otherwise. | ||
|
|
There was a problem hiding this comment.
This will be elaborated on the implementation details section. Does it make sense to split it like this ?
|
|
||
| The `ipam-extensions` mutating webhook will kick in whenever a virt launcher | ||
| pod is created - it will identify when the VM has a primary UDN attachment | ||
| (already happens today), and will also identify when the pod network attachment |
There was a problem hiding this comment.
Tried to disambiguate by pointing at the KubeVirt API reference attribute where the MAC is actually defined.
| OVN-Kubernetes will then act upon this information, by configuring the | ||
| requested MAC and IPs in the pod. If the allocation of the IP is successful, | ||
| said IPs will be persisted in the corresponding `IPAMClaim` CR (which already | ||
| happens today). If it fails (e.g. that IP address is already in use in the | ||
| subnet), the CNI will fail, crash-looping the pod. The error condition will be | ||
| reported in the associated `IPAMClaim` CR, and an event logged in the pod. | ||
|
|
||
| This flow is described in the following sequence diagram: | ||
| ```mermaid |
There was a problem hiding this comment.
This will be handled in detail in the upstream OVN-Kubernetes OKEP.
|
|
||
| #### New IPPool CRD | ||
|
|
||
| The IPPool CRD will operate as a place to store the MAC to IP addresses |
| address from the range that VMs have already been assigned outside of the | ||
| cluster (or for secondary IP addresses assigned to the VM's interfaces) | ||
|
|
||
| ### Non-Goals |
| address from the range that VMs have already been assigned outside of the | ||
| cluster (or for secondary IP addresses assigned to the VM's interfaces) | ||
|
|
||
| ### Non-Goals |
| OpenShift Virt, MTV (or a separate, dedicated component) will provision / | ||
| update this object with the required information (MAC to IPs association). | ||
| This object is providing to the admin user a single place to check the IP | ||
| address MAC to IPs mapping. On an first implementation phase, we can have the |
| MTV -->> VM Owner: OK | ||
| ``` | ||
|
|
||
| Hence, the required changes would be: |
| - SuccessfulAllocation: reports the IP address was successfully allocated for | ||
| the workload | ||
| - AllocationConflict: reports the requested allocation was not successful - i.e. | ||
| the requested IP address is already present in the network |
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
maiqueb
force-pushed
the
routed-ingress-primary-udn-with-static-ips
branch
2 times, most recently
from
d695f23 to
fac8ab7
Compare
| There should be no hypershift platform-specific considerations with this | ||
| feature. |
There was a problem hiding this comment.
@qinqon please keep me honest here.
There was a problem hiding this comment.
@maiqueb we have see hypershift customers asking for fixed IPs, but I think that is different kind of animal to what we need with MTV, feels more of a possible centralized approach although hypershift VMs name and mac is randomtly generated.
| - As the owner of a VM running in a traditional virtualization platform, I want | ||
| to import said VM into Kubernetes, attaching it to an overlay network. I want | ||
| to have managed IPs (IPAM) for my VMs, since that feature was already available | ||
| in my previous platform. |
There was a problem hiding this comment.
This is still very confusing to me, like this case we import the VM, but users are fine with ovn-kubernetes assigning IPs ?
There was a problem hiding this comment.
No - users want to specify their IPs.
But OVN-K is fully aware of which IPs are in which pods, thus it can have network policies (with pod / namespace selectors) and have IP spoofing protection enabled.
I'll try to reword the user story.
There was a problem hiding this comment.
I think I'll drop it, I think this user story is represented in the other two:
- As the owner of a VM running in a traditional virtualization platform, I want
to import said VM - whose IPs were statically configured - into Kubernetes,
attaching it to an overlay network. I want to ingress/egress using the same IP
address (the one assigned to the VM). - As the owner of a VM running in a traditional virtualization platform, I want
to import said VM into Kubernetes, attaching it to an overlay network. I want to
be able to consume Kubernetes features like network policies, and services, to
benefit from the Kubernetes experience.
| be able to consume Kubernetes features like network policies, and services, to | ||
| benefit from the Kubernetes experience. | ||
|
|
||
| ### Goals |
There was a problem hiding this comment.
Ahh after migration virtual machine get restarted so it will take the new MTU for new system also the new DNS servers, nah all good here.
| OVN-Kubernetes would read the CR, attempt to reserve the requested MAC and IP, | ||
| and then persist that information in the IPAMClaim status - reporting a | ||
| successful sync in the `IPAMClaim` status - or a failure otherwise. | ||
|
|
There was a problem hiding this comment.
ahh ok, implementation details is the place.
| This object is providing to the admin user a single place to check the IP | ||
| address MAC to IPs mapping. On an first implementation phase, we can have the | ||
| admin provision these CRs manually. Later on, MTV (or any other cluster | ||
| introspection tool) can provision these on behalf of the admin. |
There was a problem hiding this comment.
Nah all good, I was just pointing out stuff that was missing, but if it's going to be clarified I am all good.
|
|
||
| The `ipam-extensions` mutating webhook will kick in whenever a virt launcher | ||
| pod is created - it will identify when the VM has a primary UDN attachment | ||
| (already happens today), and will also identify when the pod network attachment |
There was a problem hiding this comment.
Don't use the word "attachment" since it's confusing with the multus idiom.
enhancements/network/routed-ingress-support-for-primary-udn-attached-vms-with-static-ips.md
Show resolved
Hide resolved
| - ipam-extensions (CNV component) will now also read the `IPPool` CRs for VMs | ||
| having primary UDNs in their namespaces, and requesting a specific MAC address | ||
| in their specs. These CRs will be used to generate the multus default network | ||
| annotation, which will be set in the pods by the mutating webhook. |
There was a problem hiding this comment.
Should we fail if the VMI has macAddress but there is no entry at the mac -> ip mapping ?, like maybe the admin forgot so VM keeps trying and then after admin fix the issue vm is corrected.
Also we have to think about ippool mutability, like maybe we should be able to add new entries but not to modify or remve them ?
There was a problem hiding this comment.
Should we fail if the VMI has macAddress but there is no entry at the mac -> ip mapping ?, like maybe the admin forgot so VM keeps trying and then after admin fix the issue vm is corrected.
For this scenario, I think we should just create the request for the MAC to be preserved. I.e. you'd get the original VM MAC, but an IP that was assigned by the primary UDN IP allocator.
This is getting into the behavior we expect, we should iron this out ASAP.
@tssurya / @kyrtapz thoughts ?
Also we have to think about ippool mutability, like maybe we should be able to add new entries but not to modify or remve them ?
For that we should consider what we want the pool for: e.g. do we want to sync the pool also w/ the dynamic allocations ? That would be good for observability - but it is IMHO a follow-up feature we may want to pursue.
That would require the pool to be subject to updates.
Having the pool immutable (only data provisioned during creation) would only allow you to fulfill the importing use case.
We should also consider that if we want to be able to create a VM attached to a primary UDN with a dedicated MAC / IP addresses we need the pool to be mutable.
We need to get our goals set up ASAP.
| The IPPool CRD is a cluster-scoped object associated to a UDN via the logical | ||
| network name (`NAD.Spec.Config.Name` attribute), since we want to have this | ||
| feature upstream in the k8snetworkplumbingwg, rather than in OVN-Kubernetes. | ||
|
|
||
| The `IPPool` spec will have an attribute via which the admin can point to a | ||
| cluster UDN - by the logical network name. The admin (which is the only actor | ||
| able to create the `IPPool`) has read access to all NADs in all namespaces, | ||
| hence they can inspect the NAD object to extract the network name. We could | ||
| even update the cluster UDN type to feature the generated network name in its | ||
| status sub-resource, to simplify the UX of the admin user. |
There was a problem hiding this comment.
Is the fact that networkName ippool field is not indexed a problem ? at the end ipam extensions will have to iterate them instead of using "List", but I understand that they will be at the informer's cache.
There was a problem hiding this comment.
Is the fact that networkName ippool field is not indexed a problem ? at the end ipam extensions will have to iterate them instead of using "List", but I understand that they will be at the informer's cache.
It is what it is ... we either use label selectors, we list then filter, or we associate using a different mean - i.e. point at the NAD or cluster UDN name (both are bad IMHO).
I'm going with what I think is the least worse option. Maybe relying on selectors is not that bad though ...
Waiting for more feedback.
| Preserving the gateway will require changes to the OVN-Kubernetes API. The | ||
| cluster UDN CRD should be updated - adding a gateway definition - and the | ||
| OVN-Kubernetes | ||
| [NetConf](https://github.com/ovn-kubernetes/ovn-kubernetes/blob/2643dabe165bcb2d4564866ee1476a891c316fe3/go-controller/pkg/cni/types/types.go#L10) |
There was a problem hiding this comment.
Maybe we can show an example with it ?
apiVersion: v1
kind: Pod
metadata:
name: pod-example
annotations:
v1.multus-cni.io/default-network: '{
"name": "isolated-net",
"namespace": "myisolatedns",
...
"default-route":[
"192.0.2.5",
"fd90:1234::5"
]
}'There was a problem hiding this comment.
that's really not what I mean here - you're showing the network selection element for the multus default network.
I'm saying the NAD should have a place for the user to indicate which GW to use on all attachments to that network. I don't see how showing an NSE can be an example of that.
enhancements/network/routed-ingress-support-for-primary-udn-attached-vms-with-static-ips.md
Show resolved
Hide resolved
maiqueb
force-pushed
the
routed-ingress-primary-udn-with-static-ips
branch
from
5c37dfe to
12b1a2c
Compare
This commits fills out the following sections: - summary - motivation - user stories - goals - non-goals Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
This commit describes how an imported VM's gateway will be preserved, which is required for the network configuration on the guests to be identical on both the source/destination clusters. Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
…tion Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
…ternative Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
…ay" section Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
maiqueb
force-pushed
the
routed-ingress-primary-udn-with-static-ips
branch
from
01ad846 to
c117013
Compare
| - Preserve the original VM's IP address | ||
| - Specify the gateway IP address of the imported VM so it can keep the same | ||
| default route | ||
| - Allow excludeSubnets to be used with L2 UDNs to ensure OVNK does not use an IP |
There was a problem hiding this comment.
tks for catching @kyrtapz ; yes, I think this should be moved to future goals or something (since it seems to be about supporting evpn / networks spanning multiple clusters use-cases).
|
|
||
| - Preserve the original VM's MAC address | ||
| - Preserve the original VM's IP address | ||
| - Specify the gateway IP address of the imported VM so it can keep the same |
There was a problem hiding this comment.
My understanding is that the gateway IP we want to allow the user to specify is the one of the UDN, we do not want to allow each imported VM to specify it's own. Could you clarify that here?
There was a problem hiding this comment.
not sure how to do that, but I'll try.
FWIW, yes: the idea is to specify the gateway of the network - i.e. the GW of each workload attached to the network will be the same.
|
|
||
| ### Non-Goals | ||
|
|
||
| - Handle importing VMs without a managed IP experience - i.e. IPs were defined |
There was a problem hiding this comment.
Is this a limitation of MTV?
There was a problem hiding this comment.
no, this simply is being requested in a different epic, which we have not prioritized.
There was a problem hiding this comment.
MTV can migrate powered-off VMs to which we don't have IPs.
We do require the IPs to preserve static IPs but users can still skip it.
There was a problem hiding this comment.
MTV can migrate powered-off VMs to which we don't have IPs.
AFAIK there are solution which set IP addresses explicitly on the management system that apply on VMs that come up (through DHCP or cloudinit). We do not have such users that expect it to pass to OCP-V?
@maiqueb , is it possible to specify from where we learn what is the configuration used (IP, etc)? If we do not know at the moment, maybe specify that it is expected as input. WDYT?
| - Importing a VM whose gateway is outside the subnet of the network. | ||
| - Adding non default routes to the VM when importing it into OpenShift Virt. | ||
| - Modifying the default gateway and management IPs of a primary UDN after it was created. | ||
| - Modifying a pod's network configuration after the pod was created. |
There was a problem hiding this comment.
Do you think it is worth it to specify that we are not going to support "live" migrating VMs? i.e importing VMs without a reboot.
There was a problem hiding this comment.
I don't, but I've added it. Better now ?
| - Modifying the default gateway and management IPs of a primary UDN after it was created. | ||
| - Modifying a pod's network configuration after the pod was created. | ||
|
|
||
| **NOTE:** implementing support on UDNs (achieving the namespace isolation |
There was a problem hiding this comment.
Not sure I understand this note, is it the same as https://github.com/openshift/enhancements/pull/1793/files#diff-ef7ea92bb7f0198174529787f912ec7898b84fa65e3e1b102ce30ed76735ab43R90-R91?
There was a problem hiding this comment.
yes, it's the same one.
What don't you understand ?
Do you recommend any course of action ? I can drop one of the entries if that makes it less confusing.
@kyrtapz
| This approach requires having N CRs with a 1:1 association between a primary | ||
| UDN attachment and the MAC and IPs it had on the original platform. | ||
|
|
||
| We could either introduce a new CRD with IPs and MAC association (which would |
There was a problem hiding this comment.
The de-cetralized approach described here changes how CNV integrates with OVN-K.
There would no longer be a need to pass the IP/MAC requests through the v1.multus-cni.io/default-network annotation.
There was a problem hiding this comment.
Yes. It would just do what it does today, read the ipam-claim-reference from an annotation, and attempt to consume the data present in the IPAMClaim.
enhancements/network/routed-ingress-support-for-primary-udn-attached-vms-with-static-ips.md
Show resolved
Hide resolved
| #### Centralized IP management | ||
|
|
||
| A new CRD - named `IPPool`, or `DHCPLeaseConfig` (or the like) - will be | ||
| created, and is associated to a cluster UDN. This CRD holds the association of |
There was a problem hiding this comment.
This CRD would be cluster scoped and limited to work with cluster UDNs only?
Are there any specific reasons limiting it to cluster UDNs?
There was a problem hiding this comment.
This CRD would be cluster scoped and limited to work with cluster UDNs only?
Yes.
Are there any specific reasons limiting it to cluster UDNs?
The entire feature is scoped to cluster UDNs because that's the type of CRD that works for BGP.
| Interface string `json:"interface"` | ||
| + // The IPs requested by the user | ||
| + // +optional | ||
| + IPRequests []CIDR `json:"ipRequests,omitempty"` |
There was a problem hiding this comment.
De-centralized IP management chapter states that the IPAMClaimSpec would also include the MAC address, it's missing here.
There was a problem hiding this comment.
Well, not sure .
I say the following:
This approach requires having N CRs with a 1:1 association between a primary
UDN attachment and the MAC and IPs it had on the original platform.
We could either introduce a new CRD with IPs and MAC association (which would
deprecate the IPAMClaim CRD), or, assuming clunkiness in all its glory, we
could change the IPAMClaim CRD to have MAC and IP addresses being requested in
the spec stanza as well.
I'll add these to the IPAMClaimSpec object, but I do think this is the least preferred option.
I'd even rather have a new CRD with the required info in it, and deal with the upgrade issues, than having this clunky API. IMHO we're putting things here because it suits the design, not because the API actually makes sense.
enhancements/network/routed-ingress-support-for-primary-udn-attached-vms-with-static-ips.md
Show resolved
Hide resolved
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
| #### Roles | ||
| - admin: a user with cluster admin rights. They can list NADs in all | ||
| namespaces, create cluster UDNs, and create / update `IPPool`s. | ||
| - VM owner: a user without cluster admin rights. They own their namespaces. |
There was a problem hiding this comment.
Yes, you don't need to be a cluster admin to use MTV, we have MTV roles which specify the minimal requirements to operate.
|
|
||
| ### Non-Goals | ||
|
|
||
| - Handle importing VMs without a managed IP experience - i.e. IPs were defined |
There was a problem hiding this comment.
MTV can migrate powered-off VMs to which we don't have IPs.
We do require the IPs to preserve static IPs but users can still skip it.
| required MAC address. We would need MTV to somehow also figure out what IP | ||
| addresses are on the aforementioned interfaces. |
There was a problem hiding this comment.
Yeah MTV has this MAC:IP mapping, for the static IPs we even know this from the guest level, but that doesn't apply here.
| - Support importing a "live" VM into the OpenShift virtualization cluster (i.e. | ||
| without requiring a VM restart). |
There was a problem hiding this comment.
I know it's not goal for this, but MTV will need to manage these resources aswell for the live migration.
And if Kubvirt is not exposing the IP:MAC it might be problematic.
| MTV already introspects the VM on the source cluster, and templates the VM with | ||
| its original MAC addresses. | ||
|
|
||
| ### Preserving the original VM IP address |
There was a problem hiding this comment.
MTV could start applying some annotation right now and we could remap it to the CR later. We are migrating lot of VMs right now and this might take some time to implement and deliver it to the customer. So in the meanwhile we could start preparing the VMs for this Feature. WDYT?
There was a problem hiding this comment.
That would be nice.
I want to run a PoC with the multus default net annotation to see how it plays along if we define a VM with that.
I'll report the findings ASAP.
There was a problem hiding this comment.
Right, it does not.
I have also to say there's a check in place that prevents the users from setting an IP via the NSE annotation and providing an ipam-claim-reference in it. We need to relax that constraint if we take this route.
There was a problem hiding this comment.
Please give me some annotation example, maybe you could have a controller that would consume that annotation and create the necessary CRs, and cleanup the annotation from VM CR.
There was a problem hiding this comment.
this would be the annotation you would set on your VM.spec.template:
v1.multus-cni.io/default-network: '[{
"name": "pony", # you'll need a NAD with this name on the "blue" namespace
"namespace": "blue", # this is the namespace where the VM will be created / imported
"mac": "02:03:04:05:06:07",
"ips": [
"10.0.0.5/24"
]
}]'There was a problem hiding this comment.
@mnecas threw me on a different course; what if:
- MTV templated the VM with the multus default network annotation (featuring both IPs and MAC requests, like the example in this comment. - KubeVirt does what it does today: carry over the VM's template metadata annotations to the pod's metadata annotations (i.e. the pod would be templated w/ the multus default network annotation)
- the ipam-extensions webhook mutates the launcher pod, adding to this annotation the ipam claim reference (OR it keeps using the current annotation ...)
- OVN-Kubernetes would apply the IPs / MAC requested on the NSE; it would persist that IP in the status of the IPAMClaim it was pointed to by the webhook.
This is a flow that doesn't require nor the IPPool CRD, nor updates to the IPAMClaim spec.
Pure profit. (... until we spot why it's a bad idea ... 😅 )
|
|
||
| #### Centralized IP management | ||
|
|
||
| A new CRD - named `IPPool`, or `DHCPLeaseConfig` (or the like) - will be |
There was a problem hiding this comment.
I would be careful about global/centrilized VM configs, as there can be many VM and the CR might run out of size.
There was a problem hiding this comment.
the max size of the CR is 1.5 MBi (by default) - which is AFAIU defined in the etcd configuration.
We should at least have a slight notion of how many MAC <=> IPs that can hold.
But for sure this is an argument against having a centralized place to store this info - tks for raising.
There was a problem hiding this comment.
yeah MTV hit that limit quite easily we have limit of migrating 500 VMs at once.
So don't underestimate perf/scale team
| The flow is described in more detail in the | ||
| [implementation details](#implementation-detailsnotesconstraints) section. | ||
|
|
||
| #### De-centralized IP management |
There was a problem hiding this comment.
From MTV's perspective, we are capable of doing both centralised and decentralised. Personally, I would lean more towards the decentralised. The centralised would have a lot of collisions when updating the CR. MTv can migrate lot of VMs at once.
There was a problem hiding this comment.
That said I would focus more on user experience, and if you will say that it's better for users to have it at one place we will manage it.
There was a problem hiding this comment.
From MTV's perspective, we are capable of doing both centralised and decentralised. Personally, I would lean more towards the decentralised. The centralised would have a lot of collisions when updating the CR. MTv can migrate lot of VMs at once.
Still it can compute everything and perform a single write.
| For IPv6 the flow will be different; IPv6 gateways are dynamic ! Hence, when a | ||
| VM is imported into OpenShift Virtualization, the OVN control plane must send a | ||
| RouterAdvertisement to the VM instructing it to forget about the old gateway - | ||
| i.e. send an RA with `lifetime = 0` with the source address of the gateway | ||
| defined for the VM in the old cluster. |
There was a problem hiding this comment.
I don't think we need to care about ipv6 LLAs - unless that's the old gateway.
As I see it, you'd just have a place in the C-UDN to define what was the network's old gateway, and you'd tell your workloads to forget about it.
We would only allow migrating from IPv6 networks with a single GW per network though ... if the workloads are using the IPv6 LLAs on the nodes where they run (I don't know how they implement this) we'd not be able to do it, since each workload might have a different GW.
There was a problem hiding this comment.
@maiqueb I was thinking the other day that maybe instead of removing gateway paths with the RA lifetime=0 trick we can do the following, depending on the ipv6 gateway IP nature:
- if it's global (ip from the subnet) we configure it as gateway router "Networks" like ipv4
- if it's a LLA maybe we can add it to gateway router Networks as /128 so VM's path is still valid, also the gateway router Networks will have the subnet .1 too.
What do you think ? we have to confirm with OVN people if we can configure /128 Networks at gateway router.
Also if in the future we go with transit router topology change we will be able to configure one mac fo all the nodes as the gateway then MTV importing the gateway mac make sense.
There was a problem hiding this comment.
@maiqueb I was thinking the other day that maybe instead of removing gateway paths with the RA lifetime=0 trick we can do the following, depending on the ipv6 gateway IP nature:
* if it's global (ip from the subnet) we configure it as gateway router "Networks" like ipv4
Is that even possible ? Won't it advertise the LLA of the interface even when the ip from the subnet is defined in the LRP ?... I thought so.
* if it's a LLA maybe we can add it to gateway router Networks as /128 so VM's path is still valid, also the gateway router Networks will have the subnet .1 too.
What's the advantage of this over forgetting the old gw and adjusting to the new ? If possible, I'd like to have a single code path that works for all scenarios.
What do you think ? we have to confirm with OVN people if we can configure /128 Networks at gateway router.
Also if in the future we go with transit router topology change we will be able to configure one mac fo all the nodes as the gateway then MTV importing the gateway mac make sense.
I still don't get what's the advantage that would bring, would you elaborate ? As I see it, since the VMs have to restart when they are imported, I don't see any advantage here.
There was a problem hiding this comment.
@maiqueb I was thinking the other day that maybe instead of removing gateway paths with the RA lifetime=0 trick we can do the following, depending on the ipv6 gateway IP nature:
* if it's global (ip from the subnet) we configure it as gateway router "Networks" like ipv4Is that even possible ? Won't it advertise the LLA of the interface even when the ip from the subnet is defined in the LRP ?... I thought so.
Yes, and that's why VM will still have a multipath ipv6 gateway, but both paths will be point to the same mac, OVN will also answer the NDP to discover the mac of the original ipv6 global IP gateway.
* if it's a LLA maybe we can add it to gateway router Networks as /128 so VM's path is still valid, also the gateway router Networks will have the subnet .1 too.What's the advantage of this over forgetting the old gw and adjusting to the new ? If possible, I'd like to have a single code path that works for all scenarios.
The advantage is that we then don't need to send the lifetime=0 RAs and we do like on ipv4, we set the gatway router Network with it.
What do you think ? we have to confirm with OVN people if we can configure /128 Networks at gateway router.
Also if in the future we go with transit router topology change we will be able to configure one mac fo all the nodes as the gateway then MTV importing the gateway mac make sense.I still don't get what's the advantage that would bring, would you elaborate ? As I see it, since the VMs have to restart when they are imported, I don't see any advantage here.
As said just the fact that we will not need to send the lifetime=0 RAs.
There was a problem hiding this comment.
@maiqueb I was thinking the other day that maybe instead of removing gateway paths with the RA lifetime=0 trick we can do the following, depending on the ipv6 gateway IP nature:
* if it's global (ip from the subnet) we configure it as gateway router "Networks" like ipv4Is that even possible ? Won't it advertise the LLA of the interface even when the ip from the subnet is defined in the LRP ?... I thought so.
Yes, and that's why VM will still have a multipath ipv6 gateway, but both paths will be point to the same mac, OVN will also answer the NDP to discover the mac of the original ipv6 global IP gateway.
IIUC, you're suggesting solving the "importing GW MAC" instead of forgetting old gateway (assuming it is needed - @kyrtapz made me wonder about that, we really don't know if the old IPv6 default route survives a VM restart).
* if it's a LLA maybe we can add it to gateway router Networks as /128 so VM's path is still valid, also the gateway router Networks will have the subnet .1 too.What's the advantage of this over forgetting the old gw and adjusting to the new ? If possible, I'd like to have a single code path that works for all scenarios.
The advantage is that we then don't need to send the lifetime=0 RAs and we do like on ipv4, we set the gatway router Network with it.
But we already have the code to do that. I'd rather re-use existing "fixes" than having to come up with solutions for different problems. I'd really rather not have to care about the gateway MAC address unless it is required. Generally speaking, I'd rather re-use the solution to a problem I know I can solve, than focusing on solving a new optional problem with a different solution.
What do you think ? we have to confirm with OVN people if we can configure /128 Networks at gateway router.
Also if in the future we go with transit router topology change we will be able to configure one mac fo all the nodes as the gateway then MTV importing the gateway mac make sense.
We can tackle it as part of that epic when it is prioritized. Prematurely optimizing for a feature we don't know when / if will be prioritized does not sound good (to me).
I still don't get what's the advantage that would bring, would you elaborate ? As I see it, since the VMs have to restart when they are imported, I don't see any advantage here.
As said just the fact that we will not need to send the lifetime=0 RAs.
But we already have code for that.
There was a problem hiding this comment.
Since MTV do not support IPv6 yet and we have found that routed get ditched on restart maybe we can skip this and when time comes implement IPv6 missing features for gateway (for example if by that time we are doing a live migration insteaf of restart).
| - DuplicateMACAddresses: will indicate to the admin that a MAC address appears | ||
| multiple times in the `Entries` list |
There was a problem hiding this comment.
Will the DuplicateMACAddresses block VM creation?
Bit worried about cross namespace migrations.
There was a problem hiding this comment.
IMHO it should, but this would only happen if there are duplicates in your network.
i.e. if you have two primary UDNs (one for each namespace) you can have the same MAC in both namespaces / networks.
But if you have a cluster UDN interconnecting two namespaces, you must have unique MACs in your network.
I don't see how you'll end up in this situation if you didn't have MAC collisions in the source cluster ... Unless you're mapping your networks wrong.
Could you elaborate on how we would reach this scenario ?
There was a problem hiding this comment.
As of the moment MTV is using the VirtualMachineExport [1] to migrate between cluster and namespaces.
In future we will be live migrating the VMs. During these migrations we will preserve the MACs so I'm bit worried that this could cause problems in future.
[1] https://kubevirt.io/user-guide/storage/export_api/
There was a problem hiding this comment.
Yes, but unless said VMs already had duplicate MACs on the source cluster, they won't have duplicate MACs in the dst cluster, right ?
Keep in mind we're (kind of ...) attempting to re-create the network that existed in the source cluster - we don't have L2 connectivity to it.
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
| This approach has the following advantages: | ||
| - single place the admin to manage for UDN | ||
| - similar to what is done on VmWare | ||
| - simple association between the `IPPool` and the logical network |
There was a problem hiding this comment.
I would also add a Pro: it would be easier to spot nasty duplicate IPs that can cause unintended traffic behavior.
(not saying I prefer this approach of course)
| **NOTE:** all the goals mentioned above will be fulfilled **only** for the | ||
| **cluster** UDN type. | ||
|
|
||
| ### Non-Goals |
There was a problem hiding this comment.
Should we add that we are not going to support ip overlapping since we are using BGP without EVPN ?
There was a problem hiding this comment.
I would assume that to be a limitation of the BGP integration w/ primary UDN.
Let them call that out.
If you insist, I can add a short sentence about it though.
| - MTV | ||
| - KubeVirt's ipam-extensions | ||
| - OVN-Kubernetes |
There was a problem hiding this comment.
I don't think so - there's nothing we need to design / implement in multus.
| **NOTE:** all the goals mentioned above will be fulfilled **only** for the | ||
| **cluster** UDN type. | ||
|
|
||
| ### Non-Goals |
There was a problem hiding this comment.
Should we mention ipv6 as non goal, kind of extra mile since MTV do not support it.
There was a problem hiding this comment.
we could ... still, nothing in the solution being presented here is IPv4 centric ..
it would work with IPv6 if the client app (MTV) managed to instruct which MACs and IPs to use.
There was a problem hiding this comment.
If some component/product supports something or not should be less relevant to the "need".
Is this needed by the user stories / users, that is the focus that needs to be expressed IMO in this section.
IPv6 has implications on implementation in some cases and possible requirements or limitations on the guest. I would prefer to have this proposal covering IPv6 as well, but if it requires follow up work, then I would recommend to emphasize it explicitly.
enhancements/network/routed-ingress-support-for-primary-udn-attached-vms-with-static-ips.md
Outdated
Show resolved
Hide resolved
| v1.multus-cni.io/default-network: '[{ | ||
| "name": "default", | ||
| "namespace": "openshift-ovn-kubernetes", | ||
| "mac": "02:03:04:05:06:07", | ||
| "ips": [ | ||
| "10.0.0.5/24" | ||
| ] | ||
| }]' |
There was a problem hiding this comment.
We have mention the possibility of excluding "name" and "namespace" and ipam extensions adding it, this way users do not need to know the ovn-kubernetes namespace
'[{
"name": "default",
"namespace": "openshift-ovn-kubernetes",
"mac": "02:03:04:05:06:07",
"ips": [
"10.0.0.5/24"
]
}]There was a problem hiding this comment.
it's not a bad idea - that'd allow MTV to only care about MACs and IPs.
How would we configure this in ipam-ext ? A configuration knob ?
There was a problem hiding this comment.
It's kind of defaulting, for ipam extensions if there is no "name" then it's "default" or if there is no "namespace" then it's the namespace where ovn-kubernetes is living (it will have to discover it).
There was a problem hiding this comment.
hm, I'd rather have a configuration knob to do that.
i.e. the operator that deploy the component should be aware of this, and pass the information along.
having to figure out where ovn-k is living is IMHO more work than adding configuration.
Still, it's a good alternative.
We need to listen for feedback. @kyrtapz / @tssurya / @trozet thoughts ?
| There should be no hypershift platform-specific considerations with this | ||
| feature. |
There was a problem hiding this comment.
@maiqueb we have see hypershift customers asking for fixed IPs, but I think that is different kind of animal to what we need with MTV, feels more of a possible centralized approach although hypershift VMs name and mac is randomtly generated.
| - is this feature only for VMs ? | ||
| - is this feature only about importing VMs ? Should we allow creating new VMs | ||
| with dedicated MAC / IP / gateway requests ? | ||
| - are the end users (admins) after a centralized IP management alternative ? |
There was a problem hiding this comment.
If the customers want centralized it can be implemented later on and it will annotate the VM with needed stuff.
|
|
||
| ## Alternatives (Not Implemented) | ||
|
|
||
| ### Requesting IP addresses on a VM's interfaces |
There was a problem hiding this comment.
I have added a comment bellow doing this but with annotations, maybe is not a bad option, this way we don't differenciate between primary and secondaries that's done by ipam extensions.
There was a problem hiding this comment.
for that to happen, we'd need to have a network dimension in the annotations, which your proposal lacks.
Could you update it ?
Maybe it's worth pursuing. I just assumed that using existing structures would streamline the development - I could be persuaded to adding yet another struct that looks like a trimmed down network selection element.
|
|
||
| The VM owner just has to use MTV to import the VM into CNV. | ||
|
|
||
| #### De-centralized IP management |
There was a problem hiding this comment.
The default-network approach is de-centralized too, we should say that this is the CRD backed de-centralized ip management, where the IP and MAC are at the CRD instead of default-network annotation.
There was a problem hiding this comment.
Do we really need to add more nomenclature for keeping track of discarded options ?
| The gateway for the network must be configured in the cluster UDN CR at | ||
| creation time, as any other cluster UDN parameter. |
There was a problem hiding this comment.
We should not support updating gateway field right ? like MTV will create new UDNs for the VMs.
There was a problem hiding this comment.
yeah, pretty much anything in UDN / C-UDN is immutable right now.
|
|
||
| The `ipam-extensions` mutating webhook will kick in whenever a virt launcher | ||
| pod is created - it will identify when the VM has a primary UDN attachment | ||
| (already happens today), and will also identify when the pod network attachment |
There was a problem hiding this comment.
why not let MTV/other create the v1.multus-cni.io/default-network annotation, and ipam-ext will only add the ipamClaimReference?
why should we add the MAC to VMI.Spec.Domain.Devices.Interfaces? I think it would get kubemacpool involved. I don't think we do that now on primary-udn right now. Am I mistaken?
There was a problem hiding this comment.
why not let MTV/other create the v1.multus-cni.io/default-network annotation, and ipam-ext will only add the ipamClaimReference?
that's my proposal, isn't it ?
why should we add the MAC to VMI.Spec.Domain.Devices.Interfaces? I think it would get kubemacpool involved. I don't think we do that now on primary-udn right now. Am I mistaken?
Isn't this what MTV does ? @mnecas please keep me honest.
kubemacpool should stay away from this.
There was a problem hiding this comment.
@maiqueb can you please be explicit as to what MTV does exactly? is it just:
- set the
v1.multus-cni.io/default-networkannotation - set the VM's VM.Spec.Template.Spec.Domain.Devices.Interfaces with the MAC
kubemacpool should stay away from this.
Setting the VM's (not VMIs) interface's MACs on the interfaces will get kubemacpool involved. it will cache the MACs and will prevent duplications if needed.
The only way to prevent KMP from doing this is to manually opt-out the namespace.
There was a problem hiding this comment.
@maiqueb can you please be explicit as to what MTV does exactly? is it just:
1. set the `v1.multus-cni.io/default-network` annotation 2. set the VM's VM.Spec.Template.Spec.Domain.Devices.Interfaces with the MAC
Let's have @mnecas reply to this.
kubemacpool should stay away from this.
Setting the VM's (not VMIs) interface's MACs on the interfaces will get kubemacpool involved. it will cache the MACs and will prevent duplications if needed. The only way to prevent KMP from doing this is to manually opt-out the namespace.
I don't want to change what MTV does today - if it sets that MAC on the interfaces, it'll still do it. If it doesn't, well, let's keep it that way.
All I want is for the annotation to be there, which is the only thing the ipam-extensions will care about.
There was a problem hiding this comment.
I don't want to change what MTV does today - if it sets that MAC on the interfaces, it'll still do it. If it doesn't, well, let's keep it that way.
All I want is for the annotation to be there, which is the only thing the ipam-extensions will care about.
just be aware that the way CNV is deployed now - if the VM's MAC is involved then KMP will also be in the game, so might never be able to recreate a duplicate MAC scenario.
There was a problem hiding this comment.
yeah we are already adding the mac to the interfaces
There was a problem hiding this comment.
There was a problem hiding this comment.
mmm thinking about this again - I was wrong. KMP will not interfere, beacause the VM's interface is neither masq nor the corresponding network is set to multus (see here).
| } | ||
| ``` | ||
|
|
||
| The `IPAMClaim` status will have (at least) the following conditions: |
There was a problem hiding this comment.
Are we also going to denote mac collision at the IPAMClaim status ? is so we have to define it here.
There was a problem hiding this comment.
Hm ... didn't think of that.
It would be useful, at least for persisting the error ...
Still, I'd rather keep anything MAC related out of the IPAMClaim.
Meaning, the answer is no - unless we come up w/ compelling reasons to do it.
maiqueb
force-pushed
the
routed-ingress-primary-udn-with-static-ips
branch
from
5d1c47b to
dfef0c9
Compare
enhancements/network/routed-ingress-support-for-primary-udn-attached-vms-with-static-ips.md
Outdated
Show resolved
Hide resolved
enhancements/network/routed-ingress-support-for-primary-udn-attached-vms-with-static-ips.md
Outdated
Show resolved
Hide resolved
| + // The IPs requested by the user | ||
| + // +optional | ||
| + IPRequests []CIDR `json:"ipRequests,omitempty"` | ||
| + MACRequest net.HardwareAddr `json:"macRequest,omitempty"` |
There was a problem hiding this comment.
What is a net.HardwareAddr? I suspect we will need to find a way to represent and validate this within the API as a string rather than using a net type
enhancements/network/routed-ingress-support-for-primary-udn-attached-vms-with-static-ips.md
Outdated
Show resolved
Hide resolved
| + // The name of the pod holding the IPAMClaim | ||
| + OwnerPod string `json:"ownerPod"` |
There was a problem hiding this comment.
Will it always be a Pod that is the owner of this IPAMClaim? Are there any other properties you might want to include about the ownership, for example, a lastTransitionTime?
Is it possible for an IPAMClaim to be unowned? How would you represent an unowned IPAMClaim? Would that be a condition?
There was a problem hiding this comment.
All claims are created without an owner.
When the CNI's IPAM component allocates an IP address for the pod, it'll update the IPAMClaim status.IPs and also the status.OwnerPod.
Meaning, a claim without owner is represented by an empty (or missing) OwnerPod.
There was a problem hiding this comment.
I assume that the ownerPod cannot change over time? Have you considered making this immutable once set?
There was a problem hiding this comment.
No, it surely can mutate.
Take into account the following virt use cases:
- vm live migration: a new pod is scheduled on the target node. the live VM is copied from the src pod to the dst pod. If the migration is successful, the VM then runs in the dst pod. We would need to mutate the owner pod, now indicating it is owned by the dst pod.
- vm stopped: we want to stop a VM, but we don't want to lose that IP assignment. Once the VM is started again, it will run in a new pod, and we'll have to set that as the IPAMClaim owner pod. Probably we'd want to remove the owner pod value when the vm is stopped.
There was a problem hiding this comment.
Ack that makes sense.
So I'm wondering if there are additional properties that the user might want to know about the owner pod, rather than just its name
Does the namespace matter? Does it matter when that owner pod was last changed? Anything about the pod status itself?
Would it make sense to make this a struct, so we at least have the option of adding additional information about the owner later
There was a problem hiding this comment.
Ack that makes sense.
So I'm wondering if there are additional properties that the user might want to know about the owner pod, rather than just its name
Does the namespace matter? Does it matter when that owner pod was last changed? Anything about the pod status itself?
No the namespace doesn't matter - the IPAMClaim must be in the same namespace of the pod for this to work.
Would it make sense to make this a struct, so we at least have the option of adding additional information about the owner later
This would be a nice addition - as you say, we can later on extend it to include more info - e.g. the timestamp where pod XYZ "claimed" the claim.
| + // The name of the pod holding the IPAMClaim | ||
| + OwnerPod string `json:"ownerPod"` | ||
| + // Conditions contains details for one aspect of the current state of this API Resource | ||
| + Conditions []metav1.Condition `json:"conditions,omitempty"` |
There was a problem hiding this comment.
Where does this API actually live? Is there a WIP PR that we can work on to work out validations and markers that are appropriate (e.g. the SSA listType=map markers are missing here)
There was a problem hiding this comment.
There was a problem hiding this comment.
Has this had any upstream API review at all? It might be worthwhile creating a DNM PR to o/api and then we can go through rounds of API review there to help get the API validations/documentation/shape into a convention happy place
| lastTransitionTime: "2025-05-13T11:56:00Z" | ||
| ``` | ||
|
|
||
| #### New IPPool CRD |
There was a problem hiding this comment.
Out of interest, have you ever looked at/compared this to how ClusterAPI is managing IPAM? It looks fairly similar
There was a problem hiding this comment.
no ... would you point me in the right direction ? Do you have some links at hand, or can point me towards who would know how to get them ?
There was a problem hiding this comment.
There are some details in https://cluster-api.sigs.k8s.io/developer/providers/contracts/ipam
@rvanderp3 has an implementation of the contract in https://github.com/rvanderp3/machine-ipam-controller/blob/main/pkg/apis/ipamcontroller.openshift.io/v1/types.go
There may be little overlap in the end but thought I'd call out that there are others creating IPPool types and show how they are working.
The Machine creates an IPAddressClaim, and then the controller in the referenced IPPool is responsible for populating the IPAddressClaim with the allocated IP address, and of course freeing that from the pool when deleted
This commit adds a simper approach as the preferred approach: it doesn't require any CRD spec updates, and relies on MTV templating the VM's metadata with the network selection element in the multus default network annotation key; we essentially customize the default cluster network attachment - OVN-Kubernetes will understand there's a primary UDN attachment configured in the namespace, and will ensure said customization applies to the primary UDN attachment, rather than to the cluster default network attachment. The other presented alternatives were moved to the discarded alternatives section, for clarity. Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
maiqueb
force-pushed
the
routed-ingress-primary-udn-with-static-ips
branch
from
dfef0c9 to
23070ef
Compare
|
@maiqueb: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
|
||
| - As the owner of a VM running in a traditional virtualization platform, I want | ||
| to import said VM - whose IPs were statically configured - into Kubernetes, | ||
| attaching it to an overlay network. I want to ingress/egress using the same IP |
There was a problem hiding this comment.
Attaching to the overlay network is a requirement or solution?
Is it possible to phrase it in a way we understand the need to use an overlay network vs something like localnet?
Based on the overview/summary, my understanding is that it comes from the existing situation where everything is managed for them by the solution (vs having a self managed separate network solution).
Do I understand it right?
It may also be more accurate if it is mentioned that the IP addresses are statically configured by the VM operator but uses dynamic methods to apply it on the guest (e.g. DHCP).
| For this kind of users, we need to have a way to enable their existing VMs to | ||
| run properly after being migrated into OpenShift, without any guest | ||
| configuration changes. For that, we need to import the VMs from their existing | ||
| platforms, preserving their existing MACs, IPs, and gateway configuration. |
There was a problem hiding this comment.
I understand why the IP and gateway needs to be preserved in case the guest configuration for these settings is statically configured.
It is less trivial why the MAC needs to be preserved.
Any chance the reason can be mentioned? I am asking about this because some reasons are local (e.g. the OS used can detect wrongly a mac change as a new NIC) and some may be "remote" (e.g. some access rule on a network device/machine or a DHCP record that assigns sticky addresses).
| configuration changes. For that, we need to import the VMs from their existing | ||
| platforms, preserving their existing MACs, IPs, and gateway configuration. | ||
|
|
||
| ### User Stories |
There was a problem hiding this comment.
There are some IP details that are missing and it would be nice to be explicit about them:
- Is this referring to both IPv4 and IPv6?
- What about DNS entries?
What about custom routes in general? (e.g. to reach 10.10.0.0/16, pass through 192.168.1.1).
| **NOTE:** all the goals mentioned above will be fulfilled **only** for the | ||
| **cluster** UDN type. | ||
|
|
||
| ### Non-Goals |
There was a problem hiding this comment.
If some component/product supports something or not should be less relevant to the "need".
Is this needed by the user stories / users, that is the focus that needs to be expressed IMO in this section.
IPv6 has implications on implementation in some cases and possible requirements or limitations on the guest. I would prefer to have this proposal covering IPv6 as well, but if it requires follow up work, then I would recommend to emphasize it explicitly.
|
|
||
| ### Non-Goals | ||
|
|
||
| - Handle importing VMs without a managed IP experience - i.e. IPs were defined |
There was a problem hiding this comment.
MTV can migrate powered-off VMs to which we don't have IPs.
AFAIK there are solution which set IP addresses explicitly on the management system that apply on VMs that come up (through DHCP or cloudinit). We do not have such users that expect it to pass to OCP-V?
@maiqueb , is it possible to specify from where we learn what is the configuration used (IP, etc)? If we do not know at the moment, maybe specify that it is expected as input. WDYT?
No description provided.