Skip to content

fix: include maxConcurrentJobs in ConfigSpec.Equal so config edits apply without restart - #20

Merged
bborbe merged 1 commit into
masterfrom
fix/config-equal-maxconcurrent
Aug 18, 2026
Merged

fix: include maxConcurrentJobs in ConfigSpec.Equal so config edits apply without restart#20
bborbe merged 1 commit into
masterfrom
fix/config-equal-maxconcurrent

Conversation

@bborbe

Copy link
Copy Markdown
Owner

What

  • k8s/apis/agent.benjamin-borbe.de/v1/types.go: add s.MaxConcurrentJobs == o.MaxConcurrentJobs to ConfigSpec.Equal.
  • types_test.go: regression test ("Equal - maxConcurrentJobs") — false when only this field differs, true when identical.
  • CHANGELOG entry under ## Unreleased.

Why

The executor's Config watch cache (eventHandlerAlert.OnUpdate, github.com/bborbe/k8s) calls Equal to decide whether a Config changed. MaxConcurrentJobs was missing from the comparison, so a maxConcurrentJobs-only edit to a live Config CR was treated as "nothing changed => skip update" and had no effect until the executor pod restarted.

Measured in prod 2026-08-18: CR patched maxConcurrentJobs: 1 -> 3 at ~21:00, executor still logged cap=1 at 21:09, and only took effect after a rollout restart at ~21:12 forced a full informer re-sync.

Verified

  • make test PASS (all packages; includes the new Equal - maxConcurrentJobs cases).

Deploy tail

  1. Merge.
  2. Build + upload executor image (make build / make upload at the release tag).
  3. Bump EXECUTOR_VERSION pin in quant/agent + chart version if needed.
  4. BRANCH=dev make apply then BRANCH=prod make apply (restart is inherent in the deploy).

@ben-s-pull-request-reviewerben-s-pull-request-reviewerBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now have all the information needed. Let me compile the findings.

What this PR changes:

  • types.go:168 — adds s.MaxConcurrentJobs == o.MaxConcurrentJobs to ConfigSpec.Equal
  • types_test.go:285-317 — adds Describe("Equal - maxConcurrentJobs", ...) with two Ginkgo It blocks
  • CHANGELOG.md — documents the fix

Mechanical findings (ast-grep funnel, pre-computed) — all are pre-existing in the codebase, not introduced by this PR. The single-line Equal addition follows the exact same pattern as every other field in that method.

Manual review of the diff:

  • The new test correctly covers the two cases: Equal returns false when values differ, true when identical
  • The Equal implementation is correct — value equality via == is consistent with all other scalar fields in that method (Assignee, Image, Heartbeat, TaskType, SecretName, VolumeClaim, VolumeMountPath, PriorityClassName, MaxConcurrentJobs)
  • The CHANGELOG entry is clear and explains the bug (Config watch cache was ignoring maxConcurrentJobs edits until restart)

Pre-existing issues not introduced by this PR:

  • Missing *_suite_test.go for the Ginkgo test suite (noted in funnel)
  • SecretName field lacking display:"length" tag (noted in funnel)
  • reflect.DeepEqual and errors.Wrapf direct package calls in business logic (pre-existing)

The PR is correct and focused. No issues introduced.


{
"verdict": "approve",
"summary": "The single-line addition of MaxConcurrentJobs to ConfigSpec.Equal is correct, follows the existing field-comparison pattern exactly, and is backed by a proper Ginkgo test covering both false-when-different and true-when-identical cases. The CHANGELOG entry clearly explains the bug and its user-visible impact. All mechanical findings are pre-existing issues in the codebase, not introduced by this change.",
"comments": [],
"concerns_addressed": [
"correctness: MaxConcurrentJobs comparison added to ConfigSpec.Equal at types.go:168 — follows existing scalar field pattern",
"tests: new Ginkgo Describe block 'Equal - maxConcurrentJobs' covers both false-when-different and true-when-matching cases"
]
}

@bborbe
bborbe merged commit 7e346be into masterAug 18, 2026
1 check passed
@bborbe
bborbe deleted the fix/config-equal-maxconcurrent branch August 18, 2026 21:56
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@bborbe