NE-2831: address followups on Gateway API management knob EP - #2069
NE-2831: address followups on Gateway API management knob EP#2069rikatz wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rikatz: This pull request references NE-2831 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. |
|
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:
WalkthroughThe enhancement updates Gateway API management-mode contracts and workflows, controller gating, telemetry, platform and upgrade guidance, API validation examples, E2E coverage, backport scope, and graduation criteria. ChangesGateway API management mode
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Administrator
participant CIO
participant GatewayAPICRDs
participant Controllers
Administrator->>CIO: Set managementMode
CIO->>GatewayAPICRDs: Check presence and compatibility
GatewayAPICRDs-->>CIO: Return compliance state
CIO->>Controllers: Start or stop controllers
CIO-->>Administrator: Publish conditions and telemetry
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
enhancements/ingress/gateway-api-crd-management-mode.md (1)
565-572: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify the exception for transition failures.
This says that a false Gateway API condition is not a degraded state, while the following VAP-management section says a failed transition sets
Degraded=True. Explicitly state that operation failures may setDegraded=Trueindependently of the readiness conditions.🤖 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 `@enhancements/ingress/gateway-api-crd-management-mode.md` around lines 565 - 572, Clarify the Gateway API readiness statement to distinguish readiness conditions from operation failures: false conditions do not themselves set Degraded, but failures during transitions or management operations may independently set Degraded=True. Preserve the existing behavior that readiness signals do not block cluster upgrades.
🤖 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 `@enhancements/ingress/gateway-api-crd-management-mode.md`:
- Around line 194-207: Update the Unmanaged transition workflow to explicitly
stop or inhibit every CIO Gateway API controller, including gatewayapi,
gatewayclass, and related controllers, before completing ownership
relinquishment. Document the shutdown step and specify its retry ordering
relative to stopping Istio and removing the VAP, ensuring no CIO controller can
reconcile customer-owned resources after the transition.
- Around line 817-819: Resolve the “Upgrade with non-default mode” requirement
in the acceptance criteria: either specify a concrete executable E2E procedure
that upgrades a cluster with Unmanaged mode while verifying mode preservation
and no CRD takeover, or remove this scenario from the required E2E tests; do not
leave the viability caveat unresolved.
- Around line 606-614: Update the `ingress_controller_gateway_api_info`
specification to define deterministic behavior for absent, unmanaged, and
mixed-version states: use a canonical bounded `unknown` value or explicitly omit
the metric when the Gateway API or CIO-managed OSSM version is unavailable or
inconsistent, and ensure annotation changes cannot create unbounded raw-version
label series. Apply this behavior before adding the metric to the Telemetry
allowlist.
- Around line 594-604: Update the telemetry design around
ingress_controller_gateway_api_management_mode to distinguish the
desired/configured mode from the currently effective GatewayAPICRDsManaged state
during failed transitions. Add a separate effective-mode or transition-state
metric, or change this metric to report only effective mode, ensuring failed
switches do not present Unmanaged as active while the VAP remains enabled.
- Around line 451-456: Update the GatewayAPICRDsCompliant condition definitions
and related transition logic so absent CRDs use an explicit CRDsNotFound or
Unknown status rather than VersionMismatch. Ensure the condition remains
VersionMatch or VersionMismatch only when CRDs are present, and align the
workflow tests with the new absent-CRD behavior so Unmanaged mode is
distinguishable from installed incompatible CRDs.
- Around line 900-902: Update the cluster singleton description to avoid an
empty managementMode value: omit the gatewayAPI field entirely, or specify a
valid managementMode enum value while preserving the implicit Managed behavior.
Ensure the documentation does not claim that spec.gatewayAPI.managementMode is
serialized as an empty string.
---
Outside diff comments:
In `@enhancements/ingress/gateway-api-crd-management-mode.md`:
- Around line 565-572: Clarify the Gateway API readiness statement to
distinguish readiness conditions from operation failures: false conditions do
not themselves set Degraded, but failures during transitions or management
operations may independently set Degraded=True. Preserve the existing behavior
that readiness signals do not block cluster upgrades.
🪄 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: 7722b9f5-7343-48f4-8afd-e3ae66cfadf4
📒 Files selected for processing (1)
enhancements/ingress/gateway-api-crd-management-mode.md
c4d84a5 to
33c28e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@enhancements/ingress/gateway-api-crd-management-mode.md`:
- Around line 820-825: Update the “Transition to Unmanaged” acceptance criteria
and the corresponding scenarios at the referenced unmanaged steps to verify that
every CIO Gateway API controller is stopped or reconciliation-inhibited after
handoff. Add assertions that the preserved CRDs, GatewayClass, and Gateway
resources receive no further controller-driven mutations, while retaining the
existing Istio, VAP, and GatewayAPICRDsManaged checks.
- Around line 194-200: Align the mode-transition ordering across Workflow 2, the
mode-ordering section, VAP Management, and the referenced rollback sections:
choose one sequence for VAP removal versus stopping Istio/controllers and
document it consistently. Define failure handling when VAP removal or shutdown
fails, including whether to retry, restore stopped components, and how
GatewayAPICRDsManaged is set so it never claims a fully managed state while the
stack is partially offline.
🪄 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: fbaa25b5-3c3c-433d-ae61-fba21a2337a7
📒 Files selected for processing (1)
enhancements/ingress/gateway-api-crd-management-mode.md
| 2. **Transition to Unmanaged**: Set mode to `Unmanaged`. Verify | ||
| that the CIO-managed Istio instance is stopped, the VAP is | ||
| removed, and CRDs, GatewayClass, and Gateway resources are | ||
| preserved. Verify `GatewayAPICRDsManaged=False` with reason | ||
| `Unmanaged`. Verify a third-party GatewayClass can be created. | ||
| `Unmanaged`. Verify that one existing Gateway API CRD can | ||
| be changed/updated. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Verify controller deactivation in the E2E acceptance criteria.
The Unmanaged scenarios assert Istio, VAP, and CRD state, but do not verify that every CIO Gateway API controller is stopped or reconciliation-inhibited. Add assertions for the complete controller set and ensure no post-handoff mutations occur.
Also applies to: 832-834
🤖 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 `@enhancements/ingress/gateway-api-crd-management-mode.md` around lines 820 -
825, Update the “Transition to Unmanaged” acceptance criteria and the
corresponding scenarios at the referenced unmanaged steps to verify that every
CIO Gateway API controller is stopped or reconciliation-inhibited after handoff.
Add assertions that the preserved CRDs, GatewayClass, and Gateway resources
receive no further controller-driven mutations, while retaining the existing
Istio, VAP, and GatewayAPICRDsManaged checks.
|
pending: after a discussion with @JoelSpeed I will make some further clarifications on the API definition for minProperties=1 on spec, and mention that CVO will create the resource already setting Managed on GatewayAPI Config. CIO must still validate if the field is set, otherwise default to Managed on its behavior. |
|
from last linter review: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
enhancements/ingress/gateway-api-crd-management-mode.md (1)
647-650: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCall this the effective mode, not the configured mode.
The metric is defined as derived from
GatewayAPICRDsManaged, so it reports effective ownership state. Saying it makes the “configured mode” visible contradicts the preceding definition and may mislead telemetry consumers.🤖 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 `@enhancements/ingress/gateway-api-crd-management-mode.md` around lines 647 - 650, Update the telemetry description for GatewayAPICRDsManaged to call the reported value the effective mode rather than the configured mode, while preserving the existing explanation of fleet-wide visibility and Story 3.
🤖 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 `@enhancements/ingress/gateway-api-crd-management-mode.md`:
- Around line 386-397: Move the ObservedGeneration ratchet from the field-level
validation to the parent status object so unset-to-set and set-to-unset
transitions are validated. Add explicit presence checks for current and previous
status.observedGeneration, enforce the valid numeric range, and require newly
present values to be at least the prior value; remove the field-scoped
XValidation rule.
---
Outside diff comments:
In `@enhancements/ingress/gateway-api-crd-management-mode.md`:
- Around line 647-650: Update the telemetry description for
GatewayAPICRDsManaged to call the reported value the effective mode rather than
the configured mode, while preserving the existing explanation of fleet-wide
visibility and Story 3.
🪄 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: 60f2f9c4-5515-4682-add5-59f6382926bd
📒 Files selected for processing (1)
enhancements/ingress/gateway-api-crd-management-mode.md
65add9c to
7ab72e5
Compare
7ab72e5 to
af8faa7
Compare
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)
enhancements/ingress/gateway-api-crd-management-mode.md (1)
648-651: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe the allowlisted metric as effective mode.
The metric is defined above as reporting the effective mode from
GatewayAPICRDsManaged, but this paragraph says it exposes the “configured mode.” That contradiction can mislead telemetry consumers during failed transitions. Change “configured mode” to “effective mode,” or define a separate configured-mode metric.Suggested wording
- so that the configured mode is visible in + so that the effective mode is visible in🤖 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 `@enhancements/ingress/gateway-api-crd-management-mode.md` around lines 648 - 651, The telemetry description should identify the allowlisted metric as exposing the effective mode, matching the metric defined by GatewayAPICRDsManaged. Update the phrase “configured mode” in the Story 3 paragraph to “effective mode,” without introducing a separate metric.
🤖 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 `@enhancements/ingress/gateway-api-crd-management-mode.md`:
- Around line 648-651: The telemetry description should identify the allowlisted
metric as exposing the effective mode, matching the metric defined by
GatewayAPICRDsManaged. Update the phrase “configured mode” in the Story 3
paragraph to “effective mode,” without introducing a separate metric.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e660005e-eac4-4829-bc30-90a4740e3704
📒 Files selected for processing (1)
enhancements/ingress/gateway-api-crd-management-mode.md
|
@rikatz: 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. |
|
/assign @gcs278 |
|
/assign @Miciah |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
API updates LGTM
/lgtm
|
Adding a hold in case my labels cause it to merge - feel free to remove as needed. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Looks like my labels didn't stick 🫠 /hold cancel |
openshift-ci
Bot
added
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven 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 |
gcs278
left a comment
gcs278
left a comment
There was a problem hiding this comment.
Overall solid updates. No major issues. Maybe we should talk about degraded and progressing for cluster operator - I'm not sure if we are using it correctly here.
|
|
||
| If an operation required by a mode transition fails -- for | ||
| example, VAP removal fails when transitioning to `Unmanaged` -- | ||
| CIO's `ingress` `ClusterOperator` reports `Degraded=True`, with |
There was a problem hiding this comment.
Edit: Full transparency - I think I've been wrong about this in the past, and commented regarding this in the original EP.
I'm not sure if this is right. I've seen @Miciah say before that Gateway API should not influence Degraded=True. In other words, if it doesn't impact the default IngressController functionality, it shouldn't change the cluster operator status.
Now, I also acknowledge that we added other various Degraded=True for Gateway API in the past (that I've reviewed too). But, I don't know if that's right either. Degraded is a particularly strong signal that degrades an entire cluster, and we've been reserving that for core cluster functionality.
I think you are trying to find somewhere we can alert a cluster admin to a failure right? However, is there really anything that would be surface that wouldn't be an OCPBUGS-worthy?
Everything should go according to plan in a transition as far as I can tell. For both:
- Managed to Unmanaged: this is very straightforward, I can't see any actionalble recoveries
- Unmanaged to Managed: more tricky, but
TakeoverBlockedalready handles the "your CRDs don't match" use case.
TLDR; I think we need to be careful about bubbling up to degraded and progressing. Degraded is for communicating actionable information to the user. If the only action is "file a bug," it feels like CIO logs are the right channel. And Progressing should probably be reserved for core cluster ingress impacted components).
There was a problem hiding this comment.
That's a good point — the ClusterOperator status is really reporting on core cluster functionality, which includes the "default" IngressController — because the console and oauth routes use it — but excludes any custom IngressController and excludes Gateway API resources (as long as components such as console and oauth don't rely on Gateway API).
The existing status conditions in Ingress.config.openshift.io should suffice for communicating errors in transitioning between management modes. If more is needed, we could additionally define Prometheus alerts.
| readiness. | ||
| controllers are not started. These three conditions being not | ||
| `True` does not, by itself, contribute to the operator's `Degraded` | ||
| status condition, and does not block cluster upgrades -- they are |
There was a problem hiding this comment.
Degraded=True does block upgrades, I don't think this is correct. See other comment about Degraded signal.
| fleet-wide telemetry, addressing Story 3 (Operational Monitoring). | ||
|
|
||
| CIO also exposes a second metric, | ||
| `ingress_controller_gateway_api_info`, an info-style `GaugeVec` |
There was a problem hiding this comment.
what's the point of this if we are in Managed mode? We know what the versions are based on the OCP Version.
I thought the real value would be in Unmanaged mode, but this EP says it's only in Managed mode.
| 5. **Upgrade with non-default mode**: Upgrade a cluster that has | ||
| `Unmanaged` mode set. Verify the mode is preserved and CIO does | ||
| not attempt to take over CRDs during upgrade. | ||
| 5. **Upgrade with non-default mode**: Set mode to `Unmanaged` on a |
There was a problem hiding this comment.
FYI: I actually don't really know how'd you automate this. If you use the origin upgrade framework like I did here: https://github.com/openshift/origin/blob/main/test/extended/router/gatewayapi_upgrade.go, you'll break/interfere with our existing GWAPI upgrade tests. I suppose you could have a unique CI Job for Gateway API opt-out knob unmanaged, but that feels like overkill
The main issue is that this is a global setting. Unless you think of something, this may just need to be a manual verification for now.
| - Feature gate `GatewayAPIManagementMode`, added directly to the | ||
| `Default` feature set as part of this backport (not | ||
| `TechPreviewNoUpgrade` first). This is required so customers who | ||
| have not opted into `TechPreviewNoUpgrade` can still set | ||
| `Unmanaged` on 4.18 before a 4.18-to-4.19 upgrade, where Gateway | ||
| API itself moves into the `Default` feature set. |
There was a problem hiding this comment.
this is a bit odd. Unless you got explicit permission from API team, backports still need to go through TP-->GA promotion process. I think I previously commented on this, so maybe I confused something.
This is required so customers who
have not opted intoTechPreviewNoUpgradecan still set
Unmanagedon 4.18 before a 4.18-to-4.19 upgrade
nit This is a bit redundant, you can't upgrade TechPreviewNoUpgrade. I guess you call this a circular justification or something like that?
6 participants
Original PR #2023 had some followups to be addressed.
This change addresses the followup comments
Summary by CodeRabbit
Ingressguidance and schema validation, including required metadata, explicit managed mode, strengthened spec/status validation, condition limits, and monotonicobservedGeneration.