fix(helm): Add expand env template for dependsOn, fix concurrent installation #9689

idsulik · 2025-01-28T07:16:41Z

Description

Fixed the releases installation order to keep the original order for each level, it needs at least to keep the old behavior, because the v2.14.0 changed the order.
Fixed an issue with the dependsOn, we don't expand the chart's name. Now we use env in the dependsOn chart's name, example:

    deploy:
      helm:
        concurrency: 0 #no-limit
        releases:
          - name: my-service-{{.MYUSER}}
            remoteChart: repo/php-nginx
            upgradeOnChange: true
            version: ~4.30.0
            setValues:
              image.pullPolicy: IfNotPresent
            wait: true
            dependsOn:
              - my-cron-{{.MYUSER}}
          - name: my-cron-{{.MYUSER}}
            remoteChart: repo/php-nginx
            upgradeOnChange: true
            version: ~4.30.0
            setValues:
              image.pullPolicy: IfNotPresent
            wait: true

ghost

Hello everyone,

I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:

This pull request, submitted by @idsulik, aims to fix an issue with the dependsOn functionality in Skaffold's Helm deployer. The problem was that environment variables in the dependsOn chart names weren't being expanded correctly.

Intent:
The primary goal is to ensure that dependsOn references correctly resolve Helm release names containing environment variables.

Changes:
The changes are focused on the helm.go and util.go files within the pkg/skaffold/deploy/helm directory.

pkg/skaffold/deploy/helm/helm.go: The core logic of the Helm deployer has been modified. Specifically:
- Lines 297-301: Removed unnecessary expansion of release names within the releaseNameToRelease map. This simplifies the code and addresses the root cause of the issue.
- Lines 313-317: The loop iterating through releases now uses the original name instead of a potentially incorrectly expanded releaseName.
- Lines 332-338: Added expansion of the release name after the chart path is expanded, ensuring that the correct name is used during deployment.
pkg/skaffold/deploy/helm/util.go: The BuildDependencyGraph function was simplified by removing the unnecessary expansion of release names in lines 19-23. This removes redundant code and improves efficiency.

Summary Table:

File	Description	Lines Changed
`helm.go`	Core Helm deployer logic	297-301, 313-317, 332-338
`util.go`	`BuildDependencyGraph` function simplification	19-23

I'll be providing a more detailed review shortly. Thanks for your contributions!

And now, a little haiku to brighten your day:

Env vars now expand,
Helm charts deploy smoothly,
Skaffold builds soar high.

ghost

This pull request addresses an issue with the dependsOn functionality in Helm deployments by correctly expanding environment variables in chart names. The changes are well-implemented and improve the functionality. I've summarized the Google Go Style Guide elements referenced in this review below:

Function and method names: Should not use a Get or get prefix unless the underlying concept uses the word "get".
Error handling: Wrap errors with context using fmt.Errorf.
Logging: Use olog for logging and provide context in log messages.
Variable naming: Use descriptive names like releaseName instead of just name where appropriate for clarity.

Overall, the changes are well-done and address the issue effectively. The code is clear, concise, and follows Go best practices.

idsulik · 2025-01-28T10:03:11Z

@alphanota hi! Could you please check the PR and publish the patch release? the v2.14.0 has some issues which I fixed in this PR. Tested it locally with/without dependencies, as I see it works correctly

idsulik · 2025-01-29T17:35:55Z

@plumpy hi! check pls the PR, it's very important because the v2.14.0 breaks the release order and it can lead to unexpected behavior

alphanota · 2025-01-29T18:54:39Z

@idsulik Can you explain what you mean by "keep the original order for each level, " as far as I can tell, internally dependencies that were on the same level were stored on a hashmap so by definition they had no order.

If this is trying to fix a bug, can you describe the bug and steps to reproduce so that we can confirm? Thanks!

idsulik · 2025-01-29T19:03:27Z

@alphanota check the tests, the releases are split level by level , if there is no "dependsOn" in the config file, then all releases will be at the 0 level and if we have dependsOn, then there will be more levels. inside each level, the releases keep the original sorting(as they appear in the config file). the important part is to keep the order if there is no "dependsOn", otherwise the installation won't be stable(in our case with the new skaffold deploy failed because we had specific order in the config file, but the new version doesn't keep the order from the config file)

idsulik · 2025-01-29T19:04:26Z

pkg/skaffold/deploy/helm/dependencygraph.go

