Fix that Service routes may get lost when starting on Windows #4470

hongliangl · 2022-12-13T07:34:22Z

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

codecov · 2022-12-13T07:51:13Z

Codecov Report

Merging #4470 (e8582e2) into main (b503a46) will increase coverage by 0.95%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #4470      +/-   ##
==========================================
+ Coverage   65.89%   66.84%   +0.95%     
==========================================
  Files         402      378      -24     
  Lines       57247    54978    -2269     
==========================================
- Hits        37723    36751     -972     
+ Misses      16822    15519    -1303     
- Partials     2702     2708       +6

Flag	Coverage Δ
integration-tests	`34.69% <100.00%> (+0.10%)`	⬆️
kind-e2e-tests	`40.22% <50.00%> (-7.63%)`	⬇️
unit-tests	`53.27% <50.00%> (+2.23%)`	⬆️

Impacted Files	Coverage Δ
...gent/controller/noderoute/node_route_controller.go	`58.50% <0.00%> (-0.79%)`	⬇️
pkg/agent/route/route_linux.go	`70.89% <100.00%> (-1.43%)`	⬇️
...g/agent/controller/serviceexternalip/controller.go	`35.83% <0.00%> (-45.74%)`	⬇️
pkg/features/antrea_features.go	`64.00% <0.00%> (-36.00%)`	⬇️
pkg/agent/flowexporter/exporter/exporter.go	`56.18% <0.00%> (-25.50%)`	⬇️
...nt/apiserver/handlers/serviceexternalip/handler.go	`29.62% <0.00%> (-22.23%)`	⬇️
cmd/antrea-agent/options.go	`3.55% <0.00%> (-20.31%)`	⬇️
pkg/agent/ipassigner/responder/arp_responder.go	`55.29% <0.00%> (-17.65%)`	⬇️
...pportbundlecollection/support_bundle_controller.go	`47.98% <0.00%> (-17.59%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`61.48% <0.00%> (-15.55%)`	⬇️
... and 86 more

pkg/agent/controller/noderoute/node_route_controller.go

tnqn · 2022-12-13T07:55:59Z

pkg/agent/controller/noderoute/node_route_controller.go

+			for _, ingressIP := range svc.Status.LoadBalancer.Ingress {
+				ingressIPNet := util.NewIPNet(net.ParseIP(ingressIP.IP)).String()
+				desiredClusterIPSvcIPs.Insert(ingressIPNet)


It would cause potential issue by appending the ingress IPs to a set for cluster IPs. For example, it could cause the calculated ClusterIP CIDR much bigger than actual value.
If the needed set is supposed to contain both IPs, please rename the variable.

This is only for Windows, Linux ClusterIP CIDR is not calculated from this.

By potential issue, I meant it could cause a bug in future if others think the set contains pure cluster IPs, which is mislead by the name.

Got that. Then how about change desiredClusterIPSvcIPs to desiredSvcIPNets?

test/integration/agent/route_test.go

wenyingd

LGTM

shettyg · 2022-12-13T15:25:09Z

Thanks @hongliangl !

It looks to me that addServiceRoute() still has the issue. The key at [1] is different than the key at [2].
[1]:

antrea/pkg/agent/route/route_windows.go

Line 263 in 19a1254

obj, found := c.hostRoutes.Load(svcIP.String())

[2]:

antrea/pkg/agent/route/route_windows.go

Line 291 in 19a1254

c.hostRoutes.Store(route.DestinationSubnet.String(), route)

shettyg · 2022-12-13T15:30:03Z

Also, it looked to me that Reconcile() and Add addServiceRoute() can be called at the same time. So, addServiceRoute() can add a route and at the same time, Reconcile() can delete that added route. So there is a race condition.

hongliangl · 2022-12-13T15:31:14Z

antrea/pkg/agent/route/route_windows.go

Good catch! I'll correct it.

hongliangl · 2022-12-13T15:44:28Z

Also, it looked to me that Reconcile() and Add addServiceRoute() can be called at the same time. So, addServiceRoute() can add a route and at the same time, Reconcile() can delete that added route. So there is a race condition.

Yes, they can be called at the same time, but all routes can be installed correctly finally. For example:

For a Service, if Reconcile is called first, corresponding route will not be delete, and a mapping SvcIPNet -> route will be stored to sync.Map hostRoutes; then addServiceRoute is called, and the mapping SvcIPNet -> route is stored in hostRoutes, then do nothing.

For a Service, if addServiceRoute is called first and no mapping ipNetString -> route is found, then install the route and update SvcIPNet -> route to hostRoutes; then Reconcile is called, the route may be included or not in c.listRoutes(), but the route will be delete in Reconcile finally.

shettyg · 2022-12-13T16:22:17Z

@hongliangl Consider the following scenario

Reconcile() is about to be called with 500 services. Before Reconcile() calls listRoutes(), the following happens.
501st new service is created in k8s and addServiceRoute() is called.
addServiceRoute() installs a new route
Reconcile() calls listRoute() and gets newly added route in the list
Reconcile() thinks that the newly added route is stale and deletes it.

hongliangl · 2022-12-13T16:32:16Z

@hongliangl Consider the following scenario

Reconcile() is about to be called with 500 services. Before Reconcile() calls listRoutes(), the following happens.

501st new service is created in k8s and addServiceRoute() is called.

addServiceRoute() installs a new route

Reconcile() calls listRoute() and gets newly added route in the list

Reconcile() thinks that the newly added route is stale and deletes it.

You are right for this case, but if no new Service is created (without step 2), current code is ok. To cover the case you mentioned above, I'll change the code which is like in route_linux.go.

In Reconcile(), skip the route whose gateway is the virtual IP or the destination is the virtual IP.
When starting, every route for Service will installed unconditionally.

shettyg · 2022-12-13T21:55:54Z

When starting, every route for Service will installed unconditionally.

Thanks. So with the new code, stale routes remain as-is, correct?

hongliangl · 2022-12-14T01:31:37Z

When starting, every route for Service will installed unconditionally.

Thanks. So with the new code, stale routes remain as-is, correct?

Yes, in this way, at least there will be no ClusterIP that is unavailable.

pkg/agent/route/route_windows.go

hongliangl · 2023-03-01T07:46:40Z

Do we need to backport this PR? If we don't need to backport this, I'll close this PR since #4221 can also fix the issue. @tnqn @shettyg @wenyingd

pkg/agent/route/route_windows.go

pkg/agent/controller/noderoute/node_route_controller.go

tnqn · 2023-03-09T16:50:03Z

pkg/agent/route/route_windows.go

 	svcIPNet := util.NewIPNet(svcIP)
+	obj, found := c.hostRoutes.Load(svcIPNet.String())


L309 also needs update, can we add unit tests to cover them?

L309 also needs update
Done

can we add unit tests to cover them?

Add some test code to verify the cached route in c.hostRoutes

pkg/agent/route/route_windows.go

Fix antrea-io#4467 Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

tnqn

LGTM

tnqn · 2023-03-10T10:13:43Z

/test-all
/test-windows-all

…-io#4470) Fix antrea-io#4467 Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl requested review from wenyingd and tnqn December 13, 2022 07:34

hongliangl added action/backport Indicates a PR that requires backports. 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 Dec 13, 2022

hongliangl mentioned this pull request Dec 13, 2022

Antrea1.4: Windows node service route logic looks to be broken #4467

Closed

tnqn reviewed Dec 13, 2022

View reviewed changes

hongliangl force-pushed the 20221213-fix-4467 branch 2 times, most recently from 186d6f5 to 19a1254 Compare December 13, 2022 08:49

wenyingd previously approved these changes Dec 13, 2022

View reviewed changes

hongliangl dismissed wenyingd’s stale review via cf9e7cc December 13, 2022 15:45

hongliangl force-pushed the 20221213-fix-4467 branch from 19a1254 to cf9e7cc Compare December 13, 2022 15:45

hongliangl force-pushed the 20221213-fix-4467 branch from cf9e7cc to 38309b2 Compare December 13, 2022 16:45

hongliangl force-pushed the 20221213-fix-4467 branch 2 times, most recently from 2be6ee2 to e8582e2 Compare December 14, 2022 02:10

hongliangl added this to the Antrea v1.11 release milestone Jan 3, 2023

wenyingd reviewed Feb 6, 2023

View reviewed changes

pkg/agent/route/route_windows.go Outdated Show resolved Hide resolved

jianjuns reviewed Mar 8, 2023

View reviewed changes

pkg/agent/route/route_windows.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20221213-fix-4467 branch from e8582e2 to e1093e0 Compare March 9, 2023 02:57

tnqn reviewed Mar 9, 2023

View reviewed changes

hongliangl force-pushed the 20221213-fix-4467 branch 3 times, most recently from 4e35382 to 2cac702 Compare March 10, 2023 04:46

Fix that Service routes may get lost when starting on Windows

93dbdde

Fix antrea-io#4467 Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl force-pushed the 20221213-fix-4467 branch from 2cac702 to 93dbdde Compare March 10, 2023 07:19

tnqn approved these changes Mar 10, 2023

View reviewed changes

tnqn merged commit cc2f40f into antrea-io:main Mar 10, 2023

jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023

Fix that Service routes may get lost when starting on Windows (antrea…

5a534db

…-io#4470) Fix antrea-io#4467 Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl deleted the 20221213-fix-4467 branch May 6, 2023 01:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix that Service routes may get lost when starting on Windows #4470

Fix that Service routes may get lost when starting on Windows #4470

hongliangl commented Dec 13, 2022

codecov bot commented Dec 13, 2022 •

edited

tnqn Dec 13, 2022

hongliangl Dec 13, 2022

tnqn Dec 13, 2022

hongliangl Dec 13, 2022

wenyingd left a comment

shettyg commented Dec 13, 2022

shettyg commented Dec 13, 2022

hongliangl commented Dec 13, 2022

hongliangl commented Dec 13, 2022

shettyg commented Dec 13, 2022

hongliangl commented Dec 13, 2022

shettyg commented Dec 13, 2022

hongliangl commented Dec 14, 2022

hongliangl commented Mar 1, 2023 •

edited

tnqn Mar 9, 2023

hongliangl Mar 10, 2023

tnqn left a comment

tnqn commented Mar 10, 2023

		svcIPNet := util.NewIPNet(svcIP)
		obj, found := c.hostRoutes.Load(svcIPNet.String())

Fix that Service routes may get lost when starting on Windows #4470

Fix that Service routes may get lost when starting on Windows #4470

Conversation

hongliangl commented Dec 13, 2022

codecov bot commented Dec 13, 2022 • edited

Codecov Report

tnqn Dec 13, 2022

Choose a reason for hiding this comment

hongliangl Dec 13, 2022

Choose a reason for hiding this comment

tnqn Dec 13, 2022

Choose a reason for hiding this comment

hongliangl Dec 13, 2022

Choose a reason for hiding this comment

wenyingd left a comment

Choose a reason for hiding this comment

shettyg commented Dec 13, 2022

shettyg commented Dec 13, 2022

hongliangl commented Dec 13, 2022

hongliangl commented Dec 13, 2022

shettyg commented Dec 13, 2022

hongliangl commented Dec 13, 2022

shettyg commented Dec 13, 2022

hongliangl commented Dec 14, 2022

hongliangl commented Mar 1, 2023 • edited

tnqn Mar 9, 2023

Choose a reason for hiding this comment

hongliangl Mar 10, 2023

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Mar 10, 2023

codecov bot commented Dec 13, 2022 •

edited

hongliangl commented Mar 1, 2023 •

edited