-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: performance recession when propagating namespace to helm #5971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: performance recession when propagating namespace to helm #5971
Conversation
|
Hi @Skaronator. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions 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. |
20486a7 to
c7612d1
Compare
|
This PR has multiple commits, and the default merge method is: merge. Instructions 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. |
|
Thanks, @Skaronator. Since you're already working on this one, would you be willing to include the test from #2869? If not, I can plan to do it myself, but that's likely to be a little while - I won't have the time for at least a few days. |
|
Hi @ephesused, |
|
@Skaronator, no worries. Thanks for this fix! |
|
I ran this change through the same testing process I noted over in #5940 (comment). This change looks good to me: |
|
Reverted the origin tracking which causes the performance issue. Now relying on a annotation to keep track that these resources are generated by Helm. Long term it would be probably better to revisit the Origin Tracking feature. That feature is behind a feature flag but the 20x slowdown still exist. |
|
Tests now pass. Somehow forget to commit that one file. |
plugin/builtin/namespacetransformer/NamespaceTransformer_test.go
Outdated
Show resolved
Hide resolved
|
/ok-to-test |
|
/uncc |
|
/retest |
|
/test kustomize-presubmit-master |
|
kustomize-presubmit-master now passes. Can anyone trigger the GitHub Actions again? Not sure why they don't run. I'm already a Contributor and the PR has ok-to-test label. |
|
Still LGTM: |
koba1t
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think looks like almost good with code review.
Please fix this comments.
plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go
Outdated
Show resolved
Hide resolved
Could you open new issue that tracking this performance problem? |
|
Thanks @Skaronator And thanks for your help @ephesused! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: koba1t, Skaronator The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [kubernetes-sigs/kustomize](https://github.com/kubernetes-sigs/kustomize) | minor | `v5.7.1` -> `v5.8.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>kubernetes-sigs/kustomize (kubernetes-sigs/kustomize)</summary> ### [`v5.8.0`](https://github.com/kubernetes-sigs/kustomize/releases/tag/kustomize/v5.8.0) [Compare Source](kubernetes-sigs/kustomize@kustomize/v5.7.1...kustomize/v5.8.0) ### Highlights ##### implements to replacements value in the structured data Now, We can edit yaml/json in yaml manifests with replacements transformer. See [#​5679](kubernetes-sigs/kustomize#5679) ##### For example ```yaml ## source apiVersion: v1 kind: ConfigMap metadata: name: source-configmap data: HOSTNAME: www.example.com --- apiVersion: v1 kind: ConfigMap metadata: name: target-configmap data: config.json: |- {"config": { "id": "42", "hostname": "REPLACE_TARGET_HOSTNAME" }} ``` ```yaml ## replacement replacements: - source: kind: ConfigMap name: source-configmap fieldPath: data.HOSTNAME targets: - select: kind: ConfigMap name: target-configmap fieldPaths: - data.config\.json.config.hostname ``` ##### fix: Propagate Namespace correctly to Helm The long-standing bug where kustomize's namespace transformer did not pass namespaces to helmCharts has been fixed. See [#​5940](kubernetes-sigs/kustomize#5940) ##### For example ```yaml ## define namespace namespace: any-namespace helmCharts: - name: minecraft repo: https://kubernetes-charts.storage.googleapis.com version: v1.2.0 # namespace: any-namespace ## propagates without additional namespace specific valuesFile: values.yaml ``` #### Feature [#​5679](kubernetes-sigs/kustomize#5679): implements to replacements value in the structured data [#​5863](kubernetes-sigs/kustomize#5863): Add regex support for Replacement selectors [#​5930](kubernetes-sigs/kustomize#5930): feat: add PatchArgs API type to populate patch options #### fix [#​5940](kubernetes-sigs/kustomize#5940): fix: Propagate Namespace correctly to Helm [#​5971](kubernetes-sigs/kustomize#5971): fix: performance recession when propagating namespace to helm [#​5942](kubernetes-sigs/kustomize#5942): fix fnplugin storagemounts validation [#​5958](kubernetes-sigs/kustomize#5958): fix: make AbsorbAll conflict error more verbose [#​5961](kubernetes-sigs/kustomize#5961): refactor: nested format string [#​5967](kubernetes-sigs/kustomize#5967): Fix infinite loop in HTTP client by validating URLs before requests [#​5985](kubernetes-sigs/kustomize#5985): fix(kyaml/yaml): minor nil safety fix for RNode.Content etc [#​5991](kubernetes-sigs/kustomize#5991): Fix duplicate key error when adding multiple labels with --without-selector #### Dependencies [#​5962](kubernetes-sigs/kustomize#5962): chore: update dependencies from security alert [#​5959](kubernetes-sigs/kustomize#5959): update go 1.24.6 #### chore [#​6007](kubernetes-sigs/kustomize#6007): Update kyaml to v0.21.0 [#​6008](kubernetes-sigs/kustomize#6008): Update cmd/config to v0.21.0 [#​6009](kubernetes-sigs/kustomize#6009): Update api to v0.21.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS4xNzMuMSIsInVwZGF0ZWRJblZlciI6IjQxLjE3My4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
This PR addresses the significant performance regression introduced by #5940 (namespace propagation to Helm charts). The original implementation
required expensive resource origin tracking, causing build times to increase from ~0.6s to over 13s.
Solution
Instead of using expensive resource origin tracking, this PR introduces a lightweight annotation-based approach:
Performance Impact
Technical Notes