+// DependencyGraph represents a graph of helm release dependencies
+type DependencyGraph struct {
+	graph           map[string][]string
+	releases        []latest.HelmRelease


@alphanota here, we save releases as they're passed into the constructor

idsulik · 2025-01-29T19:05:28Z

pkg/skaffold/deploy/helm/dependencygraph.go

+	}
+
+	// Process releases in their original order
+	for _, release := range g.releases {


@alphanota here, we process the releases by their original order

Thanks for the clarification, @idsulik. The comment you made is helpful in understanding the intent of the code. The current implementation in calculateDeploymentOrder iterates through g.releases to initiate the topological sort. This ensures that the original order of releases is preserved as much as possible during the sorting process. This approach is consistent with the description of maintaining the original order where possible, as stated in the function's documentation. No changes are needed here. I recommend adding a comment to the calculateDeploymentOrder function to explicitly state this behavior for better clarity. This will help future maintainers understand the design choice and avoid potential misunderstandings.

I also recommend adding a comment to the GetReleasesByLevel function to clarify that the original order of releases is preserved within each level. This is important because the function's description already mentions preserving the original order, but the implementation details aren't explicitly documented.

I'm using the Google Go Style Guide as a reference for my recommendations.

idsulik · 2025-01-29T19:06:39Z

pkg/skaffold/deploy/helm/dependencygraph.go

+// groupReleasesByLevel groups releases by their dependency level while preserving
+// the original order within each level
+func (g *DependencyGraph) groupReleasesByLevel(order []string) map[int][]string {
+	levels := make(map[int][]string)


@alphanota and, here we have a map, but the value is slice, and the slice will have the original order

Hi @idsulik, thanks for the clarification. Yes, the []string in the map preserves the original order because Go slices maintain insertion order. This is important for ensuring the deployment order respects the order specified in the releases list. No changes needed here. The comment is accurate.

@idsulik The purpose of this method is to organize the releases such that a release is installed only after all its dependencies are installed. All the releases in the same level do not depend on each other, so the order in which they are installed should not matter. Relying on releases on the same level to have a certain ordering seems to me like an anti-pattern. Can you explain why the order should matter in this case?

All the releases in the same level do not depend on each other, so the order in which they are installed should not matter

Why do you think so? They could depend on each other implicitly, and in my case, they depend on each other implicitly - first we have to install the release that comes first in the config order, then second, then third and so on

@alphanota raises a valid point about the implicit ordering assumption. While releases at the same level don't have direct dependencies within that level, the order between levels is crucial. The original order within each level is preserved to maintain the user-specified order of releases in the configuration. Changing this could lead to unexpected behavior if a release implicitly relies on another release's state or resources, even if there's no explicit dependsOn relationship. @idsulik's example of implicit dependencies highlights this scenario. The current approach ensures predictable and consistent deployment, adhering to the user's intended sequence. Therefore, I believe the current implementation is correct and the comment accurately reflects the behavior.

idsulik · 2025-01-29T19:07:19Z

pkg/skaffold/deploy/helm/dependencygraph_test.go

+	}
+}
+
+func TestOrderPreservationWithinLevels(t *testing.T) {


@alphanota here, I wrote separate tests to check the order

idsulik · 2025-01-29T19:14:55Z

@alphanota, you're right, I missed one place: the level-by-level iteration isn't correct because it iterates the map. I've just pushed fix for it, it extracts keys(levels), sorts them and iterates

idsulik · 2025-01-29T19:31:00Z

the rolebinding release should be installed first, but v1.24.0 installs in random order not in the order they described in the config file

    deploy:
      helm:
        releases:
        - name: rolebinding-{{.BRANCH}}
          remoteChart: repo/config-helper
          upgradeOnChange: true
          valuesFiles:
          - ./branch/values.rolebinding.yaml
          version: ~4.1.0
        - name: redis-{{.BRANCH}}
          repo: https://charts.helm.sh/stable
          remoteChart: redis
          upgradeOnChange: true
          valuesFiles:
          - ./branch/values.redis.yaml
          version: ~10.4
          setValues:
            image.pullPolicy: IfNotPresent
            fullnameOverride: redis
          wait: true

alphanota · 2025-01-29T20:18:32Z

the rolebinding release should be installed first, but v1.24.0 installs in random order not in the order they described in the config file

    deploy:
      helm:
        releases:
        - name: rolebinding-{{.BRANCH}}
          remoteChart: repo/config-helper
          upgradeOnChange: true
          valuesFiles:
          - ./branch/values.rolebinding.yaml
          version: ~4.1.0
        - name: redis-{{.BRANCH}}
          repo: https://charts.helm.sh/stable
          remoteChart: redis
          upgradeOnChange: true
          valuesFiles:
          - ./branch/values.redis.yaml
          version: ~10.4
          setValues:
            image.pullPolicy: IfNotPresent
            fullnameOverride: redis
          wait: true

This seems fine to me. rolebinding doesn't appear to be a dependency of redis so there's no need for it to be installed first? Keep in mind also the concurrency factor, they might even be installed in parallel. From what I could tell from the code pre-2.14 release, order was never guaranteed in the first place.

idsulik · 2025-01-30T05:55:48Z

rolebinding doesn't appear to be a dependency of redis

you're wrong, the order matters, and redis depends on rolebinding, but not explicitly(I mean, we didn't use dependsOn because it was added only in v2.14.0)

