OCPCLOUD-3074: Validate Azure dual-stack load balancers - #31452
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Skipping CI for Draft Pull Request. |
|
@bandrade: This pull request references OCPCLOUD-3074 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds an Azure dual-stack end-to-end test that deploys a readiness-probed backend, creates public and internal LoadBalancer Services, validates IPv4 and IPv6 ingress publication and reachability, and performs context-aware cleanup. ChangesAzure dual-stack validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GinkgoTest
participant KubernetesAPI
participant AzureLoadBalancer
participant ProbePod
GinkgoTest->>KubernetesAPI: Create readiness-probed backend
GinkgoTest->>KubernetesAPI: Create public and internal dual-stack Services
KubernetesAPI->>AzureLoadBalancer: Provision load balancers
AzureLoadBalancer-->>KubernetesAPI: Publish IPv4 and IPv6 ingress addresses
GinkgoTest->>KubernetesAPI: Poll Service state
GinkgoTest->>ProbePod: Execute curl for each ingress address
ProbePod->>AzureLoadBalancer: Request /hostname
AzureLoadBalancer-->>ProbePod: Return backend response
GinkgoTest->>KubernetesAPI: Delete Services and wait for removal
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bandrade 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 |
![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
🧹 Nitpick comments (2)
test/extended/cloud_controller_manager/azure_dualstack.go (2)
263-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCleanup discards the spec context instead of detaching cancellation from it.
deleteAzureLoadBalancerServicesAndWaitfabricates a freshcontext.Background(), so framework/audit values carried on the spec context are lost during cleanup; the call site compounds this by registering aDeferCleanupclosure with no ctx parameter.
test/extended/cloud_controller_manager/azure_dualstack.go#L263-L265: acceptctx context.Contextand derive the bounded cleanup context withcontext.WithTimeout(context.WithoutCancel(ctx), …)instead ofcontext.Background().test/extended/cloud_controller_manager/azure_dualstack.go#L92-L95: change the closure tofunc(ctx context.Context)and forwardctxto the helper, matching the other twoDeferCleanupblocks at Line 77 and Line 117.♻️ Proposed change
- g.DeferCleanup(func() { - err := deleteAzureLoadBalancerServicesAndWait(client, namespace, serviceNames...) + g.DeferCleanup(func(ctx context.Context) { + err := deleteAzureLoadBalancerServicesAndWait(ctx, client, namespace, serviceNames...) o.Expect(err).NotTo(o.HaveOccurred()) })-func deleteAzureLoadBalancerServicesAndWait(client kubernetes.Interface, namespace string, names ...string) error { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) - defer cancel() +func deleteAzureLoadBalancerServicesAndWait(ctx context.Context, client kubernetes.Interface, namespace string, names ...string) error { + // Detach cancellation so cleanup still runs after the spec context is canceled, but keep it bounded. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Minute) + defer cancel()Based on learnings, avoid switching to
context.Background()for deferred cleanup that must run after the spec context is canceled; instead detach cancellation withcontext.WithoutCancel(ctx)and apply an explicit timeout.🤖 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 `@test/extended/cloud_controller_manager/azure_dualstack.go` around lines 263 - 265, Update deleteAzureLoadBalancerServicesAndWait to accept a context.Context and derive its 10-minute timeout from context.WithoutCancel(ctx) rather than context.Background(), preserving context values while allowing cleanup after cancellation. At the call site, change the DeferCleanup closure to accept ctx context.Context and forward it to the helper; no direct changes are needed at the other DeferCleanup blocks.Source: Learnings
122-127: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a shared deadline for both LoadBalancer waits.
GetServiceLoadBalancerCreationTimeoutreturns up to 45m on larger clusters, and each service consumes that budget independently here; combined with four 5m probes this can blow past the declared[Timeout:45m]and surface as an opaque spec timeout. Both services are created up-front, so a single bounded context for the whole wait phase is sufficient.♻️ Sketch
+ lbCtx, cancelLB := context.WithTimeout(ctx, e2eservice.GetServiceLoadBalancerCreationTimeout(ctx, client)) + defer cancelLB() for i, service := range services { g.By(fmt.Sprintf("waiting for %s to publish IPv4 and IPv6 ingress addresses", service.Name)) - service, err = waitForAzureDualStackLoadBalancer(ctx, client, namespace, service.Name) + service, err = waitForAzureDualStackLoadBalancer(lbCtx, client, namespace, service.Name) o.Expect(err).NotTo(o.HaveOccurred()) services[i] = service }🤖 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 `@test/extended/cloud_controller_manager/azure_dualstack.go` around lines 122 - 127, Use one shared timeout/deadline context for the entire service-wait loop in the dual-stack test, rather than allowing each waitForAzureDualStackLoadBalancer call to consume GetServiceLoadBalancerCreationTimeout independently. Create the bounded context before iterating over services, pass it to both waits, and preserve the existing per-service validation and assignment 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.
Inline comments:
In `@test/extended/cloud_controller_manager/azure_dualstack.go`:
- Around line 272-312: Update the two PollUntilContextTimeout phases in the
service deletion flow to have independent time budgets: derive a separate
timeout context for each phase, or extend the parent deadline to cover both
10-minute polls. Ensure the delete-request phase and the finalizer-wait phase
each receive their full intended budget and timeout errors remain attributable
to the phase that exhausts its budget.
- Around line 65-74: The container setup loop in the backend.Run callback must
validate the generated readiness probe and its HTTPGet handler before assigning
the port. Guard container.ReadinessProbe and ReadinessProbe.HTTPGet, and fail
cleanly through the test’s existing error mechanism when either is absent; only
assign the port when both are present.
---
Nitpick comments:
In `@test/extended/cloud_controller_manager/azure_dualstack.go`:
- Around line 263-265: Update deleteAzureLoadBalancerServicesAndWait to accept a
context.Context and derive its 10-minute timeout from context.WithoutCancel(ctx)
rather than context.Background(), preserving context values while allowing
cleanup after cancellation. At the call site, change the DeferCleanup closure to
accept ctx context.Context and forward it to the helper; no direct changes are
needed at the other DeferCleanup blocks.
- Around line 122-127: Use one shared timeout/deadline context for the entire
service-wait loop in the dual-stack test, rather than allowing each
waitForAzureDualStackLoadBalancer call to consume
GetServiceLoadBalancerCreationTimeout independently. Create the bounded context
before iterating over services, pass it to both waits, and preserve the existing
per-service validation and assignment behavior.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 41211148-db78-434f-b162-d5537c0b7916
📒 Files selected for processing (1)
test/extended/cloud_controller_manager/azure_dualstack.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
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 `@test/extended/cloud_controller_manager/azure_dualstack.go`:
- Around line 125-130: Update the sequential service-wait loop around
waitForAzureDualStackLoadBalancer so each service receives its own load-balancer
creation timeout. Stop sharing loadBalancerContext across iterations; pass ctx
directly, relying on waitForAzureDualStackLoadBalancer to create a fresh timeout
per call, or create and cancel a per-iteration timeout context.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a6c2e457-ca8f-4c54-9cda-0dda85302b85
📒 Files selected for processing (1)
test/extended/cloud_controller_manager/azure_dualstack.go
|
Scheduling required tests: |
|
/test e2e-gcp-ovn |
|
/unhold |
|
@bandrade: all tests passed! 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. |
|
@JoelSpeed @MaysaMacedo can you PTAL? |
2 participants
What changed
RequireDualStackLoadBalancer ServicesWhy
This provides the targeted automation requested by
OCPCLOUD-3074 and complements
the manual Azure dual-stack validation performed for
openshift/installer#10656.
The test is selected by
[Suite:openshift/conformance/parallel]and is gated bythe
AzureDualStackInstallfeature gate, so the existing IPv4-primary andIPv6-primary Azure optional jobs can exercise it.
Validation
go test ./test/extended/cloud_controller_managergo vet ./test/extended/cloud_controller_managermake openshift-tests[Suite:openshift/conformance/parallel]Summary by CodeRabbit
/hostnamefrom a probe running on the control-plane.