perf(helm): reuse the v4 post-renderer plugin and helm version across a run - #10153
Draft
kvey wants to merge 2 commits into
Draft
perf(helm): reuse the v4 post-renderer plugin and helm version across a run#10153kvey wants to merge 2 commits into
kvey wants to merge 2 commits into
Conversation
added 2 commits
August 6, 2026 20:09
… release Since Helm 4.0.0-beta.1, `PreparePostRenderer` installs Skaffold's post-renderer as a Helm plugin, and it does so on every call: once per release on the deploy path, once per config on the render path. Each call creates a temp dir, writes a manifest, shells out to `helm plugin install`, and the caller's defer shells out again to `helm plugin uninstall`. The manifest depends only on the Skaffold binary path. Everything that varies per release — labels, image replacements, debug settings, kubeconfig — travels in SKAFFOLD_CMDLINE at `helm template` / `helm install` invocation time, not in the manifest. So the plugin installed for release N is byte-identical to the one for release N+1 apart from its randomly generated name, and a single installed plugin serves the whole run. It is now installed once, keyed on the Skaffold binary, and removed at process shutdown by CleanupSharedPostRenderer. A different binary still forces a reinstall. On a repository with 21 Helm releases across 21 configs, `skaffold render` went from 42 plugin install/uninstall invocations to 2. Rendered output is unchanged (byte-identical). Helm v3 is unaffected: that path passes `--post-renderer <executable>` and installs no plugin at all. The cache is process-wide, so tests asserting on the executed command sequence must clear it between cases; ResetSharedPostRendererForTest does that and is wired into the affected tests.
BinVer shells out to `helm version` on every call, and it is called once per deployer and once per renderer. On a repository with 21 configs that is 23 invocations of a command whose answer cannot change: the helm binary on PATH is fixed for the lifetime of the process. Memoized, including the error result so a genuinely broken helm still reports on every call rather than being retried per config. The cache is process-wide, so tests asserting on the executed command sequence must clear it between cases; ResetBinVerCacheForTest does that and is wired into the affected tests.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Draft — opening early for direction before I invest further. Happy to split,
reshape, or drop either commit.
Summary
Two pieces of per-call work in the Helm renderer/deployer that are invariant for
the lifetime of the process, but are paid once per release on the deploy path
and once per config on the render path.
Two independent commits, reviewable separately:
perf(helm): install the v4 post-renderer plugin once per run, not per releaseSince
4.0.0-beta.1,PreparePostRendererinstalls Skaffold's post-rendereras a Helm plugin, and does so on every call: create a temp dir, write a
manifest,
helm plugin install, then the caller'sdeferrunshelm plugin uninstall.The manifest depends only on the Skaffold binary path. Everything that varies
per release — labels, image replacements, debug settings, kubeconfig —
travels in
SKAFFOLD_CMDLINEathelm template/helm installinvocationtime, not in the manifest. So the plugin installed for release N is
byte-identical to the one for release N+1 apart from its randomly generated
name.
Now installed once, keyed on the Skaffold binary, and removed at process
shutdown via
CleanupSharedPostRenderer. A different binary still forces areinstall.
Helm v3 is unaffected — that path passes
--post-renderer <executable>and installs no plugin at all, which is why this is invisible to most users
today.
perf(helm): memoizehelm version``BinVershells out per deployer and per renderer. The helm binary on PATHcannot change mid-run.
Impact
Measured on a repository with 21 Helm releases across 21 configs (
skaffold render, warm caches, alternating A/B against the same commit, 6 runs each):plugin install/uninstallhelm versionThe distributions do not overlap (before 32.01–35.21s, after 19.03–20.70s), and
rendered output is byte-identical before and after (179,484 bytes).
The saving scales with release count, so it is negligible for a handful of
releases and material for a large monorepo. It also compounds with
#8363 /
#5417 (serial
render/deploy of Helm releases) — this doesn't address that, it just stops each
serialized release from paying avoidable subprocess overhead.
Risks / things worth reviewer attention
assert on the executed command sequence observe each other. I added
ResetSharedPostRendererForTestandResetBinVerCacheForTestand wired theminto the affected tests. If you'd prefer a different mechanism (a
TestMainreset, or threading state through a struct rather than package-level), say so —
package-level state was chosen to keep the diff small, not because it's the
nicest shape.
CleanupSharedPostRendereris called frommain(), afterapp.Run. Thatis a new shutdown responsibility; if there's a more idiomatic teardown hook I
missed, point me at it.
Clientused at install time to uninstall with, becausegenerateHelmCommanddereferences theClientforGlobalFlags()regardlessof subcommand, so passing
nilthere would panic.BinVernow memoizes failures too, so abroken
helmreports the same error per call instead of re-executing. Thatseemed right, but it is a change.
Testing
binary, Helm v3 installs no plugin, cleanup safe when nothing installed.
go test ./pkg/skaffold/helm/... ./pkg/skaffold/deploy/helm/... ./pkg/skaffold/render/renderer/helm/...passes.go build ./...andgo vetclean.I have not run the integration suite — happy to if that's expected before this
leaves draft.
CLA
Not yet signed. I'll get that sorted before this is ready for review.
🤖 Generated with Claude Code