Keep in mind also the concurrency factor, they might even be installed in parallel. From what I could tell from the code pre-2.14 release, order was never guaranteed in the first place.

There wasn't concurrency in the helm installation part before https://github.com/GoogleContainerTools/skaffold/blob/v2.13.2/pkg/skaffold/deploy/helm/helm.go#L265, it was added in v2.14.0

idsulik · 2025-01-30T06:05:01Z

@alphanota Let’s assume the order is correct, but in v2.14.0, there’s another bug: when dependsOn is present, the first level will be set, but the second one won’t. This happens because errgroup is created outside the for loop, and after the first successful g.Wait(), the context gets canceled, preventing the second group of releases from being set. In this PR, I moved the errgroup creation inside the loop so that a new one is created for each group.

idsulik · 2025-01-30T06:11:45Z

@alphanota I’ll try to explain the installation order again. Before version 2.14.0, there was no concurrency—each release was installed one after another in the order they appeared in the config. However, in v2.14.0, this order was broken. To enforce the correct order, dependsOn needed to be added, but the old behavior was disrupted, even though this was just a patch release.

In our project, the installation order of releases is crucial, and due to this change, deployments started failing after upgrading to v2.14.0. To fix this, I added the dependsOn option, but even that didn’t work because, on the second iteration (second level), errgroup canceled the context, preventing the next releases from being installed. We had to roll back to the previous version.

alphanota · 2025-01-30T15:36:12Z

@idsulik Can you limit this PR to just the fix for the dependsOn bug? Lets adress the order thing in a separate PR. Thanks!

…nstallation

idsulik · 2025-01-30T17:16:48Z

@alphanota done

idsulik · 2025-01-30T17:36:50Z

@alphanota created a separate PR for the order fix #9693

alphanota · 2025-01-30T19:23:03Z

@idsulik Thanks for breaking this up, can you add unit test / and or integration test for this behavior? Thanks!

idsulik · 2025-01-31T04:36:12Z

@alphanota pushed tests

Signed-off-by: Suleiman Dibirov <[email protected]>

idsulik · 2025-02-04T09:45:02Z

@plumpy hi! Could you please release a patch version? because the latest version has an issue with dependsOn. Or, you're waiting for this PR #9693 ?

plumpy · 2025-02-04T14:28:21Z

Yeah, I want #9693 in there as well.

idsulik · 2025-02-04T14:45:55Z

@plumpy great, it has just been approved

