Skip to content
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

Hypershift live migration #3478

Merged
merged 8 commits into from
Jul 19, 2023
Merged

Conversation

qinqon
Copy link
Contributor

@qinqon qinqon commented Mar 9, 2023

- What this PR does and why is it needed
Add support to hypershift kubevirt provider live migration by doing the following at hypershift workers virt-launcher pods:

  • Skip CNI network configuration
  • Add DHCP options at pod's LSP
  • Configure ARP Proxy at router type LSP
  • Use point to point routes to enroute pod's traffic
  • Copy the ovn pod annotations from source to target virt-launchers during live migration.

- Special notes for reviewers
Changes for IC not included at the enhancement:

  • Add a static route matching cluster wide cidr with nexthop to gw router.
  • At remote zone add a ptp static route to the transit switch port
  • Prevent remote zone ip deallocation for VMs
  • Do remote zone static routes and DHCP Options cleanup after live migration.
  • Add sync function that will remove policy/routes that do not belong to vm and zone

Depends on:

- Follow ups

  • Check if we can configure ipv6 autoconf with ovn
  • Check why post-copy live migration test do not work at github actions.
    • event for worker1: {virt-handler ovn-worker2} Migrated: VirtualMachineInstance migration uid 75effe84-8f58-4890-b8ac-6995d74bd4ae failed. reason:Live migration failed error encountered during MigrateToURI3 libvirt api call: virError(Code=1, Domain=10, Message='internal error: unable to execute QEMU command 'migrate-set-capabilities': Postcopy is not supported')
  • Use address set to reduce the number of logical flows
  • Configure dhcp options dns using pod's resolver.conf or explicit from CNO
  • IPv6 single stack:
    • OVN do not support autoconf so it has to be disabled at node to receive an ipv6 address
    • OVN only support delivering dns_server (which is not working for us)
    • ARP proxy default gw and hostname will have to be configured with ignition
  • Move getExpectedSwitches to free network
  • Try to move EnsureRoutingForVM to addLogicalPortToNetwork to prevent readiness until routing is not in place.
  • Refactor deleteLogicalPort so be able to call ip deallocate without calling it.

- How to verify it
Run KubeVirt e2e tests or follow example at the doc on the PR

- Description for the changelog
Add live migration support for KubeVirt VMs using default network

@qinqon qinqon force-pushed the hypershift-live-migration branch 2 times, most recently from f380c09 to aee8984 Compare March 9, 2023 13:46
@coveralls
Copy link

coveralls commented Mar 9, 2023

Coverage Status

Coverage: 52.925% (+0.06%) from 52.869% when pulling 93a0ba8 on qinqon:hypershift-live-migration into 9e21b3a on ovn-org:master.

@qinqon qinqon force-pushed the hypershift-live-migration branch 19 times, most recently from 188e45b to 948521c Compare March 15, 2023 08:24
@qinqon qinqon force-pushed the hypershift-live-migration branch 8 times, most recently from 95908a0 to af18346 Compare March 23, 2023 09:06
@qinqon qinqon force-pushed the hypershift-live-migration branch 5 times, most recently from a847ac8 to ced79da Compare July 18, 2023 05:26
qinqon added a commit to qinqon/machine-config-operator that referenced this pull request Jul 18, 2023
The live migration feature implemented at [1] creates a stable default
gw for IPv4 and IPv6, for IPv4 it's delivered using DHCPv4 but for IPv6
there is not support for routes at DHCPv6. This change harcoded the IPv6
stable default gw using ignition.

[1] ovn-org/ovn-kubernetes#3478

Signed-off-by: Enrique Llorente <[email protected]>

// There is a VM still running we should not deallocate the IP
// check to make sure no other pods are using this IP before we try to release it if this is a completed pod.
if !isMigratedSourcePodStale && util.PodCompleted(pod) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this. Shouldn't we be doing

