feat(ha): drift backstop and failure events for the state mirror - #415
Open
sumanthd032 wants to merge 17 commits into
Open
feat(ha): drift backstop and failure events for the state mirror#415sumanthd032 wants to merge 17 commits into
sumanthd032 wants to merge 17 commits into
Conversation
Introduce pkg/ha, the foundation for cross-cluster (Active/Standby) high availability per ADR kubeslice#293 (issue kubeslice#294). - HAMode (active|standby|standalone) with fail-safe parsing: empty or unknown input maps to standalone so a misconfig never disables writes. - Lease helpers over coordination.k8s.io/v1: acquire/renew (bumping leaseTransitions on takeover), get, and a leaseDuration+padding staleness check. - ClusterLeaderElector: IsLeader() reads an atomic flag kept current by background loops, so it is cheap enough to call at the top of every Reconcile and always reflects live leadership. StartLeaseRenewal (Active) renews the local Lease and releases leadership once the renew deadline is exceeded (natural fencing). WatchRemoteLease (Standby) reads the Active's Lease and logs staleness but does not promote; promotion is issue kubeslice#297. Standalone is the default and is always the leader, preserving today's single-hub behaviour (no regression). Unit tests are race-clean and cover leadership by mode, renew-deadline loss, lease staleness, and the standby-never-promotes boundary. vendor: add controller-runtime fake client + interceptor packages (test-only) via go mod vendor. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Wire pkg/ha into the controller so only the Active hub writes (issue kubeslice#294). - Add a LeaderElector field to all nine reconcilers and a per-call guard at the top of every Reconcile: a Standby logs "standby mode, skipping reconcile" and returns without writing. The guard is nil-safe, so a reconciler built without an elector keeps today's behaviour. - main.go: add --ha-mode, --ha-identity, --ha-active-kubeconfig, --ha-lease-duration, --ha-renew-deadline, --ha-retry-period and --ha-padding-seconds; construct the elector (building a remote client from the mounted Active kubeconfig in standby mode); start StartLeaseRenewal (active) or WatchRemoteLease (standby) and pass the shared signal-handler context to the manager. - Add coordination.k8s.io/leases RBAC for the Lease. --ha-mode=standalone is the default and is always the leader, so existing single-hub deployments are unaffected (no regression). The existing --leader-elect (in-cluster pod election) is left untouched. A controller test asserts the Standby skips and logs on every call. vendor: add go.uber.org/zap/zaptest/observer (test-only). Signed-off-by: Sumanth D <sumanthd032@gmail.com>
The HA leader-election Lease lives in the controller's own namespace and is already covered by the existing leader-election Role's leases grant (config/rbac/leader_election_role.yaml). The kubebuilder marker added a redundant cluster-wide grant and left the generated manifests out of sync (make manifests was not run). Replace it with a note pointing at the role that provides the permission, keeping markers and manifests consistent. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…hutdown - acquireOrRenewLease rounds LeaseDurationSeconds up to whole seconds and clamps to a minimum of 1, so a sub-second --ha-lease-duration is not truncated to 0 (invalid, and skews staleness checks). - StartLeaseRenewal and WatchRemoteLease return nil instead of ctx.Err() on context cancellation, so a graceful shutdown is not logged as an error. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
The Lease namespace defaulted to a hard-coded constant, but the controller runs in a namespace injected at runtime via KUBESLICE_CONTROLLER_MANAGER_NAMESPACE (downward API), and the leader-election Role that grants leases is namespaced to that deploy namespace. Deploying into any other namespace would create the Lease where the controller has no leases permission, so the Active could not renew it and would fence itself permanently. Add --ha-lease-namespace defaulting to KUBESLICE_CONTROLLER_MANAGER_NAMESPACE so the Lease lands in the controller's own namespace. Empty (local runs) falls back to the pkg/ha default. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
NewClusterLeaderElector fell back to the hard-coded DefaultLeaseNamespace whenever LeaseNamespace was empty, independent of main.go's flag default. Since the controller already exposes KUBESLICE_CONTROLLER_MANAGER_NAMESPACE to represent its actual runtime namespace, check that env var first so the package resolves correctly on its own, not only by accident of how main.go wires the --ha-lease-namespace flag default. Only falls back to DefaultLeaseNamespace when the env var is unset too (e.g. running outside a pod). Regression tests included. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
StartLeaseRenewal/WatchRemoteLease returning nil (not ctx.Err()) on context cancellation was fixed in 0da6d82 but never actually got a regression test. Also add coverage for: the mode-guard no-op branches, WatchRemoteLease failing fast without a remote client, getLease/ checkRemoteLeaseOnce propagating a missing-Lease error instead of reporting fresh, renewOnce keeping leadership on a transient failure within renewDeadline, and setLeader logging Acquired/Lost exactly once per transition (F4 in 294-evaluation.md). No production code changes. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…uard, ownerRef strip, status mirroring) Part of kubeslice#295 — the write path RemoteSyncer's workqueue will drive (next commit) to mirror KubeSlice CRDs from the Active hub onto the Standby. - MirroredResource + CRDMirrorSet (pkg/ha/mirror_set.go): the hub-side resource table (Project, Cluster, SliceConfig, ServiceExportConfig, SliceQoSConfig, VpnKeyRotation, WorkerSliceConfig, WorkerSliceGateway, WorkerServiceImport, plus core Namespace). Deliberately does not match issue kubeslice#295's own CRD table, which names Slice/SliceGateway/ ServiceExport — none of those types exist in this repo; they are worker-cluster data-plane CRDs owned by the separate worker-operator repo, irrelevant to hub-to-hub mirroring. - mirrorCreateOrUpdate/mirrorDelete (pkg/ha/mirror.go): strip resourceVersion/uid/managedFields/finalizers (+ ownerReferences only for VpnKeyRotation, the one mirrored type that carries one) before writing, label mirrored objects ha.kubeslice.io/synced-from=active, and only ever overwrite or delete a target object that already carries that label — the conflict guard that keeps the syncer off anything the Standby's own reconcilers or an operator created directly, including pre-existing namespaces like kube-system/default now that Namespace is an ordinary mirrored type rather than a special-cased cold-start step. - Every mirrored type has a status subresource. A plain Update() never touches .status once one is registered, so mirrorCreateOrUpdate always follows up with an explicit Status().Update() when the source object has a non-empty status — matching this repo's own UpdateStatus/CleanupUpdateStatus convention. Regression-tested. Fake-client unit tests cover create, update-of-existing, the conflict guard on both update and delete, delete-idempotent-on-NotFound, StripOwnerRefs true/false, and status mirroring. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Part of kubeslice#295. Registered via prometheus.NewHistogramVec/NewCounterVec + prometheus.MustRegister in pkg/ha's own init(), the same self-registration idiom metrics/prometheus.go already uses for KubeSliceEventsCounter — not routed through metrics.StartMetricsCollector, whose default labels are slice-specific and don't apply to a cross-cluster mirror. - ha_sync_lag_seconds{kind,operation}: time.Now() minus CreationTimestamp for creates; minus first-enqueue time for update/delete (RemoteSyncer's workqueue coalesces repeated events for the same object, so there's no single "delivery time" once a retry has backed off a few times — first-enqueue is the more useful number to alert on, since it reflects total time since the triggering change). - ha_sync_errors_total{kind,operation}: counts mirror failures. The syncer keeps running and retries via its workqueue on every increment; this metric never indicates a crash. vendor: add prometheus/client_golang/prometheus/testutil (test-only) via go mod vendor, used to assert metric samples/labels directly. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Part of kubeslice#295. RemoteSyncer runs only in standby mode: builds a controller-runtime cache.Cache against the Active hub's rest.Config, registers one informer per CRDMirrorSet entry, and starts a small worker pool draining a rate-limited workqueue.TypedRateLimitingInterface [syncKey] — the same primitive controller-runtime's own Controller uses internally (internal/controller.Controller). Informer callbacks (handlersFor) only enqueue a syncKey; no mirror logic runs on the informer's own goroutine. A burst of Update events for the same object coalesces into one queued item, and a worker determines the real action at dequeue time by re-reading the Active cache (found -> mirrorCreateOrUpdate, NotFound -> mirrorDelete) — the same way a Reconcile call would. This is the commit that satisfies issue kubeslice#295's own acceptance criterion that the syncer "retries without crashing": on any mirror failure, processOnce calls queue.AddRateLimited instead of dropping the key. The concrete failure mode this fixes: a namespaced object (e.g. a new Project's Cluster) created on Active after the Standby's initial sync has already completed previously had no path to retry — nothing fires again for an object that didn't change on Active once its first mirror attempt failed on "namespace not found". Now it just gets retried a few seconds later once the Namespace mirror (an ordinary CRDMirrorSet row, no special-casing needed) has landed. Start(ctx) delegates its blocking wait directly to remoteCache.Start(ctx), which itself blocks on <-ctx.Done() and returns nil — giving the "return nil, not ctx.Err(), on graceful shutdown" contract StartLeaseRenewal/WatchRemoteLease already use, for free. remoteGetFunc is a small seam (defaulted to a real cache.Cache.Get in the constructor) so the retry engine's tests exercise real workqueue backoff/redelivery behaviour without a *rest.Config or live cluster. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Part of kubeslice#295. Constructs ha.RemoteSyncer alongside the existing ClusterLeaderElector, reusing the same remote *rest.Config and local client the elector already builds rather than loading the Active kubeconfig twice — hoisted the standby block's remoteCfg to an outer remoteHACfg variable so both consumers can see it. Starts remoteSyncer.Start(ctx) in its own goroutine alongside the existing leaderElector.WatchRemoteLease(ctx) in the ha.ModeStandby switch case; a no-op in any other mode, matching RemoteSyncer's own mode check. New flag: --ha-sync-workers (default ha.DefaultSyncWorkers), matching the existing --ha-* flag style. --ha-sync-interval is intentionally not added yet — it belongs to the periodic prune backstop (pkg/ha/ prune.go), which is out of scope for this PR and would otherwise be a flag with no consumer. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…mote read identity Part of kubeslice#295. --ha-active-kubeconfig's RBAC scope on the Active cluster has been undefined since kubeslice#294 introduced the flag — the dev demo uses a full-admin kubeconfig, and config/rbac/leader_election_role.yaml only ever granted configmaps/leases, nothing for the CRD/Namespace reads RemoteSyncer now needs. The ADR (kubeslice#293) itself names this as an unaddressed deployment-level boundary without resolving it. config/ha/active-cluster-clusterrole.yaml: a read-only (get/list/watch) ClusterRole covering Namespace plus every pkg/ha.CRDMirrorSet type, plus a ClusterRoleBinding template (subject left as a placeholder — it's deployment-specific, either a ServiceAccount for in-cluster dialing or a client-cert User for a flattened kubeconfig, and can't be hardcoded). config/ha/README.md explains this must be applied on the Active cluster by whoever provisions it, not through this repo's own deploy/kustomize flow — confirmed neither config/rbac/kustomization.yaml nor config/default/kustomization.yaml reference this directory, so it can never be accidentally auto-applied to the Standby's own cluster, where it would be meaningless. Also documents, ahead of time, that a later credential-mirroring PR appending Secrets to this grant exposes every Secret in the project namespaces on Active, not just the ones actually mirrored — RBAC can't scope Secrets by .type — a real tradeoff to weigh when that lands. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Start gave up permanently on the first GetInformer/AddEventHandler error instead of retrying, unlike StartLeaseRenewal/WatchRemoteLease. Split setup into registerInformers (retries with backoff) wrapping registerInformersOnce (one attempt). A naive retry would double-register handlers on resources that already succeeded, since AddEventHandlerWithResyncPeriod isn't idempotent. Added a handlerRegistered map so retries skip resources already done. CRDMirrorSet's Namespace entry had no filter, so it mirrored every namespace on the Active hub, not just kubeslice ones — and mirrorDelete's label-only guard meant an unrelated Active-side delete could cascade-delete one of those on the Standby. Scoped the Namespace informer to util.LabelsKubeSliceController via cache.Options.ByObject. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
An Active-side object mid-Terminating (deletionTimestamp set, contents still being garbage-collected) is delivered by the informer as an ordinary Update, not a Delete yet. mirrorCreateOrUpdate's payload carried deletionTimestamp straight through, and updating the Standby's non-terminating mirror with it set fails real API server immutable- field validation. Stripping it surfaced a second, related failure: the status-mirror block was still copying a Terminating status onto that now-non-terminating payload, which a real server also rejects (status.Phase may only be Terminating if deletionTimestamp is set). Both are stripped now: deletionTimestamp/deletionGracePeriodSeconds unconditionally alongside the other identity fields already cleared there, and status explicitly via delete(payload.Object, "status") rather than relying on a real API server silently ignoring .status on the main resource endpoint for subresource-registered types — a fake client does not replicate that behavior, so the code's correctness would otherwise depend on which client it's running against. The Standby still converges correctly once Active reports NotFound and mirrorDelete takes over; there's nothing useful to reflect about the in-between Terminating state. New tests: TestMirrorCreateOrUpdate_StripsDeletionTimestampFromTerminatingSource, TestMirrorCreateOrUpdate_SkipsStatusMirrorWhenSourceIsTerminating. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Dockerfile never copied pkg/ into the build context, so no image has been buildable since pkg/ha was introduced — go build works directly, docker build did not. Added COPY pkg/ pkg/ alongside the other source directories. config/rbac/role.yaml never granted namespaces/status: every other CRDMirrorSet type already has an explicit <kind>/status rule, but Namespace never needed one before it became a mirrored type with explicit status mirroring. Under real RBAC this permanently fails the Namespace status write. Added the +kubebuilder:rbac marker in main.go next to the existing namespaces rule and regenerated via make manifests. config/ha/active-cluster-clusterrole.yaml never granted coordination.k8s.io/leases: it was scoped only to what RemoteSyncer itself reads (Namespace + CRDMirrorSet), but the same --ha-active-kubeconfig identity is also used by WatchRemoteLease to read the Active's own Lease directly. Under a least-privilege identity using exactly this sample, the Standby could mirror correctly but never observe Lease staleness at all. Added the leases rule and updated the README to describe both consumers of this grant. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
Informers self-heal missed updates via periodic resync and the workqueue
owns retry-on-failure, but neither can remove a mirror whose Active-side
original was deleted while the Standby wasn't watching (e.g. between two
Standby runs): cold-start informers only deliver what currently exists,
so such an orphan would survive forever.
Add a prune loop to RemoteSyncer that periodically lists Standby objects
carrying the ha.kubeslice.io/synced-from label per mirrored type, diffs
them against the remote cache, and enqueues anything no longer present
on the Active hub onto the ordinary mirror workqueue. The worker re-reads
Active at dequeue time, so the existing conflict guard, NotFound->delete
semantics, and rate-limited retry apply unchanged, and no second write
path races the workers.
Fail-safe choices:
- The first pass waits for the remote cache to sync; an unsynced cache
lists empty, which would otherwise read as "everything was deleted"
and prune every mirror on the Standby.
- A failed list (remote or local) skips that kind for the round and
increments ha_sync_errors_total{kind,"prune"} instead of pruning on
partial information.
Configurable via --ha-sync-interval (default 60s).
Part of kubeslice#295
Signed-off-by: Sumanth D <sumanthd032@gmail.com>
…lure Surface mirror failures as Kubernetes events on the Standby, attached to the object that failed to sync, using the EventRecorder main.go already builds for the reconcilers. The entry lives in config/events/controller.yaml with the generated map and config-map output from make generate-events committed alongside — RecordEvent hard-fails for any EventName missing from the generated EventsMap, so skipping that step would silently no-op the whole feature. A regression test pins the entry's presence in the generated map (and that a missing entry errors loudly), so an accidental revert of the generated code fails in go test rather than at runtime. One event per failure episode, not per retry attempt: NumRequeues is 0 only on the first failure since the last success, and early workqueue backoff retries arrive milliseconds apart — although the recorder aggregates repeats into one Event's Count, every call is still an API-server write. ha_sync_errors_total continues to count every attempt. The recorder is called directly rather than through util.RecordEvent: that helper logs via util.CtxLogger, which panics on any context that didn't pass through a reconciler's request-context setup — true for the syncer's own context (main.go's signal-handler context). Caught by the new event-emission test before it could crash a live Standby. Part of kubeslice#295 Signed-off-by: Sumanth D <sumanthd032@gmail.com>
7 tasks
There was a problem hiding this comment.
Pull request overview
This PR hardens the HA Standby “state mirror” by adding (1) a prune-only drift backstop loop to catch deletions missed while the Standby wasn’t watching, and (2) a new HAMirrorSyncFailed Kubernetes event surfaced on the Standby for mirror failure episodes. It also wires cross-cluster HA mode/lease settings into main.go and gates reconcilers behind the HA leader elector.
Changes:
- Add
RemoteSyncerprune backstop (--ha-sync-interval) and supporting tests to enqueue/delete orphaned mirrored objects. - Emit a
HAMirrorSyncFailedevent once per failure episode (and pin the generated event registration with tests). - Wire HA configuration + leader write-fence through
main.go, reconcilers, RBAC markers/manifests, and vendored test dependencies.
Reviewed changes
Copilot reviewed 34 out of 52 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vendor/sigs.k8s.io/controller-runtime/pkg/internal/objectutil/objectutil.go | Adds helper for label-based filtering used by fake client behavior. |
| vendor/sigs.k8s.io/controller-runtime/pkg/client/interceptor/intercept.go | Adds interceptor client used by tests (injectable failures). |
| vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/doc.go | Adds/upgrades fake client docs (vendored). |
| vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/client.go | Adds/upgrades fake client implementation used heavily by HA unit tests. |
| vendor/modules.txt | Updates vendored module package list for new test/runtime dependencies. |
| vendor/k8s.io/apimachinery/pkg/util/rand/rand.go | Adds vendored rand util used by fake client. |
| vendor/go.uber.org/zap/zaptest/observer/observer.go | Adds zap log observer for log-asserting tests. |
| vendor/go.uber.org/zap/zaptest/observer/logged_entry.go | Adds log entry representation for zap observer. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/testutil.go | Adds prometheus test utilities used by metrics tests. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/units.go | Adds vendored promlint validation support. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/histogram_validations.go | Adds vendored promlint validation support. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/help_validations.go | Adds vendored promlint validation support. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/generic_name_validations.go | Adds vendored promlint validation support. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validations/counter_validations.go | Adds vendored promlint validation support. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/validation.go | Adds vendored promlint validation registry. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/promlint.go | Adds vendored promlint linter implementation. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/promlint/problem.go | Adds vendored promlint problem type. |
| vendor/github.com/prometheus/client_golang/prometheus/testutil/lint.go | Adds prometheus metric lint helpers. |
| pkg/ha/remote_syncer.go | Implements RemoteSyncer workqueue mirroring + prune loop + failure event emission. |
| pkg/ha/remote_syncer_test.go | Tests RemoteSyncer reconcile paths, retry behavior, and informer registration retry/dedup. |
| pkg/ha/prune.go | Implements periodic prune backstop diffing Standby mirrors vs Active cache. |
| pkg/ha/prune_test.go | Tests prune safety behaviors (cache sync gate, failed list, unlabeled objects, worker delete). |
| pkg/ha/mode.go | Defines HA mode parsing/validation. |
| pkg/ha/mode_test.go | Unit tests for HA mode parsing/validation. |
| pkg/ha/mirror.go | Implements create/update/status mirroring plus conflict-guarded delete. |
| pkg/ha/mirror_test.go | Tests mirror semantics including status subresource handling and termination edge cases. |
| pkg/ha/metrics.go | Adds HA mirror lag/error Prometheus metrics and registration. |
| pkg/ha/metrics_test.go | Validates metrics can be recorded/collected with expected labels. |
| pkg/ha/lease.go | Lease acquire/renew + staleness checks for cross-cluster HA. |
| pkg/ha/lease_test.go | Tests lease logic and provides shared HA test helpers (scheme, fake client, etc.). |
| pkg/ha/leader_elector.go | Implements Active renew loop and Standby remote lease watch loop (no promotion yet). |
| pkg/ha/leader_elector_test.go | Tests leadership transitions, natural fencing behavior, and log-on-transition semantics. |
| pkg/ha/events_test.go | Tests HAMirrorSyncFailed episode semantics and generated event registration. |
| main.go | Adds HA flags, builds HA clients, wires leader elector + remote syncer, and passes elector to reconcilers. |
| Dockerfile | Ensures pkg/ is copied into build context for the new HA code. |
| controllers/worker/workerslicegateway_controller.go | Adds leader write-fence gate to skip reconcile in standby. |
| controllers/worker/workersliceconfig_controller.go | Adds leader write-fence gate to skip reconcile in standby. |
| controllers/worker/workerserviceimport_controller.go | Adds leader write-fence gate to skip reconcile in standby. |
| controllers/controller/vpnkey_rotation_controller.go | Adds leader write-fence gate to skip reconcile in standby. |
| controllers/controller/sliceqosconfig_controller.go | Adds leader write-fence gate to skip reconcile in standby. |
| controllers/controller/sliceconfig_controller.go | Adds leader write-fence gate to skip reconcile in standby. |
| controllers/controller/serviceexportconfig_controller.go | Adds leader write-fence gate to skip reconcile in standby. |
| controllers/controller/project_controller.go | Adds leader write-fence gate to skip reconcile in standby. |
| controllers/controller/cluster_controller.go | Adds leader write-fence gate to skip reconcile in standby. |
| controllers/controller/leader_gate_test.go | Adds test verifying standby reconcile gate returns without invoking service. |
| events/events_generated.go | Registers new HAMirrorSyncFailed event name/schema in generated map. |
| config/rbac/role.yaml | Adds local RBAC for namespaces/status get/patch/update. |
| config/ha/README.md | Documents Active-cluster-side RBAC required for Standby read access. |
| config/ha/active-cluster-clusterrole.yaml | Provides template ClusterRole/Binding for Standby identity on the Active cluster. |
| config/events/events_config_map.yaml | Adds HAMirrorSyncFailed to event config map list. |
| config/events/controller.yaml | Adds HAMirrorSyncFailed schema definition for event generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+49
to
+53
| // HA write fence: only the Active hub (or a standalone controller) writes. | ||
| // A Standby evaluates this on every call and no-ops. | ||
| if r.LeaderElector != nil && !r.LeaderElector.IsLeader() { | ||
| r.Log.Info("standby mode, skipping reconcile") | ||
| return ctrl.Result{}, nil |
pnavali
requested review from
Rahul-D78,
gourishkb and
rajendra-avesha
and
a lite review from Copilot
and removed request for
that-backend-guy
August 18, 2026 15:08
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 52 changed files in this pull request and generated no new comments.
Suppressed comments (6)
pkg/ha/prune.go:1
runPrunenever performs an initial prune pass after the cache syncs; it waits until the first ticker tick. This contradicts the doc comment (“before the first pass”) and leaves startup orphans unpruned for up topruneInterval. Consider callings.pruneOnce(ctx)once immediately afterwaitForCacheSyncsucceeds (before starting the ticker), or adjust the comment/contract if delayed-first-pass is intended.
pkg/ha/remote_syncer.go:1- Using
time.Afterin a retry loop allocates a new timer on every iteration and can create avoidable GC pressure under prolonged failure. Prefer a reusabletime.Timer(create once,Reset, andStop/drain as needed) to reduce allocations and improve behavior under sustained retry conditions.
pkg/ha/remote_syncer.go:1 - The SPDX identifier formatting here is non-standard (
# #inline with the copyright line). Many license scanners expect a dedicatedSPDX-License-Identifier: Apache-2.0line (typically without extra tokens) near the top of the file. Recommend normalizing this header across newly added files to a canonical SPDX format to avoid automated compliance tooling missing it.
pkg/ha/remote_syncer.go:1 - In standby mode,
schemeis required forcache.Newbut isn’t validated; passingnilcould lead to runtime errors or misconfigured cache behavior. Consider explicitly validatingscheme != nil(and returning a clear error) alongside the existingremoteCfgcheck to make the constructor contract safer and clearer.
controllers/controller/sliceconfig_controller.go:52 - Logging an
Infomessage on every reconcile while in standby can be extremely noisy (especially for high-churn resources) and may increase log volume/costs and mask real issues. Consider downgrading toDebug, adding rate-limiting/sampling, or logging only on leadership transitions (the elector already logs transitions) while keeping the no-op behavior.
// HA write fence: only the Active hub (or a standalone controller) writes.
// A Standby evaluates this on every call and no-ops.
if r.LeaderElector != nil && !r.LeaderElector.IsLeader() {
r.Log.Info("standby mode, skipping reconcile")
return ctrl.Result{}, nil
}
main.go:319
- Creating a second controller-runtime client for the local cluster is likely unnecessary and may increase connection count and API QPS (separate REST client, caches not reused, etc.). Consider reusing
mgr.GetClient()for local operations (writes already bypass the cache) unless there is a concrete requirement for an uncached client; if an uncached client is needed, documenting that rationale here would help future maintainers.
haRunMode := ha.ParseHAMode(haMode)
localHAClient, err := client.New(mgr.GetConfig(), client.Options{Scheme: scheme})
if err != nil {
setupLog.Error(err, "unable to build HA local client")
os.Exit(1)
}
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two hardening pieces for the state mirror:
--ha-sync-interval, default 60s) that removes Standby mirrors whose Active-side original was deleted while the Standby wasn't watching.HAMirrorSyncFailedKubernetes event on mirror failure, registered viaconfig/events/controller.yaml.Stacked on #411. Only 2 commits are new here.
Part of #295
How Has This Been Tested?
go test -race -count=1 ./pkg/ha/..., including the prune loop's fail-safe paths (unsynced cache, failed list) and the event emission path.Checklist:
Does this PR introduce a breaking change for other components like worker-operator?
No. Internal hardening of the Standby's own mirror loop.