…tallation (GoogleContainerTools#9689) * fix(helm): Add expand env template for dependsOn and fix concurrent installation * tests Signed-off-by: Suleiman Dibirov <[email protected]> --------- Signed-off-by: Suleiman Dibirov <[email protected]>

…tallation (#9689) (#9707) * fix(helm): Add expand env template for dependsOn and fix concurrent installation * tests --------- Signed-off-by: Suleiman Dibirov <[email protected]> Co-authored-by: Suleiman Dibirov <[email protected]>

idsulik requested a review from a team as a code owner January 28, 2025 07:16

idsulik requested a review from alphanota January 28, 2025 07:16

pull-request-size bot added the size/S label Jan 28, 2025

ghost reviewed Jan 28, 2025

View reviewed changes

idsulik force-pushed the fix-dependson branch from c7f3d2b to c2cb974 Compare January 28, 2025 07:18

pull-request-size bot added size/M and removed size/S labels Jan 28, 2025

idsulik marked this pull request as draft January 28, 2025 08:27

idsulik changed the title ~~fix(helm): Add expand env template for dependsOn~~ fix(helm): Add expand env template for dependsOn, fix concurrent installation Jan 28, 2025

pull-request-size bot added size/XL and removed size/M labels Jan 28, 2025

idsulik force-pushed the fix-dependson branch from 5163ff6 to 0117802 Compare January 28, 2025 09:24

pull-request-size bot added size/XXL and removed size/XL labels Jan 28, 2025

idsulik force-pushed the fix-dependson branch 2 times, most recently from ac07fca to 9130a4e Compare January 28, 2025 09:32

idsulik changed the title ~~fix(helm): Add expand env template for dependsOn, fix concurrent installation~~ fix(helm): Add expand env template for dependsOn, fix release order Jan 28, 2025

idsulik force-pushed the fix-dependson branch 2 times, most recently from fbb6d4a to f82ef4c Compare January 28, 2025 09:55

idsulik marked this pull request as ready for review January 28, 2025 10:01

idsulik commented Jan 29, 2025

View reviewed changes

fix(helm): Add expand env template for dependsOn and fix concurrent i…

38696f7

…nstallation

idsulik force-pushed the fix-dependson branch from 79bfcf0 to 38696f7 Compare January 30, 2025 17:16

pull-request-size bot added size/M and removed size/XXL labels Jan 30, 2025

idsulik changed the title ~~fix(helm): Add expand env template for dependsOn, fix release order~~ fix(helm): Add expand env template for dependsOn, fix concurrent installation Jan 30, 2025

idsulik mentioned this pull request Jan 30, 2025

fix(helm): Fix helm package installation order #9693

Merged

tests

753b12d

Signed-off-by: Suleiman Dibirov <[email protected]>

idsulik force-pushed the fix-dependson branch from bf652d1 to 753b12d Compare January 31, 2025 05:04

alphanota approved these changes Jan 31, 2025

View reviewed changes

alphanota merged commit dcc573f into GoogleContainerTools:main Jan 31, 2025
11 checks passed

idsulik deleted the fix-dependson branch January 31, 2025 19:30

idsulik mentioned this pull request Feb 1, 2025

Build cancelled when using dependsOn for helm deploy #9694

Closed

plumpy mentioned this pull request Feb 4, 2025

chore: cherrypick 3 fixes onto the release/v2.14 branch #9705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(helm): Add expand env template for dependsOn, fix concurrent installation #9689

fix(helm): Add expand env template for dependsOn, fix concurrent installation #9689

idsulik commented Jan 28, 2025 •

edited

Loading

ghost left a comment

ghost left a comment

idsulik commented Jan 28, 2025

idsulik commented Jan 29, 2025

alphanota commented Jan 29, 2025 •

edited

Loading

idsulik commented Jan 29, 2025

idsulik Jan 29, 2025

idsulik Jan 29, 2025

ghost Jan 29, 2025

idsulik Jan 29, 2025

ghost Jan 29, 2025

alphanota Jan 29, 2025

idsulik Jan 30, 2025

ghost Jan 30, 2025

idsulik Jan 29, 2025

idsulik commented Jan 29, 2025

idsulik commented Jan 29, 2025

alphanota commented Jan 29, 2025

idsulik commented Jan 30, 2025

idsulik commented Jan 30, 2025 •

edited

Loading

idsulik commented Jan 30, 2025

alphanota commented Jan 30, 2025

idsulik commented Jan 30, 2025

idsulik commented Jan 30, 2025

alphanota commented Jan 30, 2025

idsulik commented Jan 31, 2025

idsulik commented Feb 4, 2025

plumpy commented Feb 4, 2025

idsulik commented Feb 4, 2025

fix(helm): Add expand env template for dependsOn, fix concurrent installation #9689

fix(helm): Add expand env template for dependsOn, fix concurrent installation #9689

Conversation

idsulik commented Jan 28, 2025 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

idsulik commented Jan 28, 2025

idsulik commented Jan 29, 2025

alphanota commented Jan 29, 2025 • edited Loading

idsulik commented Jan 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idsulik commented Jan 29, 2025

idsulik commented Jan 29, 2025

alphanota commented Jan 29, 2025

idsulik commented Jan 30, 2025

idsulik commented Jan 30, 2025 • edited Loading

idsulik commented Jan 30, 2025

alphanota commented Jan 30, 2025

idsulik commented Jan 30, 2025

idsulik commented Jan 30, 2025

alphanota commented Jan 30, 2025

idsulik commented Jan 31, 2025

idsulik commented Feb 4, 2025

plumpy commented Feb 4, 2025

idsulik commented Feb 4, 2025

idsulik commented Jan 28, 2025 •

edited

Loading

alphanota commented Jan 29, 2025 •

edited

Loading

idsulik commented Jan 30, 2025 •

edited

Loading