mirror of
https://hubproxy.babadafafafafa.cn/https://github.com/kube-vip/kube-vip.git
synced 2026-09-20 08:03:47 +08:00
cfd86de936
* fix(lease): do not let a stale cleanup cancel a recreated lease Every object that starts leader election also starts a goroutine that calls Manager.Delete once its context is cancelled. Manager.Delete looked the lease up by name only, so it acted on whatever lease held that name at the time it ran, not the one the caller was given. When a service is torn down and rebuilt straight away, the replacement lease is already registered by the time the old cleanup goroutine runs, so the cleanup cancels the live replacement and removes it from the manager. The service is then never handled again: its election loop exits, the lease keeps an empty holderIdentity, and the VIP is never re-advertised. This is reachable from an ordinary service update. Flipping externalTrafficPolicy makes serviceChanged cancel the service context and rebuild it, which reproduced the stuck lease for minutes. Pass the lease the caller owns to Manager.Delete and ignore a stale caller, which keeps cleanup scoped to the instance it belongs to. Callers already hold that lease. Passing nil keeps the previous behaviour of deleting whichever lease currently holds the name, which is what the existing tests assert. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * fix(lease): retire a lease when its service is torn down The instance guard in Delete stops a late cleanup from cancelling a replacement lease, but it cannot help when the replacement *is* the same instance. A service teardown cancels the service context and leaves the lease registered, because the cleanup that removes it is deferred to a goroutine. The rebuild that follows calls Add, which finds that lease still in the map and hands it straight back, so the new service context is parented to a lease the pending cleanup is about to cancel. The service then cycles: acquire, lose, re-acquire, every few seconds, and never settles. Add Manager.Retire, and call it from the serviceChanged teardown next to the svcMap purge, so the lease is out of the map before the replacement context is built. Add also refuses to hand out a lease whose context is already cancelled, which closes the same hazard for any other path that cancels a lease directly. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * fix(lease): keep a common lease alive for the services still using it Review feedback from Patryk on #1669: Retire cancelled the lease context outright, so with a common lease a modification of one service would also tear down every sibling sharing that lease. Retire was only ever needed for its side effect of getting the lease out of the map before the rebuild, and Delete already does exactly that once the last object is gone. Drop Retire and have the teardown path call Delete with its own object name, so siblings keep the lease alive and the manager API stays Add/Delete/Get. TestManager_LeaseLifetimeInvariant replaces the single-scenario test with the rule for the whole surface: a lease stays usable for exactly as long as at least one object holds it, and a rebuild afterwards gets a fresh one. It is table driven over 1, 2 and 4 objects, and the 2 and 4 cases fail against the reviewed behaviour with "lease was cancelled with N object(s) still holding it". Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * fix(services): start the leader-election loop once per service - #1665 startLeaderElection restarts itself until the service context is cancelled, so it only needs to be started once per service lifetime. It was started from startServiceHandlingIfNeeded on every AddOrModify call instead, and AddOrModify runs on every EndpointSlice event, so endpoint churn accumulated duplicate permanent loops for the same service, all contending on the same lease. Guard the spawn with a sync.Once on the service context. A Once needs no clearing: the loop is bound to the service context, which is replaced whenever the service is recreated. Also drop the les *atomic.Int64 parameter, which was only ever incremented. Adds a unit test that drives AddOrModify three times and asserts the loop starts once (fails pre-fix with 3), and an -endpointFlap service e2e test that flaps the backend 1->0->1 five times and asserts the VIP serves traffic and the lease has a holder again. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * test(services): fault leases and API server access in the e2e test - #1665 The endpointFlap e2e test only asserted that the service recovered after endpoint churn, which the unfixed code also satisfies, so it did not prove anything about the reported failure. Inject faults that actually stress the per-service leader election and assert convergence after each one: - endpoint churn: backend scaled 1->0->1 five times - lease faults: the lease is deleted, then its holderIdentity is blanked, which is the exact state reported in the issue - API server faults: the apiserver is blocked from the leader for longer than the lease duration, so its election client loses its backend, then restored The assertions are on the feature contract, a held lease and a served VIP, rather than on election internals, so they stay meaningful if the implementation changes. Duplicate loops are not observable from outside the process; that part stays pinned by the unit test. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * test(services): assert election faults recover, via new election metrics - #1665 The previous e2e test only asserted the service recovered, which the unfixed code also satisfies, so it proved nothing. The duplicate election loops are not observable from outside the process, so there was nothing to assert on. Export the missing state as metrics, replacing the write-only les *atomic.Int64 that used to sit in startLeaderElection with real instrumentation: - kube_vip_service_election_loops{namespace,name}: live election loops, tracked for the lifetime of the goroutine. More than 1 per service means loops leaked. - kube_vip_service_election_attempts_total{namespace,name}: election attempts, so a wedged restart loop is visible as a counter that stops advancing. - kube_vip_service_election_errors_total{namespace,name,reason}: election failures, with reason="no_lease" for the service context and lease manager desync. The e2e test now injects a fault per reported failure mode and asserts a signal that is actually broken when that bug is present: - endpoint churn, five debounce-separated flaps (#1665): loops stay <= 1 - endpointslice deletion (#1663 / #1664): no reason="no_lease" errors - lease deletion and blanked holderIdentity (#1650): attempts keep advancing - apiserver blocked from the leader past the lease duration: same loss path Every fault is also followed by the feature contract: a held lease and a VIP that serves traffic. The unit test asserts the loop gauge alongside the call count so both layers agree. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * test(services): assert the election error counter settles, not that it is zero Both the fixed and the unfixed branch recorded one reason="no_lease" increment while the service was first set up, so asserting the counter is zero failed on correct code too. The #1664 desync makes the counter climb for the lifetime of the process, so assert it stops growing instead. Also sample the election loop gauge a few times rather than once, since a loop that is about to start may not be visible in a single scrape while a leaked one never goes away. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * test(services): assert election progress after a real leadership loss Deleting the lease object does not make the election client lose leadership, it just recreates the lease on the next renew, so no new election attempt follows and the progress assertion failed on correct code. Move that assertion to the apiserver partition, which is the fault that actually drives OnStoppedLeading and returns the election, and is therefore where the #1650 WaitGroup deadlock would wedge the restart loop. The lease object faults keep asserting convergence only. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * test(services): cover the remaining election fault scenarios - #1665 The suite only faulted the paths that the three known bugs live on. Add the neighbouring ones so a regression in this subsystem is caught wherever it lands: - VIP release on zero endpoints: with a local traffic policy and no endpoints anywhere, the address has to stop answering instead of black-holing traffic. Nothing asserted the yield half of the endpoint churn cycle before. - externalTrafficPolicy flip: makes serviceChanged cancel the service context and drop it from svcMap, so the next event has to build a fresh context and lease. Same desync class as the endpointslice fault, reached by a different trigger. - service event storm: 15 annotation patches, driving the spawn-once invariant from the service watch instead of the endpoint watch. - follower partition: cutting a non-leader off from the apiserver must not move the lease, stop traffic, or leak a loop on the node that comes back. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * test(services): let the election loop count settle before asserting it A fault that rebuilds the service context, such as the externalTrafficPolicy flip, legitimately has the old and the new loop alive at the same moment, so a single scrape saw 2 loops on correct code. Poll until the count settles instead. A leaked loop only exits with its service context, which outlives the test, so it never settles and is still caught: the unfixed code reports 11 loops after endpoint flapping. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * test(services): fault the externalTrafficPolicy teardown path With the lease retirement fix underneath, a service that is torn down and rebuilt by a traffic policy change converges again, so this fault can be asserted like the others: at most one election loop, a settled error counter, a held lease and a served VIP. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * test(services): stop the fault suite exhausting the client rate limiter The fault tests poll the API while waiting for convergence, and by the last fault the shared clientset had spent client-go's default 5 QPS budget, so a lease read failed with failed to get lease "kubevip-kube-vip-service": client rate limiter Wait returned an error: context deadline exceeded which looked like a convergence failure but was the test's own client giving up. Raise QPS for the harness and halve the lease polling rate. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> * test(services): fault a common lease sibling teardown Cluster-level cover for the case Patryk raised in review of #1669: two services share one lease, the first is deleted, and the second has to keep serving on that same lease. A teardown that cancels the lease instead of just releasing the leaving service takes the sibling down with it. Adds a commonLease field to the Service helper, which sets the kube-vip.io/serviceLease annotation along with the cluster traffic policy that a common lease requires. Also initialises the annotation map unconditionally: it was only allocated on the egress path, so setting any other annotation first would have nil-panicked. Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de> --------- Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>