fix: pull helm charts atomically to survive concurrent kustomize builds - #6260
fix: pull helm charts atomically to survive concurrent kustomize builds#6260KR-Ravindra wants to merge 1 commit into
Conversation
|
Welcome @KR-Ravindra! |
|
|
|
Hi @KR-Ravindra. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: KR-Ravindra The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/easycla |
1 similar comment
|
/easycla |
e84b8ff to
6a51abe
Compare
|
Round 1 self-review (AI agent operated by me; I review every revision). Staying a draft until this is fixed:
Verified against |
Concurrent kustomize processes sharing a chartHome all observe the chart as missing and all run 'helm pull --untar' into the same directory; helm fails every process but the first with 'already exists'. Pull into a private temporary directory inside the chart home and rename the extracted chart into place. The rename is atomic, so the chart directory is either absent or complete; a rename that loses to another process is treated as success and the temporary directory is removed. Add HarnessEnhanced.LoadGenerator so a test can run several configured generators concurrently, and cover the race with a test that serves a local chart repository and pulls it from eight generators at once.
6a51abe to
f88ef73
Compare
|
/easycla |
|
Self-review before marking ready. The pull now extracts into a per-process temporary directory inside the chart home and moves the chart into place atomically, so concurrent builds sharing a chartHome never collide and a half-extracted chart is never used; the new test runs eight generators concurrently against a local Helm repository and fails on master with the exact error from the issue. |
|
@koba1t @sarab97 @varshaprasad96 when one of you has a moment, could this get |
Problem
Running several
kustomize build --enable-helmprocesses at the same time against overlays that share a base withhelmCharts(the usual CI or Argo CD setup) fails intermittently with:Only the first process succeeds; every other process that decided to pull the same chart is failed by
helm pull, even though the chart it wanted is now present and complete.Root cause
plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go:281-289checkschartExistsLocally()and, when the chart directory is absent, runshelm pull --untar --untardir <absChartHome>(pullCommand(), line 336). Nothing ties the check to the pull: two processes sharing achartHomeboth observe "missing", both runhelm pullinto the same directory, and helm refuses to untar into a chart directory that another process has meanwhile created (helm/helm#12315 documents that helm considers this the caller's responsibility). The pull error is returned as-is, so the whole build fails.Simply ignoring the helm error is not safe either: the winning process may still be extracting the archive when the loser re-checks, so the loser could run
helm templateagainst a half-extracted chart.Fix
pullChart()now pulls into a private temporary directory created inside the chart home (os.MkdirTemp(chartHome, ".kustomize-pull-")) and then moves the extracted chart into place withos.Rename. Every process extracts into its own directory, so helm never sees a collision, and the rename is atomic, so<chartHome>/<chart>is either absent or complete. If the rename fails because another process already placed the chart there, that chart is used and no error is raised. The temporary directory is removed either way.pullCommand()takes the untar directory as a parameter. No change to the on-disk layout ofchartHome, so existing pulled charts and local charts keep working.The generated copy in
api/internal/builtins/HelmChartInflationGenerator.gowas regenerated withmake api/internal/builtins/HelmChartInflationGenerator.go.api/testutils/kusttest.HarnessEnhancedgainsLoadGenerator(config), which loads and configures a generator without running it;LoadAndRunGeneratorWithBuildAnnotationsis now built on top of it. This lets the test run several configured generators concurrently.How tested
New test
TestHelmChartInflationGeneratorConcurrentPull(plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator_test.go) packagestestdata/charts/test-chart, serves it from anhttptesthelm repository, and runs eight generators that share onechartHomeconcurrently. It asserts that every generator succeeds with identical output and that only the chart directory remains in the chart home (no leftover temporary directories). It shells out tohelmV3like the neighbouring tests and skips when helm is unavailable.Before the fix (
go test -run TestHelmChartInflationGeneratorConcurrentPullin the plugin module):After the fix:
gofmt,go vetandgolangci-lint(v1.64.8, repo config) are clean for the plugin module and the changedapipackages. Verified with helm v3.21.4.Links
This change was prepared with an AI agent operated by KR-Ravindra, who reviewed and tested it.