HYPERFLEET-1488 - bug: Change namespace and configmap name for negative adapters - #156
HYPERFLEET-1488 - bug: Change namespace and configmap name for negative adapters#156ma-hill wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds the Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
3a88970 to
bfac8ba
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@e2e/adapter/adapter_with_maestro.go`:
- Around line 487-495: Correct the condition around
FindAllResourceBundlesByClusterID so lookup errors are logged and the cleanup
description plus deletion loop execute only when the lookup succeeds. Preserve
the existing per-resource deletion warnings while moving the ginkgo.By call and
iteration over rbs into the successful branch.
In `@pkg/helper/cleanup.go`:
- Around line 166-175: Update the Maestro cleanup flow around
FindResourceBundlesByRunId and DeleteResourceBundle to collect lookup and
deletion errors instead of only logging them. After Kubernetes cleanup and
polling complete, return or propagate the aggregated Maestro failures so
SweepLabeledResources cannot report success while ResourceBundles remain;
preserve best-effort behavior only with an explicit comment and result signal.
In `@testdata/adapter-configs/cl-m-bad-api/adapter-task-config.yaml`:
- Line 97: The nested discovery configurations in
testdata/adapter-configs/cl-m-bad-api/adapter-task-config.yaml:97-97 and
testdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yaml:121-121
use the same name for different resource kinds, allowing DiscoverNestedManifest
to select the wrong object. Update both configurations to use distinct resource
names, or make identity matching kind-aware in both configurations, while
preserving the intended Namespace and ConfigMap discovery behavior.
🪄 Autofix
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: 16d0fe9f-a6ee-427e-a78a-f863af3f3b3b
📒 Files selected for processing (14)
e2e/adapter/adapter_with_maestro.goe2e/cluster/adapter_failure.goe2e/cluster/crash_recovery.goe2e/cluster/stuck_deletion.gopkg/client/maestro/client.gopkg/helper/cleanup.gopkg/labels/labels.gotestdata/adapter-configs/cl-crash/adapter-task-config.yamltestdata/adapter-configs/cl-m-bad-api/adapter-task-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-param-error/adapter-task-config.yamltestdata/adapter-configs/cl-stuck/adapter-task-config.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)
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 62-64: Update the labels override handling near
countRemainingResources so an options value with an empty selector does not
replace the default run-ID LabelSelector; retain the existing selector in that
case, or explicitly reject the empty-selector input before resource listing.
🪄 Autofix
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: 09f983c2-9483-4b0d-a699-a3c948aa2056
📒 Files selected for processing (14)
e2e/adapter/adapter_with_maestro.goe2e/cluster/adapter_failure.goe2e/cluster/crash_recovery.goe2e/cluster/stuck_deletion.gopkg/client/maestro/client.gopkg/helper/cleanup.gopkg/labels/labels.gotestdata/adapter-configs/cl-crash/adapter-task-config.yamltestdata/adapter-configs/cl-m-bad-api/adapter-task-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-param-error/adapter-task-config.yamltestdata/adapter-configs/cl-stuck/adapter-task-config.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 as they are similar to previous changes (13)
- testdata/adapter-configs/cl-crash/adapter-task-config.yaml
- pkg/labels/labels.go
- testdata/adapter-configs/cl-stuck/adapter-task-config.yaml
- e2e/cluster/stuck_deletion.go
- e2e/cluster/adapter_failure.go
- testdata/adapter-configs/cl-param-error/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-bad-api/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yaml
- pkg/client/maestro/client.go
- e2e/adapter/adapter_with_maestro.go
- e2e/cluster/crash_recovery.go
- testdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yaml
| if len(labels) > 0 { | ||
| labelSelectorListOptions = labels[0] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the run-ID selector when an override has no selector.
A caller can pass metav1.ListOptions{}. This replaces the default selector with an empty selector.
countRemainingResources then lists cluster-wide resources. Unrelated resources can keep Phase 3 polling until timeout. This is CWE-20.
Keep the default LabelSelector when the supplied options have an empty selector, or reject that input.
Proposed fix
if len(labels) > 0 {
labelSelectorListOptions = labels[0]
+ if strings.TrimSpace(labelSelectorListOptions.LabelSelector) == "" {
+ labelSelectorListOptions.LabelSelector = labelSelector
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(labels) > 0 { | |
| labelSelectorListOptions = labels[0] | |
| } | |
| if len(labels) > 0 { | |
| labelSelectorListOptions = labels[0] | |
| if strings.TrimSpace(labelSelectorListOptions.LabelSelector) == "" { | |
| labelSelectorListOptions.LabelSelector = labelSelector | |
| } | |
| } |
🤖 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 62 - 64, Update the labels override
handling near countRemainingResources so an options value with an empty selector
does not replace the default run-ID LabelSelector; retain the existing selector
in that case, or explicitly reject the empty-selector input before resource
listing.
Source: Path instructions
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Would deleting the resourceBundle via the API remove amw as well?
If so, should we remove countRemainingResources since they will be managed by the cascading?
There was a problem hiding this comment.
Yes good catch, let me cleanup
| bundles = append(bundles, result.Items[i]) | ||
| } | ||
| } | ||
| // FindResourceBundlesByAdapterName finds all resource bundles created by a specific adapter |
There was a problem hiding this comment.
nit:
| // FindResourceBundlesByAdapterName finds all resource bundles created by a specific adapter | |
| // FindResourceBundlesByRunId finds all resource bundles created during a specific test run | |
| // Uses the e2e.hyperfleet.io/run-id label to filter by test runID |
| } | ||
| // FindResourceBundlesByAdapterName finds all resource bundles created by a specific adapter | ||
| // Uses the e2e.hyperfleet.io/run-id label to filter by test runID | ||
| func (c *Client) FindResourceBundlesByRunId(ctx context.Context, runID string) ([]ResourceBundle, error) { |
There was a problem hiding this comment.
nit
| func (c *Client) FindResourceBundlesByRunId(ctx context.Context, runID string) ([]ResourceBundle, error) { | |
| func (c *Client) FindResourceBundlesByRunID(ctx context.Context, runID string) ([]ResourceBundle, error) { |
| logger.Info("Phase 1: Removing finalizers from AppliedManifestWorks") | ||
| c.removeAppliedManifestWorkFinalizers(ctx) | ||
| // Phase 1: Delete ResourceBundles Created by Run ID | ||
| logger.Info("Phase 1: Best effore delete Resource Bundles by Run ID") |
There was a problem hiding this comment.
| logger.Info("Phase 1: Best effore delete Resource Bundles by Run ID") | |
| logger.Info("Phase 1: Best effort delete Resource Bundles by Run ID") |
| } | ||
|
|
||
| // Best effort delete outstanding resource bundles created on cluster creation | ||
| if rbs, err := h.MaestroClient.FindAllResourceBundlesByClusterID(ctx, clusterID); err == nil { |
There was a problem hiding this comment.
Should we log the error if it's not null?
| // FindResourceBundlesByAdapterName finds all resource bundles created by a specific adapter | ||
| // Uses the maestro.io/source-id label to filter by adapter name | ||
| func (c *Client) FindResourceBundlesByAdapterName(ctx context.Context, adapterName string) ([]ResourceBundle, error) { | ||
| func (c *Client) FindResourcesByLabel(ctx context.Context, labelQuery string) ([]ResourceBundle, error) { |
There was a problem hiding this comment.
Since every method in this file has comments:
| func (c *Client) FindResourcesByLabel(ctx context.Context, labelQuery string) ([]ResourceBundle, error) { | |
| // FindResourcesByLabel finds all resource bundles matching a Maestro JSONB label search expression. | |
| func (c *Client) FindResourcesByLabel(ctx context.Context, labelQuery string) ([]ResourceBundle, error) { |
There was a problem hiding this comment.
Actually cleaned up these functions even more, will verify
| ) | ||
|
|
||
| const ( | ||
| Adapter = "adapter" |
There was a problem hiding this comment.
Since this file organizes constants by dimensions, wdyt on moving this under https://github.com/openshift-hyperfleet/hyperfleet-e2e/pull/156/changes#diff-9ce3de8edad5f1fa0dfb92e0721b51a5d9b94a458e7226da8c9f18601214d888R26?
| // Trust the search query worked as expected | ||
| return result.Items, nil | ||
| } | ||
|
|
||
| // Filter and return all matching resource bundles | ||
| var bundles []ResourceBundle | ||
| for i := range result.Items { | ||
| if result.Items[i].Metadata.Labels != nil && | ||
| result.Items[i].Metadata.Labels["maestro.io/source-id"] == adapterName { | ||
| bundles = append(bundles, result.Items[i]) | ||
| } | ||
| } | ||
| // FindResourceBundlesByAdapterName finds all resource bundles created by a specific adapter | ||
| // Uses the e2e.hyperfleet.io/run-id label to filter by test runID | ||
| func (c *Client) FindResourceBundlesByRunId(ctx context.Context, runID string) ([]ResourceBundle, error) { | ||
| return c.FindResourcesByLabel(ctx, toJSONBLabelSearch("e2e.hyperfleet.io/run-id", runID)) | ||
| } | ||
|
|
||
| return bundles, nil | ||
| // FindResourceBundlesByAdapterName finds all resource bundles created by a specific adapter | ||
| // Uses the maestro.io/source-id label to filter by adapter name | ||
| func (c *Client) FindResourceBundlesByAdapterName(ctx context.Context, adapterName string) ([]ResourceBundle, error) { | ||
| return c.FindResourcesByLabel(ctx, toJSONBLabelSearch("maestro.io/source-id", adapterName)) | ||
| } |
There was a problem hiding this comment.
The refactor makes FindResourcesByLabel trust Maestro search results, but FindAllResourceBundlesByClusterID still client-filters by label in the same file. Worth picking one approach and applying it consistently, either keep the post-search label check in the RunId/AdapterName helpers or remove it from the cluster-ID path too.
3 participants
Summary
Update negative adapter task configs to create resources without the clusterId prefix.
Replaced naming convention to: <adapter_name>- this is to avoid collisions with base testing resources that get created with the clusterID prefix. SInce our e2e suites are now running in parallel we hit a failure here in the tier1 nightly:
Add per-test cleanup for negative adapter tests where the resources created don't get properly cleaned up when the cluster gets deleted (e.g. Namespaces, Configmaps, AppliedManifestWorks) which are only getting swept up on end of test cleanup. Added both during test run and post test suite cleanup. Refactored cleanup logic: extracted
FindResourcesByLabelas a generic helper, addedFindResourceBundlesByRunIdfor run-ID based cleanup. UpdatedCleanupHelperto delete ResourceBundles by run ID in Phase 1 of cleanup, removing the dependency on finalizer manipulationhttps://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-hyperfleet-hyperfleet-e2e-main-e2e-tier1-nightly/2084965232177844224/build-log.txt
Jira
HF-1488
Test Plan
make test-allpassesmake lint