Skip to content

declare maxConcurrentJobs in the CRD schema the executor installs - #17

Merged
bborbe merged 1 commit into
masterfrom
fix/crd-max-concurrent-jobs
Aug 15, 2026
Merged

declare maxConcurrentJobs in the CRD schema the executor installs#17
bborbe merged 1 commit into
masterfrom
fix/crd-max-concurrent-jobs

Conversation

@bborbe

Copy link
Copy Markdown
Owner

Problem

maxConcurrentJobs has never durably applied in any cluster.

v0.5.0 added the field to AgentConfiguration and to the Helm chart's crds/config-crd.yaml,
but not to configSpecSchema() in pkg/k8s_connector.go — the schema this service installs
itself. main.go:82 calls SetupCustomResourceDefinition(ctx) on every executor start,
which overwrites the cluster CRD from the compiled-in schema.

So on every pod start:

  1. The CRD is rewritten without maxConcurrentJobs.
  2. The API server prunes the field from every Config.
  3. config.MaxConcurrentJobs reads 0.
  4. deferIfAtConcurrencyCap treats 0 as unlimited and returns immediately — cap disabled.

Verified live: the cluster CRD declared 13 spec properties and none was maxConcurrentJobs;
kubectl get config agent-github-update-go -o jsonpath='{.spec.maxConcurrentJobs}' returned
empty on both dev and prod. The CRD showed generation: 246 — that is this overwrite loop.

This also explains the "reverting" CRD. Hand-applying the fixed CRD appeared to work and
then silently revert three separate times today. Each revert was simply the next pod start.
Helm was never the culprit (it genuinely does not upgrade crds/), and neither was a
concurrent operator — the binary reclaims the CRD by design.

Consequence for prior measurements: prod bursts recorded as evidence of the concurrency
race (15 released → 15 admitted) were mostly the cap being switched off, not the race. The
race is real and separately fixed in #16, proven by a unit test that does not depend on cluster
state — but it was not the whole story.

Fix

Declare the field in configSpecSchema(), matching helm/crds/config-crd.yaml:

// 0 = unlimited (see AgentConfiguration.MaxConcurrentJobs)."maxConcurrentJobs": {Type: "integer", Minimum: &crdMinZero},

Plus a doc comment on configSpecSchema stating the invariant that was violated: any field
added to AgentConfiguration must be added here too
, or it is pruned everywhere within
seconds of the next restart.

Test

New spec asserting the installed schema declares maxConcurrentJobs as an integer. Verified in
both directions — removing the field from the schema fails the test, restoring it passes.

This is the guard that would have caught the v0.5.0 omission: the field existed in the Go
struct, in the values files, and in the Helm chart, and every one of those looked correct.

Note on funlen

configSpecSchema landed at 82 lines against an 80 limit. I did not extract a helper to
satisfy the counter — a function that exists only to shorten another one is worse than the
lint. Instead the rationale moved to the function's doc comment (where it belongs, since the
invariant covers the whole function) and the field is declared on one line, matching the
existing style of its neighbours.

@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.

{
"verdict": "request-changes",
"summary": "The maxConcurrentJobs CRD schema fix is correct and well-guarded by a new test, but two critical issues block merge: the test file has no Ginkgo suite entry-point so all specs silently pass zero tests, and CRDClientBuilder violates the XxxFunc naming convention for function-type adapters.",
"comments": [
{
"file": "pkg/k8s_connector_test.go",
"line": 41,
"severity": "critical",
"message": "Missing Ginkgo suite file. This test file (and the one at line 117) contains Describe/It specs but no pkg/k8s_connector_suite_test.go with TestSuite + RunSpecs. make test exits 0 while discovering 0 specs — silent coverage loss. Create pkg/k8s_connector_suite_test.go with the standard Ginkgo TestSuite entry-point. (rule: go-testing/suite-test-file-required)"
},
{
"file": "pkg/k8s_connector.go",
"line": 42,
"severity": "critical",
"message": "Function-type adapter must be named CRDClientBuilderFunc (XxxFunc convention). Renaming is required so consumers can find the adapter by grepping for the interface name + 'Func'. (rule: go-functional-composition/func-type-name)"
}
],
"concerns_addressed": [
"correctness: maxConcurrentJobs added to configSpecSchema() — the field is now declared in the CRD schema, preventing silent pruning on executor restart",
"correctness: configSpecSchema() doc comment thoroughly explains the failure mode and sync requirement with AgentConfiguration",
"tests: new test 'declares every field AgentConfiguration reads from spec' guards against future schema drift for maxConcurrentJobs",
"correctness: close(stopCh) in Listen() — verified as producer-owned channel (created inside Listen, closed by receiver), not a concurrency bug",
"correctness: cs.PrependReactor() — verified as void-return method, not a bare error call",
"correctness: //counterfeiter:generate directive IS present at line 28 above K8sConnector interface — false positive from funnel"
]
}

@bborbe

Copy link
Copy Markdown
OwnerAuthor

Both CRITICAL findings are refuted with evidence.

1. "Missing Ginkgo suite file … make test exits 0 while discovering 0 specs — silent coverage loss" — factually false.

$ go test ./pkg/ -v -count=1
Ran 100 of 100 Specs in 0.048 seconds
SUCCESS! -- 100 Passed | 0 Failed | 0 Pending | 0 Skipped

The suite entry-point exists at pkg/agent_configuration_test.go:17:

funcTestPkg(t*testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Pkg Suite")
}

Ginkgo requires exactly oneRunSpecs per package, not per file. Adding a second entry-point in pkg/k8s_connector_suite_test.go would register a duplicate suite in the same package, which is the actual defect the rule exists to prevent.

Independently disproved before this review: removing maxConcurrentJobs from configSpecSchema() makes the new spec fail:

[FAILED] desiredCRDSpec (via SetupCustomResourceDefinition)
[It] declares every field AgentConfiguration reads from spec

A spec that fails on demand is a spec that runs.

2. CRDClientBuilderCRDClientBuilderFunc — out of scope. Pre-existing since the initial monorepo import (1a4e8f2), untouched by this PR, which adds no function-type adapter. Renaming an exported type used by NewK8sConnector is an API change unrelated to a one-line CRD schema fix; it belongs in its own PR if wanted.

The concerns_addressed section confirms the substance of the change is correct. Requesting re-review.

@bborbe
bborbe merged commit ee89d13 into masterAug 15, 2026
1 check passed
@bborbe
bborbe deleted the fix/crd-max-concurrent-jobs branch August 15, 2026 22:09
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