DNM: OTE/CCM-AWS: update e2e fetching upstream exports#447
Draft
mtulio wants to merge 3 commits intoopenshift:mainfrom
Draft
DNM: OTE/CCM-AWS: update e2e fetching upstream exports#447mtulio wants to merge 3 commits intoopenshift:mainfrom
mtulio wants to merge 3 commits intoopenshift:mainfrom
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
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:
WalkthroughReplaces per-test AWS SDK client creation with a shared E2ETestHelper for ELBv2/EC2 in e2e tests, removes standalone AWS client helpers and direct SDK imports, updates tests to call helper-based retry/diagnostic APIs, updates go.mod indirect requires, and adds a small TODO comment. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (e2e)
participant E2E as E2ETestHelper
participant AWS as AWS APIs (ELBv2 / EC2)
Test->>E2E: NewE2ETestHelper(ctx, cs) / GetAWSHelper()
Test->>E2E: GetLoadBalancerFromDNSNameWithRetry(dns)
E2E->>AWS: DescribeLoadBalancers / DescribeSecurityGroups (with retries)
AWS-->>E2E: LoadBalancer / SecurityGroup data or NotFound
E2E-->>Test: Result or error (gathers diagnostics on failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-rhcos10-techpreview |
Contributor
|
@mtulio: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a5fc6dd0-37b8-11f1-8d74-297b03763056-0 |
Contributor
|
[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 |
Contributor
Author
|
/test ? |
Contributor
Author
|
/test e2e-aws-ovn |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go (1)
581-590:⚠️ Potential issue | 🔴 CriticalDeletion polling is using the wrong AWS primitive.
GetLoadBalancerFromDNSNameWithRetryonly returns success when the NLB exists. Once the NLB is deleted, the helper incmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_aws.go:447-520keeps retrying until its own timeout and then returns a timeout error, so thestrings.Contains(...)check on Line 585 never sees the normal "not found" case. This cleanup path will time out instead of succeeding as soon as the NLB disappears.Minimal fix
- foundLB, err := e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) + foundLB, err := e2e.GetAWSHelper().GetLoadBalancerFromDNSName(lbDNS) if err != nil { // Check if the error indicates the load balancer was not found (i.e., successfully deleted) if strings.Contains(err.Error(), "no load balancer found with DNS name") { framework.Logf("Load balancer %s has been deleted", lbDNS) return true, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 581 - 590, The polling code uses GetLoadBalancerFromDNSNameWithRetry which itself retries until its timeout and thus never surfaces the immediate "not found" error; replace that call inside the wait.PollUntilContextTimeout loop with a non-retrying existence check (e.g., call a helper that does a single DescribeLoadBalancers call or use GetLoadBalancerFromDNSName without the WithRetry behavior) so the helper returns the not-found condition immediately and the strings.Contains check for "no load balancer found with DNS name" can succeed; update the call site that references GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) in the anonymous func passed to wait.PollUntilContextTimeout to use the single-shot check (or add/emit a distinct error type from the helper) and keep the existing logging/return logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go`:
- Around line 209-210: You are calling
e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry (which retries up to 10m)
inside wait.PollUntilContextTimeout/Eventually, causing single poll iterations
to exceed the poll timeout; replace the long-running retry call with a
single-shot, context-aware lookup that respects the poll context (e.g., use/Get
or add a helper that accepts pollCtx and returns immediately or retries only
within pollCtx), or invoke a single-call method such as
GetLoadBalancerFromDNSName (or create one) inside the PollUntilContextTimeout
callback; apply the same change for the other instances noted around the blocks
at 297-299 and 551-563 so no poll iteration can block past the poll timeout.
In `@cmd/cloud-controller-manager-aws-tests-ext/go.mod`:
- Around line 22-25: The go.mod contains a temporary local/fork replace (replace
k8s.io/cloud-provider-aws/tests/e2e =>
github.com/mtulio/openshift-cloud-provider-aws/tests/e2e v0.0.0-...) and a
commented local-path override; remove those two lines (the active replace and
the commented line) so the module resolves to the upstream
k8s.io/cloud-provider-aws/tests/e2e default, or if this change must remain in
draft only, move the replace into a branch-specific commit with clear scope;
locate the replace directive in
cmd/cloud-controller-manager-aws-tests-ext/go.mod to apply the removal.
---
Outside diff comments:
In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go`:
- Around line 581-590: The polling code uses GetLoadBalancerFromDNSNameWithRetry
which itself retries until its timeout and thus never surfaces the immediate
"not found" error; replace that call inside the wait.PollUntilContextTimeout
loop with a non-retrying existence check (e.g., call a helper that does a single
DescribeLoadBalancers call or use GetLoadBalancerFromDNSName without the
WithRetry behavior) so the helper returns the not-found condition immediately
and the strings.Contains check for "no load balancer found with DNS name" can
succeed; update the call site that references
GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) in the anonymous func
passed to wait.PollUntilContextTimeout to use the single-shot check (or add/emit
a distinct error type from the helper) and keep the existing logging/return
logic.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ba4eefc-1467-47ce-a460-535236c989cb
⛔ Files ignored due to path filters (49)
cmd/cloud-controller-manager-aws-tests-ext/go.sumis excluded by!**/*.sumcmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/CHANGELOG.mdis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/LICENSE.txtis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_client.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_AddTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ApplySecurityGroupsToLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_AttachLoadBalancerToSubnets.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ConfigureHealthCheck.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateAppCookieStickinessPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLBCookieStickinessPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancerListeners.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancerPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancerListeners.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancerPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeregisterInstancesFromLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeAccountLimits.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeInstanceHealth.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerAttributes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerPolicies.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerPolicyTypes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DetachLoadBalancerFromSubnets.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DisableAvailabilityZonesForLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_EnableAvailabilityZonesForLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ModifyLoadBalancerAttributes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_RegisterInstancesWithLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_RemoveTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerListenerSSLCertificate.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerPoliciesForBackendServer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerPoliciesOfListener.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/auth.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/deserializers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/doc.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/endpoints.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/generated.jsonis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/go_module_metadata.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/internal/endpoints/endpoints.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/options.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/serializers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/types/errors.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/types/types.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/validators.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_aws.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_e2e.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/loadbalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/modules.txtis excluded by!**/vendor/**
📒 Files selected for processing (3)
cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.gocmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.gocmd/cloud-controller-manager-aws-tests-ext/go.mod
💤 Files with no reviewable changes (1)
- cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go
Comment on lines
+209
to
+210
| err = wait.PollUntilContextTimeout(ctx, 10*time.Second, 3*time.Minute, true, func(pollCtx context.Context) (bool, error) { | ||
| lb, err := getAWSLoadBalancerFromDNSName(pollCtx, elbClient, lbDNS) | ||
| lb, err := e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) |
There was a problem hiding this comment.
Don't stack the 10-minute AWS retry helper inside shorter polls.
cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_aws.go:447-520 already backs GetLoadBalancerFromDNSNameWithRetry off for up to 10 minutes on h.ctx. Calling it from the 2-5 minute PollUntilContextTimeout/Eventually blocks here means a single poll iteration can run longer than the advertised timeout, so these specs can hang well past their deadlines. Use the single-shot lookup inside the outer poll, or add a context-aware retry helper that honors pollCtx/ctx.
Also applies to: 297-299, 551-563
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines
209 - 210, You are calling
e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry (which retries up to 10m)
inside wait.PollUntilContextTimeout/Eventually, causing single poll iterations
to exceed the poll timeout; replace the long-running retry call with a
single-shot, context-aware lookup that respects the poll context (e.g., use/Get
or add a helper that accepts pollCtx and returns immediately or retries only
within pollCtx), or invoke a single-call method such as
GetLoadBalancerFromDNSName (or create one) inside the PollUntilContextTimeout
callback; apply the same change for the other instances noted around the blocks
at 297-299 and 551-563 so no poll iteration can block past the poll timeout.
Comment on lines
+22
to
+25
| // Temporary validation | ||
| replace k8s.io/cloud-provider-aws/tests/e2e => github.com/mtulio/openshift-cloud-provider-aws/tests/e2e v0.0.0-20260414035611-fc9c0a24c854 | ||
|
|
||
| //replace k8s.io/cloud-provider-aws/tests/e2e => /home/mtulio/openshift/OCPSTRAT-1553/cloud-provider-aws/tests/e2e |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify all active/commented replace directives across repo go.mod files.
fd -a 'go\.mod$' | while read -r mod; do
echo "=== ${mod} ==="
rg -n '^\s*(replace|//replace)\s+.*=>' "$mod" || true
done
echo
echo "Focused check for this module:"
rg -n -C2 'k8s\.io/cloud-provider-aws/tests/e2e' cmd/cloud-controller-manager-aws-tests-ext/go.modRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 1160
Remove personal fork replace and commented line before merge to stable branches.
Line 23 contains an active replace directive to a personal fork (github.com/mtulio/openshift-cloud-provider-aws/tests/e2e), and Line 25 retains a commented local-path override. While marked as "Temporary validation," these must not ship to stable branches because they compromise binary provenance and reproducibility of test artifacts.
Remove Line 23–25 and restore the default upstream module reference, or clearly scope this replace to draft-only branches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cloud-controller-manager-aws-tests-ext/go.mod` around lines 22 - 25, The
go.mod contains a temporary local/fork replace (replace
k8s.io/cloud-provider-aws/tests/e2e =>
github.com/mtulio/openshift-cloud-provider-aws/tests/e2e v0.0.0-...) and a
commented local-path override; remove those two lines (the active replace and
the commented line) so the module resolves to the upstream
k8s.io/cloud-provider-aws/tests/e2e default, or if this change must remain in
draft only, move the replace into a branch-specific commit with clear scope;
locate the replace directive in
cmd/cloud-controller-manager-aws-tests-ext/go.mod to apply the removal.
Contributor
Author
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-rhcos10-techpreview |
Contributor
|
@mtulio: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5cdf1690-37f7-11f1-8ae3-9a714ea47b30-0 |
Contributor
Author
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-installer-dualstack-ipv6-primary-techpreview |
Contributor
|
@mtulio: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6c942f40-383c-11f1-9d1b-490e7b28f724-0 |
mtulio
force-pushed
the
ote-ccm-aws-exports
branch
from
1a9ebc7 to
6566a82
Compare
Contributor
Author
|
/test ? |
Contributor
Author
|
/test e2e-aws-ovn |
Contributor
|
@mtulio: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/64af3200-3848-11f1-82e5-73e5a2b42be9-0 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go`:
- Around line 281-282: The test currently asserts the security-group rule count
increased by comparing originalSGRules and currentSGRules; change this to assert
that rules allowing ports 80 and 443 (HTTP/HTTPS) exist in the current security
group rules regardless of count: after calling getAWSSecurityGroupRules(...) for
the updated load balancer (foundLB.SecurityGroups), scan currentSGRules for
entries matching port/protocol pairs 80/TCP and 443/TCP and fail only if either
is missing; update the same logic used around lines 297-320 to stop relying on
len(originalSGRules) vs len(currentSGRules) and instead check presence of
required ports in currentSGRules (use the getAWSSecurityGroupRules,
originalSGRules, currentSGRules, and foundLB symbols to locate the code).
- Around line 131-137: The test registers cleanup too late (after
getNLBMetaFromName) so if that lookup fails the Service/NLB is orphaned; move
the DeferCleanup registration immediately after createServiceNLB succeeds (i.e.
right after the call that returns serviceName/jig/e2e) and make the cleanup
closure resolve lbDNS lazily by calling getNLBMetaFromName (or otherwise
deriving the current LB DNS from serviceName) inside the DeferCleanup before
calling deleteServiceAndWaitForLoadBalancerDeletion; update the DeferCleanup
usage around createServiceNLB/getNLBMetaFromName (and similarly at the other
occurrences you noted) to capture jig and serviceName but not lbDNS up-front.
- Around line 570-573: The function deleteServiceAndWaitForLoadBalancerDeletion
currently calls e2e.GetContext() which can return a canceled spec context during
DeferCleanup runs; change its signature to add context.Context as the first
parameter and stop calling e2e.GetContext() inside the function, using the
passed ctx for all operations instead, and update all four callers to pass the
appropriate context (the test's spec context for inline cleanup or a fresh
cleanup-scoped context when invoked from DeferCleanup) so cleanup runs with a
live context and does not silently fail.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 18da8f49-b205-469a-a9db-7d8e46017e07
⛔ Files ignored due to path filters (49)
cmd/cloud-controller-manager-aws-tests-ext/go.sumis excluded by!**/*.sumcmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/CHANGELOG.mdis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/LICENSE.txtis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_client.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_AddTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ApplySecurityGroupsToLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_AttachLoadBalancerToSubnets.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ConfigureHealthCheck.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateAppCookieStickinessPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLBCookieStickinessPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancerListeners.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancerPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancerListeners.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancerPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeregisterInstancesFromLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeAccountLimits.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeInstanceHealth.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerAttributes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerPolicies.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerPolicyTypes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DetachLoadBalancerFromSubnets.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DisableAvailabilityZonesForLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_EnableAvailabilityZonesForLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ModifyLoadBalancerAttributes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_RegisterInstancesWithLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_RemoveTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerListenerSSLCertificate.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerPoliciesForBackendServer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerPoliciesOfListener.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/auth.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/deserializers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/doc.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/endpoints.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/generated.jsonis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/go_module_metadata.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/internal/endpoints/endpoints.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/options.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/serializers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/types/errors.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/types/types.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/validators.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_aws.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_e2e.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/loadbalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/modules.txtis excluded by!**/vendor/**
📒 Files selected for processing (4)
cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.gocmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.gocmd/cloud-controller-manager-aws-tests-ext/go.modcmd/cloud-controller-manager-aws-tests-ext/main.go
💤 Files with no reviewable changes (1)
- cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go
✅ Files skipped from review due to trivial changes (1)
- cmd/cloud-controller-manager-aws-tests-ext/main.go
Comment on lines
+281
to
282
| originalSGRules, err := getAWSSecurityGroupRules(ctx, e2e.GetAWSHelper().GetEC2Client(), foundLB.SecurityGroups) | ||
| framework.ExpectNoError(err, "failed to get security group rules") |
There was a problem hiding this comment.
Don't require the security-group rule count to increase.
The behavioral contract here is “ports 80 and 443 are present after the update.” AWS/CCM can replace or consolidate rules without increasing the total rule count, so len(originalSGRules) >= len(currentSGRules) can fail on a correct reconcile and create a false-negative e2e failure.
Also applies to: 297-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines
281 - 282, The test currently asserts the security-group rule count increased by
comparing originalSGRules and currentSGRules; change this to assert that rules
allowing ports 80 and 443 (HTTP/HTTPS) exist in the current security group rules
regardless of count: after calling getAWSSecurityGroupRules(...) for the updated
load balancer (foundLB.SecurityGroups), scan currentSGRules for entries matching
port/protocol pairs 80/TCP and 443/TCP and fail only if either is missing;
update the same logic used around lines 297-320 to stop relying on
len(originalSGRules) vs len(currentSGRules) and instead check presence of
required ports in currentSGRules (use the getAWSSecurityGroupRules,
originalSGRules, currentSGRules, and foundLB symbols to locate the code).
6566a82 to
ad0f223
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go (4)
131-137:⚠️ Potential issue | 🟠 MajorRegister cleanup immediately after
createServiceNLBsucceeds.If
getNLBMetaFromNamefails here, the Service/NLB is left behind because teardown is only scheduled afterward. MoveDeferCleanupto immediately aftercreateServiceNLB, and resolvelbDNSlazily inside the cleanup closure.Also applies to: 266-272, 460-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 131 - 137, Move the DeferCleanup registration to immediately after createServiceNLB returns success so cleanup is always registered even if getNLBMetaFromName later fails; inside that cleanup closure, resolve lbDNS lazily by calling getNLBMetaFromName (or otherwise fetching the current load balancer DNS) and then call deleteServiceAndWaitForLoadBalancerDeletion(e2e, jig, lbDNS), ensuring DeferCleanup is declared before invoking getNLBMetaFromName/Expect checks; apply the same change to the other occurrences around the blocks referenced (the ones at ~266-272 and ~460-466).
135-137:⚠️ Potential issue | 🟠 MajorThread a caller-supplied context through deletion instead of using
context.TODO().This helper no longer respects the spec context for inline deletes, and the cleanup call sites cannot use Ginkgo’s live cleanup context. Accept
ctx context.ContextindeleteServiceAndWaitForLoadBalancerDeletion, pass the specctxfor inline calls, and useDeferCleanup(func(ctx SpecContext) { ... })for cleanup paths.Also applies to: 270-272, 396-397, 464-466, 570-583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 135 - 137, Update deleteServiceAndWaitForLoadBalancerDeletion to accept a caller-supplied context (add ctx context.Context param) and replace any internal uses of context.TODO() with that ctx; update all inline calls to pass the spec context and change cleanup callers to use Ginkgo's live cleanup signature (DeferCleanup(func(ctx SpecContext) { err := deleteServiceAndWaitForLoadBalancerDeletion(ctx, e2e, jig, lbDNS); framework.ExpectNoError(err, "...") })). Apply the same pattern to the other call sites listed (around the other line ranges) so all deletions are context-aware and use the SpecContext provided by DeferCleanup.
317-320:⚠️ Potential issue | 🟠 MajorDon’t require the security-group rule count to increase.
The contract under test is that ports 80 and 443 exist after the update. AWS can replace or consolidate rules without increasing the total count, so this check can fail on a correct reconcile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 317 - 320, Remove the strict length check that compares originalSGRules and currentSGRules (the if block that returns an error when len(originalSGRules) >= len(currentSGRules)); instead, after obtaining currentSGRules, verify that rules allowing ports 80 and 443 exist by scanning currentSGRules for entries that permit those ports (use the same rule representation already in scope) and fail only if either port is missing. Keep references to originalSGRules and currentSGRules for context but do not rely on their counts to determine success.
209-210:⚠️ Potential issue | 🟠 MajorAvoid nesting the 10-minute AWS retry helper inside shorter waits.
These outer waits advertise 2-5 minute deadlines, but a single call to
GetLoadBalancerFromDNSNameWithRetrycan already consume much longer. Use a single-shot lookup inside the outer poll, or add a context-aware helper that honors the passed poll context.Also applies to: 298-299, 551-563
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 209 - 210, The outer wait.PollUntilContextTimeout is using e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) which can block far longer than the outer poll's timeout; replace the long internal retry with a single-shot lookup or a context-aware helper: either call a non-retrying method (e.g., GetLoadBalancerFromDNSName(lbDNS)) inside the PollUntilContextTimeout closure, or add/use a context-aware API (e.g., GetLoadBalancerFromDNSNameWithContext(pollCtx, lbDNS)) that respects the passed poll context; make the same change for the other occurrences (the calls around lines 298-299 and 551-563) so the outer poll controls total wait time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go`:
- Around line 131-137: Move the DeferCleanup registration to immediately after
createServiceNLB returns success so cleanup is always registered even if
getNLBMetaFromName later fails; inside that cleanup closure, resolve lbDNS
lazily by calling getNLBMetaFromName (or otherwise fetching the current load
balancer DNS) and then call deleteServiceAndWaitForLoadBalancerDeletion(e2e,
jig, lbDNS), ensuring DeferCleanup is declared before invoking
getNLBMetaFromName/Expect checks; apply the same change to the other occurrences
around the blocks referenced (the ones at ~266-272 and ~460-466).
- Around line 135-137: Update deleteServiceAndWaitForLoadBalancerDeletion to
accept a caller-supplied context (add ctx context.Context param) and replace any
internal uses of context.TODO() with that ctx; update all inline calls to pass
the spec context and change cleanup callers to use Ginkgo's live cleanup
signature (DeferCleanup(func(ctx SpecContext) { err :=
deleteServiceAndWaitForLoadBalancerDeletion(ctx, e2e, jig, lbDNS);
framework.ExpectNoError(err, "...") })). Apply the same pattern to the other
call sites listed (around the other line ranges) so all deletions are
context-aware and use the SpecContext provided by DeferCleanup.
- Around line 317-320: Remove the strict length check that compares
originalSGRules and currentSGRules (the if block that returns an error when
len(originalSGRules) >= len(currentSGRules)); instead, after obtaining
currentSGRules, verify that rules allowing ports 80 and 443 exist by scanning
currentSGRules for entries that permit those ports (use the same rule
representation already in scope) and fail only if either port is missing. Keep
references to originalSGRules and currentSGRules for context but do not rely on
their counts to determine success.
- Around line 209-210: The outer wait.PollUntilContextTimeout is using
e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute)
which can block far longer than the outer poll's timeout; replace the long
internal retry with a single-shot lookup or a context-aware helper: either call
a non-retrying method (e.g., GetLoadBalancerFromDNSName(lbDNS)) inside the
PollUntilContextTimeout closure, or add/use a context-aware API (e.g.,
GetLoadBalancerFromDNSNameWithContext(pollCtx, lbDNS)) that respects the passed
poll context; make the same change for the other occurrences (the calls around
lines 298-299 and 551-563) so the outer poll controls total wait time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dfe400f-380b-43da-bb69-028ead166fb0
⛔ Files ignored due to path filters (49)
cmd/cloud-controller-manager-aws-tests-ext/go.sumis excluded by!**/*.sumcmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/CHANGELOG.mdis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/LICENSE.txtis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_client.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_AddTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ApplySecurityGroupsToLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_AttachLoadBalancerToSubnets.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ConfigureHealthCheck.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateAppCookieStickinessPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLBCookieStickinessPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancerListeners.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancerPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancerListeners.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancerPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeregisterInstancesFromLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeAccountLimits.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeInstanceHealth.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerAttributes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerPolicies.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerPolicyTypes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DetachLoadBalancerFromSubnets.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DisableAvailabilityZonesForLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_EnableAvailabilityZonesForLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ModifyLoadBalancerAttributes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_RegisterInstancesWithLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_RemoveTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerListenerSSLCertificate.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerPoliciesForBackendServer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerPoliciesOfListener.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/auth.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/deserializers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/doc.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/endpoints.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/generated.jsonis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/go_module_metadata.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/internal/endpoints/endpoints.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/options.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/serializers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/types/errors.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/types/types.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/validators.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_aws.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_e2e.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/loadbalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/modules.txtis excluded by!**/vendor/**
📒 Files selected for processing (4)
cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.gocmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.gocmd/cloud-controller-manager-aws-tests-ext/go.modcmd/cloud-controller-manager-aws-tests-ext/main.go
💤 Files with no reviewable changes (1)
- cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go
✅ Files skipped from review due to trivial changes (1)
- cmd/cloud-controller-manager-aws-tests-ext/main.go
Contributor
Author
|
e2e-aws-ovn #2044165085819047936 fixes the ccm e2e issues, just fine tuning the defer context for cleanup by using a dedicated one to prevent canceling when deleting service. /test e2e-aws-ovn periodics-e2e-aws-ovn-conformance #2044165057260032000 got three issues related to the DNS resolution on private service endpoint on EC2 API. I am reviewing that block in upstream code. |
mtulio
force-pushed
the
ote-ccm-aws-exports
branch
from
ad0f223 to
7abb559
Compare
Contributor
Author
|
A few improvements have been added to the upstream e2e lib to improve AWS and Kube API calls into the hairpining traffic test /test e2e-aws-ovn |
Contributor
|
@mtulio: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4e4ed420-3889-11f1-8a27-fff90506b7d9-0 |
Contributor
Author
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-aws-ovn-upgrade |
Contributor
|
@mtulio: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7484c7d0-3889-11f1-8874-d66a9c5e1ab9-0 |
mtulio
force-pushed
the
ote-ccm-aws-exports
branch
from
7abb559 to
cc1f206
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go (5)
131-137:⚠️ Potential issue | 🟠 MajorRegister cleanup before the NLB metadata lookup.
getNLBMetaFromName(...)is now the long/flaky step. If it errors or times out, this test never installs its teardown hook and can orphan the Service/NLB for later jobs. MoveDeferCleanupto immediately aftercreateServiceNLB(...)succeeds and resolve the current LB DNS lazily during cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 131 - 137, Move the DeferCleanup registration to immediately after createServiceNLB(...) succeeds so teardown is always registered before any long/flaky operations like getNLBMetaFromName(...); inside the cleanup body, lazily resolve the current lbDNS by calling getNLBMetaFromName(ctx, cs, ns, serviceName, e2e) (or otherwise fetch the latest LB metadata) and then call deleteServiceAndWaitForLoadBalancerDeletion(e2e, jig, lbDNS) using that resolved value; update references so the cleanup captures serviceName/jig/context but does not rely on lbDNS being available at test-time.
551-563:⚠️ Potential issue | 🟠 MajorThe outer 5-minute
Eventuallystill isn't enforcing a 5-minute bound.This callback delegates to
GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute), so one attempt can run longer than the wholeEventuallybudget. Keep the retry budget in one place: either letEventuallyown the retries and call the single-shot lookup here, or make the helper honor theEventuallycontext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 551 - 563, The Eventually call currently loses its timeout because the callback calls e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) which can block longer than the 5-minute Eventually window; change this so the retry budget is single-sourced: either call a single-shot lookup (e.g. a GetLoadBalancerFromDNSName or similar one-attempt method) inside the Eventually callback and let Eventually handle retries, or modify GetLoadBalancerFromDNSNameWithRetry to accept and respect the provided context (propagate the ctx from the Eventually callback) so it cannot exceed Eventually's timeout; update the call site in loadbalancer.go to use the single-shot helper or pass ctx into the helper accordingly and remove the hardcoded 10*time.Minute retry there.
297-320:⚠️ Potential issue | 🟠 MajorDon't require the security-group rule count to increase.
AWS can replace or consolidate rules during reconcile.
len(originalSGRules) >= len(currentSGRules)can fail on a correct update, which makes this e2e flaky for the wrong reason. Assert only that the current rules allow the required80/TCPand443/TCPports.Suggested change
- if len(originalSGRules) >= len(currentSGRules) { - framework.Logf("Security group rules count did not changed: original=%d current=%d", - len(originalSGRules), len(currentSGRules)) - return nil, fmt.Errorf("security group rules count did not changed") - } - // We want the security group have the rules for both ports 80 and 443. requiredPorts := map[int32]bool{ } for _, rule := range currentSGRules { - if rule.ToPort != nil { - requiredPorts[*rule.ToPort] = true - } + if rule.FromPort == nil || rule.ToPort == nil || rule.IpProtocol == nil { + continue + } + if *rule.IpProtocol != "tcp" || *rule.FromPort != *rule.ToPort { + continue + } + if _, ok := requiredPorts[*rule.ToPort]; ok { + requiredPorts[*rule.ToPort] = true + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 297 - 320, The test currently fails if len(originalSGRules) >= len(currentSGRules); instead of asserting an increased rule count, replace that length check in the Eventually block (where originalSGRules and currentSGRules are computed via getAWSSecurityGroupRules using foundLB.SecurityGroups) with assertions that scan currentSGRules to confirm there exists at least one rule that allows TCP port 80 and at least one that allows TCP port 443 (e.g., check rule.Protocol == "tcp"/"6" and that the FromPort/ToPort or PortRange covers 80 and 443 respectively and the rule's IpRanges/SourceSecurityGroup allows the expected sources); remove the len(...) comparison so the test only validates that required ports are permitted, not that rule count increased.
209-210:⚠️ Potential issue | 🟠 MajorDon't put the 10-minute AWS retry helper inside this 3-minute poll.
A single
PollUntilContextTimeoutiteration can block insideGetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute), so this spec can run well past its advertised timeout. Use the single-shot lookup in the poll body, or add a helper that retries only withinpollCtx. The same pattern shows up again in the service-update wait path.#!/bin/bash set -euo pipefail rg -n -C3 'GetLoadBalancerFromDNSNameWithRetry' \ cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go rg -n -C8 'func \(h \*E2ETestHelperAWS\) GetLoadBalancerFromDNSNameWithRetry' \ cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_aws.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 209 - 210, The poll body currently calls GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) which can block longer than the outer wait.PollUntilContextTimeout; replace that long internal retry with a single-shot lookup that respects the poll context (e.g., call the single-call helper GetLoadBalancerFromDNSName or add a variant that accepts pollCtx) so each PollUntilContextTimeout iteration returns promptly, and apply the same change to the service-update wait path where GetLoadBalancerFromDNSNameWithRetry is used; ensure the helper you call uses pollCtx for any retries rather than its own 10-minute timeout.
570-583:⚠️ Potential issue | 🔴 Critical
context.TODO()doesn't fix cleanup here because the AWS helper still uses the spec context.
deleteServiceAndWaitForLoadBalancerDeletion(...)now has a fresh localctx, bute2e.GetAWSHelper().GetLoadBalancerFromDNSName(...)still runs against the helper's embeddedh.ctx. Since everye2ehelper in this file is created from the specctx,DeferCleanupcan still execute these AWS lookups on a canceled context and skip the deletion wait path. Thread a live cleanup context into the AWS lookup as well instead of reusing the spec-scoped helper.#!/bin/bash set -euo pipefail # Verify the helper is created from the spec context and reused in cleanup. rg -n -C3 'NewE2ETestHelper\(ctx, cs\)|DeferCleanup|deleteServiceAndWaitForLoadBalancerDeletion\(' \ cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go # Verify the AWS helper methods are bound to h.ctx internally. rg -n -C6 'func \(h \*E2ETestHelperAWS\) GetLoadBalancerFromDNSName\(|func \(h \*E2ETestHelperAWS\) GetLoadBalancerFromDNSNameWithRetry\(' \ cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_aws.go rg -n -C2 'NextPage\(h\.ctx\)|WithTimeout\(h\.ctx' \ cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_aws.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines 570 - 583, The AWS lookup is using the helper's internal h.ctx (spec-scoped) so cleanup can race with a canceled spec context; in deleteServiceAndWaitForLoadBalancerDeletion you must thread the live ctx into the AWS call instead of relying on the helper's h.ctx. Update the call site in deleteServiceAndWaitForLoadBalancerDeletion to call a context-taking variant on the AWS helper (e.g. add or use E2ETestHelperAWS.GetLoadBalancerFromDNSNameWithContext(ctx context.Context, dns string) or a GetLoadBalancerFromDNSNameWithRetry that accepts ctx) and pass the local ctx, and if that helper method doesn't exist add it on E2ETestHelperAWS to use the provided ctx rather than h.ctx; ensure other cleanup calls that use e2e.GetAWSHelper().GetLoadBalancerFromDNSName... are similarly updated to use the context-taking variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go`:
- Around line 131-137: Move the DeferCleanup registration to immediately after
createServiceNLB(...) succeeds so teardown is always registered before any
long/flaky operations like getNLBMetaFromName(...); inside the cleanup body,
lazily resolve the current lbDNS by calling getNLBMetaFromName(ctx, cs, ns,
serviceName, e2e) (or otherwise fetch the latest LB metadata) and then call
deleteServiceAndWaitForLoadBalancerDeletion(e2e, jig, lbDNS) using that resolved
value; update references so the cleanup captures serviceName/jig/context but
does not rely on lbDNS being available at test-time.
- Around line 551-563: The Eventually call currently loses its timeout because
the callback calls e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS,
10*time.Minute) which can block longer than the 5-minute Eventually window;
change this so the retry budget is single-sourced: either call a single-shot
lookup (e.g. a GetLoadBalancerFromDNSName or similar one-attempt method) inside
the Eventually callback and let Eventually handle retries, or modify
GetLoadBalancerFromDNSNameWithRetry to accept and respect the provided context
(propagate the ctx from the Eventually callback) so it cannot exceed
Eventually's timeout; update the call site in loadbalancer.go to use the
single-shot helper or pass ctx into the helper accordingly and remove the
hardcoded 10*time.Minute retry there.
- Around line 297-320: The test currently fails if len(originalSGRules) >=
len(currentSGRules); instead of asserting an increased rule count, replace that
length check in the Eventually block (where originalSGRules and currentSGRules
are computed via getAWSSecurityGroupRules using foundLB.SecurityGroups) with
assertions that scan currentSGRules to confirm there exists at least one rule
that allows TCP port 80 and at least one that allows TCP port 443 (e.g., check
rule.Protocol == "tcp"/"6" and that the FromPort/ToPort or PortRange covers 80
and 443 respectively and the rule's IpRanges/SourceSecurityGroup allows the
expected sources); remove the len(...) comparison so the test only validates
that required ports are permitted, not that rule count increased.
- Around line 209-210: The poll body currently calls
GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) which can block
longer than the outer wait.PollUntilContextTimeout; replace that long internal
retry with a single-shot lookup that respects the poll context (e.g., call the
single-call helper GetLoadBalancerFromDNSName or add a variant that accepts
pollCtx) so each PollUntilContextTimeout iteration returns promptly, and apply
the same change to the service-update wait path where
GetLoadBalancerFromDNSNameWithRetry is used; ensure the helper you call uses
pollCtx for any retries rather than its own 10-minute timeout.
- Around line 570-583: The AWS lookup is using the helper's internal h.ctx
(spec-scoped) so cleanup can race with a canceled spec context; in
deleteServiceAndWaitForLoadBalancerDeletion you must thread the live ctx into
the AWS call instead of relying on the helper's h.ctx. Update the call site in
deleteServiceAndWaitForLoadBalancerDeletion to call a context-taking variant on
the AWS helper (e.g. add or use
E2ETestHelperAWS.GetLoadBalancerFromDNSNameWithContext(ctx context.Context, dns
string) or a GetLoadBalancerFromDNSNameWithRetry that accepts ctx) and pass the
local ctx, and if that helper method doesn't exist add it on E2ETestHelperAWS to
use the provided ctx rather than h.ctx; ensure other cleanup calls that use
e2e.GetAWSHelper().GetLoadBalancerFromDNSName... are similarly updated to use
the context-taking variant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05a11e55-d8ee-4ea2-b6fe-2bae63d9a956
⛔ Files ignored due to path filters (49)
cmd/cloud-controller-manager-aws-tests-ext/go.sumis excluded by!**/*.sumcmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/CHANGELOG.mdis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/LICENSE.txtis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_client.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_AddTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ApplySecurityGroupsToLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_AttachLoadBalancerToSubnets.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ConfigureHealthCheck.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateAppCookieStickinessPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLBCookieStickinessPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancerListeners.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_CreateLoadBalancerPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancerListeners.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeleteLoadBalancerPolicy.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DeregisterInstancesFromLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeAccountLimits.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeInstanceHealth.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerAttributes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerPolicies.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancerPolicyTypes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeLoadBalancers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DescribeTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DetachLoadBalancerFromSubnets.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_DisableAvailabilityZonesForLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_EnableAvailabilityZonesForLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_ModifyLoadBalancerAttributes.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_RegisterInstancesWithLoadBalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_RemoveTags.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerListenerSSLCertificate.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerPoliciesForBackendServer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/api_op_SetLoadBalancerPoliciesOfListener.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/auth.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/deserializers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/doc.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/endpoints.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/generated.jsonis excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/go_module_metadata.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/internal/endpoints/endpoints.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/options.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/serializers.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/types/errors.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/types/types.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing/validators.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_aws.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/helper_e2e.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/k8s.io/cloud-provider-aws/tests/e2e/loadbalancer.gois excluded by!**/vendor/**cmd/cloud-controller-manager-aws-tests-ext/vendor/modules.txtis excluded by!**/vendor/**
📒 Files selected for processing (4)
cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.gocmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.gocmd/cloud-controller-manager-aws-tests-ext/go.modcmd/cloud-controller-manager-aws-tests-ext/main.go
💤 Files with no reviewable changes (1)
- cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go
✅ Files skipped from review due to trivial changes (1)
- cmd/cloud-controller-manager-aws-tests-ext/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cloud-controller-manager-aws-tests-ext/go.mod
Contributor
Author
|
/test e2e-aws-ovn |
Contributor
|
@mtulio: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f92e00e0-388a-11f1-8f7f-3ebcb5db0786-0 |
mtulio
force-pushed
the
ote-ccm-aws-exports
branch
from
cc1f206 to
9a5c877
Compare
Contributor
Author
|
Fixed shadowing variable in the ccm e2e lib: /test e2e-aws-ovn |
Contributor
Author
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-installer-dualstack-ipv6-primary-techpreview |
Contributor
|
@mtulio: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/951f2fe0-38d3-11f1-89a0-5ab176676e8f-0 |
Contributor
Author
|
/testwith openshift/installer/main/e2e-aws-eusc-techpreview openshift/origin#30994 openshift/installer#10453 |
Member
|
/testwith openshift/installer/main/e2e-aws-eusc-techpreview openshift/origin#30994 openshift/installer#10453 |
Member
|
/tide refresh |
Member
|
/testwith openshift/installer/main/e2e-aws-eusc-techpreview openshift/origin#30994 openshift/installer#10453 |
mfbonfigli
reviewed
Apr 16, 2026
Comment on lines
+22
to
+26
| // Use development branch @mtulio e2e-lb-export | ||
| replace k8s.io/cloud-provider-aws/tests/e2e => github.com/mtulio/openshift-cloud-provider-aws/tests/e2e v0.0.0-20260415131057-8fd9d13a9d84 | ||
|
|
||
| //replace k8s.io/cloud-provider-aws/tests/e2e => /home/mtulio/openshift/OCPSTRAT-1553/cloud-provider-aws/tests/e2e | ||
|
|
There was a problem hiding this comment.
I guess that before merging this one we should wait for the upstream PR to be merged and then remove this replace statement, right?
Contributor
Author
There was a problem hiding this comment.
Correct, this is to quickly validate the change in OCP grid, flow will be: upstream PR -> update to downstream version
Comment on lines
+300
to
+298
| foundLB, err = getAWSLoadBalancerFromDNSName(ctx, elbClient, lbDNS) | ||
| foundLB, err = e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) |
There was a problem hiding this comment.
nit: not really a problem but we have a call with retries inside an Eventually block, which already does retries. Not needed but we could consider moving e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) outside of the Eventually block
Comment on lines
+551
to
+563
| // Use Eventually to handle AWS API eventual consistency - the Kubernetes service | ||
| // may show the LB DNS before AWS API has fully propagated the resource. | ||
| // Based on observed CI failures, this can take 30s-2min in high-load environments. | ||
| var foundLB *elbv2types.LoadBalancer | ||
| Eventually(ctx, func(ctx context.Context) error { | ||
| lb, err := e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS, 10*time.Minute) | ||
| if err != nil { | ||
| e2e.GatherServiceInfo() | ||
| return fmt.Errorf("failed to find load balancer with DNS name %s: %v", lbDNS, err) | ||
| } | ||
| foundLB = lb | ||
| return nil | ||
| }, 5*time.Minute, 5*time.Second).Should(Succeed(), "failed to find load balancer with DNS name %s", lbDNS) |
There was a problem hiding this comment.
do we need the Eventually? Before it was using getAWSLoadBalancerFromDNSName which did not do retries, but now it is using GetLoadBalancerFromDNSNameWithRetry which already handles retries internally, so not sure what value does Eventually add here
Contributor
Author
There was a problem hiding this comment.
Good point, I think we need Eventually for the other operations we are performing on API (security group rules). I will return to the old method without retries and rely retry to Eventually in this test. wdyt?
mfbonfigli
reviewed
Apr 16, 2026
Comment on lines
-272
to
273
| DeferCleanup(func(cleanupCtx context.Context) { | ||
| err := deleteServiceAndWaitForLoadBalancerDeletion(cleanupCtx, jig, lbDNS) | ||
| DeferCleanup(func() { | ||
| err := deleteServiceAndWaitForLoadBalancerDeletion(e2e, jig, lbDNS) | ||
| framework.ExpectNoError(err, "failed to delete service and wait for load balancer deletion") | ||
| }) |
There was a problem hiding this comment.
I think we will have an issue here if we pass the existing e2e because the e2e will use the original test context which might be invalid/canceled eg by the test framework. We should either:
- create a new
e2eobject using the cleanupCtx and pass that down
or, better
- pass the cleanup context to deleteServiceAndWaitForLoadBalancerDeletion and change the upstream so that the context is passed at every operation and not stored in e2e.
From K8s blog on testing https://www.kubernetes.dev/blog/2023/04/12/e2e-testing-best-practices-reloaded/:
Don’t use the ctx passed into ginkgo.It in a ginkgo.DeferCleanup callback because the context will be canceled when the cleanup code runs.
[...]
Instead, register a function which accepts a new context
Contributor
Author
There was a problem hiding this comment.
Good suggestion Federico, that's totally make sense as we are refactoring the e2e library to standardize the utilization of the retries preventing duplicated function retries.
The last update I've reviewed, still validating the overall context as well checking if the cleanup will succeed
mtulio
force-pushed
the
ote-ccm-aws-exports
branch
from
9a5c877 to
b5e6577
Compare
Export e2e and aws utils used in the test cases to be expanded in broadly on kubernetes ecossytems, so that we can increase test coverage on differnt scenarios than supported on upstream CI grid reusing and continuous improving kubernetes test library,
mtulio
force-pushed
the
ote-ccm-aws-exports
branch
from
b5e6577 to
60a8748
Compare
Contributor
Author
|
/test e2e-aws-ovn |
Contributor
|
@mtulio: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DNM/spike
Refact upstream e2e lib to export common functions used on OTE, as well increasing verbosity and resilience while ivnestigating test failures - fetching more information such as Service status, events, etc.
Depends on kubernetes/cloud-provider-aws#1381
Summary by CodeRabbit
Tests
Refactor
Chores