OCPSTRAT-886: Support dual-stack LB in service disruption monitor#30936
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughInspect Infrastructure CR AWS IPFamily and, when dual-stack, annotate TCP LoadBalancer services for NLB and set Service.Spec.IPFamilyPolicy to RequireDualStack with IPFamilies ordered per AWS primary IP family (IPv4-primary or IPv6-primary). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
Scheduling required tests: |
|
/testwith openshift/installer/main/e2e-aws-ovn-dualstack-ipv6-primary-techpreview openshift/cloud-provider-aws#135 |
|
@alebedev87, |
|
/testwith openshift/installer/main/e2e-aws-ovn-dualstack-ipv6-primary-techpreview openshift/cloud-provider-aws#135 |
|
@alebedev87, |
|
/testwith openshift/installer/main/e2e-aws-ovn-dualstack-ipv6-primary-techpreview openshift/cloud-provider-aws#135 |
|
@alebedev87, |
|
/testwith openshift/installer/main/e2e-aws-ovn-dualstack-ipv6-primary-techpreview openshift/cloud-provider-aws#135 openshift/installer#10380 |
| s.Annotations["service.beta.kubernetes.io/aws-load-balancer-type"] = "nlb" | ||
| dualStack := corev1.IPFamilyPolicyRequireDualStack | ||
| s.Spec.IPFamilyPolicy = &dualStack |
There was a problem hiding this comment.
Minor and shouldn't block merging if it's passing tests, but could we move these 3 lines out of the case statements so it's more clear that the only variable is the order of IP families?
There was a problem hiding this comment.
Yeah, the boilerplate bothered me too. I tried a couple of approaches but they brought even more code. If you have an idea, I'm open for suggestions.
There was a problem hiding this comment.
I am a little late to the party, but we probably can try something like:
// Configure dual-stack if the Infrastructure CR indicates dual-stack IP families.
// NLB is required on AWS for dual-stack load balancers.
if infra.Status.PlatformStatus.AWS != nil {
var ipFamilies []corev1.IPFamily
switch infra.Status.PlatformStatus.AWS.IPFamily {
case configv1.DualStackIPv4Primary:
ipFamilies = []corev1.IPFamily{corev1.IPv4Protocol, corev1.IPv6Protocol}
case configv1.DualStackIPv6Primary:
ipFamilies = []corev1.IPFamily{corev1.IPv6Protocol, corev1.IPv4Protocol}
}
if len(ipFamilies) > 1 {
s.Annotations["service.beta.kubernetes.io/aws-load-balancer-type"] = "nlb"
dualStack := corev1.IPFamilyPolicyRequireDualStack
s.Spec.IPFamilyPolicy = &dualStack
s.Spec.IPFamilies = ipFamilies
}
} Not blocking though as tests passed 😁
There was a problem hiding this comment.
Good idea! Thanks @tthvo! Updated the PR.
The modified test passed for the dualstack-ipv6primary job but interesting that it failed on the presubmit, all is pointing to some hiccups from AWS, retrying to be sure.. /test e2e-aws-ovn-fips |
Configure the service load balancer disruption test to use NLB with `RequireDualStack` IP family policy when the Infrastructure CR indicates dual-stack IP families on AWS, matching the primary IP family order.
ffa7b5a to
ed71c18
Compare
|
Scheduling required tests: |
|
FIPS job is broken, latest router changes are suspected, Claude Code points to a TLS curve which is not available in FIPS: |
|
/test e2e-vsphere-ovn-upi |
|
/lgtm |
|
/test e2e-aws-ovn-fips |
|
/verified by e2e |
|
@ShudiLi: This PR has been marked as verified by 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. |
|
/approve |
|
/retitle NO-JIRA: Support dual-stack LB in service disruption monitor |
|
@alebedev87: This pull request explicitly references no jira issue. 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. |
|
@alebedev87: This pull request references OCPSTRAT-886 which is a valid jira issue. 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, neisw, nrb, sadasu, tthvo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Scheduling required tests: |
|
/test e2e-aws-ovn-serial-1of2 |
|
/test e2e-aws-ovn-fips |
|
@alebedev87: 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. |
Configure the service load balancer disruption test to use NLB with
RequireDualStackIP family policy when the Infrastructure CR indicates dual-stack IP families on AWS, matching the primary IP family order.Failure on IPv6Primary cluster:
Example of a failed job: link.