From 13c23424b273e2092942d6b30ba93fdcc135524c Mon Sep 17 00:00:00 2001 From: Rohit Gudi <50377477+caretak3r@users.noreply.github.com> Date: Sun, 3 May 2026 13:11:31 -0400 Subject: [PATCH] fix(hip-0025): thread context through updateAndWait and chartPath through nested subcharts Two findings from Copilot's review of the consolidated commit on 2026-05-03: - updateAndWait silently dropped its context arg (named `_`), so sequenced upgrades and rollbacks never honored cancellation through Build/Update/Wait. Thread the context through and add ctx.Done() select gates matching createAndWait. - GroupManifestsByDirectSubchart used the bare chart name as path prefix. At top level this is fine because manifest names are rooted there, but on recursion into a subchart the call passed the subchart's own name (e.g. "sub") while manifest names are still rooted at the top-level chart ("parent/charts/sub/charts/nested/..."). Result: 3+ deep nesting flattened into the parent subchart's batch, silently bypassing nested sequencing. Fix: thread chartPath through deployChartLevel and deleteChartLevelReverse so each recursion level sees the full path-prefix it should match against. Add a unit test exercising the deeper-recursion case and update the uninstall test that had documented this as "a known limitation." Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com> --- pkg/action/sequencing.go | 65 +++++++++++++++----------- pkg/action/sequencing_test.go | 18 +++++++ pkg/action/uninstall.go | 9 ++-- pkg/action/uninstall_sequenced_test.go | 10 ++-- 4 files changed, 66 insertions(+), 36 deletions(-) diff --git a/pkg/action/sequencing.go b/pkg/action/sequencing.go index 65bcad88d..102e30649 100644 --- a/pkg/action/sequencing.go +++ b/pkg/action/sequencing.go @@ -45,32 +45,29 @@ func computeDeadline(timeout time.Duration) time.Time { } // GroupManifestsByDirectSubchart groups manifests by the direct subchart they belong to. -// The current chart's own manifests are returned under the empty string key "". -// Subcharts are keyed by their immediate name under the first `/charts//` -// segment found in the manifest source path. -// Nested subcharts (e.g., `/charts/sub/charts/nested/`) are grouped under -// the direct subchart name ("sub"), since nested sequencing is handled recursively. -func GroupManifestsByDirectSubchart(manifests []releaseutil.Manifest, chartName string) map[string][]releaseutil.Manifest { +// chartPath is the full path-prefix for the current chart level — at the top level +// it is the chart name (e.g. "parent"); at deeper recursion levels it is the joined +// path through "/charts/" segments (e.g. "parent/charts/sub"). +// The current chart level's own manifests are returned under the empty string key "". +// Direct subcharts are keyed by their immediate directory name under +// "/charts//". Nested grandchildren are grouped under their +// direct subchart parent ("sub"), since nested sequencing is handled recursively. +func GroupManifestsByDirectSubchart(manifests []releaseutil.Manifest, chartPath string) map[string][]releaseutil.Manifest { result := make(map[string][]releaseutil.Manifest) - if chartName == "" { - // Fallback: assign everything to parent + if chartPath == "" { result[""] = append(result[""], manifests...) return result } - chartsPrefix := chartName + "/charts/" + chartsPrefix := chartPath + "/charts/" for _, m := range manifests { if !strings.HasPrefix(m.Name, chartsPrefix) { - // Parent chart manifest result[""] = append(result[""], m) continue } - // Extract the direct subchart name (first segment after "/charts/") rest := m.Name[len(chartsPrefix):] - // rest is like "subchart1/templates/deploy.yaml" or "subchart1/charts/nested/..." subchartName, _, ok := strings.Cut(rest, "/") if !ok { - // Unlikely: a file directly under charts/ with no subdirectory result[""] = append(result[""], m) continue } @@ -126,10 +123,15 @@ type sequencedDeployment struct { // It first handles subcharts in dependency order (recursively), then deploys the // parent chart's own resource-group batches. func (s *sequencedDeployment) deployChartLevel(ctx context.Context, chrt *chartv2.Chart, manifests []releaseutil.Manifest) error { - // Group manifests by direct subchart - grouped := GroupManifestsByDirectSubchart(manifests, chrt.Name()) + return s.deployChartLevelAt(ctx, chrt, manifests, chrt.Name()) +} + +// deployChartLevelAt is the recursive worker. chartPath tracks the manifest +// path-prefix of the current chart level so nested subcharts route correctly: +// top-level "parent" → child "parent/charts/sub" → grandchild "parent/charts/sub/charts/nested". +func (s *sequencedDeployment) deployChartLevelAt(ctx context.Context, chrt *chartv2.Chart, manifests []releaseutil.Manifest, chartPath string) error { + grouped := GroupManifestsByDirectSubchart(manifests, chartPath) - // Build subchart DAG and deploy in topological order dag, err := chartutil.BuildSubchartDAG(chrt) if err != nil { return fmt.Errorf("building subchart DAG for %s: %w", chrt.Name(), err) @@ -140,7 +142,6 @@ func (s *sequencedDeployment) deployChartLevel(ctx context.Context, chrt *chartv return fmt.Errorf("getting subchart batches for %s: %w", chrt.Name(), err) } - // Deploy each subchart batch in order for batchIdx, batch := range batches { for _, subchartName := range batch { subManifests := grouped[subchartName] @@ -148,11 +149,8 @@ func (s *sequencedDeployment) deployChartLevel(ctx context.Context, chrt *chartv continue } - // Find the subchart chart object for recursive nested sequencing subChart := findSubchart(chrt, subchartName) if subChart == nil { - // Subchart not found in chart object (may have been disabled or aliased differently) - // Fall back to flat resource-group deployment for these manifests s.cfg.Logger().Warn("subchart not found in chart dependencies; deploying without subchart sequencing", "subchart", subchartName, "batch", batchIdx, @@ -163,14 +161,13 @@ func (s *sequencedDeployment) deployChartLevel(ctx context.Context, chrt *chartv continue } - // Recursively deploy the subchart (handles its own nested subcharts and resource-groups) - if err := s.deployChartLevel(ctx, subChart, subManifests); err != nil { + subPath := chartPath + "/charts/" + subchartName + if err := s.deployChartLevelAt(ctx, subChart, subManifests, subPath); err != nil { return fmt.Errorf("deploying subchart %s: %w", subchartName, err) } } } - // Deploy parent chart's own resources (after all subchart batches complete) parentManifests := grouped[""] if len(parentManifests) > 0 { if err := s.deployResourceGroupBatches(ctx, parentManifests); err != nil { @@ -392,7 +389,12 @@ func (s *sequencedDeployment) createAndWait(ctx context.Context, manifests []rel // updateAndWait applies an upgrade batch using KubeClient.Update() and waits for readiness. // It matches current (old) resources by objectKey to compute the per-batch diff. -func (s *sequencedDeployment) updateAndWait(_ context.Context, manifests []releaseutil.Manifest) error { +func (s *sequencedDeployment) updateAndWait(ctx context.Context, manifests []releaseutil.Manifest) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } if len(manifests) == 0 { return nil } @@ -414,9 +416,12 @@ func (s *sequencedDeployment) updateAndWait(_ context.Context, manifests []relea return fmt.Errorf("stripping sequencing annotations: %w", err) } - // Find the subset of current (old) resources that are represented in this batch. - // Update() will handle creates (target resources not in matchingCurrent) and - // updates (resources in both). Deletions are handled separately after all batches. + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + targetKeys := make(map[string]bool, len(target)) for _, r := range target { targetKeys[objectKey(r)] = true @@ -440,6 +445,12 @@ func (s *sequencedDeployment) updateAndWait(_ context.Context, manifests []relea } s.createdResources = append(s.createdResources, result.Created...) + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + return s.waitForResources(target, manifests) } diff --git a/pkg/action/sequencing_test.go b/pkg/action/sequencing_test.go index 0eb92c1c4..fc146c477 100644 --- a/pkg/action/sequencing_test.go +++ b/pkg/action/sequencing_test.go @@ -312,6 +312,24 @@ func TestSequencing_GroupManifestsByDirectSubchart(t *testing.T) { require.Len(t, grouped["database"], 2) } +// TestSequencing_GroupManifestsByDirectSubchart_Nested verifies that when called +// with a deeper chartPath (i.e., during recursion into a subchart), nested +// grandchildren are routed to the correct subchart key instead of being merged +// into the parent batch. +func TestSequencing_GroupManifestsByDirectSubchart_Nested(t *testing.T) { + manifests := []releaseutil.Manifest{ + makeTestManifest("db-own", "parent/charts/database/templates/one.yaml", nil), + makeTestManifest("cache", "parent/charts/database/charts/cache/templates/one.yaml", nil), + } + + grouped := GroupManifestsByDirectSubchart(manifests, "parent/charts/database") + + require.Len(t, grouped[""], 1, "database's own resources should be under the empty key") + require.Len(t, grouped["cache"], 1, "nested cache subchart should be routed under its own key") + require.Equal(t, "parent/charts/database/templates/one.yaml", grouped[""][0].Name) + require.Equal(t, "parent/charts/database/charts/cache/templates/one.yaml", grouped["cache"][0].Name) +} + func TestSequencing_BuildManifestYAML(t *testing.T) { yaml := buildManifestYAML([]releaseutil.Manifest{ makeTestManifest("one", "chart/templates/one.yaml", nil), diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index 0e8afeb98..b6df13539 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -341,11 +341,11 @@ func (u *Uninstall) sequencedDeleteManifests(chrt *chart.Chart, manifests []rele if chrt == nil { return u.deleteResourceGroupBatchesReverse(manifests, waiter, deadline) } - return u.deleteChartLevelReverse(chrt, manifests, waiter, deadline) + return u.deleteChartLevelReverseAt(chrt, manifests, waiter, deadline, chrt.Name()) } -func (u *Uninstall) deleteChartLevelReverse(chrt *chart.Chart, manifests []releaseutil.Manifest, waiter kube.Waiter, deadline time.Time) (kube.ResourceList, []error) { - grouped := GroupManifestsByDirectSubchart(manifests, chrt.Name()) +func (u *Uninstall) deleteChartLevelReverseAt(chrt *chart.Chart, manifests []releaseutil.Manifest, waiter kube.Waiter, deadline time.Time, chartPath string) (kube.ResourceList, []error) { + grouped := GroupManifestsByDirectSubchart(manifests, chartPath) allDeleted, errs := u.deleteResourceGroupBatchesReverse(grouped[""], waiter, deadline) if len(errs) > 0 { @@ -375,11 +375,12 @@ func (u *Uninstall) deleteChartLevelReverse(chrt *chart.Chart, manifests []relea subchart := findSubchart(chrt, subchartName) var deleted kube.ResourceList + subPath := chartPath + "/charts/" + subchartName if subchart == nil { u.cfg.Logger().Warn("subchart not found in chart dependencies during sequenced uninstall; falling back to flat resource-group deletion", "subchart", subchartName) deleted, errs = u.deleteResourceGroupBatchesReverse(subchartManifests, waiter, deadline) } else { - deleted, errs = u.deleteChartLevelReverse(subchart, subchartManifests, waiter, deadline) + deleted, errs = u.deleteChartLevelReverseAt(subchart, subchartManifests, waiter, deadline, subPath) } allDeleted = append(allDeleted, deleted...) if len(errs) > 0 { diff --git a/pkg/action/uninstall_sequenced_test.go b/pkg/action/uninstall_sequenced_test.go index f48cd8d51..917a1d58b 100644 --- a/pkg/action/uninstall_sequenced_test.go +++ b/pkg/action/uninstall_sequenced_test.go @@ -231,11 +231,11 @@ func TestUninstall_Sequenced_WithSubcharts(t *testing.T) { _, err := uninstall.Run(rel.Name) require.NoError(t, err) - // Nested subcharts within a subchart's subtree are flattened into one delete - // batch because GroupManifestsByDirectSubchart receives full-path manifest - // names ("parent/charts/bar/charts/nginx/...") that don't match the nested - // chart prefix ("bar/charts/"). This is a known limitation. - assert.Equal(t, [][]string{{"ConfigMap/parent"}, {"ConfigMap/nginx", "ConfigMap/bar"}}, client.deleteCalls) + // Reverse-DAG uninstall: parent first, then bar (its own resources before + // recursing into bar's subchart batch), then nginx (innermost). With the + // chart-path threading fix, nested subcharts route through their proper + // subtree level instead of flattening. + assert.Equal(t, [][]string{{"ConfigMap/parent"}, {"ConfigMap/bar"}, {"ConfigMap/nginx"}}, client.deleteCalls) assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) }