kubevirt: Force neighbors cache update after live migrtation #4416

qinqon · 2024-06-04T11:44:13Z

What this PR does and why is it needed

Improve connectivity after kubevirt live migration by forcing updating neighbors cache at VM and pods, it remove flakiness at difference scenarios. The details are at the first commit on the design doc.

Which issue(s) this PR fixes

Fixes #4237
Fixes #3986
Fixes https://issues.redhat.com/browse/SDN-4860

Special notes for reviewers

The main change to review is the first commit that contains the design doc changes, this is where review has to go. The rest of the PR is the PoC's code to illustrate the design doc, after design doc is ok the rest of the PR will be reworked.

PoC TODO:

Replace LSP deletion with mac address removal.

Details to documentation updates

live migration design docs updated

Description for the changelog

kubevirt: Force neighbors cache update after live migrtation

Does this PR introduce a user-facing change?

NONE

For more information on release notes see: TBD
-->

kubevirt: Force neighbors cache update after live migrtation

tssurya · 2024-06-04T12:46:51Z

docs, docs, please fix the docs issue here as well if possible @qinqon ?

qinqon · 2024-06-04T13:25:40Z

docs, docs, please fix the docs issue here as well if possible @qinqon ?

You mean https://issues.redhat.com/browse/SDN-4904, right ?

tssurya · 2024-06-04T13:45:41Z

I meant #4398 :D

ah they are the same!

qinqon · 2024-06-04T14:19:48Z

I meant #4398 :D

ah they are the same!

Addressed at different PR #4418

Sorry for the delay.

qinqon · 2024-06-11T10:51:26Z

Rebased

coveralls · 2024-06-11T11:19:26Z

coverage: 52.485% (-0.2%) from 52.729%
when pulling fa77351 on qinqon:kubevirt-neighbors-after-live-migration
into 17dce5c on ovn-org:master.

ricky-rav

Pushing my review comments for the first commit only. Will move to the other commits right away.

ricky-rav · 2024-06-11T10:08:09Z

docs/design/live-migration.md

+┌──────────────────────┐  │     │   ┌──────────────────────┐      
+│ lsp ns-virt-launcher1│──┘     └───│ lsp pod1             │     
+│ address:             │            │ address:             │    
+│  0a:58:0a:f4:00:01   │            │  0a:58:0a:f1:00:02   │


The MAC address for lsp pod1 shouldn't have changed, I suppose? Here I see that the f4 bytes became f1

My bad, will fix it.

ricky-rav · 2024-06-11T10:08:40Z

docs/design/live-migration.md

+```text
+    ┌────────────────────────┐ ┌────────────────────────┐
+    │logical switch node2    │ │logic switch node1      │ 
+    │   (10.244.1.0/24)      │ │   (10.244.1.0/24)      │


So here the subnet for logical switch node1 should be the same as before: 10.244.0.0/24

ricky-rav · 2024-06-11T10:09:30Z

docs/design/live-migration.md

+└─────────────────────────────────┘
+```
+
+Now the neighbors tables are incorrect since non of those mac address are 


non -> none, address -> addresses

ricky-rav · 2024-06-11T10:10:21Z

docs/design/live-migration.md

+Now the neighbors tables are incorrect since non of those mac address are 
+are part of the logical switches, after some time or traffic neighbors cache
+get invalidated and updated so they will point to the arp_proxy mac `0a:58:0a:f3:00:00`, problem
+is that it may take too much times and connections will be broken.


times -> time

ricky-rav · 2024-06-11T10:11:02Z

docs/design/live-migration.md

+is that it may take too much times and connections will be broken.
+
+To improve the situation ovn-k sends the following GARPs and unsolicited NA taking
+into account tha the arp proxy mac is `0a:58:0a:f3:00:00`:


tha -> that

ricky-rav · 2024-06-11T10:30:51Z

docs/design/live-migration.md

+
+At node2 advertise to VM that pod's from the same subnet should go over proxy_arp:
+- `GARP(10.244.0.9 -> 0a:58:0a:f3:00:00, vm mac(0a:58:0a:f4:00:01))`
+- one per pod at same subnet.


