ipam: Fix race in NodeManager.Resync

[jaffcheng]

please see commit message

Related: #26617

[jaffcheng]

@tommyp1ckles @christarazi Please take a look. This flake clearly happens more often lately, but I'm not quite sure why recent changes would introduce this.

[jaffcheng]

/test

[tommyp1ckles]

@jaffcheng Interesting, I've been looking into this as well, I'm pretty sure its related to racey code related to appending to the AzureInterface Addresses array in the mock API implementation.

I have a local reproduction that can cause the failure to occur fairly quickly, let me validate if this fixes it.

[tommyp1ckles]

Even with these changes, I stil run into the failure.

[jaffcheng]

Even with these changes, I stil run into the failure.

Thanks for trying, could you share your reproduction method and test output? My local test is running pkg/ipam unit tests a lot of times with/without -race and lockdebug. But haven’t tried pkg/azure yet.

[tommyp1ckles]

@jaffcheng Sure, my branch is quite messy, ill push that up later today. The main part is here in the Upsert function in node_manager.go. Basically just removing the exponential backoff between triggers and running the poolMaintainer trigger in a loop causes this to occur usually in a couple seconds. I'm running the test with

go test -count=100 -v -failfast -race

		poolMaintainer, err := trigger.NewTrigger(trigger.Parameters{
			Name:            fmt.Sprintf("ipam-pool-maintainer-%s", resource.Name),
			MinInterval:     10 * time.Millisecond,
			MetricsObserver: n.metricsAPI.PoolMaintainerTrigger(),
			TriggerFunc: func(reasons []string) {
				if err := node.MaintainIPPool(ctx); err != nil {
					node.logger().WithError(err).Warning("Unable to maintain ip pool of node")
					//backoff.Wait(ctx) Remove exponential backoff wait to increase chance of race occuring.
				}
			},
			ShutdownFunc: cancel,
		})
                // Trigger poolMaintainer continuously!
		go func() {
			for {
				<-time.After(time.Millisecond)
				poolMaintainer.Trigger()
			}
		}()

[tommyp1ckles]

Race reveals a couple data races in this, I was hoping fixing those would solve the problem but yet it still persists 😕

[jaffcheng]

Although I think this patch should significantly lower the possibility of flakes, there are still some races found in /pkg/ipam with -race on. Need more digging...

[tommyp1ckles]

Although I think this patch should significantly lower the possibility of flakes, there are still some races found in /pkg/ipam with -race on. Need more digging...

there appears to be many races there 😄 I have a few race fixes as well I'll put PRs for. Considering how hard im finding it to get to a single cause, its possible the issue is caused by multiple races. We can start fixing these and see if the flakes improve.

[jaffcheng]

Change Fixes: to Related: since this does not fix all races.

[jaffcheng]

/test

[tommyp1ckles]

When I was looking at this, another place I thought it might be racing was here: https://github.com/cilium/cilium/blob/main/pkg/ipam/node.go#L430

i.e. the interfaces are checked in parallel and then on the next line they will race to get the Node lock, so in theory a stale set of values could overwrite the latest ones.

But... fixing this didn't fix the flake in practice.

[tommyp1ckles]

I think these changes make sense, lock changes in this code scare me a bit, I'll do another review to double check that we aren't creating a deadlock somewhere in shortly.

Resolved

[jaffcheng]

Changed to GetNodesByIPWatermarkLocked and slightly changed the commit message given that this only fixes one source of the flake.

[ti-mo]

/test

[tommyp1ckles]

@jaffcheng The integration tests are failing I believe to CI using a newer version of Go, can you rebase your branch - that should fix it.

[jaffcheng]

/test

Edit: ConformanceGatewayAPI hit #27647

error log

=== CONT  TestConformance/HTTPRouteRequestMirror/0_request_to_'/mirror'_should_go_to_infra-backend-v1
  httproute-request-mirror.go:69: Making GET request to http://172.18.255.200/mirror
  http.go:222: Response expectation failed for request: {URL: {Scheme:http Opaque: User: Host:172.18.255.200 Path:/mirror RawPath: OmitHost:false ForceQuery:false RawQuery: Fragment: RawFragment:}, Host: , Protocol: HTTP, Method: GET, Headers: map[X-Echo-Set-Header:[]], UnfollowRedirect: false, Server: , CertPem: <truncated>, KeyPem: <truncated>}  not ready yet: expected status code to be 200, got 404 (after 2.1µs)
  http.go:228: Request passed
  mirror.go:41: Searching for the mirrored request log
  mirror.go:42: Reading "gateway-conformance-infra/infra-backend-v2" logs
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xc0 pc=0x2088cd9]

goroutine 1493 [running]:
k8s.io/client-go/kubernetes.(*Clientset).CoreV1(...)
  /home/runner/work/cilium/cilium/vendor/k8s.io/client-go/kubernetes/clientset.go:309
sigs.k8s.io/gateway-api/conformance/utils/kubernetes.DumpEchoLogs({0x287200f, 0x19}, {0x285c32e, 0x10}, {0x2c54560, 0xc0006f45a0}, 0x0)
  /home/runner/work/cilium/cilium/vendor/sigs.k8s.io/gateway-api/conformance/utils/kubernetes/logs.go:50 +0x399
sigs.k8s.io/gateway-api/conformance/utils/http.ExpectMirroredRequest.func1()
  /home/runner/work/cilium/cilium/vendor/sigs.k8s.io/gateway-api/conformance/utils/http/mirror.go:43 +0x1df
github.com/stretchr/testify/assert.Eventually.func1()
  /home/runner/work/cilium/cilium/vendor/github.com/stretchr/testify/assert/assertions.go:1852 +0x22
created by github.com/stretchr/testify/assert.Eventually in goroutine 1483
  /home/runner/work/cilium/cilium/vendor/github.com/stretchr/testify/assert/assertions.go:1852 +0x21e
FAIL	github.com/cilium/cilium/operator/pkg/gateway-api	53.044s
FAIL
Error: Process completed with exit code 1.

[joestringer]

What's the consequence of this bugfix? Should we backport it to v1.14 per the backport criteria? Does the race condition affect other branches too?

[joestringer]

Reviews are in, CI is passing. LGTM to merge.

[tommyp1ckles]

@joestringer The changes that causes this don't appear to be in the v1.14 branch