Pipeline KNE cluster init deploys - #768
Merged
Merged
Conversation
fake.Command served double duty as both the shared response registry and the exec.Cmd handed back to the caller: Command() mutated cmd/args on the receiver and returned the receiver itself. Any code under test that ran commands from more than one goroutine therefore raced on the command name, args and stdio, and could observe another goroutine's output. Split the two roles. Command keeps the response bookkeeping and now hands out an independent invocation per call, and the response matching happens under a mutex. The lock is held across the LogCommand callback as well, so test hooks that record commands stay serialized and need no locking of their own. Response ordering semantics are unchanged: responses are still matched in order by default, so tests that issue commands concurrently need to mark the affected responses OutOfOrder. No change to the exported API.
CNI and controllers had SetKClient called immediately *after* their Deploy, not before. That works only because their Deploy is currently a plain kubectl apply that never touches the client; the moment one of them needs it, as Ingress already does, it would see a nil client. Move all of the client injection into a single block before anything is deployed. No behavior change today, but it removes an ordering trap and is a prerequisite for deploying the components concurrently, where "set it just after Deploy" has no well-defined meaning.
Deploy walked the components one at a time, deploying each and then
blocking until it reported healthy before touching the next. The
components are independent: each applies its own manifests and waits for
its own workloads in its own namespace, and none reads state produced by
another. Serializing them made a deployment cost the sum of every
component's rollout instead of the slowest one.
Run them in an errgroup instead. Deployment.Healthy gets the same
treatment, since it had the identical serialized shape.
Timeouts are per component, not shared. Each component's health budget
starts when that component finished deploying, which keeps the existing
meaning of healthTimeout exactly ("one rollout should take under a
minute") rather than silently charging a component for a slow sibling.
The decisive argument is debuggability: under a single shared deadline
everything still pending expires at the same instant, so a failure
reports N simultaneous timeouts and says nothing about which component
was actually stuck.
Because the components share a context, the first failure cancels the
rest, so errors are classified before being returned: exceeded its own
health budget, hit the overall deployment timeout, or was abandoned when
something else failed. Without that the one real error is buried in a
pile of indistinguishable "context canceled". Controllers, which were
previously anonymous in both logs and errors, now name themselves by
type, so a failure says "SRLinux controller" rather than "controller".
Also add an overall timeout for the phase, exposed as --deploy_timeout
and defaulting to 10 minutes. Nothing bounded a hung Deploy before: the
per-component budgets only cover the health wait, and kubectl has no
timeout of its own, so a stuck deployment relied on the calling
automation to eventually kill it. The timeout deliberately does not
cover bringing up the cluster, which happens first and is largely not
interruptible, and folding it in would reintroduce exactly the
"charged for someone else's slowness" problem the per-component budgets
avoid.
One behavior change worth noting: a nil Ingress or CNI is now skipped
with a warning. Sequentially a nil component was frequently never
reached because an earlier one failed first; running them together
always reaches it, and panicking there would be a poor trade.
Deployment.Deploy had no test, so extract the concurrent phase into
deployComponents, which is testable without a cluster, kubeconfig or
kubectl, and cover overlap, error attribution and both timeout paths.
Operator readiness costs are larger than the 60s budget allowed for, and they are not noise. Measured across two from-scratch deployments, MetalLB took 69.15s and 69.25s to become healthy, Lemming 45.36s and 45.43s, and IxiaTG 44.12s and 44.18s: three of five components reproduce to within 100ms, which points at fixed costs like leader election and webhook cert setup rather than at scheduling variance. MetalLB has therefore never fit inside a one minute budget. It did not fail because it was never held to one: the wait that matters happens inside MetalLBSpec.Deploy, so the budget applied to Healthy only ever saw an already-healthy deployment. Concurrency did not create this, it just made it visible. The remaining components sat at 44-46s, which cleared 60s but not by much, and would not survive a slower machine. Being generous here is safe now in a way it was not before. The budget used to be the only limit on the phase, so it had to double as the backstop against a stuck deployment; defaultDeployTimeout now plays that role, leaving healthTimeout to express what it is actually for, namely how long one rollout should reasonably take.
Deploying the components concurrently interleaves their kubectl output. Every line is prefixed only with the binary that produced it, so a real deployment emits runs like three "(kubectl): ..." lines sharing a timestamp with nothing to say which component each belongs to. That undoes much of the debuggability the per-component error attribution was meant to provide. Let a context carry a label, and prefix the output of commands run with it as "(kubectl/SRLinux controller): ...". A context is the right carrier here: the components already receive one, and a component does not know its own name, since the name is derived by the deployment that drives it. The label is logging only. It deliberately does not tie a command's lifetime to the context, as nothing today cancels a running kubectl; that remains the gap defaultDeployTimeout covers from the outside. LogCommand and the other existing helpers are unchanged, so callers that do not run commands concurrently, notably cluster bringup, need not care.
bstoll
approved these changes
Sep 23, 2026
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
2 participants
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.
Fair warning - there's nothing important gated by this, but it addresses something that annoys me when doing KNE development and thus running it repeatedly.
Summary
kne deploybrought the ingress, CNI and controllers up one at a time, deployingeach and blocking until it reported healthy before starting the next. These
components are independent — each applies its own manifests and waits for its own
workloads in its own namespace, and none reads state produced by another — so the
serialization made a deployment cost the sum of every component's rollout rather
than the slowest one.
This deploys them concurrently.
Measured on a from-scratch kind deployment, the component phase went from an
estimated ~4m00s to a measured 1m10s, a ~3.4x improvement. Re-running against
an existing cluster completes in 1.8s.
Per-component health waits from that run:
Commits
Each commit builds and passes tests on its own.
exec/fake: makeCommandsafe for concurrent use —fake.Commandservedas both the shared response registry and the
exec.Cmdhanded to the caller:Command()mutated the receiver and returned it. Any code under test runningcommands from more than one goroutine raced. Split the two roles; no exported
API change.
deploy: inject the k8s client into all components before deploying —CNI and controllers had
SetKClientcalled after theirDeploy, whichworked only because their
Deploynever touched the client. No behaviorchange; prerequisite for the next commit.
deploy: bring up ingress, CNI and controllers concurrently — the mainchange. Also applies to
Deployment.Healthy, which had the same serializedshape.
deploy: raise the per-component health budget to 3 minutes — see below.exec/run,deploy: tag command output with the component that ran it —restores attributability of interleaved
kubectloutput.Design notes
Timeouts are per component, not shared. Each component's health budget starts
when that component finished deploying. A single shared deadline would charge a
component for a slow sibling, and — the deciding argument — would expire for
everything still pending at the same instant, so a failure would report N
simultaneous timeouts and say nothing about which component was actually stuck.
Errors are classified before being returned. The components share a context,
so the first failure cancels the rest. Without classification the one real error
is buried among indistinguishable
context canceled. Failures now report as oneof: exceeded its own health budget, hit the overall deployment timeout, or was
abandoned because something else failed. Controllers, previously anonymous in
both logs and errors, now name themselves by type —
SRLinux controller was not healthy within 3m0srather thanfailed to check controller is healthy.New overall timeout,
--deploy_timeout, default 10 minutes. Nothing boundeda hung
Deploybefore: the per-component budgets only cover the health wait, andkubectlhas no timeout of its own, so a stuck deployment relied on the callingautomation to eventually kill it. It deliberately does not cover cluster
bringup, which happens first and is largely not interruptible; folding that in
would reintroduce the "charged for someone else's slowness" problem the
per-component budgets avoid.
Why the health budget went from 1m to 3m. Operator readiness is mostly a fixed
cost, not noise. Across two from-scratch deployments MetalLB took 69.15s and
69.25s, Lemming 45.36s and 45.43s, IxiaTG 44.12s and 44.18s — three of five
reproduce to within 100ms. MetalLB has therefore never fit inside a one minute
budget; it never failed only because the wait that matters happens inside
MetalLBSpec.Deploy, so the budget applied toHealthyonly ever saw analready-healthy deployment. Concurrency did not create this, it made it visible.
The remaining components sat at 44–46s, clearing 60s but not by much. Being
generous is safe now in a way it was not before: the budget used to be the only
limit on the phase and had to double as the backstop against a stuck deployment,
a role
--deploy_timeoutnow plays.Behavior changes reviewers should notice
healthTimeout1m → 3m (rationale above).IngressorCNIis now skipped with a warning rather thanpanicking. Sequentially a nil component was frequently never reached because an
earlier one failed first; running them together always reaches it. The CLI
already rejects a config missing these, so this only affects direct library
users.
(kubectl/<component>). Cluster-level commands are unchanged as(kubectl).--deploy_timeout(duration, 0 = default) and a correspondingDeployment.Timeoutfield.Testing
Deployment.Deploypreviously had no test coverage. The concurrent phase isextracted as
deployComponents, which is testable without a cluster,kubeconfig or kubectl, with new tests covering actual overlap, error
attribution, both timeout paths, nil components and command labelling.
exec/fakeconcurrency test; verified it reproduces the old data racesunder
-raceand passes on the fix.go test -raceacross affected packages, stable over repeated runs.against an existing cluster.
Not included
the model uniform would mean splitting
MetalLBSpec.Deployin two — theIPAddressPoolCRs genuinely require the webhook to be up first — and changingthe
Ingressinterface. Not worth it for the uniformity alone.MetalLBSpec.Deployalready waits internally, so the subsequent
Healthyis near a no-op(measured at 3ms).
Ingressis an exported interface and an out-of-treeimplementation may rely on the caller invoking
Healthy, so dropping it wouldbe a silent contract change.
kubectl apply. In principlesimultaneous applies could race on shared cluster-scoped resources, but the
bundled manifests do not overlap — each operator owns its own namespace and
CRDs — and no conflict has been observed.