Repository navigation
refactor(reconciler): modernize controllers to modern controller-runtime - #807
VLukiantsev wants to merge 3 commits into
Conversation
- Migrate JobSet and Workload controller registration to ctrl.NewControllerManagedBy(mgr) builder pattern - Register UpdateBatcher and KueueSync reconciler as manager.Runnable instances, eliminating unmanaged background goroutines - Add graceful shutdown flush with 10s budget and context.WithoutCancel(ctx) in UpdateBatcher - Remove bespoke clusterqueue.NameCache and reconciler; use informer cache (r.Client.List) with pre-warmed SelectorsByObject - Fix Pod query in listJobSetPods to use client.MatchingLabels with v1alpha2.JobSetNameKey - Remove redundant metadata.name indexer on JobSet - Remove cargo-culted logging.Strip(ctx) workarounds across reconcilers and predicates TAG=agy CONV=e4cc9b60-fdd6-4d8e-961b-43be76e282da
There was a problem hiding this comment.
Code Review
This pull request refactors the reconciler architecture by removing the dedicated clusterqueue reconciler and its cache, opting instead to list ClusterQueues directly from the client cache in kueue_sync. It registers background tasks like UpdateBatcher and KueueSync as runnables with the controller manager, simplifies controller setup using ctrl.NewControllerManagedBy, and cleans up unnecessary logging.Strip(ctx) calls. Additionally, UpdateBatcher now flushes pending updates on shutdown. Feedback is provided regarding a potential deadlock or panic risk in kueue_sync.go if concurrency limits are misconfigured, along with a bottleneck that restricts global concurrency.
…e_sync Ensure global workload concurrency is bounded by MaxConcurrentKueueSyncReconciles instead of being constricted to 1/10th across all queues. Validate that the limit is at least 1 to prevent unbuffered channel deadlocks or negative size panics. Comment link: #807 (comment) TAG=agy CONV=e4cc9b60-fdd6-4d8e-961b-43be76e282da
- shared/batcher: ensure shutdown flush context cancellation is deferred in closure and expose Flush() - manager: register post-drain batcher flush in Close() cleanup LIFO chain - controllers: simplify registerControllers signature by removing unused reconciler return - kueue_sync: default KueueSyncTickDuration to 60s when non-positive to prevent NewTicker panic - kueue_sync: select on ctx.Done() when acquiring queueSem and workloadSem during shutdown - kueue_sync_test: remove obsolete fakeClient wrapper in favor of standard fake.Client - jobset_test: remove redundant Pod WithIndex indexers since MatchingLabels is used - compat.bzl: remove dead c.Watch compatibility replacement rules - helm: add nodes RBAC to clusterrole and runAsNonRoot: true to reconciler deployment TAG=agy CONV=e4cc9b60-fdd6-4d8e-961b-43be76e282da
There was a problem hiding this comment.
Please run the e2e workflow for this PR - you're touching a bunch of files that have interactions with each other that might not be caught by unit tests.
| // This function CANNOT BE CALLED WITHOUT A STRIPPED CONTEXT. If it gets it's hand on an slog, | ||
| // it blocks the entire logging system for some reason. | ||
| if err := r.Get(logging.Strip(ctx), req.NamespacedName, &workload); err != nil { | ||
| if err := r.Get(ctx, req.NamespacedName, &workload); err != nil { |
There was a problem hiding this comment.
Previously we've had a stripped context here with a comment:
// This function CANNOT BE CALLED WITHOUT A STRIPPED CONTEXT. If it gets it's hand on an slog,
// it blocks the entire logging system for some reason.
how have you validated that this is not the case anymore?
| return nil, fmt.Errorf("unable to start manager: %w", err) | ||
| } | ||
|
|
||
| if err := mgr.GetFieldIndexer().IndexField(ctx, &v1alpha2.JobSet{}, "metadata.name", func(rawObj runtimeclient.Object) []string { |
There was a problem hiding this comment.
JobSet Indexing & Pod Query Fix: Removed the unused metadata.name indexer on v1alpha2.JobSet. Fixed listJobSetPods to query Pods by client.MatchingLabels{v1alpha2.JobSetNameKey: jobSet.Name} rather than MatchingFields. Cleaned up redundant WithIndex Pod indexers in jobset/reconciler_test.go.
From what I see we used to have this call inside fetchJobsetByName - why are removing it? I don't understand the rationale here.
| - apiGroups: [""] | ||
| resources: ["namespaces"] | ||
| verbs: ["get", "list", "watch"] | ||
| - apiGroups: [""] |
There was a problem hiding this comment.
Why do we need this permission now?
Summary
Modernize the XMC reconciler (
cmd/reconciler/) to moderncontroller-runtimeparadigms. This replaces the bespokeCQCachewith native Informer cache lookups, migrates controller registration to the declarativectrl.NewControllerManagedBy(mgr)builder pattern, integrates background worker loops intomanager.Runnablewith graceful flush on shutdown and LIFO cleanup, fixes JobSet Pod label querying, hardens concurrency semaphores, aligns Helm RBAC and Pod Security Standards, and strips cargo-cultedlogging.Strip(ctx)workarounds.Motivation & Context
The XMC reconciler was originally built against an older version of
controller-runtime(v0.5.0) and retained custom synchronization workarounds (such as a separateclusterqueue.NameCachereconciler, manualcontroller.New+c.Watchwiring, unmanaged background goroutines, and private context stripping). Modernizing these components eliminates redundant caching layers (-550 lines), ensures deterministic goroutine lifecycles bounded by the Kubernetes manager, prevents dropped updates during pod termination, and tightens security and RBAC invariants.Key Changes
cmd/reconciler/clusterqueuepackage andNameCache. Registered&v1beta1.ClusterQueue{}: {}undercache.SelectorsByObjectinmanager.goand switchedkueue_sync.goto list ClusterQueues directly from the informer cache viar.Client.List(ctx, &cqList).jobsetandworkloadcontroller setup from manualcontroller.New+c.Watchtoctrl.NewControllerManagedBy(mgr).For(...).WithEventFilter(...).WithOptions(...).Complete(r).manager.Runnable& Graceful Batch Flush: RegisteredUpdateBatcherandkueue_sync.Reconcilerasmanager.Runnableinstances withmgr.Add(...). Added graceful shutdown flushing inBatchProcessorusing a 10s budget withcontext.WithoutCancel(ctx)in an isolated closure. Addedbatcher.Flushto theManager.Close()LIFO cleanup chain so any late updates buffered while controllers drained during shutdown are committed before the Spanner client terminates. Eliminated the detachedgo m.kueueSync.RunPositionInQueueSync(ctx)goroutine.workloadSemusingr.MaxConcurrentKueueSyncReconcilesglobally across all queues. Added defensive boundary checks inNewReconcilerensuring concurrency limits default toKueueSyncTickDurationdefaults to 60s when non-positive (preventingtime.NewTicker(0)panics). Addedselectonctx.Done()when acquiringqueueSemandworkloadSemduring shutdown to terminate queue worker loops immediately without leaking goroutines.metadata.nameindexer onv1alpha2.JobSet. FixedlistJobSetPodsto query Pods byclient.MatchingLabels{v1alpha2.JobSetNameKey: jobSet.Name}rather thanMatchingFields. Cleaned up redundantWithIndexPod indexers injobset/reconciler_test.go.fakeClientwrapper struct inkueue_sync_test.goin favor of standardfake.Clientnow thatfetchJobSetByNameusesClient.Get. Removed deadc.Watchstring replacement rules fromcompat.bzl.nodes(get, list, watch) RBAC permissions toreconciler-rolefor TPU topology queries. AddedrunAsNonRoot: trueto both pod and containersecurityContextindeploy/helm/templates/reconciler/deployment.yaml.logging.Strip(ctx)workarounds acrossjobset,workload, andkueue_syncpredicates and API calls.Assumptions
deploy/helm/templates/reconciler/clusterrole.yamllacksleasespermissions, the reconciler is deployed as a single replica (replicas: 1) with VPA, and ungraceful leader failover risks dropping in-flight in-memory batches.metav1.Condition.LastTransitionTimechanges during upstream controller heartbeats; genericDeepEqualwould cause continuous reconcile loops and Spanner write storms.v1alpha2.JobSetNameKey(jobset.sigs.k8s.io/jobset-name) is injected as a label by the JobSet controller on all member Pods; verified via unit tests and Kubernetes API specs.Verification & Testing
bazel test --nocache_test_results //cmd/reconciler/...(6/6 targets passed)://cmd/reconciler:reconciler_test: PASSED//cmd/reconciler/hypercomputeclient:hypercomputeclient_test: PASSED//cmd/reconciler/jobset:jobset_test: PASSED//cmd/reconciler/kueue_sync:kueue_sync_test: PASSED (including newTestNewReconciler_Defaults)//cmd/reconciler/shared:shared_test: PASSED (includingTestBatchProcessorFlushesOnShutdown)//cmd/reconciler/workload:workload_test: PASSED//cmd/reconciler:reconciler_image_tarballbuilt successfully.helm lint deploy/helmpassed cleanly (1 chart linted, 0 failed)../.githooks/pre-commitpassed cleanly (Buildifier,gofmt,golangci-lint,go test determinism,addlicense, Helm lint, and Trivy security scanner).Related Issues / Bugs
N/A
TAG=agy
CONV=e4cc9b60-fdd6-4d8e-961b-43be76e282da