CORS-4387: Promote AWS DualStack to Default#2797
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @sadasu! Some important instructions when contributing to openshift/api: |
|
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:
📝 WalkthroughWalkthroughThis pull request promotes the Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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
size/L
|
[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 |
Review Summary by QodoPromote AWS DualStack to Default and enable in OKD
WalkthroughsDescription• Promote AWSDualStackInstall feature gate to default in OCP and OKD • Enable feature in both Default and OKD product scopes • Add ipFamily field to AWS platform status with validation • Remove cloudLoadBalancerConfig from test expectations • Fix indentation in FeatureGateConfidentialCluster definition Diagramflowchart LR
A["AWSDualStackInstall Feature Gate"] -->|"Enable in Default"| B["Default Product Scope"]
A -->|"Enable in OKD"| C["OKD Product Scope"]
B -->|"Add ipFamily field"| D["AWS Platform Status"]
C -->|"Add ipFamily field"| D
D -->|"Validation Rules"| E["Immutable, IPv4/DualStack options"]
File Changes1. features/features.go
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ipFamily: | ||
| default: IPv4 | ||
| description: |- | ||
| ipFamily specifies the IP protocol family that should be used for AWS | ||
| network resources. This controls whether AWS resources are created with | ||
| IPv4-only, or dual-stack networking with IPv4 or IPv6 as the primary | ||
| protocol family. | ||
| enum: | ||
| - IPv4 | ||
| - DualStackIPv6Primary | ||
| - DualStackIPv4Primary | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: ipFamily is immutable once set | ||
| rule: oldSelf == '' || self == oldSelf |
There was a problem hiding this comment.
1. ipfamily docs omit constraints 📘 Rule violation ⚙ Maintainability
The new ipFamily schema includes a default, an enum, and an immutability x-kubernetes-validations rule, but its description does not document the default/allowed values or the immutability constraint. This violates the requirement that validation marker semantics be documented in field comments/user-facing docs.
Agent Prompt
## Issue description
The `ipFamily` field’s schema adds `default`, `enum`, and an immutability `XValidation`, but the user-facing field documentation does not describe:
- what happens when the field is omitted (defaulting)
- the allowed enum values (and what each means)
- that the value is immutable once set
## Issue Context
This is surfaced in generated CRD/OpenAPI documentation, so missing details violate the requirement that validation marker semantics be documented.
## Fix Focus Areas
- config/v1/types_infrastructure.go[567-577]
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml[1188-1202]
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yaml[1188-1202]
- machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml[1480-1494]
- machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml[1480-1494]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
This sounds like a legitimate issue to me
There was a problem hiding this comment.
Right. I think this is just saying the comment should contain something like:
// Valid values are "IPv4", "DualStackIPv4Primary", and "DualStackIPv6Primary". When omitted, the default value is "IPv4".
It's also outside the scope of this PR, so I would consider it more of a nice to have
There was a problem hiding this comment.
@nrb, @patrickdillon I can add a follow-up patch for all 3 platforms. I don't think we had followed that pattern in the past. But a good recommendation to follow for the future.
sadasu
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sadasu: This pull request references CORS-4387 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 "4.22.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. |
|
/jira refresh |
|
@sadasu: This pull request references CORS-4387 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. |
|
Checking the promotion verification results, it looks like:
I also noticed openshift/release#76769 - this seems like testing that we would want to make sure is in place and has captured sufficient runs prior to promoting? |
It's not in the plan to support this in hypershift, so no. But it's a good question: hypershift should be able to take advantage of this work. On the other hand I'm sure further work is required to enable it, so we will move forward with no hypershift support.
Exactly. This is the only testing that is actually exercising the AWS Dual Stack functionality, so the other test failures/results are mostly noise... We have been running these tests in various forms, analyzing the test failures and fix tech debt in the e2e tests for a while now. openshift/release#76769 should merge today and we can accumulate some results quickly with gangway. Overall the feature is stable, and well tested. |
|
/test verify-feature-promotion |
|
PR-Agent: could not fine a component named |
|
/test verify-feature-promotion |
|
PR-Agent: could not fine a component named |
|
/test verify-feature-promotion |
|
PR-Agent: could not fine a component named |
|
/test verify-feature-promotion |
|
PR-Agent: could not fine a component named |
|
/test verify-feature-promotion |
|
PR-Agent: could not fine a component named |
|
/hold Promotion has been moved to 5.0 |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test verify-feature-promotion |
|
/test verify-feature-promotion |
1 similar comment
|
/test verify-feature-promotion |
|
@everettraven, @JoelSpeed We are finally ready to promote this feature. Looking at the results for verify-feature-promotion, it appears that we satisfy all conditions except for HyperShift. As mentioned in #2797 (comment), HyperShift support is out of scope for this work at this time. [Working with PMs to figure out HyperShift support.] To indicate that, I did not include |
|
Hi @JoelSpeed and @everettraven ! I also see that Sippy's view of the feature gate is AWSDualstackInstall Is there any way to not include HyperShift in this list? Thanks! |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@sadasu the current Looking at what actually matters here, it would be good to look specifically at AWS dual stack cluster E2Es, can you link sippy for these jobs? Would that be: Looks like there were some failures there in the last few days, could you take a look at those and see if they could be related to this feature |
1a5ecce to
2812e68
Compare
|
/test verify-feature-promotion |
|
I'm not convinced the new feature promotion logic is quite working (cc @everettraven), but looking directly at the jobs, they also look fairly sad in the last week so lets look into that |
|
#2928 Will fix the broken featuregate analysis |
Yeah, there is a recent degradation. I am looking into those failures. |
|
/test verify-feature-promotion |
2 similar comments
|
/test verify-feature-promotion |
|
/test verify-feature-promotion |
|
Looking at https://sippy-auth.dptools.openshift.org/sippy-ng/feature_gates/5.0/AWSDualStackInstall we are in good shape but if we look for runs having 95% pass rate with 6K conformance tests on DualStack clusters, we are likely to get there only after all other TP features have hardened. |
|
#2937 updates |
|
/test verify-feature-promotion |
Also make it available in OKD.
2812e68 to
748a7af
Compare
openshift-ci
Bot
added
size/XL
748a7af to
3b667b9
Compare
openshift-ci
Bot
added
size/XXL
Augment `verify-feature-promotion` output to indicate pass percentage for `install should succeed` tests for featuregates that include "Install" in their name. This update gives a better indication of whether Install features are failing at installation or later during execution of e2e conformance tests. This update does not change the criteria for reporting success but adds more information in the output for easier analysis of feature state.
3b667b9 to
fcb2ab3
Compare
|
@sadasu: The following tests 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. |
7 participants
Also make it available in OKD.