OCPBUGS-86299: skip upstream LB tests on dual-stack and patch NLB service IPFamilyPolicy#466
OCPBUGS-86299: skip upstream LB tests on dual-stack and patch NLB service IPFamilyPolicy#466mtulio wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@mtulio: This pull request references Jira Issue OCPBUGS-86299, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
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:
WalkthroughAdds shared CCM cloud-config helpers and kube/OpenShift client builders, detects dual-stack at test startup to conditionally filter specs, and refactors AWS NLB tests to use helpers and set Service.IPFamilyPolicy for dual-stack clusters. ChangesNLB test infrastructure refactoring for cloud-config and dual-stack support
Sequence DiagramsequenceDiagram
participant main as main (test runner)
participant common as openshift-tests/ccm-aws-tests/e2e/common
participant kube as Kubernetes API (ConfigMap/Service)
participant tests as NLB test code
main->>common: GetCloudConfig(ctx)
common->>kube: GET ConfigMap openshift-cloud-controller-manager/cloud-conf
kube-->>common: ConfigMap
common->>common: IsDualStack(cm)
common-->>main: isDualStack bool
main->>tests: build spec selectors (exclude upstream loadbalancer when isDualStack)
tests->>common: GetCloudConfig(ctx) / IsNLBSecurityGroupModeManaged(cm)
tests->>common: IsDualStack(cm)
tests->>kube: Services(...).Create(...) with IPFamilyPolicy=RequireDualStack (if dual-stack)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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 |
|
@mtulio: This pull request references Jira Issue OCPBUGS-86299, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
mtulio
changed the title
|
@mtulio: This pull request references Jira Issue OCPBUGS-86299, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-installer-dualstack-ipv6-primary-techpreview |
|
@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/dfe82140-5551-11f1-976e-fc5230192bd3-0 |
![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
🧹 Nitpick comments (1)
openshift-tests/ccm-aws-tests/main.go (1)
50-65: 💤 Low valueConsider using a context with timeout for dual-stack detection.
The detection uses
context.TODO()which has no timeout. If the cluster is unreachable or slow, this could block indefinitely during test initialization. A bounded context would make startup behavior more predictable.🤖 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 `@openshift-tests/ccm-aws-tests/main.go` around lines 50 - 65, Replace the unbounded context.TODO() used in the dual-stack detection call chain with a bounded context: create a context with timeout (e.g. context.WithTimeout(context.Background(), 30*time.Second)), defer its cancel, and pass that context into common.GetCloudConfig instead of context.TODO(); ensure error handling/log messages for the LoadConfig -> kclientset.NewForConfig -> common.GetCloudConfig -> common.IsDualStack sequence still log the error (including timeout) and that isDualStackCluster and dualStackDetectionReady are only set when detection completes successfully, and keep the existing log.Debugf("Dual-stack cluster detected: %v", isDualStackCluster) after successful detection.
🤖 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 `@docs/dev/e2e-ote-ccm-aws.md`:
- Around line 166-168: The quoted test name in the HyperShift run example passed
to the run-test command is missing a closing quote around Managed; update the
test string used in the
./openshift-tests/bin/cloud-controller-manager-aws-tests-ext run-test invocation
(the long bracketed test selector starting with
"[cloud-provider-aws-e2e-openshift] ... with 'Managed value in cloud-config") to
close the single quote around Managed (e.g., "with 'Managed' value in
cloud-config ...") so the quoted test name is well-formed.
In `@openshift-tests/ccm-aws-tests/e2e/common/helper.go`:
- Around line 161-164: The error returned when fetching the ConfigMap via
mgmtClient.CoreV1().ConfigMaps(hcpNamespace).Get(...) drops the underlying err;
update the return to include/wrap the original error (e.g., append ": %w" or
include err with %v) so the failure from Get is preserved and actionable; change
the fmt.Errorf call that currently formats "failed to get HCP cloud-config
ConfigMap %s/%s" to include the err and wrap it using the original err variable
(referencing hcpNamespace and hcpCloudConfigName to locate the site).
- Around line 151-159: The error returns for BuildConfigFromFlags and
NewForConfig discard the underlying error; update the error wrapping in the
blocks where restConfig is built (clientcmd.BuildConfigFromFlags with
mgmtKubeconfig) and where mgmtClient is created (clientset.NewForConfig) to
include the original err (e.g. use fmt.Errorf with %w or include %v) so callers
see the underlying failure details; locate the two error branches around
restConfig and mgmtClient and wrap or append err to the returned fmt.Errorf
messages.
---
Nitpick comments:
In `@openshift-tests/ccm-aws-tests/main.go`:
- Around line 50-65: Replace the unbounded context.TODO() used in the dual-stack
detection call chain with a bounded context: create a context with timeout (e.g.
context.WithTimeout(context.Background(), 30*time.Second)), defer its cancel,
and pass that context into common.GetCloudConfig instead of context.TODO();
ensure error handling/log messages for the LoadConfig -> kclientset.NewForConfig
-> common.GetCloudConfig -> common.IsDualStack sequence still log the error
(including timeout) and that isDualStackCluster and dualStackDetectionReady are
only set when detection completes successfully, and keep the existing
log.Debugf("Dual-stack cluster detected: %v", isDualStackCluster) after
successful detection.
🪄 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: Enterprise
Run ID: ec60e4a6-62e6-4798-a902-08de766c83c7
📒 Files selected for processing (7)
docs/dev/e2e-ote-ccm-aws.mddocs/dev/ote-ccm-aws.mdopenshift-tests/ccm-aws-tests/e2e/aws/helper.goopenshift-tests/ccm-aws-tests/e2e/aws/loadbalancer.goopenshift-tests/ccm-aws-tests/e2e/common/helper.goopenshift-tests/ccm-aws-tests/go.modopenshift-tests/ccm-aws-tests/main.go
💤 Files with no reviewable changes (1)
- docs/dev/ote-ccm-aws.md
| ./openshift-tests/bin/cloud-controller-manager-aws-tests-ext run-test \ | ||
| "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should have NLBSecurityGroupMode with 'Managed value in cloud-config [Suite:openshift/conformance/parallel]" | ||
| ``` |
There was a problem hiding this comment.
Fix malformed quoted test name in the HyperShift run example.
The example test string appears to miss a closing quote around Managed (with 'Managed value...), which can cause failed copy/paste execution for run-test.
Suggested doc fix
./openshift-tests/bin/cloud-controller-manager-aws-tests-ext run-test \
- "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should have NLBSecurityGroupMode with 'Managed value in cloud-config [Suite:openshift/conformance/parallel]"
+ "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should have NLBSecurityGroupMode with 'Managed' value in cloud-config [Suite:openshift/conformance/parallel]"📝 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.
| ./openshift-tests/bin/cloud-controller-manager-aws-tests-ext run-test \ | |
| "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should have NLBSecurityGroupMode with 'Managed value in cloud-config [Suite:openshift/conformance/parallel]" | |
| ``` | |
| ./openshift-tests/bin/cloud-controller-manager-aws-tests-ext run-test \ | |
| "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should have NLBSecurityGroupMode with 'Managed' value in cloud-config [Suite:openshift/conformance/parallel]" |
🤖 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 `@docs/dev/e2e-ote-ccm-aws.md` around lines 166 - 168, The quoted test name in
the HyperShift run example passed to the run-test command is missing a closing
quote around Managed; update the test string used in the
./openshift-tests/bin/cloud-controller-manager-aws-tests-ext run-test invocation
(the long bracketed test selector starting with
"[cloud-provider-aws-e2e-openshift] ... with 'Managed value in cloud-config") to
close the single quote around Managed (e.g., "with 'Managed' value in
cloud-config ...") so the quoted test name is well-formed.
|
Fixed the dualstack check from config, collecting data sets from CI again after running successfully locally in a dualstack cluster: $ oc get cm -n openshift-cloud-controller-manager cloud-conf -o yaml | grep NodeIPFamilies
NodeIPFamilies = ipv6
NodeIPFamilies = ipv4
$ run_test(){
while IFS= read -r t; do
echo "=== Running: $t";
$BIN run-test "$t" < /dev/null;
done < <($BIN list tests | grep -v I0526 | jq -r '.[].name' | grep "AWSServiceLBNetworkSecurityGroup"); }
$ run_test | tee -a e2e-ote.log
$ grep -E "(name\"\:|\"result\")" e2e-ote.log
grep: warning: stray \ before :
"name": "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should have NLBSecurityGroupMode with 'Managed value in cloud-config [Suite:openshift/conformance/parallel]",
"result": "passed",
"name": "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should create NLB service with security group attached [Suite:openshift/conformance/parallel]",
"result": "passed",
"name": "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should have security groups attached to default ingress controller NLB [Suite:openshift/conformance/parallel]",
"result": "passed",
"name": "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should update security group rules when service is updated [Suite:openshift/conformance/parallel]",
"result": "passed",
"name": "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should cleanup security groups when service is deleted [Suite:openshift/conformance/parallel]",
"result": "passed",
"name": "[cloud-provider-aws-e2e-openshift] loadbalancer NLB [OCPFeatureGate:AWSServiceLBNetworkSecurityGroup] should have correct security group rules for service ports [Suite:openshift/conformance/parallel]",
"result": "passed",
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-installer-dualstack-ipv6-primary-techpreview |
|
@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/5d7de3c0-5916-11f1-825a-985e881b83e7-0 |
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 `@openshift-tests/ccm-aws-tests/e2e/common/helper.go`:
- Around line 263-268: The switch that checks the IP family string (switch v {
case "ipv6": ... case "ipv4": ... }) assigns hasIPv4 and hasIPv6 backwards;
change the case bodies so case "ipv6" sets hasIPv6 = true and case "ipv4" sets
hasIPv4 = true (leave the variable names hasIPv4 and hasIPv6 and the switch on v
unchanged).
🪄 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: Enterprise
Run ID: c3489038-5d9a-40b5-89d2-5a570b967712
📒 Files selected for processing (4)
openshift-tests/ccm-aws-tests/e2e/aws/helper.goopenshift-tests/ccm-aws-tests/e2e/aws/loadbalancer.goopenshift-tests/ccm-aws-tests/e2e/common/helper.goopenshift-tests/ccm-aws-tests/main.go
|
@tthvo: This PR was included in a payload test run from openshift/origin#31217
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/82f96140-593b-11f1-9177-54846f2a0e77-0 |
|
@tthvo: This PR was included in a payload test run from openshift/origin#31217
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/8afa2d70-593b-11f1-94c3-bb46dcb523ca-0 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openshift-tests/ccm-aws-tests/main.go (1)
53-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor the fail-closed path when dual-stack detection is unavailable.
Because
dualStackDetectionReadyis never consulted after Line 63, any error in Lines 53-60 leavesisDualStackCluster == false, and Line 90 still appends the upstream loadbalancer selector. That means detection failures will run the unsupported upstream LB suite instead of skipping it.Suggested fix
- if isDualStackCluster { - framework.Logf("Dual-stack environment detected, skipping test name that contains '[cloud-provider-aws-e2e] loadbalancer'") - } else { + if !dualStackDetectionReady { + framework.Logf("Dual-stack detection was not available, skipping test name that contains '[cloud-provider-aws-e2e] loadbalancer'") + } else if isDualStackCluster { + framework.Logf("Dual-stack environment detected, skipping test name that contains '[cloud-provider-aws-e2e] loadbalancer'") + } else { specSelectors = append(specSelectors, extensiontests.NameContains("[cloud-provider-aws-e2e] loadbalancer")) }Also applies to: 87-90
🤖 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 `@openshift-tests/ccm-aws-tests/main.go` around lines 53 - 65, The dual-stack detection block (using framework.LoadConfig, kclientset.NewForConfig, common.GetCloudConfig, common.IsDualStack) currently leaves isDualStackCluster false on errors but still allows later code to behave as if detection succeeded; change the logic so that dualStackDetectionReady is only set true when detection completes successfully and ensure downstream code that appends the upstream loadbalancer selector checks dualStackDetectionReady before using isDualStackCluster (i.e., do not append the upstream LB selector unless dualStackDetectionReady == true and isDualStackCluster == true); apply the same check in the other occurrence mentioned (the append block around upstream LB handling) so detection failures skip the unsupported upstream LB suite rather than defaulting to non-dual-stack behavior.
🤖 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.
Outside diff comments:
In `@openshift-tests/ccm-aws-tests/main.go`:
- Around line 53-65: The dual-stack detection block (using framework.LoadConfig,
kclientset.NewForConfig, common.GetCloudConfig, common.IsDualStack) currently
leaves isDualStackCluster false on errors but still allows later code to behave
as if detection succeeded; change the logic so that dualStackDetectionReady is
only set true when detection completes successfully and ensure downstream code
that appends the upstream loadbalancer selector checks dualStackDetectionReady
before using isDualStackCluster (i.e., do not append the upstream LB selector
unless dualStackDetectionReady == true and isDualStackCluster == true); apply
the same check in the other occurrence mentioned (the append block around
upstream LB handling) so detection failures skip the unsupported upstream LB
suite rather than defaulting to non-dual-stack behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ebae86b-b6e7-485c-a091-059d5ca08572
📒 Files selected for processing (3)
openshift-tests/ccm-aws-tests/e2e/aws/loadbalancer.goopenshift-tests/ccm-aws-tests/e2e/common/helper.goopenshift-tests/ccm-aws-tests/main.go
💤 Files with no reviewable changes (1)
- openshift-tests/ccm-aws-tests/e2e/common/helper.go
mtulio
force-pushed
the
OCPBUGS-86299
branch
3 times, most recently
from
4fe49ef to
ad1461f
Compare
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 `@openshift-tests/ccm-aws-tests/main.go`:
- Around line 53-57: The code ignores errors from common.IsDualStack and thus
can “fail-open”; change the detection flow so that when common.GetCloudConfig or
common.IsDualStack returns an error you set dualStackDetectionReady = false (and
do not set isDualStackCluster), logging the error, and only treat dual-stack as
present if dualStackDetectionReady == true && isDualStackCluster == true; update
the later conditional that currently checks only isDualStackCluster to require
dualStackDetectionReady as well so upstream LB specs are included only when
detection succeeded.
🪄 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: Enterprise
Run ID: 68360838-876c-4b19-b1fb-c1aeaa10e6fe
📒 Files selected for processing (4)
openshift-tests/ccm-aws-tests/e2e/aws/loadbalancer.goopenshift-tests/ccm-aws-tests/e2e/common/helper.goopenshift-tests/ccm-aws-tests/go.modopenshift-tests/ccm-aws-tests/main.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openshift-tests/ccm-aws-tests/main.go (1)
53-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail-closed dual-stack gating is still bypassed on detection errors.
Line 56 discards
common.IsDualStackerrors and Line 81 gates only onisDualStackCluster. This can still include upstream LB specs when detection fails.Suggested fix
- if cm, err := common.GetCloudConfig(context.TODO(), nil); err != nil { - log.Debugf("failed to get cloud-config for dual-stack detection: %v", err) - } else { - isDualStackCluster, _ = common.IsDualStack(cm) - dualStackDetectionReady = true - log.Debugf("Dual-stack cluster detected: %v", isDualStackCluster) - } + if cm, err := common.GetCloudConfig(context.TODO(), nil); err != nil { + log.Warnf("failed to get cloud-config for dual-stack detection: %v", err) + } else if dual, err := common.IsDualStack(cm); err != nil { + log.Warnf("failed to detect dual-stack from cloud-config: %v", err) + } else { + isDualStackCluster = dual + dualStackDetectionReady = true + log.Debugf("Dual-stack cluster detected: %v", isDualStackCluster) + } @@ - if isDualStackCluster { + if !dualStackDetectionReady || isDualStackCluster { framework.Logf("Dual-stack environment detected, skipping test name that contains '[cloud-provider-aws-e2e] loadbalancer'") } else { specSelectors = append(specSelectors, extensiontests.NameContains("[cloud-provider-aws-e2e] loadbalancer")) }Also applies to: 81-85
🤖 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 `@openshift-tests/ccm-aws-tests/main.go` around lines 53 - 57, If cloud-config or dual-stack detection returns an error, fail-closed: treat the cluster as dual-stack and mark detection as ready so downstream gating prevents unsafe upstream LB specs. Concretely, in the block that calls common.GetCloudConfig(...) and common.IsDualStack(...), do not discard errors from common.IsDualStack; if IsDualStack returns an error, set isDualStackCluster = true and dualStackDetectionReady = true (otherwise set isDualStackCluster to the returned value and dualStackDetectionReady = true only on success). Also update the later gating logic that uses isDualStackCluster/dualStackDetectionReady to rely on dualStackDetectionReady to decide readiness and to treat any detection-error case as dual-stack based on the variables above.
🤖 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 `@openshift-tests/ccm-aws-tests/e2e/common/helper.go`:
- Around line 190-198: The IsDualStack detection loop currently matches values
from the slice `values` case-sensitively and can miss "IPv4"/"IPv6" inputs;
before the switch in the loop that sets `hasIPv4` and `hasIPv6`, normalize each
`v` (e.g., strings.ToLower(v) or equivalent) and switch on the normalized value
so the cases "ipv4" and "ipv6" reliably match; update the loop that iterates
`for _, v := range values { ... switch v { ... } }` to switch on the lowercased
value.
---
Duplicate comments:
In `@openshift-tests/ccm-aws-tests/main.go`:
- Around line 53-57: If cloud-config or dual-stack detection returns an error,
fail-closed: treat the cluster as dual-stack and mark detection as ready so
downstream gating prevents unsafe upstream LB specs. Concretely, in the block
that calls common.GetCloudConfig(...) and common.IsDualStack(...), do not
discard errors from common.IsDualStack; if IsDualStack returns an error, set
isDualStackCluster = true and dualStackDetectionReady = true (otherwise set
isDualStackCluster to the returned value and dualStackDetectionReady = true only
on success). Also update the later gating logic that uses
isDualStackCluster/dualStackDetectionReady to rely on dualStackDetectionReady to
decide readiness and to treat any detection-error case as dual-stack based on
the variables above.
🪄 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: Enterprise
Run ID: 5d4bb127-87d1-4e4f-a8bb-afd1f7772f12
📒 Files selected for processing (4)
openshift-tests/ccm-aws-tests/e2e/aws/loadbalancer.goopenshift-tests/ccm-aws-tests/e2e/common/helper.goopenshift-tests/ccm-aws-tests/go.modopenshift-tests/ccm-aws-tests/main.go
|
/test all |
OCPBUGS-86299: Upstream cloud-provider-aws load balancer tests do not support dual-stack clusters yet. Detect dual-stack configuration from the cloud-config (`NodeIPFamilies` key) at startup and exclude upstream LB tests when the cluster is dual-stack. When detection fails, upstream LB tests are also excluded to avoid false positives (fail-closed). For the downstream AWSServiceLBNetworkSecurityGroup tests, patch the NLB service with IPFamilyPolicy=RequireDualStack when dual-stack is detected so the service matches the cluster's network configuration.
|
@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. |
|
Overall changes isolated from #464, and PRs can move in parallel - without dependencies. AFAICT /test all |
|
@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/b7ea5e90-5972-11f1-9f2b-ec634464a62f-0 |
|
Triggering primary IPv6 as well: /payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-installer-dualstack-ipv6-primary-techpreview |
|
@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/11d2e120-5973-11f1-81d8-7538de235758-0 |
3 participants
Summary
Upstream
cloud-provider-awsload balancer tests do not support dual-stack clusters — they create single-stack NLB services that fail when the cluster network requires dual-stack IPFamilyPolicy. This PR detects dual-stack configuration from the CCM cloud-config and adapts the test suite accordingly.Changes
Exclude upstream LB tests on dual-stack clusters: Detect dual-stack at startup by reading the
ipFamilieskey from the cloud-config ConfigMap. When the cluster is dual-stack, upstream[cloud-provider-aws-e2e] loadbalancertests are excluded from the spec selector. When detection fails, upstream LB tests are also excluded (fail-closed) to avoid false positives.Patch NLB services for dual-stack: The downstream
createServiceNLBhelper now setsIPFamilyPolicy=RequireDualStackon NLB services when dual-stack is detected, so theAWSServiceLBNetworkSecurityGrouptests create services matching the cluster's network configuration.Add
IsDualStackhelper: Newcommon.IsDualStack()function parses theipFamiliesconfig key from the cloud-config ConfigMap and returns true when both IPv4 and IPv6 are present.Dependencies
Test plan
list testsshould not contain[cloud-provider-aws-e2e] loadbalancerentries)AWSServiceLBNetworkSecurityGrouptests create dual-stack NLB services on dual-stack clusters🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Refactor
Chores