Unify some functions in pkg/agent/route #3889

hongliangl · 2022-06-13T06:21:44Z

This PR unifies the functions in route_linux.go and route_windows.go:

AddLoadBalancer
DeleteLoadBalancer

Remove:

DeleteClusterIPRoute
AddClusterIPRoute

Fix:

Add the route for calculated ClusterIP CIDR to Service route cache,
then the route can be restored when it was deleted since members of
the route cache are synchronized periodically.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

hongliangl · 2022-06-13T06:33:10Z

/test-windows-proxyall-e2e

hongliangl · 2022-06-13T10:35:23Z

/test-windows-proxyall-e2e

hongliangl · 2022-06-13T10:35:32Z

/test-windows-e2e

hongliangl · 2022-06-13T13:32:22Z

/test-all

hongliangl · 2022-06-13T14:58:04Z

/test-e2e

pkg/agent/route/route_windows.go

wenyingd · 2022-06-21T04:26:35Z

pkg/agent/route/route_windows.go

+			return fmt.Errorf("error listing ip routes: %w", err)
+		}
+		for _, rt := range routes {
+			if rt.GatewayAddress.Equal(gw) && !rt.DestinationSubnet.IP.Equal(svcIP) && rt.DestinationSubnet.Contains(svcIP) {


The newly installed route in line 269 also satisfies this condition, so is it possible it is removed by mistake?

Since AntreaProxy only has one worker (goroutine), it cannot be removed by mistake since it is only used when the first Service is installed, and curClusterIPCIDR is nil.

pkg/agent/route/route_windows.go

wenyingd · 2022-06-21T04:38:33Z

pkg/agent/route/route_windows.go

+	nodeConfig      *config.NodeConfig
+	networkConfig   *config.NetworkConfig
+	nodeRoutes      *sync.Map
+	serviceRoutes   *sync.Map


I didn't find functions to load route entry from serviceRoutes, all calls are for store or delete. Is this cache really needed?

Add comments to explain what is stored in serviceRoutes?

pkg/agent/route/route_windows.go

codecov-commenter · 2022-06-21T13:11:24Z

Codecov Report

Merging #3889 (161692c) into main (6590c2a) will decrease coverage by 4.15%.
The diff coverage is 21.05%.

❗ Current head 161692c differs from pull request most recent head 3975a91. Consider uploading reports for the commit 3975a91 to get more accurate results

@@            Coverage Diff             @@
##             main    #3889      +/-   ##
==========================================
- Coverage   71.95%   67.81%   -4.15%     
==========================================
  Files         406      403       -3     
  Lines       60766    58449    -2317     
==========================================
- Hits        43723    39635    -4088     
- Misses      14113    16046    +1933     
+ Partials     2930     2768     -162

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.59% <21.05%> (+0.20%)`	⬆️
integration-tests	`34.47% <ø> (+0.69%)`	⬆️	Carriedforward from 3ecf6fd
kind-e2e-tests	`38.55% <ø> (-8.56%)`	⬇️	Carriedforward from 3ecf6fd
unit-tests	`59.97% <ø> (-2.65%)`	⬇️	Carriedforward from 3ecf6fd

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <ø> (ø)`
pkg/agent/agent.go	`55.87% <ø> (-1.20%)`	⬇️
...gent/controller/noderoute/node_route_controller.go	`63.93% <0.00%> (-5.53%)`	⬇️
pkg/agent/nodeportlocal/rules/netnat_rule.go	`0.00% <ø> (ø)`
pkg/agent/proxy/proxier.go	`66.40% <ø> (-1.96%)`	⬇️
pkg/agent/route/route_windows.go	`30.76% <ø> (-6.54%)`	⬇️
pkg/agent/util/net.go	`47.56% <0.00%> (-35.39%)`	⬇️
pkg/agent/util/net_windows.go	`7.25% <ø> (-76.68%)`	⬇️
pkg/agent/route/route_linux.go	`69.42% <23.52%> (-4.66%)`	⬇️

... and 106 files with indirect coverage changes

pkg/agent/route/route_linux.go

wenyingd · 2022-07-08T03:15:15Z

pkg/agent/route/route_linux.go

 			}
 		}
 	}

 	// Remove stale routes.
 	for _, rt := range staleRoutes {
 		if err = netlink.RouteDel(rt); err != nil {
-			return fmt.Errorf("failed to uninstall stale ClusterIP route %s: %w", rt.String(), err)
+			if err.Error() == "no such process" {
+				klog.InfoS("Failed to delete stale ClusterIP route since the route doesn't exist", "route", route)


No need for this log if the routing entry does not exist?

This was introduced in this PR https://github.com/antrea-io/antrea/pull/3827/files.

wenyingd · 2022-07-08T03:24:00Z

pkg/agent/route/route_linux.go

 		for i := 0; i < len(routes); i++ {
-			if routes[i].Gw.Equal(gw) && !routes[i].Dst.IP.Equal(svcIP) && routes[i].Dst.Contains(svcIP) {
+			// Skip the route whose destination CIDR only has one address.


Why skip the routing entry for a single IP address?

The routing entry for a single IP address (mask is 32 or 128) could be for LoadBalancer or the first ClusterIP, and such routing entry should be reserved.

The code comment is unclear and puzzling.
And I feel the above explanation still doesn't make sense. It will never delete stale LoadBalancer IPs, and it doesn't make sense to handle first ClusterIP in this way.

How about

// Skip the route whose destination CIDR has only one address since it could be the route for the first // ClusterIP newly installed. Note that, this could also match a route for a LoadBalancer ingress IP, but // it doesn't affect skipping the route for the first ClusterIP.

@tnqn

wenyingd · 2022-07-08T03:33:17Z

pkg/agent/route/route_windows.go

+	nodeConfig      *config.NodeConfig
+	networkConfig   *config.NetworkConfig
+	nodeRoutes      *sync.Map
+	serviceRoutes   *sync.Map


Add comments to explain what is stored in serviceRoutes?

pkg/agent/route/route_windows.go

pkg/agent/util/net_windows.go

hongliangl · 2022-08-26T00:36:32Z

/test-windows-proxyall-e2e
/test-windows-e2e
/test-e2e

hongliangl · 2022-08-26T02:04:00Z

/test-windows-proxyall-e2e
/test-windows-e2e

pkg/agent/route/route_windows.go

wenyingd · 2022-08-26T03:35:03Z

pkg/agent/route/route_windows.go

+	route := generateRoute(svcIPNet, gw, linkIndex, metric)
+	if err := util.RemoveNetRoute(route); err != nil {
+		if strings.Contains(err.Error(), "No matching MSFT_NetRoute objects") {
+			klog.InfoS("Failed to delete LoadBalancer ingress IP route since the route doesn't exist", "route", route)


No need for this log since the final result is the route is not existing, which is as expected?

This is used to consistent with Linux code.

pkg/agent/route/route_windows.go

hongliangl · 2022-08-26T10:41:32Z

/test-windows-proxyall-e2e
/test-windows-e2e

tnqn

Some nits, otherwise LGTM

pkg/agent/route/route_linux.go

tnqn · 2023-03-30T15:00:20Z

pkg/agent/route/route_windows.go

+		if reflect.DeepEqual(rt.DestinationSubnet, util.NewIPNet(c.nodeConfig.GatewayConfig.IPv4)) {
+			continue
+		}


was there a bug causing this route to be deleted?

This is for the comment #3889 (comment)

I know why it is added. But was there a bug causing this route to be deleted? If yes, what's the consequence of missing this route?

I think there was no bug causing this route to be deleted. The route could be only deleted manually by mistake. This is copied from Linux code, see

antrea/pkg/agent/route/route_linux.go

Line 269 in d7ffc85

// These routes are installed automatically by the kernel when the address is configured on

.

Does listRoutes contain the route? If yes, does the checks in Reconcile exclude the route?

For example, add IP 192.168.2.1 to interface antrea-gw0, and the following routes will be installed automatically.

192.168.2.0 255.255.255.0 On-Link 192.168.2.1 281 192.168.2.1 255.255.255.255 On-Link 192.168.2.1 281 192.168.2.255 255.255.255.255 On-Link 192.168.2.1 281

In listRoutes, the broadcast route (the third one) will be ignored.

if reflect.DeepEqual(rt.DestinationSubnet, c.nodeConfig.PodIPv4CIDR) { continue }

The code above is used to exclude the first route in Reconcile.

if reflect.DeepEqual(rt.DestinationSubnet, util.NewIPNet(c.nodeConfig.GatewayConfig.IPv4)) { continue }

The code above is used to exclude the second route in Reconcile.

I'm aware of this, but it's not related to my question:

But was there a bug causing this route to be deleted? If yes, what's the consequence of missing this route?

Not sure how to further explain. The check is newly added, right? Then how did it work previously if the route was removed?

The check is newly added, right?

Yes, the check is newly added.

Then how did it work previously if the route was removed?

If I remember correctly, if the route is removed, it will not be restored.

pkg/agent/route/route_windows.go

pkg/agent/util/net_windows.go

hongliangl · 2023-03-31T01:54:33Z

/test-all
/test-windows-all

hongliangl · 2023-03-31T09:25:48Z

pkg/agent/route/route_windows.go

+		// 10.10.0.0             255.255.255.0    On-link  10.10.0.1  281
+		// 10.10.0.255           255.255.255.255  On-link  10.10.0.1  281
+		// The host (10.10.0.1) and broadcast (10.10.0.255) routes should be ignored since they are unnecessary and redundant.
+		// We can ignore them by comparing them to the calculated broadcast IP.
 		if !rt.GatewayAddress.Equal(config.VirtualServiceIPv4) && rt.DestinationSubnet.IP.Equal(iputil.GetLocalBroadcastIP(rt.DestinationSubnet)) {


The host route is ignored here since the broadcast IP of the destination IP in host route is the destination itself. @tnqn

then should we remove the new check added here https://github.com/antrea-io/antrea/pull/3889/files/720023a9ed444a790842d8a62db295f70161d315#r1153399542? otherwise the code is contradictory.

Yes, we should remove the check since the route it should match will not be collected in Antrea.

tnqn · 2023-03-31T14:40:46Z

pkg/agent/route/route_windows.go

+		// 10.10.0.0             255.255.255.0    On-link  10.10.0.1  281
+		// 10.10.0.255           255.255.255.255  On-link  10.10.0.1  281
+		// The host (10.10.0.1) and broadcast (10.10.0.255) routes should be ignored since they are unnecessary and redundant.
+		// We can ignore them by comparing them to the calculated broadcast IP.
 		if !rt.GatewayAddress.Equal(config.VirtualServiceIPv4) && rt.DestinationSubnet.IP.Equal(iputil.GetLocalBroadcastIP(rt.DestinationSubnet)) {


then should we remove the new check added here https://github.com/antrea-io/antrea/pull/3889/files/720023a9ed444a790842d8a62db295f70161d315#r1153399542? otherwise the code is contradictory.

pkg/agent/route/route_windows.go

This PR unifies the functions in route_linux.go and route_windows.go: - AddLoadBalancer - DeleteLoadBalancer Remove: - DeleteClusterIPRoute - AddClusterIPRoute Fix: - Add the route for calculated ClusterIP CIDR to Service route cache, then the route can be restored when it was deleted since members of the route cache are synchronized periodically. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

tnqn

LGTM

tnqn · 2023-03-31T16:10:53Z

/test-all
/test-windows-all

hongliangl · 2023-04-04T06:34:08Z

/test-windows-networkpolicy
/test-windows-conformance
/test-windows-networkpolicy

hongliangl · 2023-04-04T06:34:46Z

/test-windows-containerd-networkpolicy

hongliangl · 2023-04-04T12:08:14Z

/test-windows-conformance

hongliangl · 2023-04-04T12:08:19Z

/test-windows-containerd-networkpolicy

This reverts commit 6143e78.

This commit unifies the following functions in route_linux.go and route_windows.go: - AddLoadBalancer - DeleteLoadBalancer Removes: - DeleteClusterIPRoute - AddClusterIPRoute Adds a fix to restore the ClusterIP CIDR route after it is deleted, by saving the route to the Service route cache which will be re-sync'd periodically. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl requested review from wenyingd and jianjuns June 13, 2022 06:21

hongliangl added action/release-note Indicates a PR that should be included in release notes. area/OS/windows Issues or PRs related to the Windows operating system. labels Jun 13, 2022

hongliangl force-pushed the unify-route branch from 0ddc1f8 to 58abaf6 Compare June 13, 2022 06:32

hongliangl force-pushed the unify-route branch from f5a9d68 to a1fdc25 Compare June 13, 2022 10:33

hongliangl requested review from antoninbas and tnqn June 14, 2022 00:01

wenyingd reviewed Jun 21, 2022

View reviewed changes

hongliangl force-pushed the unify-route branch from a1fdc25 to 50c1db6 Compare June 21, 2022 12:00

hongliangl requested a review from wenyingd June 21, 2022 12:14

hongliangl force-pushed the unify-route branch from 50c1db6 to 3122cec Compare June 23, 2022 15:22

wenyingd reviewed Jul 8, 2022

View reviewed changes

hongliangl force-pushed the unify-route branch from 3122cec to 701c17b Compare July 10, 2022 16:21

hongliangl force-pushed the unify-route branch from 701c17b to d22cf11 Compare August 26, 2022 00:14

hongliangl force-pushed the unify-route branch from d22cf11 to 70c4aff Compare August 26, 2022 02:39

wenyingd reviewed Aug 26, 2022

View reviewed changes

hongliangl force-pushed the unify-route branch from 70c4aff to 14e414e Compare August 26, 2022 10:40

hongliangl force-pushed the unify-route branch 2 times, most recently from 8d51d79 to 400476a Compare March 16, 2023 15:22

hongliangl mentioned this pull request Mar 16, 2023

Fix route deletion for Service ClusterIP and LoadBalancerIP #4711

Merged

hongliangl force-pushed the unify-route branch from 400476a to 1b0e891 Compare March 17, 2023 01:31

hongliangl modified the milestones: Antrea v1.11 release, Antrea v1.12 release Mar 20, 2023

hongliangl force-pushed the unify-route branch 3 times, most recently from 0d04ed2 to 2761075 Compare March 27, 2023 11:54

tnqn reviewed Mar 30, 2023

View reviewed changes

hongliangl force-pushed the unify-route branch from 2761075 to 3975a91 Compare March 31, 2023 01:53

hongliangl requested a review from tnqn March 31, 2023 01:54

hongliangl force-pushed the unify-route branch from 3975a91 to 720023a Compare March 31, 2023 09:23

hongliangl commented Mar 31, 2023

View reviewed changes

tnqn reviewed Mar 31, 2023

View reviewed changes

hongliangl force-pushed the unify-route branch from 720023a to e6fd626 Compare March 31, 2023 14:48

tnqn approved these changes Mar 31, 2023

View reviewed changes

jianjuns merged commit 6143e78 into antrea-io:main Apr 4, 2023

XinShuYang added a commit to XinShuYang/antrea that referenced this pull request Apr 5, 2023

Revert "Unify some functions in pkg/agent/route (antrea-io#3889)"

f7e368f

This reverts commit 6143e78.

tnqn mentioned this pull request May 24, 2023

Release 1.12.0 #5014

Merged

NamanAg30 mentioned this pull request Jun 13, 2023

[Windows] Additional line breaks exists when parsing NetNAT CIDR #4915

Closed

hongliangl deleted the unify-route branch December 5, 2023 05:43

Unify some functions in pkg/agent/route #3889

Unify some functions in pkg/agent/route #3889

Conversation

hongliangl commented Jun 13, 2022 • edited Loading

hongliangl commented Jun 13, 2022

hongliangl commented Jun 13, 2022

hongliangl commented Jun 13, 2022

hongliangl commented Jun 13, 2022

hongliangl commented Jun 13, 2022

Choose a reason for hiding this comment

hongliangl Jun 21, 2022 • edited Loading

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

codecov-commenter commented Jun 21, 2022 • edited by codecov bot Loading

Codecov Report

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

hongliangl Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl commented Aug 26, 2022

hongliangl commented Aug 26, 2022

Choose a reason for hiding this comment

hongliangl Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

hongliangl commented Aug 26, 2022

tnqn 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

hongliangl commented Mar 31, 2023

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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Mar 31, 2023

hongliangl commented Apr 4, 2023

hongliangl commented Apr 4, 2023

hongliangl commented Apr 4, 2023

hongliangl commented Apr 4, 2023

hongliangl commented Jun 13, 2022 •

edited

Loading

hongliangl Jun 21, 2022 •

edited

Loading

codecov-commenter commented Jun 21, 2022 •

edited by codecov bot

Loading

hongliangl Sep 6, 2022 •

edited

Loading

hongliangl Aug 26, 2022 •

edited

Loading