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 |
|
[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 |
![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
| - Stops the CIO-managed Istio instance. | ||
| - Stops the CIO Gateway API controllers (`gatewayapi`, | ||
| `gatewayclass`, `gateway-labeler`, `gateway-network-policy`, | ||
| `gateway-service-dns`, `gateway-status`, and any others that | ||
| may be added in the future), so none of them reconcile | ||
| customer-owned Gateway API resources after the transition. | ||
| - Removes the VAP protecting Gateway API CRDs. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the mode-transition order and rollback semantics consistent.
Workflow 2 and the mode-ordering section stop Istio/controllers before removing the VAP, while VAP Management says VAP removal must happen first. Also define rollback behavior: if VAP removal fails after controllers stop, GatewayAPICRDsManaged=True describes a managed state while part of the managed stack is offline. Choose one ordering and specify retry/rollback semantics consistently.
Also applies to: 221-225, 583-585, 592-597, 680-686
🧰 Tools
🪛 LanguageTool
[grammar] ~200-~200: Use a hyphen to join words.
Context: ...ter the transition. - Removes the VAP protecting Gateway API CRDs. - Does *...
(QB_NEW_EN_HYPHEN)
🤖 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 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.
| 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 |
5 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.