NE-2750: implement feature test for GatewayAPIManagementMode - #31503
NE-2750: implement feature test for GatewayAPIManagementMode#31503rikatz wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@rikatz: This pull request references NE-2750 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. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds end-to-end tests for Gateway API management modes, including transitions, compliance, takeover blocking, metrics, routing, and upgrade persistence. Registers the upgrade test and updates OpenShift API dependencies. ChangesGateway API management-mode testing
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant UpgradeTest as GatewayAPIManagementModeUpgradeTest
participant IngressResource
participant GatewayAPIResources
participant Istiod
UpgradeTest->>IngressResource: Set management mode
UpgradeTest->>GatewayAPIResources: Create GatewayClass, Gateway, and HTTPRoute
GatewayAPIResources-->>UpgradeTest: Return resource status and connectivity state
UpgradeTest->>IngressResource: Switch mode after upgrade
IngressResource->>Istiod: Create or remove mode-specific state
UpgradeTest->>GatewayAPIResources: Verify resource persistence and reconciliation
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: rikatz 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 |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/baf0cf60-958c-11f1-8ef9-db390a0f6457-0 |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d88faa50-958c-11f1-966f-44422d7a35b5-0 |
![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
🧹 Nitpick comments (4)
test/extended/router/gatewayapi_management_mode.go (2)
224-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
platformAwareTimeoutfor consistency.Every other transition wait in this file wraps the timeout with
platformAwareTimeout. This call hardcodes5*time.Minute. On slow platforms the surrounding calls scale, but this one does not.♻️ Proposed change
- err = waitForManagementModeTransition(ctx, oc, operatorv1alpha1.GatewayAPIManagementModeManaged, 5*time.Minute) + err = waitForManagementModeTransition(ctx, oc, operatorv1alpha1.GatewayAPIManagementModeManaged, platformAwareTimeout(oc, 5*time.Minute))🤖 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/router/gatewayapi_management_mode.go` at line 224, Update the waitForManagementModeTransition call for GatewayAPIManagementModeManaged to pass platformAwareTimeout(5*time.Minute) instead of the hardcoded 5*time.Minute, matching the other transition waits in the file.
839-841: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
ptr.Tofor the boolean pointer.
k8s.io/utils/ptrprovidesptr.To(true)and is already used by extended tests. This removes the single-useboolPtrhelper.🤖 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/router/gatewayapi_management_mode.go` around lines 839 - 841, Replace the single-use boolPtr helper with k8s.io/utils/ptr.To at its call sites, using ptr.To for boolean pointers and removing boolPtr once unused.test/extended/router/gatewayapi_management_mode_upgrade.go (2)
293-306: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDetach cleanup from the canceled test context.
Teardown receives
ctxfrom the upgrade framework. If the spec context is canceled after a failure, every client call in Teardown fails immediately and the Gateway, HTTPRoute, and GatewayClass leak into the cluster. Detach cancellation and apply an explicit timeout.♻️ Proposed change
func (t *GatewayAPIManagementModeUpgradeTest) Teardown(ctx context.Context, f *e2e.Framework) { if t.oc == nil || t.gatewayName == "" { e2e.Logf("Skipping cleanup because setup did not initialize resources") return } + + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Minute) + defer cancel()Based on learnings, in openshift/origin test helpers avoid
context.Background()for deferred cleanup; detach cancellation withcontext.WithoutCancel(ctx)to preserve context values, then bound it withcontext.WithTimeout.🤖 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/router/gatewayapi_management_mode_upgrade.go` around lines 293 - 306, Update GatewayAPIManagementModeUpgradeTest.Teardown to derive a cleanup context with context.WithoutCancel(ctx), then wrap it with an explicit timeout and defer its cancellation. Use this bounded, cancellation-independent context for setManagementMode and waitForManagementModeTransition so cleanup still runs after the test context is canceled.Source: Learnings
294-297: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClean up the GatewayClass when Gateway creation does not complete.
The guard returns early when
t.gatewayNameis empty. Setup setst.gatewayClassNameat line 109 and creates the GatewayClass at line 111, before it setst.gatewayNameat line 124. If Setup fails between those points, the GatewayClass stays in the cluster. Gate each delete on its own recorded name.♻️ Proposed change
- if t.oc == nil || t.gatewayName == "" { + if t.oc == nil || (t.gatewayClassName == "" && t.gatewayName == "") { e2e.Logf("Skipping cleanup because setup did not initialize resources") return }Then guard the individual delete steps with
if t.routeName != "",if t.gatewayName != "", andif t.gatewayClassName != "".🤖 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/router/gatewayapi_management_mode_upgrade.go` around lines 294 - 297, Update the cleanup method’s initial guard so it only skips when the test client is unavailable, then gate each resource deletion independently using t.routeName, t.gatewayName, and t.gatewayClassName. This must delete the GatewayClass even when Gateway creation failed after its name was recorded, while preserving skips for empty resource names.
🤖 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 `@go.mod`:
- Around line 68-71: Run go mod tidy followed by go mod vendor to refresh
dependency metadata and vendored sources for the OpenShift modules in go.mod,
removing obsolete go.sum checksums for prior API and client-go versions while
retaining the versions that provide the required symbols.
In `@test/extended/router/gatewayapi_management_mode_upgrade.go`:
- Around line 120-125: Update the custom-domain setup near
getDefaultIngressClusterDomainName and the customDomain assignment to verify
that replacing "apps." actually changes defaultIngressDomain before using it;
fail the test clearly when the expected segment is absent, while preserving the
existing gateway hostname construction.
- Around line 89-106: Update Teardown to restore the recorded initial mode from
t.startMode rather than the post-upgrade current mode, preserving the original
cluster state. Keep Managed mode during any resource-deletion steps that require
it, then transition to t.startMode as the final cleanup action and wait for that
transition to complete.
- Around line 47-73: Update GatewayAPIManagementModeUpgradeTest.Skip so this
scenario is excluded from real upgrade runs on TechPreviewNoUpgrade clusters; do
not allow those clusters to proceed into Setup. Move the scenario to a
non-upgrade suite or gate it on a feature configuration that supports upgrades,
while preserving the existing skip checks for other environments.
In `@test/extended/router/gatewayapi_management_mode.go`:
- Around line 509-517: The VAP binding cleanup in the DeferCleanup callback must
clear metadata that cannot be reused on create, including UID and
CreationTimestamp alongside ResourceVersion. Handle Get errors other than
NotFound by reporting or failing cleanup instead of silently skipping
restoration, while preserving the existing recreation path when the binding is
absent.
- Around line 843-855: Update platformAwareTimeout to return baseTimeout when
infra.Status.PlatformStatus is nil before dereferencing it. Rename the
infrastructure and type variables to reflect their values, compare the platform
against configv1.PowerVSPlatformType instead of "IBMPowerVS", and remove
"IBMZPlatform" as a platform-type check; if IBM Z requires the multiplier,
determine it from node architecture instead.
---
Nitpick comments:
In `@test/extended/router/gatewayapi_management_mode_upgrade.go`:
- Around line 293-306: Update GatewayAPIManagementModeUpgradeTest.Teardown to
derive a cleanup context with context.WithoutCancel(ctx), then wrap it with an
explicit timeout and defer its cancellation. Use this bounded,
cancellation-independent context for setManagementMode and
waitForManagementModeTransition so cleanup still runs after the test context is
canceled.
- Around line 294-297: Update the cleanup method’s initial guard so it only
skips when the test client is unavailable, then gate each resource deletion
independently using t.routeName, t.gatewayName, and t.gatewayClassName. This
must delete the GatewayClass even when Gateway creation failed after its name
was recorded, while preserving skips for empty resource names.
In `@test/extended/router/gatewayapi_management_mode.go`:
- Line 224: Update the waitForManagementModeTransition call for
GatewayAPIManagementModeManaged to pass platformAwareTimeout(5*time.Minute)
instead of the hardcoded 5*time.Minute, matching the other transition waits in
the file.
- Around line 839-841: Replace the single-use boolPtr helper with
k8s.io/utils/ptr.To at its call sites, using ptr.To for boolean pointers and
removing boolPtr once unused.
🪄 Autofix
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: f8ee33e6-558f-4d06-a5bf-be02434d242e
⛔ Files ignored due to path filters (60)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/route/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/awsplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/baremetalplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gcpplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vaultkmspluginconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vsphereplatformfailuredomainspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vsphereplatformspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectorconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectordevicemappermultipathconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectornvmexpresssubsystemconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectorzoneinfoconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/remotewritespec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/controllerconfigspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmsencryptionstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspluginhealthreport.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspreflightcheck.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspreflightresult.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/nodestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/gatewayapiingressconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/ingressspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/ingressstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1alpha1/ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modtest/e2e/upgrade/upgrade.gotest/extended/router/gatewayapi_management_mode.gotest/extended/router/gatewayapi_management_mode_upgrade.go
| github.com/openshift/api v0.0.0-20260810132456-8f52beb625b5 | ||
| github.com/openshift/apiserver-library-go v0.0.0-20260715200723-42e5e402ca43 | ||
| github.com/openshift/build-machinery-go v0.0.0-20250530140348-dc5b2804eeee | ||
| github.com/openshift/client-go v0.0.0-20260715172546-dac61734e0ec | ||
| github.com/openshift/client-go v0.0.0-20260810202730-ddca5e0b7146 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify vendored openshift/api and client-go expose the management-mode API surface.
set -uo pipefail
grep -nE 'github.com/openshift/(api|client-go) ' go.mod
grep -nE '^# github.com/openshift/(api|client-go) ' vendor/modules.txt || echo "vendor/modules.txt entries missing"
grep -nE 'github.com/openshift/(api|client-go) ' go.sum | head -20
fd -t d 'v1alpha1' vendor/github.com/openshift/api/operator | head
rg -n 'GatewayAPIManagementMode(Managed|Unmanaged)?\b' vendor/github.com/openshift/api/operator -g '*.go' | head -20
rg -n 'func .*OperatorV1alpha1\(\)|Ingresses\(\)' vendor/github.com/openshift/client-go/operator -g '*.go' | head -20Repository: openshift/origin
Length of output: 3019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
go_mod = Path("go.mod").read_text()
modules = Path("vendor/modules.txt").read_text()
go_sum = Path("go.sum").read_text()
names = ["github.com/openshift/api", "github.com/openshift/client-go"]
for name in names:
mod_match = re.search(rf"(?m)^\s*{re.escape(name)}\s+(\S+)", go_mod)
vendor_match = re.search(rf"(?m)^# {re.escape(name)}\s+(\S+)", modules)
sum_versions = sorted(set(re.findall(rf"^{re.escape(name)}\s+(\S+)(?:/go\.mod)?\s+", go_sum, re.M)))
print(name)
print(" go.mod:", mod_match.group(1) if mod_match else "MISSING")
print(" vendor/modules.txt:", vendor_match.group(1) if vendor_match else "MISSING")
print(" go.sum versions:", ", ".join(sum_versions) if sum_versions else "MISSING")
if not mod_match or not vendor_match or mod_match.group(1) != vendor_match.group(1):
print(" VERSION_MISMATCH")
print("\nExact client call-chain references:")
for path in Path(".").rglob("*.go"):
if any(part in {"vendor", ".git"} for part in path.parts):
continue
text = path.read_text(errors="replace")
for i, line in enumerate(text.splitlines(), 1):
if "AdminOperatorClient().OperatorV1alpha1().Ingresses()" in line:
print(f"{path}:{i}:{line.strip()}")
PY
printf '\nGo module vendor diagnostics:\n'
go version
go mod vendor -h 2>&1 | sed -n '1,80p'
printf '\nSelected vendored module metadata:\n'
go list -mod=vendor -m github.com/openshift/api github.com/openshift/client-goRepository: openshift/origin
Length of output: 1331
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' 'Read-only go.mod/go.sum tidy diff:'
go mod tidy -diff 2>&1
tidy_status=$?
printf 'go mod tidy -diff exit status: %s\n' "$tidy_status"
printf '\nVendor-mode module query:\n'
go list -mod=vendor -m all 2>&1 | rg 'github.com/openshift/(api|client-go)|go:|cannot|error' | head -40
printf 'go list exit status: %s\n' "${PIPESTATUS[0]}"Repository: openshift/origin
Length of output: 9410
Run go mod tidy && go mod vendor. go.sum contains four obsolete checksums for the previous OpenShift API and client-go versions. The vendored packages provide the requested API symbols.
🤖 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 `@go.mod` around lines 68 - 71, Run go mod tidy followed by go mod vendor to
refresh dependency metadata and vendored sources for the OpenShift modules in
go.mod, removing obsolete go.sum checksums for prior API and client-go versions
while retaining the versions that provide the required symbols.
Source: Coding guidelines
| func (t *GatewayAPIManagementModeUpgradeTest) Skip(_ upgrades.UpgradeContext) bool { | ||
| oc := exutil.NewCLIForMonitorTest("gateway-api-mgmt-mode-upgrade-skip").AsAdmin() | ||
|
|
||
| // Check if feature gate is enabled | ||
| if !exutil.IsTechPreviewNoUpgrade(context.Background(), oc.AdminConfigClient()) { | ||
| e2e.Logf("Skipping: GatewayAPIManagementMode feature is not in TechPreviewNoUpgrade") | ||
| return true | ||
| } | ||
|
|
||
| noOLM, err := isNoOLMFeatureGateEnabled(oc) | ||
| if err != nil { | ||
| e2e.Logf("Failed to check GatewayAPIWithoutOLM feature gate: %v", err) | ||
| return true | ||
| } | ||
|
|
||
| skip, reason, err := shouldSkipGatewayAPITests(oc, noOLM) | ||
| if err != nil { | ||
| e2e.Logf("Failed to check Gateway API skip conditions: %v", err) | ||
| return true | ||
| } | ||
| if skip { | ||
| e2e.Logf("Skipping test: %s", reason) | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the TechPreviewNoUpgrade helper and look for other upgrade tests that gate on it.
rg -nP -C 8 'func IsTechPreviewNoUpgrade\b' test/extended/util
# Find upgrade tests (Skip(upgrades.UpgradeContext)) that also check TechPreviewNoUpgrade.
rg -nP -C 5 'IsTechPreviewNoUpgrade' --type=go -g '!vendor/**'Repository: openshift/origin
Length of output: 2763
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- TechPreviewNoUpgrade helper ---'
sed -n '190,225p' test/extended/util/compat_otp/clusters.go
printf '%s\n' '--- config-client TechPreviewNoUpgrade helper ---'
sed -n '2188,2225p' test/extended/util/framework.go
printf '%s\n' '--- upgrade test interface and runner ---'
rg -n -C 8 'type Test interface|func .*Skip\(|Skip\(.*UpgradeContext|Test\(\s*ctx|UpgradeContext' test/e2e test/extended --type=go -g '!vendor/**' | head -n 240
printf '%s\n' '--- relevant test lines ---'
sed -n '135,175p' test/extended/router/gatewayapi_management_mode_upgrade.go
printf '%s\n' '--- all TechPreviewNoUpgrade references outside helper definitions ---'
rg -n -C 6 'IsTechPreviewNoUpgrade' --type=go -g '!vendor/**'Repository: openshift/origin
Length of output: 21941
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- upgrade test contract and skip invocation ---'
rg -n -C 12 'type UpgradeTest|Skip\(.*UpgradeContext|\.Skip\(|checkUpgradeability|IsNoUpgradeFeatureSet|TechPreviewNoUpgrade|CustomNoUpgrade' test/e2e/upgrade test/extended/router --type=go
printf '%s\n' '--- upgradeability implementation ---'
sed -n '230,330p' test/e2e/upgrade/upgrade.go
printf '%s\n' '--- complete imports and Skip implementation ---'
sed -n '1,90p' test/extended/router/gatewayapi_management_mode_upgrade.go
printf '%s\n' '--- all source references, including non-Go files ---'
rg -n -C 4 'IsTechPreviewNoUpgrade|TechPreviewNoUpgrade|IsNoUpgradeFeatureSet' . -g '!vendor/**' -g '!node_modules/**'Repository: openshift/origin
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- management-mode upgrade test ---'
sed -n '1,210p' test/extended/router/gatewayapi_management_mode_upgrade.go
printf '%s\n' '--- upgrade test registration ---'
rg -n -C 8 'GatewayAPIManagementModeUpgradeTest|gateway-api-mgmt-mode-upgrade' test/e2e/upgrade/upgrade.go test/extended/router/gatewayapi_management_mode_upgrade.go
printf '%s\n' '--- upgrade framework interface and skip path ---'
rg -n -C 8 'type Test interface|func .*Run|Skip\(.*UpgradeContext|test\.Skip|Setup\(ctx|Teardown\(ctx' vendor test 2>/dev/null | rg 'upgrades|UpgradeContext|Skip\(.*UpgradeContext|test/e2e/upgrade' | head -n 160
printf '%s\n' '--- feature-set upgrade restrictions ---'
rg -n -C 8 'TechPreviewNoUpgrade|CustomNoUpgrade|non-upgradeable|not.*upgrade|upgrade.*feature.?set' . -g '*.go' -g '*.md' -g '*.yaml' -g '*.yml' -g '!vendor/**' | head -n 240Repository: openshift/origin
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- complete upgradeability check ---'
sed -n '234,273p' test/e2e/upgrade/upgrade.go
printf '%s\n' '--- disruption skip and execution order ---'
sed -n '165,235p' test/extended/util/disruption/disruption.go
printf '%s\n' '--- upgrade feature-set references in vendored APIs and repository code ---'
rg -n -C 5 'TechPreviewNoUpgrade|ClusterNotUpgradeable|Upgradeable.*false|not upgradeable|not upgrade' vendor/k8s.io vendor/github.com/openshift . -g '*.go' -g '*.md' 2>/dev/null | head -n 260Repository: openshift/origin
Length of output: 27021
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- CI jobs that combine upgrade tests and feature sets ---'
rg -n -C 5 'FEATURE_SET|TechPreviewNoUpgrade|techpreview|upgrade' ci-operator test -g '*.yaml' -g '*.yml' -g '*.json' -g '*.go' -g '*.md' 2>/dev/null | rg -C 3 'FEATURE_SET|TechPreviewNoUpgrade|techpreview|upgrade' | head -n 300
printf '%s\n' '--- upgrade suite documentation and job references ---'
rg -n -C 5 'Suite:upgrade|cluster-upgrade|openshift-tests.*upgrade|upgrade.*suite|upgrade.*job' . -g '*.md' -g '*.yaml' -g '*.yml' -g '*.json' -g '*.go' -g '!vendor/**' | head -n 240Repository: openshift/origin
Length of output: 40588
Do not run this test in the real upgrade suite on TechPreviewNoUpgrade clusters. TechPreviewNoUpgrade sets the cluster as non-upgradeable, so the upgrade cannot complete. Upgradeable clusters skip this test, while TechPreviewNoUpgrade clusters enter Setup and can block at <-done. Move this scenario to a non-upgrade test suite or use a feature configuration supported during upgrades.
🤖 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/router/gatewayapi_management_mode_upgrade.go` around lines 47 -
73, Update GatewayAPIManagementModeUpgradeTest.Skip so this scenario is excluded
from real upgrade runs on TechPreviewNoUpgrade clusters; do not allow those
clusters to proceed into Setup. Move the scenario to a non-upgrade suite or gate
it on a feature configuration that supports upgrades, while preserving the
existing skip checks for other environments.
| g.By("Recording initial management mode before upgrade") | ||
| ingress, err := getIngressCR(ctx, t.oc) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| t.startMode = ingress.Spec.GatewayAPI.ManagementMode | ||
| if t.startMode == "" { | ||
| t.startMode = operatorv1alpha1.GatewayAPIManagementModeManaged | ||
| } | ||
| e2e.Logf("Starting with management mode: %s", t.startMode) | ||
|
|
||
| // Ensure we're in Managed mode for test setup | ||
| if t.startMode != operatorv1alpha1.GatewayAPIManagementModeManaged { | ||
| g.By("Transitioning to Managed mode for setup") | ||
| err = setManagementMode(ctx, t.oc, operatorv1alpha1.GatewayAPIManagementModeManaged) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| err = waitForManagementModeTransition(ctx, t.oc, operatorv1alpha1.GatewayAPIManagementModeManaged, 5*time.Minute) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore the recorded start mode instead of forcing Managed.
Setup records t.startMode, but no later code reads it. Test restores currentMode (the post-upgrade mode), and Teardown always sets Managed. If the cluster began in Unmanaged mode, the test leaves the cluster in Managed mode after cleanup. This changes cluster state for subsequent tests in the same run.
Use t.startMode as the final target in Teardown.
♻️ Proposed change in Teardown
- g.By("Ensuring Managed mode for cleanup")
- err := setManagementMode(ctx, t.oc, operatorv1alpha1.GatewayAPIManagementModeManaged)
+ g.By("Restoring the original management mode for cleanup")
+ restoreMode := t.startMode
+ if restoreMode == "" {
+ restoreMode = operatorv1alpha1.GatewayAPIManagementModeManaged
+ }
+ err := setManagementMode(ctx, t.oc, restoreMode)
if err != nil {
- e2e.Logf("Failed to set Managed mode during cleanup: %v", err)
+ e2e.Logf("Failed to restore management mode %s during cleanup: %v", restoreMode, err)
} else {
- _ = waitForManagementModeTransition(ctx, t.oc, operatorv1alpha1.GatewayAPIManagementModeManaged, 5*time.Minute)
+ if waitErr := waitForManagementModeTransition(ctx, t.oc, restoreMode, 5*time.Minute); waitErr != nil {
+ e2e.Logf("Management mode did not settle on %s during cleanup: %v", restoreMode, waitErr)
+ }
}Note: deleting resources requires Managed mode in some flows. If that is the case, keep Managed for the delete steps and restore t.startMode at the end of Teardown.
Also applies to: 216-226
🤖 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/router/gatewayapi_management_mode_upgrade.go` around lines 89 -
106, Update Teardown to restore the recorded initial mode from t.startMode
rather than the post-upgrade current mode, preserving the original cluster
state. Keep Managed mode during any resource-deletion steps that require it,
then transition to t.startMode as the final cleanup action and wait for that
transition to complete.
| defaultIngressDomain, err := getDefaultIngressClusterDomainName(t.oc, 1*time.Minute) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| customDomain := strings.Replace(defaultIngressDomain, "apps.", "gw-upgrade-mgmt.", 1) | ||
|
|
||
| t.gatewayName = "upgrade-mgmt-mode-gateway" | ||
| t.hostname = "test-upgrade-mgmt." + customDomain |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Confirm the custom domain replacement always applies.
strings.Replace(defaultIngressDomain, "apps.", "gw-upgrade-mgmt.", 1) is a no-op when the default ingress domain does not contain the literal apps.. In that case customDomain equals the default ingress domain, and the Gateway listener claims the same wildcard domain that the default IngressController serves. That can produce confusing routing failures instead of a clear test error.
Assert that the replacement changed the value, or derive the custom domain by prefixing the cluster base domain.
🤖 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/router/gatewayapi_management_mode_upgrade.go` around lines 120
- 125, Update the custom-domain setup near getDefaultIngressClusterDomainName
and the customDomain assignment to verify that replacing "apps." actually
changes defaultIngressDomain before using it; fail the test clearly when the
expected segment is absent, while preserving the existing gateway hostname
construction.
| g.DeferCleanup(func(ctx context.Context) { | ||
| e2e.Logf("Cleanup: Restoring VAP binding") | ||
| // VAP binding should be recreated by CIO, but restore just in case | ||
| _, err := oc.AdminKubeClient().AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Get(ctx, gwapiCRDVAPName, metav1.GetOptions{}) | ||
| if apierrors.IsNotFound(err) { | ||
| vapBinding.ResourceVersion = "" | ||
| _, _ = oc.AdminKubeClient().AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Create(ctx, vapBinding, metav1.CreateOptions{}) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clear UID before recreating the ValidatingAdmissionPolicyBinding, and handle non-NotFound errors.
The cleanup reuses the object returned by Get. It clears ResourceVersion but keeps UID and CreationTimestamp, which the API server rejects on create for a set UID. The cleanup also skips restoration when Get returns an error other than NotFound, which hides a real failure.
🛠️ Proposed change
g.DeferCleanup(func(ctx context.Context) {
e2e.Logf("Cleanup: Restoring VAP binding")
// The CIO normally recreates the binding; recreate it only if it is still absent.
_, err := oc.AdminKubeClient().AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Get(ctx, gwapiCRDVAPName, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
- vapBinding.ResourceVersion = ""
- _, _ = oc.AdminKubeClient().AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Create(ctx, vapBinding, metav1.CreateOptions{})
+ restored := vapBinding.DeepCopy()
+ restored.ResourceVersion = ""
+ restored.UID = ""
+ restored.CreationTimestamp = metav1.Time{}
+ restored.Generation = 0
+ if _, createErr := oc.AdminKubeClient().AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Create(ctx, restored, metav1.CreateOptions{}); createErr != nil {
+ e2e.Logf("Cleanup: failed to recreate VAP binding %s: %v", gwapiCRDVAPName, createErr)
+ }
+ } else if err != nil {
+ e2e.Logf("Cleanup: failed to check VAP binding %s: %v", gwapiCRDVAPName, err)
}
})🤖 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/router/gatewayapi_management_mode.go` around lines 509 - 517,
The VAP binding cleanup in the DeferCleanup callback must clear metadata that
cannot be reused on create, including UID and CreationTimestamp alongside
ResourceVersion. Handle Get errors other than NotFound by reporting or failing
cleanup instead of silently skipping restoration, while preserving the existing
recreation path when the binding is absent.
| // platformAwareTimeout adjusts timeout for slow architectures | ||
| func platformAwareTimeout(oc *exutil.CLI, baseTimeout time.Duration) time.Duration { | ||
| platform, err := oc.AdminConfigClient().ConfigV1().Infrastructures().Get(context.Background(), "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return baseTimeout | ||
| } | ||
|
|
||
| arch := platform.Status.PlatformStatus.Type | ||
| if arch == "IBMPowerVS" || arch == "IBMZPlatform" { | ||
| return baseTimeout * 2 | ||
| } | ||
| return baseTimeout | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the configv1 PlatformType constant values and that PlatformStatus is a pointer.
set -uo pipefail
rg -nP 'PlatformType\s+PlatformType\s*=\s*"' vendor/github.com/openshift/api/config/v1 --type=go
rg -nP 'PlatformStatus\s+\*?PlatformStatus' vendor/github.com/openshift/api/config/v1 --type=go
rg -n 'IBMZ|s390x' vendor/github.com/openshift/api/config/v1 --type=go | headRepository: openshift/origin
Length of output: 2149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/extended/router/gatewayapi_management_mode.go"
printf '%s\n' '--- target helper and imports ---'
sed -n '1,80p' "$file"
sed -n '820,865p' "$file"
printf '%s\n' '--- helper call sites ---'
rg -n -C 3 'platformAwareTimeout' "$file" test/extended/router
printf '%s\n' '--- related platform handling ---'
rg -n -C 5 'getPlatformCapabilities|PlatformStatus|PowerVSPlatformType|IBMZPlatform|IBMPowerVS|s390x' test/extended/router vendor/github.com/openshift/api/config/v1 --glob '*.go'Repository: openshift/origin
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- infrastructure type definitions ---'
sed -n '70,115p' vendor/github.com/openshift/api/config/v1/types_infrastructure.go
sed -n '225,285p' vendor/github.com/openshift/api/config/v1/types_infrastructure.go
printf '%s\n' '--- architecture detection and related timeout logic ---'
rg -n -C 4 'runtime\.GOARCH|NodeArchitecture|Architecture|s390x|ppc64le|PowerVS|IBM Z|IBMZ|timeout.*2|2.*timeout' test/extended vendor/github.com/openshift/api --glob '*.go' | head -240
printf '%s\n' '--- concise platform references ---'
rg -n 'IBMPowerVS|IBMZPlatform|PowerVSPlatformType|s390x|ppc64le' --glob '*.go' --glob '*.yaml' --glob '*.md' . | head -200Repository: openshift/origin
Length of output: 50372
Guard PlatformStatus and use valid platform or architecture values
Return baseTimeout when infra.Status.PlatformStatus is nil before dereferencing it. Use configv1.PowerVSPlatformType, whose value is "PowerVS"; "IBMPowerVS" does not match a valid platform type. "IBMZPlatform" is not a platform type. If IBM Z needs a longer timeout, read the node architecture instead. Rename platform and arch to match the values they hold.
🤖 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/router/gatewayapi_management_mode.go` around lines 843 - 855,
Update platformAwareTimeout to return baseTimeout when
infra.Status.PlatformStatus is nil before dereferencing it. Rename the
infrastructure and type variables to reflect their values, compare the platform
against configv1.PowerVSPlatformType instead of "IBMPowerVS", and remove
"IBMZPlatform" as a platform-type check; if IBM Z requires the multiplier,
determine it from node architecture instead.
|
|
||
| g.BeforeEach(func(ctx context.Context) { | ||
| // Check if feature gate is enabled | ||
| if !exutil.IsTechPreviewNoUpgrade(ctx, oc.AdminConfigClient()) { |
There was a problem hiding this comment.
claude goofed on this, I will fix this soon
1cac6c8 to
bf75d20
Compare
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547 |
|
/test help |
|
@rikatz: The specified target(s) for The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/983aa1e0-95b3-11f1-8279-e88e2a3dac51-0 |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547 |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ec5620d0-95b6-11f1-8eee-421ddecb8633-0 |
|
@rikatz: 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. |
3 participants
This change implements origin tests for Gateway API Management Mode feature.
They are intended to show the right working of this feature:
Summary by CodeRabbit
New Features
Bug Fixes
Chores