Just for clarity, here I would say something like `one per pod on the same subnet (in this example, only for pod1).

coveralls · 2024-06-13T12:58:13Z

coverage: 52.472% (-0.3%) from 52.729%
when pulling 4f58658 on qinqon:kubevirt-neighbors-after-live-migration
into 17dce5c on ovn-org:master.

ricky-rav

Mostly asking for more comments. I haven't looked at the last two commits yet.

ricky-rav · 2024-06-13T07:17:28Z

test/e2e/kubevirt.go

-						WithPolling(polling).
-						WithTimeout(timeout).
-						Should(Succeed(), func() string { return stage + ": " + podName + ": " + output })
+					output, err := kubevirt.RunCommand(vmi, fmt.Sprintf("curl http://%s", net.JoinHostPort(podIP, "8000")), 5*time.Second)


could you briefly explain why there's no need for the call to Eventually?

So Eventually was enmascarading the real issue, since that it's the time it takes for arp neighbors cache to be reconciled, removing it showed the issue and force for it to be solved.

ricky-rav · 2024-06-13T07:43:03Z

test/e2e/kubevirt.go

+				resource:               virtualMachine,
+				test:                   liveMigrate,
+				topology:               "localnet",
+				eastWestTrafficTimeout: 15 * time.Second,


So we're increasing the timeout from 5 to 15 seconds because we want to allow enough time for the GARPs to be sent on both nodes (old, new) and for ovn-k to remove the VM mac address from the old node LSP, right?

The issue we fix here is for default pod network, this tests are for multi-homing live migration we have lower expectations right now, we have some jira to improve live migration at multi-homing, that's why we put this value here to be able to re-use the east/west traffic function but make it pass at multi-homing live migration.

ricky-rav · 2024-06-13T12:38:33Z

go-controller/pkg/kubevirt/pod.go

@@ -146,6 +148,31 @@ func AllVMPodsAreCompleted(client *factory.WatchFactory, pod *corev1.Pod) (bool,
 	return true, nil
 }

+// isLiveMigrationInProgress return true if a live migration is still progressing


Could you add to the comment how live migration is detected? From what I understand (to continue your comment line) "that is when there's more than one pod associated to the same VM".

This is old code.

ricky-rav · 2024-06-13T12:40:49Z

go-controller/pkg/kubevirt/pod.go

@@ -199,6 +226,29 @@ func nodeContainsPodSubnet(watchFactory *factory.WatchFactory, nodeName string,
 	return false, nil
 }

+// findNodeOwningSubnet will return the node owning the subnet
+// contains the subnets from the argument


Here the sentence in the comment is broken :)
How about findNodeOwningSubnet returns the node owning the subnet found in the pod annotation passed as input

ricky-rav · 2024-06-13T14:44:16Z

go-controller/pkg/kubevirt/switch.go

+
+// notifyARPProxyMACForIP will send an GARP to force arp caches to clean up
+// at pods and reference to the arp proxy gw mac
+func notifyARPProxyMACForIPs(ips []*net.IPNet, dstMAC net.HardwareAddr) error {


This function is one of the core points from this PR. I know you described the global picture in the markdown document, but could you also describe here what's happening in the relevant functions and in the commit message?

Hi, so the commits implementing the design doc are from the PoC, if implementatino is "sound" I will refactor them with proper commit messages and structure.

coveralls · 2024-06-17T06:06:03Z

coverage: 52.452% (-0.3%) from 52.716%
when pulling 4413548 on qinqon:kubevirt-neighbors-after-live-migration
into 701b8e0 on ovn-org:master.

This change add the design to advertise neighbors after kubevirt's VM live migration to improve connectivity Signed-off-by: Enrique Llorente <[email protected]>

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

ricky-rav · 2024-06-19T07:48:01Z

The approach looks good to me. The pod informer used in the logic is a shared informer, so it shouldn't add overhead to the existing code.
/lgtm

coveralls · 2024-06-19T07:48:43Z

coverage: 52.492% (-0.3%) from 52.749%
when pulling c0d4d35 on qinqon:kubevirt-neighbors-after-live-migration
into 9f1f3f2 on ovn-org:master.

github-actions · 2024-09-18T01:56:05Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or reach out to maintainers for code reviews or consider closing this if you do not plan to work on it.

qinqon changed the title ~~Kubevirt neighbors after live migration~~ kubevirt: Force neighbors cache update after live migrtation Jun 4, 2024

qinqon marked this pull request as ready for review June 4, 2024 11:55

qinqon requested a review from a team as a code owner June 4, 2024 11:55

qinqon requested a review from ricky-rav June 4, 2024 11:55

tssurya added feature/kubevirt-live-migration All issues related to kubevirt live migration area/e2e-testing labels Jun 4, 2024

qinqon force-pushed the kubevirt-neighbors-after-live-migration branch from 207ac71 to fa77351 Compare June 11, 2024 10:51

github-actions bot added the kind/documentation All issues related to documentation label Jun 11, 2024

ricky-rav requested changes Jun 13, 2024

View reviewed changes

qinqon force-pushed the kubevirt-neighbors-after-live-migration branch from fa77351 to 4f58658 Compare June 13, 2024 12:30

qinqon requested a review from ricky-rav June 13, 2024 12:30

ricky-rav requested changes Jun 13, 2024

View reviewed changes

qinqon requested a review from ricky-rav June 17, 2024 05:36

qinqon force-pushed the kubevirt-neighbors-after-live-migration branch from 4f58658 to 4413548 Compare June 17, 2024 05:38

qinqon added 8 commits June 19, 2024 09:18

kubevirt, design: Advertise neighbors

5cb035c

This change add the design to advertise neighbors after kubevirt's VM live migration to improve connectivity Signed-off-by: Enrique Llorente <[email protected]>

kubevirt, e2e: Remove Eventually fron connectivity check

c7a3170

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

kubevirt, e2e: Add pod->vm traffic check

4066d0a

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

kubveirt, e2e: Incrase persistentips traffic timeout for migration

16e8069

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

kubevirt, IC: Send GARPs after live migration

2a84219

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

kubevirt, ic: Remove old LSPs and send just one GARP

4322ff5

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

kubevirt, IC: Send Unsolicited NA after live migration

5045fea

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

kubevirt: Move neighbors logic go ovn-node

c0d4d35

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

qinqon force-pushed the kubevirt-neighbors-after-live-migration branch from 4413548 to c0d4d35 Compare June 19, 2024 07:20

qinqon mentioned this pull request Jun 25, 2024

node: Use mdlayher/arp at link_network_manager #4433

Merged

qinqon mentioned this pull request Jul 10, 2024

udn: auto-exclude GW and mp0 IPs from the subnet #4509

Merged

github-actions bot added the lifecycle/stale All issues (> 60 days) and PRs (>90 days) with no activity. label Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubevirt: Force neighbors cache update after live migrtation #4416

kubevirt: Force neighbors cache update after live migrtation #4416

qinqon commented Jun 4, 2024 •

edited

Loading

tssurya commented Jun 4, 2024

qinqon commented Jun 4, 2024

tssurya commented Jun 4, 2024 •

edited

Loading

qinqon commented Jun 4, 2024

qinqon commented Jun 11, 2024

coveralls commented Jun 11, 2024

ricky-rav left a comment

ricky-rav Jun 11, 2024

qinqon Jun 13, 2024

ricky-rav Jun 11, 2024

ricky-rav Jun 11, 2024

ricky-rav Jun 11, 2024

ricky-rav Jun 11, 2024

ricky-rav Jun 11, 2024

coveralls commented Jun 13, 2024

ricky-rav left a comment

ricky-rav Jun 13, 2024

qinqon Jun 17, 2024

ricky-rav Jun 13, 2024

qinqon Jun 17, 2024

ricky-rav Jun 13, 2024

qinqon Jun 17, 2024

ricky-rav Jun 13, 2024

ricky-rav Jun 13, 2024

qinqon Jun 17, 2024

coveralls commented Jun 17, 2024

ricky-rav commented Jun 19, 2024

coveralls commented Jun 19, 2024

github-actions bot commented Sep 18, 2024

kubevirt: Force neighbors cache update after live migrtation #4416

Are you sure you want to change the base?

kubevirt: Force neighbors cache update after live migrtation #4416

Conversation

qinqon commented Jun 4, 2024 • edited Loading

What this PR does and why is it needed

Which issue(s) this PR fixes

Special notes for reviewers

Details to documentation updates

Description for the changelog

Does this PR introduce a user-facing change?

tssurya commented Jun 4, 2024

qinqon commented Jun 4, 2024

tssurya commented Jun 4, 2024 • edited Loading

qinqon commented Jun 4, 2024

qinqon commented Jun 11, 2024

coveralls commented Jun 11, 2024

ricky-rav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 13, 2024

ricky-rav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 17, 2024

ricky-rav commented Jun 19, 2024

coveralls commented Jun 19, 2024

github-actions bot commented Sep 18, 2024

qinqon commented Jun 4, 2024 •

edited

Loading

tssurya commented Jun 4, 2024 •

edited

Loading