HYPERFLEET-1109 - feat: Add after suite sweep to clean up resources#139
Conversation
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Suite as pkg/e2e/suite.go
participant Cleanup as pkg/helper/cleanup.go
participant Helm as pkg/helper/helm/helm.go
participant K8s as Kubernetes API
Suite->>Cleanup: CleanupResources()
Cleanup->>Helm: ListReleasesBySelector(run-id selector)
Cleanup->>Helm: UninstallRelease(release, namespace)
Cleanup->>K8s: sweep labeled resources
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
ma-hill
force-pushed
the
HYPERFLEET-1109-2
branch
from
b53834a to
37e70b8
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
ma-hill
force-pushed
the
HYPERFLEET-1109-2
branch
from
37e70b8 to
c9f8d10
Compare
ma-hill
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 40: Remove the vulnerable containerd dependency pin from the module graph
by updating the dependency set so `github.com/containerd/containerd` is no
longer selected at v1.7.30. Regenerate the module metadata via the normal
dependency management flow, and ensure any reference to
`github.com/containerd/containerd` in go.mod/go.sum is replaced with a
non-vulnerable resolved version outside the affected range. Verify the resulting
dependency graph no longer includes the vulnerable `containerd` module.
In `@pkg/helper/adapter.go`:
- Around line 88-90: The default deployment timeout in adapter helper logic was
reduced too far, causing the main Helm wait path to time out prematurely when
AdapterDeploymentOptions.Timeout is unset. In the timeout fallback inside helper
logic (the code that checks opts.Timeout in adapter.go), restore the previous
deploy budget by setting the default back to 5 minutes instead of 1 minute so
the existing adapter deployment flow remains stable on slower CI and image
pulls.
- Around line 198-206: The run-id label passed through Helm is using an
unescaped dotted key in the commonLabels --set path, so Helm parses it as nested
fields instead of the literal label name. Update the RUN_ID handling in
adapter.go to use the Helm release labeling plus a string-based set flag with an
escaped key path for commonLabels, referencing the existing RUN_ID block in the
adapter logic. Keep the literal e2e.hyperfleet.io/run-id label intact so cleanup
can match resources correctly.
In `@pkg/helper/cleanup.go`:
- Around line 19-155: SweepTestResources currently logs most cleanup errors but
still returns nil, so AfterSuite cannot detect failed teardown. Update
Helper.SweepTestResources and any called cleanup helpers it relies on, such as
uninstallHelmReleases and deleteNamespaces, to collect and return errors instead
of dropping them; make dynamic client init, list/delete failures, and namespace
cleanup failures propagate back to the caller. Keep any intentional
log-and-continue paths explicit with comments, and ensure pkg/e2e/suite.go can
fail the suite when cleanup does not complete successfully.
- Around line 197-205: The patchRemoveFinalizers helper is updating
metadata.finalizers on the main Namespace object, which won’t clear the
terminating spec.finalizers. Update this fallback to use the Namespace
finalization path via h.K8sClient.CoreV1().Namespaces().Finalize (the /finalize
subresource) so the stuck namespace can actually be unblocked.
- Around line 130-140: The cleanup path for AppliedManifestWork deletion is
missing the object namespace, so namespaced resources may not be removed
correctly. Update the deletion call in the applied manifest work loop in
cleanup.go to use the work’s namespace from work.GetNamespace() along with
work.GetName(), keeping the existing list/delete flow in cleanup of appliedWorks
and the dynamicClient.Resource(appliedManifestWorkGVR) call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3e40f1dc-72aa-4205-96e9-652be581535a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (25)
e2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goenv/env.cienv/env.localgo.modpkg/e2e/suite.gopkg/helper/adapter.gopkg/helper/cleanup.gopkg/helper/helm/helm.gotestdata/adapter-configs/cl-crash/adapter-task-config.yamltestdata/adapter-configs/cl-crash/values.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yamltestdata/adapter-configs/cl-m-bad-api/adapter-task-config.yamltestdata/adapter-configs/cl-m-bad-api/values.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/values.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/values.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/values.yamltestdata/adapter-configs/cl-precondition-error/adapter-task-config.yamltestdata/adapter-configs/cl-precondition-error/values.yamltestdata/adapter-configs/cl-stuck/adapter-task-config.yamltestdata/adapter-configs/cl-stuck/values.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
ma-hill
force-pushed
the
HYPERFLEET-1109-2
branch
3 times, most recently
from
41c5d2e to
d5d8720
Compare
ma-hill
force-pushed
the
HYPERFLEET-1109-2
branch
from
d5d8720 to
ca055bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/helper/helm/helm.go (1)
60-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHelm SDK
List.Run()has no context support — 5-minute cleanup budget isn't enforced here.
ListReleasesBySelectordoesn't accept actxat all, andaction.List.Run()inhelm.sh/helm/v3has noRunWithContextvariant (unlikeInstall/Upgrade) — confirmed via Helm SDK docs. Call sitepkg/helper/cleanup.go:66invokes this with no timeout wrapper whatsoever:releases, err := helmClient.ListReleasesBySelector(labelSelector). If the cluster/API server hangs, this blocks indefinitely, consuming the entire 5-minuteAfterSuitecleanup budget (CWE-400: Uncontrolled Resource Consumption) before any deletion logic even starts.This is the same root issue flagged on a prior revision (ctx was ignored there too); it now persists in a different form since the ctx parameter was removed entirely.
🔧 Proposed fix — wrap the blocking Run() call with a context-aware goroutine
-func (c *Client) ListReleasesBySelector(labelSelector string) ([]string, error) { +func (c *Client) ListReleasesBySelector(ctx context.Context, labelSelector string) ([]string, error) { actionConfig, err := c.initActionConfig() if err != nil { return nil, err } listClient := action.NewList(actionConfig) listClient.All = true listClient.AllNamespaces = false listClient.SetStateMask() listClient.Selector = labelSelector - results, err := listClient.Run() - if err != nil { - return nil, fmt.Errorf("failed to list Helm releases: %w", err) - } + type listResult struct { + releases []*release.Release + err error + } + resultCh := make(chan listResult, 1) + go func() { + results, err := listClient.Run() + resultCh <- listResult{results, err} + }() + + var results []*release.Release + select { + case <-ctx.Done(): + return nil, fmt.Errorf("timed out listing Helm releases: %w", ctx.Err()) + case r := <-resultCh: + if r.err != nil { + return nil, fmt.Errorf("failed to list Helm releases: %w", r.err) + } + results = r.releases + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/helper/helm/helm.go` around lines 60 - 90, ListReleasesBySelector is still a blocking Helm list operation with no timeout enforcement, so it can hang cleanup indefinitely. Update the Client.ListReleasesBySelector flow in helm.go to accept a context or otherwise wrap action.List.Run() in a context-aware goroutine/select so the caller can abort after the cleanup budget. Then update the cleanup call path that invokes ListReleasesBySelector so it passes a bounded timeout context instead of calling it bare.pkg/helper/cleanup.go (1)
42-89: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
CleanupResourceshas no return value — teardown failures are invisible toAfterSuite.Every failure path here (
NewCleanupHelperfailure, Helm list/uninstall failures,SweepLabeledResourcesfailure) is logged and the function just returns void.pkg/e2e/suite.go'sAfterSuitecan't distinguish "cleanup succeeded" from "cleanup silently failed," so CI reports green while orphaned namespaces/resources/releases pile up. This is the same defect flagged on a prior revision (then namedSweepTestResources) — it persists across the rename.As per path instructions, "Log-and-continue MUST be intentional degradation with a comment, not a missing return."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/helper/cleanup.go` around lines 42 - 89, CleanupResources currently only logs failures and returns void, so AfterSuite cannot tell whether teardown succeeded or failed. Update CleanupResources to surface an error result instead of swallowing failures, and adjust the caller in AfterSuite to handle that result explicitly. Keep the existing log-and-continue behavior only where it is intentionally degraded, and make sure all failure paths in NewCleanupHelper, helmClient.ListReleasesBySelector, helmClient.UninstallRelease, and cleanupHelper.SweepLabeledResources are reflected in the returned status.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/helper/cleanup.go`:
- Around line 203-243: `countRemainingResources` is swallowing List() failures
and can make `SweepLabeledResources` think cleanup finished when it has not.
Update the resource checks in `countRemainingResources` to use the returned
errors from `c.dynamicClient.Resource(...).List`,
`c.k8sClient.CoreV1().Namespaces().List`, `BatchV1().Jobs().List`,
`AppsV1().Deployments().List`, and `CoreV1().ConfigMaps().List` instead of
ignoring them. Either propagate the error to the caller/poll callback or treat
any list error as “resources still remaining” so `PollUntilContextTimeout` does
not return success on a partial read. Keep the fix localized to
`countRemainingResources` and the `SweepLabeledResources` flow that consumes it.
---
Duplicate comments:
In `@pkg/helper/cleanup.go`:
- Around line 42-89: CleanupResources currently only logs failures and returns
void, so AfterSuite cannot tell whether teardown succeeded or failed. Update
CleanupResources to surface an error result instead of swallowing failures, and
adjust the caller in AfterSuite to handle that result explicitly. Keep the
existing log-and-continue behavior only where it is intentionally degraded, and
make sure all failure paths in NewCleanupHelper,
helmClient.ListReleasesBySelector, helmClient.UninstallRelease, and
cleanupHelper.SweepLabeledResources are reflected in the returned status.
In `@pkg/helper/helm/helm.go`:
- Around line 60-90: ListReleasesBySelector is still a blocking Helm list
operation with no timeout enforcement, so it can hang cleanup indefinitely.
Update the Client.ListReleasesBySelector flow in helm.go to accept a context or
otherwise wrap action.List.Run() in a context-aware goroutine/select so the
caller can abort after the cleanup budget. Then update the cleanup call path
that invokes ListReleasesBySelector so it passes a bounded timeout context
instead of calling it bare.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 39763dfa-0220-492b-9e83-0a9f55251842
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (26)
e2e/adapter/adapter_with_maestro.goenv/env.cienv/env.localgo.modpkg/client/kubernetes/client.gopkg/config/config.gopkg/e2e/suite.gopkg/helper/adapter.gopkg/helper/cleanup.gopkg/helper/helm/helm.gotestdata/adapter-configs/cl-crash/adapter-task-config.yamltestdata/adapter-configs/cl-crash/values.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yamltestdata/adapter-configs/cl-m-bad-api/adapter-task-config.yamltestdata/adapter-configs/cl-m-bad-api/values.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/values.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/values.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/values.yamltestdata/adapter-configs/cl-precondition-error/adapter-task-config.yamltestdata/adapter-configs/cl-precondition-error/values.yamltestdata/adapter-configs/cl-stuck/adapter-task-config.yamltestdata/adapter-configs/cl-stuck/values.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
✅ Files skipped from review due to trivial changes (3)
- testdata/adapter-configs/cl-invalid-resource/values.yaml
- testdata/adapter-configs/cl-m-wrong-ds/values.yaml
- testdata/adapter-configs/cl-precondition-error/values.yaml
🚧 Files skipped from review as they are similar to previous changes (20)
- testdata/adapter-configs/cl-m-bad-api/values.yaml
- pkg/helper/adapter.go
- testdata/adapter-configs/cl-crash/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/values.yaml
- testdata/adapter-configs/cl-stuck/values.yaml
- testdata/adapter-configs/cl-m-bad-api/adapter-task-config.yaml
- pkg/config/config.go
- testdata/adapter-configs/cl-m-wrong-nest/values.yaml
- e2e/adapter/adapter_with_maestro.go
- testdata/adapter-configs/cl-precondition-error/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yaml
- testdata/adapter-configs/cl-invalid-resource/adapter-task-config.yaml
- testdata/adapter-configs/cl-crash/values.yaml
- testdata/adapter-configs/cl-stuck/adapter-task-config.yaml
- env/env.local
- env/env.ci
- pkg/e2e/suite.go
- pkg/client/kubernetes/client.go
ma-hill
force-pushed
the
HYPERFLEET-1109-2
branch
from
ca055bf to
ea1c20c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/helper/cleanup.go (1)
42-89: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
CleanupResourcesswallows every failure —AfterSuitecan never detect a failed teardown.
func CleanupResources()returns nothing. Helm list/uninstall errors (lines 67-81) andSweepLabeledResourcesfailures (lines 84-87) are logged and dropped. Per context snippet,pkg/e2e/suite.gocallshelper.CleanupResources()with no return value checked — orphaned resources can remain while Ginkgo reports a clean suite. As per path instructions, "Error responses MUST be followed by return to prevent double-write" and "Log-and-continue MUST be intentional degradation with a comment, not a missing return."🔧 Proposed fix
-func CleanupResources() { +func CleanupResources() error { cfg := GetSuiteConfig() if cfg == nil { logger.Error("Suite config not initialized") - return + return fmt.Errorf("suite config not initialized") } if cfg.RunID == "" { logger.Error("RUN_ID must be set for resource cleanup tracking") - return + return fmt.Errorf("RUN_ID must be set for resource cleanup tracking") } @@ cleanupHelper, err := NewCleanupHelper(labelSelector) if err != nil { logger.Error("failed to create cleanup helper", "error", err) - return + return err } @@ if err := cleanupHelper.SweepLabeledResources(ctx); err != nil { logger.Error("failed to cleanup test resources", "error", err) - return + return err } logger.Info("test resources cleaned up") + return nil }
pkg/e2e/suite.goshould then check the error and callginkgo.Fail(...)so CI surfaces the incomplete teardown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/helper/cleanup.go` around lines 42 - 89, CleanupResources currently logs helm and sweep failures but never reports them, so AfterSuite cannot fail on teardown problems. Update CleanupResources to return an error and propagate failures from helmClient.ListReleasesBySelector, helmClient.UninstallRelease, and cleanupHelper.SweepLabeledResources instead of swallowing them; keep the early returns for missing config and RUN_ID. Then update the caller in suite teardown to handle the returned error and fail the suite explicitly, using the existing CleanupResources and NewCleanupHelper symbols to locate the flow.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 149: The dependency chain currently pulls in a vulnerable oras-go release
via Helm, so update the Helm/oras dependency versions to a patched release and
refresh go.mod/go.sum accordingly. Locate the indirect oras.land/oras-go/v2
entry and the helm.sh/helm/v3/pkg/action transitive dependency path, then bump
the affected package set to a version that includes the CVE-2026-50163 fix.
In `@pkg/helper/cleanup.go`:
- Around line 177-205: The delete helpers are swallowing per-item Delete()
failures by logging inside the loop and always returning nil, so callers never
know which resource deletions failed. Update deleteAppliedManifestWorksForce and
the related helpers deleteJobs, deleteDeployments, deleteConfigMaps, and
deleteNamespacesForce to collect and return an error when any item delete fails,
while still continuing the loop if that is the intended behavior. Make the error
path explicit in these helper functions so the caller can distinguish a
successful cleanup from a partial one.
- Around line 65-67: Bind the Helm release lookup in cleanup to the existing
cleanup deadline so it cannot run indefinitely. Update the helper around
helmClient.ListReleasesBySelector to use ctx or a timeout-backed path, and
thread that context through the cleanup flow before uninstalling releases. Keep
the change localized to the cleanup helper and the Helm client call site so the
release listing respects the 5-minute budget.
---
Duplicate comments:
In `@pkg/helper/cleanup.go`:
- Around line 42-89: CleanupResources currently logs helm and sweep failures but
never reports them, so AfterSuite cannot fail on teardown problems. Update
CleanupResources to return an error and propagate failures from
helmClient.ListReleasesBySelector, helmClient.UninstallRelease, and
cleanupHelper.SweepLabeledResources instead of swallowing them; keep the early
returns for missing config and RUN_ID. Then update the caller in suite teardown
to handle the returned error and fail the suite explicitly, using the existing
CleanupResources and NewCleanupHelper symbols to locate the flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3ca80cf5-062e-4789-ab65-a05de1f4c56d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (26)
e2e/adapter/adapter_with_maestro.goenv/env.cienv/env.localgo.modpkg/client/kubernetes/client.gopkg/config/config.gopkg/e2e/suite.gopkg/helper/adapter.gopkg/helper/cleanup.gopkg/helper/helm/helm.gotestdata/adapter-configs/cl-crash/adapter-task-config.yamltestdata/adapter-configs/cl-crash/values.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yamltestdata/adapter-configs/cl-m-bad-api/adapter-task-config.yamltestdata/adapter-configs/cl-m-bad-api/values.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/values.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/values.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/values.yamltestdata/adapter-configs/cl-precondition-error/adapter-task-config.yamltestdata/adapter-configs/cl-precondition-error/values.yamltestdata/adapter-configs/cl-stuck/adapter-task-config.yamltestdata/adapter-configs/cl-stuck/values.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
✅ Files skipped from review due to trivial changes (1)
- testdata/adapter-configs/cl-invalid-resource/values.yaml
🚧 Files skipped from review as they are similar to previous changes (23)
- testdata/adapter-configs/cl-m-wrong-ds/values.yaml
- testdata/adapter-configs/cl-crash/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-bad-api/values.yaml
- testdata/adapter-configs/cl-m-wrong-nest/values.yaml
- e2e/adapter/adapter_with_maestro.go
- testdata/adapter-configs/cl-crash/values.yaml
- pkg/config/config.go
- testdata/adapter-configs/cl-stuck/adapter-task-config.yaml
- testdata/adapter-configs/cl-precondition-error/values.yaml
- testdata/adapter-configs/cl-invalid-resource/adapter-task-config.yaml
- pkg/client/kubernetes/client.go
- testdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yaml
- pkg/helper/adapter.go
- testdata/adapter-configs/cl-precondition-error/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/values.yaml
- env/env.ci
- testdata/adapter-configs/cl-stuck/values.yaml
- pkg/e2e/suite.go
- env/env.local
- testdata/adapter-configs/cl-m-bad-api/adapter-task-config.yaml
- pkg/helper/helm/helm.go
| // Use escaped string notation so Helm treats the full label name as a single key | ||
| // instead of parsing e2e.hyperfleet.io as nested fields | ||
| helmArgs = append(helmArgs, | ||
| "--set-string", fmt.Sprintf("commonLabels.e2e\\.hyperfleet\\.io/run-id=%s", h.Cfg.RunID), |
There was a problem hiding this comment.
Warning
Blocking
Category: Bug
DeployAdapter sets the run-id label via --set-string commonLabels.e2e\.hyperfleet\.io/run-id=%s, but the hyperfleet-adapter chart's _helpers.tpl hyperfleet-adapter.labels helper never reads .Values.commonLabels (or .Values.labels) — it only emits helm.sh/chart, selector labels, app.kubernetes.io/version, and app.kubernetes.io/managed-by. So this flag is a no-op on every object the chart renders (Deployment, ConfigMap, ServiceAccount, RBAC). Only the Helm release carries the run-id (via --labels); none of its underlying Kubernetes objects do.
That's fine as long as cleanup only ever goes through Helm uninstall — but if the sweep (or anything else) ever needs to find adapter-chart resources directly by label selector, it won't see them.
Worth either:
- Updating the
hyperfleet-adapterchart to actually consumecommonLabels/labelsinto itslabelshelper, or - Dropping the
--set-string commonLabels...flag here since it currently does nothing, and relying solely on the Helm-release-level--labels+ Helm SDK release listing/uninstall for cleanup — which is the path that actually works today.
// current — the --set-string half of this is a no-op against the chart today:
"--set-string", fmt.Sprintf("commonLabels.e2e\\.hyperfleet\\.io/run-id=%s", h.Cfg.RunID),
There was a problem hiding this comment.
@tirthct , there is actually a way to pass labels and that is via labels: value in values.yml. This is already tested and should label every object deployed by the helm chart (Configmap, ClusterRole, etc...). openshift-hyperfleet/hyperfleet-adapter@91eec5e#diff-2f7bd1822b616f83b06c8d8c356e0fc48c36dde251b8f87ab81d97155ca4cbcb
There was a problem hiding this comment.
Eg.
"--set", fmt.Sprintf("labels.e2e\\.hyperfleet\\.io/run-id=%s", h.RunID),
There was a problem hiding this comment.
You're right, this was a previous implementation. Thank you
| // Validate that RunID is set for resource cleanup tracking | ||
| if !dryRun && c.RunID == "" { | ||
| return fmt.Errorf("configuration validation failed: RunID must be set for resource cleanup tracking (use RUN_ID environment variable or --dry-run to skip)") | ||
| } |
There was a problem hiding this comment.
Should we also validate the kubernetes label value grammar and 63 char limit alongside empty check?
| // Validate that RunID is set for resource cleanup tracking | |
| if !dryRun && c.RunID == "" { | |
| return fmt.Errorf("configuration validation failed: RunID must be set for resource cleanup tracking (use RUN_ID environment variable or --dry-run to skip)") | |
| } | |
| // in Validate(), alongside the existing non-empty check: | |
| if !dryRun && c.RunID != "" { | |
| if len(c.RunID) > 63 || !labelValueRegex.MatchString(c.RunID) { | |
| return fmt.Errorf("RunID %q is not a valid Kubernetes label value", c.RunID) | |
| } | |
| } |
| } | ||
|
|
||
| // SweepLabeledResources iterates through labeled resources and deletes any orphaned resources | ||
| func (c *CleanupHelper) SweepLabeledResources(ctx context.Context) error { |
There was a problem hiding this comment.
Looking into the ticket, AfterSuite is expected to sweep / clean up ClusterRoles/ClusterRoleBindings by Helm instance label, but the only calls here are:
if err := c.deleteJobs(ctx); err != nil { ... }
if err := c.deleteDeployments(ctx); err != nil { ... }
if err := c.deleteConfigMaps(ctx); err != nil { ... }
if err := c.deleteAppliedManifestWorksForce(ctx); err != nil { ... }
if err := c.deleteNamespacesForce(ctx); err != nil { ... }Are we missing this here?
Also, is PubSub deletion logic required here too?
There was a problem hiding this comment.
- Cluster Roles/ClusterRoleBindings should be taken care of by helm uninstall. That happens here: https://github.com/openshift-hyperfleet/hyperfleet-e2e/pull/139/changes#diff-96d02e24eebab940da16f0002c7bf136761b40301172ff2643b47b5207fa3f45R74
- Do you think I should add another check to ensure the ClusterRoles ClusterRoleBindings are cleaned up? Unfortunately if the helm charts fail to uninstall we are left hanging with a bunch of resources. So might be overly redundant? Thoughts?
- Pubsub deletion logic I'm going to take care of here: https://redhat.atlassian.net/browse/HYPERFLEET-1110
@tirthct
There was a problem hiding this comment.
@ma-hill this makes sense. We can keep an eye on this and if we needed, come back to it.
| _, err := cmd.CombinedOutput() | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to uninstall release: %s", err) | ||
| } |
There was a problem hiding this comment.
Should we use the output in error so that we can get more context if CombinedOutput() fails?
| _, err := cmd.CombinedOutput() | |
| if err != nil { | |
| return fmt.Errorf("failed to uninstall release: %s", err) | |
| } | |
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| return fmt.Errorf("failed to uninstall release %s: %w (output: %s)", releaseName, err, output) | |
| } |
| appliedManifestWorkGVR := schema.GroupVersionResource{ | ||
| Group: "work.open-cluster-management.io", | ||
| Version: "v1", | ||
| Resource: "appliedmanifestworks", | ||
| } |
There was a problem hiding this comment.
Since this is being used at 18=79 and 213 too, we should move this to constants.go. Wdyt?
There was a problem hiding this comment.
Well maybe not in constants.go but add it as a constant in the cleanup.go file? Thoughts @tirthct
ma-hill
force-pushed
the
HYPERFLEET-1109-2
branch
2 times, most recently
from
0133170 to
3aad29a
Compare
|
|
||
| // Validate RunID is a valid Kubernetes label value and is not empty for resource cleanup tracking | ||
| labelValueRegex := regexp.MustCompile(`^[a-zA-Z0-9]([-a-zA-Z0-9]*[a-zA-Z0-9])?$`) | ||
| if !dryRun && c.RunID != "" { |
There was a problem hiding this comment.
JIRA | Blocking | High confidence
The PR description states "The RUN_ID environment variable must be set before running tests (enforced in BeforeSuite)", and the JIRA AC requires run-ID-based cleanup for all created resources. However, there is no enforcement anywhere that RunID must be non-empty:
Config.Validate()(line 441) only validates RunID format when it's non-empty:if !dryRun && c.RunID != "" {— an empty RunID passes validation silently.BeforeSuitein suite.go just logs the RunID without checking it.CleanupResources()checks for empty RunID but only logs an error and returns silently — cleanup is skipped.
This means tests can run without RUN_ID set, deploy adapters and create resources without e2e.hyperfleet.io/run-id labels, and the AfterSuite cleanup sweep will silently skip everything. The resources will be orphaned — exactly the problem this PR is trying to solve.
Suggested fix — add a fatal check in BeforeSuite (after cfg.Display()):
if cfg.RunID == "" {
log.Fatalf("RUN_ID must be set for resource cleanup tracking. Source env/env.local or set RUN_ID environment variable.")
}Or make Validate() reject empty RunID in non-dry-run mode:
if !dryRun && c.RunID == "" {
return fmt.Errorf("RunID must be set for resource cleanup tracking. Set RUN_ID environment variable or source env/env.local")
}
|
JIRA | Blocking | Medium confidence Re: The JIRA acceptance criteria for HYPERFLEET-1109 include:
The current implementation sweeps Helm releases, namespaces, jobs, deployments, configmaps, and AppliedManifestWorks — but no PubSub cleanup exists anywhere in the PR. If tests create PubSub topics/subscriptions with the run-id prefix, they will be orphaned after the suite completes. This is the only AC item with no implementation and no mention in the PR notes about being deferred. The companion PR (openshift/release#81351) covers the Prow Suggested fix: Either add GCP PubSub cleanup to |
| } | ||
|
|
||
| // Validate RunID is a valid Kubernetes label value and is not empty for resource cleanup tracking | ||
| labelValueRegex := regexp.MustCompile(`^[a-zA-Z0-9]([-a-zA-Z0-9]*[a-zA-Z0-9])?$`) |
There was a problem hiding this comment.
Pattern | nit | High confidence
regexp.MustCompile is called inside the Validate() function, meaning the regex is recompiled on every validation call. Standard Go practice is to compile static regexes once as a package-level variable.
Suggested fix — move to package level:
| labelValueRegex := regexp.MustCompile(`^[a-zA-Z0-9]([-a-zA-Z0-9]*[a-zA-Z0-9])?$`) | |
| labelValueRegex := regexp.MustCompile(`^[a-zA-Z0-9]([-a-zA-Z0-9]*[a-zA-Z0-9])?$`) |
→
// package level
var labelValueRegex = regexp.MustCompile(`^[a-zA-Z0-9]([-a-zA-Z0-9]*[a-zA-Z0-9])?$`)Then remove the local declaration from Validate() and reference the package-level var directly.
| // Add run-id label for resource tracking and cleanup | ||
| if runID := os.Getenv("RUN_ID"); runID != "" { | ||
| // Label the Helm release itself (for Helm SDK-based cleanup) | ||
| helmArgs = append(helmArgs, |
There was a problem hiding this comment.
For adapters deployed by this, do we somewhere also inject .env vars for container? So that adapter-task-config's env.RUN_ID gets picked up and has some value (I think now they are unset if I see correctly, please prove if I'm wrong).
And, for "--labels", fmt.Sprintf("e2e.hyperfleet.io/run-id=%s", runID),, does it render correctly when you test this? Compared to --set-string", fmt.Sprintf("commonLabels.e2e\\.hyperfleet\\.io/run-id=%s", runID) (btw do we need the commonLabels.e2e if we already set labels above?)
There was a problem hiding this comment.
And, for "--labels", fmt.Sprintf("e2e.hyperfleet.io/run-id=%s", runID),, does it render correctly when you test this? Compared to --set-string", fmt.Sprintf("commonLabels.e2e\\.hyperfleet\\.io/run-id=%s", runID) (btw do we need the commonLabels.e2e if we already set labels above?)
There was a problem hiding this comment.
- I removed commonLabels
- Yes I changed the appending of labels to this -- it is the test config so it is accessible not just as an env variable...
But we only need labels because I am using it to track the helm installation so I can cleanly delete it on uninstall
// Add run-id label for resource tracking and cleanup if h.Cfg.RunID != "" { // Label the Helm release itself (for Helm SDK-based cleanup) helmArgs = append(helmArgs, "--labels", fmt.Sprintf("e2e.hyperfleet.io/run-id=%s", h.Cfg.RunID), ) }
This section here in the adapter-configs values.yaml
Is what is responsible for adding the RUN_ID to the created resources. And does not related to the labels at all.
labels:
e2e.hyperfleet.io/run-id: ${RUN_ID}
env:
- name: RUN_ID
value: ${RUN_ID}
It's a hack they did to pass in env variables to the values.yaml
hyperfleet-e2e/pkg/helper/adapter.go
Line 156 in 56d2d8a
There was a problem hiding this comment.
https://helm.sh/docs/helm/helm_install/#options
I'm adding the label to the helm release, not to the actually resources directly. This is so we can list all the helm releases with that specific tag. Technically helm should properly delete all the associated resources with it's chart, my understanding is we shouldn't be adding additional labels to the helm chart resources since they should be governed by the helm chart itself
| // Use escaped string notation so Helm treats the full label name as a single key | ||
| // instead of parsing e2e.hyperfleet.io as nested fields | ||
| helmArgs = append(helmArgs, | ||
| "--set-string", fmt.Sprintf("commonLabels.e2e\\.hyperfleet\\.io/run-id=%s", h.Cfg.RunID), |
There was a problem hiding this comment.
@tirthct , there is actually a way to pass labels and that is via labels: value in values.yml. This is already tested and should label every object deployed by the helm chart (Configmap, ClusterRole, etc...). openshift-hyperfleet/hyperfleet-adapter@91eec5e#diff-2f7bd1822b616f83b06c8d8c356e0fc48c36dde251b8f87ab81d97155ca4cbcb
| // Use escaped string notation so Helm treats the full label name as a single key | ||
| // instead of parsing e2e.hyperfleet.io as nested fields | ||
| helmArgs = append(helmArgs, | ||
| "--set-string", fmt.Sprintf("commonLabels.e2e\\.hyperfleet\\.io/run-id=%s", h.Cfg.RunID), |
There was a problem hiding this comment.
Eg.
"--set", fmt.Sprintf("labels.e2e\\.hyperfleet\\.io/run-id=%s", h.RunID),
| cmd := exec.CommandContext(cmdCtx, "helm", "uninstall", releaseName, | ||
| "-n", namespace, | ||
| "--wait", | ||
| "--timeout", "5m") |
There was a problem hiding this comment.
By introducing the helm sdk in above code, is it also possible to use it for uninstallation instead of executing command directly? Not sure which one is better but if we already use sdk, IMO would be nice to use that where possible. I found this doc: https://pkg.go.dev/helm.sh/helm/v3/pkg/action#Uninstall
Same for code in adapter.go where we deploy the adapter
There was a problem hiding this comment.
It's not yet until we use the sdk to install. I am working on those changes now. But it seems like when we use the sdk to uninstall, I've been noticing that it misses uninstall some resources. So relying on the current implementation of uninstall now.
@rafabene But for now we should be okay because the https://github.com/openshift-hyperfleet/hyperfleet-e2e/blob/main/scripts/cleanup-pubsub-resources.sh Is running at the end of the tests. The main topics we miss at the individual test level are the dlq topics. But those are getting sweeped at the end of the test. |
ma-hill
force-pushed
the
HYPERFLEET-1109-2
branch
from
e52c876 to
6736c1e
Compare
ma-hill
force-pushed
the
HYPERFLEET-1109-2
branch
from
6736c1e to
9a44e33
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tirthct The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 67258da
into
openshift-hyperfleet:main
7 participants
Summary
Adds an AfterSuite cleanup hook that sweeps orphaned test resources by RUN_ID label. When tests fail or are interrupted, resources like namespaces, adapters, jobs, and deployments can remain in the cluster indefinitely. This new cleanup mechanism runs after all tests complete and removes any resources tagged with the current test run's ID, preventing resource accumulation in CI and local development environments.
HYPERFLEET-1109
Changes
SweepTestResourcesmethod to Helper that deletes all resources with thee2e.hyperfleet.io/run-idlabel, including jobs, deployments, configmaps, AppliedManifestWorks, and Helm releasespkg/helper/helm/helm.goto list by label selector instead of shelling out tohelm listadapterReleaseBEFORE deployment instead of after, ensuring DeferCleanup runs even if deployment failshelm.sh/helm/v3, Google Pub/Sub v2, and related transitive dependencies)registry.ci.openshift.orgtoquay.iofor CI and local environmentsvalues.yamlfiles to adapter config directories to provide default Helm values for test adaptersNotes
The
RUN_IDenvironment variable must be set before running tests (enforced in BeforeSuite). This is set in ci setup configs in the release repo and set to default as NAMESPACE inenv.localNamespaces are deleted LAST to avoid stuck finalizers. If a namespace remains stuck after normal deletion, the cleanup will patch out finalizers and retry.
The helm uninstall for the adapters uses the same os.Exec command
helm uninstall ...as the UninstallAdapters function because there is a limitation to the sdk if it wasn't installed the same wayUpdate IMAGE_REGISTRY and *_IMAGE_REPO to accurately specify images - in env.ci and env.local
In conjunction with this change: openshift/release#81351
Test Plan
make test-allpassesmake lintpasses