Repository navigation
Conversation
|
Local runtime checks passed for
The stop check uses removal of active targets from the runtime API and unchanged receive counters, since UP gauges update asynchronously. Blocked API requests and release I/O are covered by the regression tests in this PR. These observations describe the exercised cases, not an exactly-once guarantee. |
|
Additional local integration checks passed for target ownership reconciliation, subscription recovery and Remote Write with this PR combined with #980. The PR's local unit and race checks also passed. GitHub Actions requiring maintainer approval remain pending; these results describe local validation. |
|
Final combined deployment validation is now complete for this Lease lifecycle change together with #980:
The PR is mergeable and all GitHub checks are green. @karimra, could you review this together with #980 when available? |
| case <-ctx.Done(): | ||
| session.cancel(ctx.Err()) | ||
| case <-session.stopped: | ||
| } | ||
| if err := ctx.Err(); err != nil { | ||
| errs <- err | ||
| close(done) | ||
| return |
There was a problem hiding this comment.
on ctx.Done() this sends err on errs chan. errs should only carry renewal failures.
There was a problem hiding this comment.
Fixed in 359cd3e1. Caller cancellation now cancels the Lease session and closes done without sending on errs; errs is used only when the lock cannot be maintained. The blocking app-side error-channel drain was removed as well. TestKeepLockCancellationCompletesWithoutErrorReceiver covers this path.
| ctx, cancel := context.WithTimeout(ctx, k.Cfg.RetryPeriod) | ||
| defer cancel() | ||
| session.cancel(context.Canceled) | ||
| select { | ||
| case <-session.stopped: | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| } |
There was a problem hiding this comment.
this context can be mostly consumed by session.stopped. That leaves almost nothing to client.Get and client.Delete coming afterwards.
There was a problem hiding this comment.
Fixed in 359cd3e1. release now creates a fresh RetryPeriod timeout for each stage: waiting for the elector to stop, reading the Lease, and deleting it. TestReleaseUsesIndependentTimeouts verifies that the stop wait cannot consume the Get or Delete budget.
| leases, err := k.leases.List(labels.Everything()) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
would be good to filter during listing rather than getting all leases and filtering them afterwards.
There was a problem hiding this comment.
Fixed in 359cd3e1. The informer list/watch remains server-side filtered to app=gnmic, and now maintains an original-key-prefix index. List uses ByIndex(prefix), so each call receives only matching cached Leases instead of enumerating the whole informer cache. TestLeaseCacheFiltersAndTracksOwnership checks the index cardinality and API-free reads.
| } | ||
| } | ||
| return errors.Join(errs...) | ||
| } |
There was a problem hiding this comment.
This functions uses a shared 5s timeout context and passes it to k.release, which applies a RetryPeriod (2s), for each session.
Either use a bounded worker pool to release the sessions and size the context timeout accordingly, or size the timeout based on len(sessions).
There was a problem hiding this comment.
Fixed in 359cd3e1. Stop uses errgroup.SetLimit(16) to bound concurrent releases. Every release has independent bounded stop/Get/Delete stages, so one shared deadline no longer starves later sessions. TestStopBoundsConcurrentReleases verifies the worker limit and that all sessions complete.
| func leaseName(key string) string { | ||
| digest := sha256.Sum256([]byte(key)) | ||
| return "gnmic-" + hex.EncodeToString(digest[:]) | ||
| } |
There was a problem hiding this comment.
This change breaks rolling upgrades.
There was a problem hiding this comment.
Fixed in 359cd3e1. Keys representable by the previous locker keep the same slash-to-hyphen Lease name and legacy label, while annotations carry the exact key/value. Digest names are used only for keys the previous implementation could not represent. The compatibility test starts from an old-format active Lease, verifies that a new replica cannot acquire a second object, and verifies dual metadata on newly created compatible Leases.
| All members sharing a cluster must stop before this upgrade. The previous and | ||
| new encodings refer to different Lease objects, so a mixed-version rolling | ||
| upgrade would create independent ownership domains. After all old members have | ||
| stopped, apply the new configuration and RBAC, then start the upgraded members. | ||
| Obsolete Lease objects can be removed after confirming their holders have stopped. |
There was a problem hiding this comment.
this is not great... we should provide an upgrade path
There was a problem hiding this comment.
Reworked in 359cd3e1. The stop-all instruction is gone. The existing ha_kubernetes.md now gives an ordered rolling path: grant watch RBAC first, retain supported keys and legacy timing fields while rolling the binary, then rename the deprecated fields after every replica is upgraded. It also documents the boundary for previously unsupported keys and the retained legacy collision behavior.
2ba66e4 to
359cd3e
Compare
|
Review feedback is addressed in Validation on the updated head:
The new regression coverage exercises caller cancellation without an The Test workflow is waiting for maintainer approval because this is a forked PR. I have left the review threads open for maintainer verification. @karimra, all six review points have corresponding fixes and replies; this is ready for re-review when available. |
This branch has not been deployed
2 participants
Fixes #982.
The Kubernetes locker can reject valid runtime target identities and can continue collection after Lease renewal is no longer reliable. This replaces the custom acquisition and renewal loop with client-go leader election and
LeaseLock, gives every acquisition a unique holder identity, and reports renewal-deadline failures through the collector's stop-and-retry path.Release verifies the session owner, uses UID and resource-version preconditions, and gives the elector shutdown, Lease read, and Lease delete independent timeout budgets. Shutdown releases run with bounded concurrency. The collector cancels its subscription before release I/O, outside the operational mutex.
A shared Lease informer serves ownership queries. Its API list/watch is restricted to
app=gnmic, and an original-key prefix index avoids scanning every cached Lease for eachListcall.qpsandburstmake the client-go request budget explicit.Existing keys supported by the previous locker keep their slash-to-hyphen Lease names and legacy labels, while annotations retain the exact key and value. Previously unsupported keys use
gnmic-<sha256>names. This lets old and new replicas coordinate during a rolling update. The deprecatedrenew-periodandretry-timersettings remain aliases during that rollout; the documented sequence removes them after every replica is upgraded.Kubernetes setup, lifecycle constraints, RBAC, limitations, and the rolling-upgrade sequence are documented in
docs/user_guide/ha_kubernetes.md, alongside the EndpointSlice discovery documentation merged in #980.Local validation at
359cd3e1a5ff37834279a218e2f675791e576a3f:./tests/run_tests.sh: passed for all three Go modules../tests/run_tests.sh --race: passed for all three Go modules.go vet ./pkg/lockers/k8s_locker ./pkg/app ./pkg/collector/managers/targets ./pkg/collector/managers/cluster: passed.pkg/app/app.goSA1019 deprecation.The earlier comments contain combined Kubernetes deployment evidence from this change and #980 before the review-fix rebase. The review fixes above are covered by the new local unit and race runs; GitHub checks on the updated head remain authoritative.