feat(executor): add MaxConcurrentReconciles config for TaskAction controller (#7205)#7356
Open
jbbqqf wants to merge 1 commit into
Open
feat(executor): add MaxConcurrentReconciles config for TaskAction controller (#7205)#7356jbbqqf wants to merge 1 commit into
jbbqqf wants to merge 1 commit into
Conversation
…troller (flyteorg#7205) Add a MaxConcurrentReconciles knob to the executor config and wire it through to the TaskAction controller via controller.Options. Before this change, SetupWithManager unconditionally built the controller without WithOptions, so operators had no way to scale the reconcile worker pool without forking the binary. The default is 1, matching controller-runtime's own default and preserving the historical single-worker behavior. Setting the config key to 0 explicitly defers to controller-runtime's default; any positive value is forwarded as-is. Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Jean-Baptiste Braun <jbaptiste.braun@gmail.com>
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.
Tracking issue
Closes #7205
Why are the changes needed?
The TaskAction controller in
executor/pkg/controller/taskaction_controller.gocurrently calls
SetupWithManagerwithout anyWithOptions, socontroller-runtime always uses its default
MaxConcurrentReconciles = 1.Operators who want to spend more CPU on parallel reconciles (because their
TaskAction queue grows faster than a single worker can drain it) have no
way to tune this without forking the binary.
Issue #7205 asks for the standard controller-runtime knob —
controller.Options.MaxConcurrentReconciles— to be exposed inthe executor config.
What changes were proposed in this pull request?
executor/pkg/config/config.go— addMaxConcurrentReconciles uint32to
Config, default1(matches controller-runtime's own default andpreserves the historical single-worker behavior). Field is registered
with the existing
pflagmechanism.executor/pkg/config/config_flags.go— register the new--executor.maxConcurrentReconcilespflag (the generated-flags file ishand-edited in this repo; same pattern was used for
maxSystemFailures).executor/pkg/controller/taskaction_controller.go— add an exportedMaxConcurrentReconciles intfield onTaskActionReconcilerand callWithOptions(controller.Options{MaxConcurrentReconciles: ...})onlywhen the field is
> 0. Zero falls through to controller-runtime's owndefault so callers that have not wired the knob yet still get the
pre-[V2] Add executor k8s controller runtime MaxConcurrentReconciles config #7205 behavior.
executor/setup.go— wirecfg.MaxConcurrentReconcilesinto thereconciler before
SetupWithManageris called, with an explanatorycomment about the
uint32 → intconversion.executor/pkg/config/config_test.go(new) — locks the default valueof
1as a tripwire; bumping the default would change worker poolsizing for every existing deployment.
executor/pkg/controller/taskaction_controller_test.go— adds aMaxConcurrentReconciles plumbing (#7205)Context with two specsasserting (a) the zero value yields the deferred-to-controller-runtime
branch and (b) an explicitly configured value is preserved on the
reconciler for
SetupWithManagerto forward.The diff is intentionally narrow — five files, +67/−3 lines — so a
reviewer can verify each piece independently. No proto changes, no
existing tests modified.
How was this patch tested?
Local checks
The controller-suite uses
envtest, which needskube-apiserver+etcdbinaries. I fetched them viasetup-envtest use 1.30.x --bin-dir /tmp/envtestand exportedKUBEBUILDER_ASSETSto that path; this matches the setup thatgetFirstFoundEnvTestBinaryDir(insuite_test.go) is looking for atbin/k8s/.... The 32 specs include the two new ones added in this PRplus the pre-existing 30.
Reproduce BEFORE/AFTER yourself (copy-paste)
A reviewer can verify the change end-to-end by pasting the block below.
The same
go testline runs onorigin/main(BEFORE) and on this PR(AFTER); the only thing that changes is the checked-out ref.
Labels
executor.maxConcurrentReconciles, no behavior change for unset / default.Setup process
No setup changes required for users on the default config. Operators who
want to scale reconciles add
executor.maxConcurrentReconciles: <N>tothe executor config (or
--executor.maxConcurrentReconciles=<N>on theCLI). A value of
0keeps the pre-PR behavior.Edge cases tested
defaultConfig.MaxConcurrentReconciles == 1TestDefaultMaxConcurrentReconcilesr := &TaskActionReconciler{}r.MaxConcurrentReconciles == 0;SetupWithManagerskips WithOptions; controller-runtime default appliesMaxConcurrentReconciles plumbing — defaults to 0 …r := &TaskActionReconciler{MaxConcurrentReconciles: 4}MaxConcurrentReconciles plumbing — preserves an explicitly configured value …make testof the controller packageRan 32 of 32 Specs in 8.871 secondsCheck all the applicable boxes
added on the new field and the
SetupWithManagermethod; no separatedocs site change since this is a runtime config knob covered by the
pflag help text.)
Related PRs
None directly. The change touches only executor-internal config
plumbing, so there is no proto or sister-service coordination required.
Risk / blast radius
Additive only. With the unset / default config (
MaxConcurrentReconciles = 1), the controller behaves exactly as before — controller-runtimealready uses 1 as its own default, and we now pass 1 explicitly. The
only observable change for an operator that does set the new key is the
intended one: more reconcile workers run in parallel, which the upstream
controller.Optionsdocumentation already covers. No proto change, noAPI surface change, no DB migration.
Release note
PR drafted with assistance from Claude Code. The change was reviewed
manually against
flyteorg/flyteHEAD and against the upstreamcontroller-runtime API; the reproducer block above is the same one used
during development. All commits signed off per project DCO policy
(
.github/dco.yml).