if isMigratedSourcePodStale {
    return false, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also

if !util.PodCompleted(pod) {                                                
        return true, nil                                                        
    } 

// controller's zone.
func (bnc *BaseNetworkController) ensurePodAnnotation(pod *kapi.Pod, nadName string) (*util.PodAnnotation, bool, error) {
zoneContainsPodSubnet := true
if !kubevirt.IsPodLiveMigratable(pod) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this the other way around?

if kubevirt.IsPodLiveMigratable(pod) {
   ....
}

For clarity, I would prefer the special case (live migratable pods) to be handled in the if block, not the other way around

@@ -339,6 +340,24 @@ func (allocator *allocator) ForSubnet(name string) NamedAllocator {
}
}

// FindSwitchBySubnets will find the switch that contains one of the subnets
// from "subnets" if not it will return "", false
func (allocator *allocator) FindSwitchBySubnets(subnets []*net.IPNet) (string, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allocator does not have the notion of a Switch. We need a different name. Maybe GetSubnetName or FindSubnetName or FindNameBySubnets , in order of my preference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going for GetSubnetName

}

return oc.removeRemoteZonePod(pod)
return kubevirt.CleanUpLiveMigratablePod(oc.nbClient, oc.watchFactory, pod)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. My comment was alluding to clarity, not to a functional issue. It's a nit, up to you.

@qinqon qinqon force-pushed the hypershift-live-migration branch 2 times, most recently from 2a2e5fc to 5ad69dd Compare July 18, 2023 12:06
@qinqon qinqon force-pushed the hypershift-live-migration branch 3 times, most recently from cfa6aab to 28b0f5c Compare July 18, 2023 18:04
At live migration the IP has to follow the VM and at kubevirt it means
that the new virt-launcher pod has to re-use the address from the old
virt-launcher. This change annotate the new virt-launcher pods with the
ip configuration of the old one so ipam can take over.

Signed-off-by: Enrique Llorente <[email protected]>
The hypershift workers use DHCP for IP configuration, this
change configure the ipv4/ipv6 DHCP options from the VM's LSP with the
cidr from switch subnet, harcode arp proxy IP as default gw and the dns
server from kubernetes or openshift service, it also configure the
"arp_proxy" option at the LSP.

Signed-off-by: Enrique Llorente <[email protected]>
Live migrated pods will keep their IP and it will be from a subnet
different from the node switch, to continue routing to the proper node a
point to point route need to be added to ovn_cluster_router that will
change at live migration. This change add that route and implement the
live migration switch.

Signed-off-by: Enrique Llorente <[email protected]>
This change document the KubeVirt live migration explaining how to use
it and illustrating it with an example.

Signed-off-by: Enrique Llorente <[email protected]>
This change add e2e tests to check pre and post copy live migration, for
that the "kind.sh" script has learn to install kubevirt. Github actions
also include jobs to exercise it, the post-copy test has being disabled
for them since it's not working at that env.

To test that tcp connection survives live migration http connection
reuse is check.

Signed-off-by: Enrique Llorente <[email protected]>
If a new node is added we have the ip pool has to allocate the vm ips
if node is taking over the vm subnet.

Signed-off-by: Enrique Llorente <[email protected]>
This change group together the ip pool re-fill logic at "syncPods" and
"addUpdateLocalNodeEvent" for live migratable pods.

Signed-off-by: Enrique Llorente <[email protected]>
@qinqon qinqon force-pushed the hypershift-live-migration branch from 28b0f5c to ee312fe Compare July 18, 2023 18:24
qinqon added a commit to qinqon/cluster-network-operator that referenced this pull request Jul 19, 2023
For the live-migration ovn-kuberntes features [1] CNO need to pass the
namespace and name of the dns resolver service.

[1] ovn-org/ovn-kubernetes#3478

Signed-off-by: Enrique Llorente <[email protected]>
qinqon added a commit to qinqon/cluster-network-operator that referenced this pull request Jul 19, 2023
For the live-migration ovn-kuberntes features [1] CNO need to pass the
namespace and name of the dns resolver service.

[1] ovn-org/ovn-kubernetes#3478

Signed-off-by: Enrique Llorente <[email protected]>
qinqon added a commit to qinqon/cluster-network-operator that referenced this pull request Jul 19, 2023
For the live-migration ovn-kuberntes features [1] CNO need to pass the
namespace and name of the dns resolver service.

[1] ovn-org/ovn-kubernetes#3478

Signed-off-by: Enrique Llorente <[email protected]>
@jcaamano jcaamano dismissed trozet’s stale review July 19, 2023 12:27

It looks like @qinqon resolved the requested changes

@jcaamano jcaamano merged commit fd7267c into ovn-org:master Jul 19, 2023
27 checks passed
@jcaamano
Copy link
Contributor

Checked downstream CI here
openshift/ovn-kubernetes#1737

@fabiand
Copy link

fabiand commented Jul 19, 2023

🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants