[CNO] Setup OTE framework and migrate test cases from tests-private#3015
Open
anuragthehatter wants to merge 1 commit into
Open
[CNO] Setup OTE framework and migrate test cases from tests-private#3015anuragthehatter wants to merge 1 commit into
anuragthehatter wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@anuragthehatter: This pull request references CORENET-6512 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:
WalkthroughAdds an extensive E2E test harness (build targets, test binary, CLI helpers, utilities, and a disruptive CNO suite), updates module dependencies and Docker image build to include compressed E2E artifact, and fixes MTU default-route detection logic. ChangesE2E Testing Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 6 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anuragthehatter 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 |
anuragthehatter
force-pushed
the
cno-ote-framework
branch
from
88d04b6 to
0cc7cad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
go.mod (1)
23-23: ⚡ Quick winAlign required and replaced
k8s.io/apimachineryversions.Line 23 requires
v0.36.0-alpha.2while Line 183 force-replaces it tov0.35.2. This split version intent is brittle and hard to reason about. Prefer keepingrequirealigned with the effective replaced version.Also applies to: 183-183
🤖 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` at line 23, The go.mod currently requires k8s.io/apimachinery at v0.36.0-alpha.2 but the replace directive forces it to v0.35.2, causing a mismatch; update the require entry for k8s.io/apimachinery to match the effective replaced version (v0.35.2) or change the replace to v0.36.0-alpha.2 so both declarations align. Locate the require line mentioning k8s.io/apimachinery and the replace clause that references k8s.io/apimachinery and make them consistent (preferably set the require to v0.35.2 to reflect the current replace), then run go mod tidy to verify module graph correctness.
🤖 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 `@test/e2e/cli.go`:
- Around line 118-123: The asAdminInternal() setter currently toggles nc.asAdmin
but nothing reads that flag; update the CLI command execution path to honor
asAdmin by appending the impersonation flag to kube commands (e.g., add
"--as=system:admin" or use a dedicated admin kubeconfig) wherever
CLI.buildCmd/CLI.run (or the methods that construct/execute kubectl invocations)
create subprocess args, checking the asAdmin boolean on the receiver and
injecting the flag; alternatively if impersonation support is not desired,
remove the AsAdmin()/asAdminInternal() API and tests that call it to avoid a
no-op. Ensure you update references to asAdminInternal, AsAdmin, and any
command-building functions so the flag is actually applied or the API is
removed.
- Around line 190-196: The current subprocess creation uses exec.Command
(variable cmd) without a timeout, which can hang the test suite; change it to
use exec.CommandContext with a context.WithTimeout (create ctx, cancel :=
context.WithTimeout(parentCtx, timeout) and defer cancel()) and pass ctx into
exec.CommandContext(c.execPath, cmdArgs...), keep stdout/stderr assignment and
cmd.Run(), and ensure you handle context.DeadlineExceeded/error to surface
timeout failures; make the timeout configurable (e.g., a constant or a field on
the test helper) rather than hardcoding.
In `@test/e2e/cno.go`:
- Around line 142-173: This test assumes Machine API and node-scaling support
but lacks guards; before calling
getInfrastructureName/createMachineSet/waitForMachineSetRunning/getNodeNameFromMachineSet
add explicit prechecks that the MachineSet (Machine API) is available and the
topology supports multi-node scaling (e.g., not SNO/MicroShift/SingleNode
platforms). Implement helper checks (e.g., hasMachineSetAPI(oc) and
isUnsupportedTopology(oc) or reuse checkPlatform/ checkIPStackType) and call
them at the start of the test to call g.Skip with a clear message when the API
is absent or topology unsupported, preventing createMachineSet and subsequent
steps from running in incompatible environments.
In `@test/e2e/util.go`:
- Line 318: Replace the hardcoded public quay.io image string in util.go with a
disconnected-compatible, configurable image source: change the literal "image"
value to use a helper or env var (e.g., call a new or existing
GetTestImage()/os.Getenv("E2E_TEST_IMAGE") with a sensible local/mirror default)
so tests can run in restricted clusters; update the pod-creation logic that sets
the "image" field accordingly and add a short comment documenting the env var
fallback so tests are flagged/configurable for disconnected environments.
- Around line 138-143: The current rollout check requires both substrings in
status which is too strict; update the condition that checks the status variable
in test/e2e/util.go to use OR (||) instead of AND (&&) so the block returns
success when either "rollout to finish" or "successfully rolled out" appears,
keeping the existing e2e.Logf calls and return values (i.e., change the if that
references strings.Contains(status, "rollout to finish") &&
strings.Contains(status, "successfully rolled out") to use ||).
---
Nitpick comments:
In `@go.mod`:
- Line 23: The go.mod currently requires k8s.io/apimachinery at v0.36.0-alpha.2
but the replace directive forces it to v0.35.2, causing a mismatch; update the
require entry for k8s.io/apimachinery to match the effective replaced version
(v0.35.2) or change the replace to v0.36.0-alpha.2 so both declarations align.
Locate the require line mentioning k8s.io/apimachinery and the replace clause
that references k8s.io/apimachinery and make them consistent (preferably set the
require to v0.35.2 to reflect the current replace), then run go mod tidy to
verify module graph correctness.
🪄 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: Enterprise
Run ID: 4294a189-973e-4ec0-85de-fe565946f696
⛔ Files ignored due to path filters (291)
go.sumis excluded by!**/*.sumvendor/github.com/distribution/reference/.gitattributesis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/CODE-OF-CONDUCT.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/GOVERNANCE.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/SECURITY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/distribution-logo.svgis excluded by!**/*.svg,!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/normalize.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/reference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/regexp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/sort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/.editorconfigis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/.gitattributesis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/LICENSE.txtis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/Taskfile.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/crypto.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/date.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/defaults.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/dict.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/functions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/list.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/numeric.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/reflect.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/regex.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/strings.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-task/slim-sprig/v3/url.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/compression.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/conn.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/join.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask_safe.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/prepared.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/connection.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/handlers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/priority.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/dictionary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/read.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/write.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/flowrate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/io.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/RELEASING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/config/deprecated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/core_dsl.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/core_dsl_patch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/decorator_dsl.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/deprecated_dsl.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/formatter/colorable_others.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/formatter/colorable_windows.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/formatter/formatter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/automaxprocs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cgroup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cgroups.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cgroups2.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cpu_quota_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cpu_quota_unsupported.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/mountpoint.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/runtime.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/subsys.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/build/build_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/command/abort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/command/command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/command/program.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/boostrap_templates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/bootstrap_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/generate_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/generate_templates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/generators_common.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/compile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/gocovmerge.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/profiles_and_reports.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/run.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/test_suite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/verify_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/labels/labels_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/main.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/ginkgo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/import.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/outline.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/outline_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/run/run_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/unfocus/unfocus_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/delta.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/delta_tracker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/dependencies.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/package_hash.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/package_hashes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/suite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/watch_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo_cli_dependencies.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo_t_dsl.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/around_node.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/counter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/failer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/focus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/global/init.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/group.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/interrupt_handler/interrupt_handler.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/interrupt_handler/sigquit_swallower_unix.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/interrupt_handler/sigquit_swallower_windows.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/node.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/ordering.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor_unix.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor_wasm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor_win.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/client_server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/http_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/http_server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/rpc_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/rpc_server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/server_handler.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_bsd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_unix.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_wasm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_win.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_reporter_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/report_entry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/reporters/gojson.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/reporters/gojson_event_writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/reporters/gojson_reporter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/spec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/spec_context.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/spec_patch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/suite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/suite_patch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/testingtproxy/testing_t_proxy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/tree.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/internal/writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/deprecated_reporter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/gojson_report.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/json_report.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/junit_report.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/reporter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/teamcity_report.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/reporting_dsl.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/table_dsl.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/around_node.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/code_location.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/deprecated_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/deprecation_support.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/enum_support.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/file_filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/flags.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/label_filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/report_entry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/semver_filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/types_patch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/gcustom/make_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/.mailmapis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/.pullapprove.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/LICENSE.docsis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/algorithm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/digest.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/digester.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/verifiers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdimages/cmdimages.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdinfo/info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdlist/list.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runtest.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdupdate/update.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/dbtime/time.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extension.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/environment.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result_writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/spec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/task.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/viewer.htmlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/registry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/component.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/concurrency.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/environment.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/names.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/output.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/suite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/logging.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/parallel.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/junit/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/byte.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/empty.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/int.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/int32.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/int64.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/set.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/chain.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/constantdelay.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/cron.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/logger.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/option.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/spec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/addr.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/addr_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/bpf_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/bridge_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/chain.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/chain_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/class.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/class_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/conntrack_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/conntrack_unspecified.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/devlink_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/filter_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/fou.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/fou_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/fou_unspecified.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/genetlink_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/gtp_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/handle_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/handle_unspecified.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/inet_diag.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/ioctl_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/ipset_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/link.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/link_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vishvananda/netlink/link_tuntap_linux.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (9)
DockerfileMakefilego.modpkg/network/mtu.gotest/Makefiletest/cmd/main.gotest/e2e/cli.gotest/e2e/cno.gotest/e2e/util.go
Contributor
|
@anuragthehatter: An error was encountered searching for bug CORENET-6512 on the Jira server at https://redhat.atlassian.net. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://redhat.atlassian.net/rest/api/2/issue/CORENET-6512": GET https://redhat.atlassian.net/rest/api/2/issue/CORENET-6512 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
openshift-ci-robot
removed
the
jira/valid-reference
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/util.go`:
- Around line 618-699: Tests using MachineSet/Machine helpers fail on MicroShift
because those APIs are absent; add a guard to skip or no-op when running on
MicroShift. Update callers of createMachineSet, deleteMachineSet,
waitForMachineSetRunning, getNodeNameFromMachineSet, and
waitForMachinesDisappear to check a cluster-capability helper (e.g.,
isMicroShift(oc) or hasMachineAPI(oc)) and either call g.Skip or return early
when the Machine API is unavailable; alternatively, add a small wrapper function
(e.g., ensureMachineAPIOrSkip(oc)) that performs the check and use it at the
start of each of the above helper functions so tests are skipped instead of
failing on MicroShift.
🪄 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: Enterprise
Run ID: db43650d-74c3-48a9-85ff-1ba73446ccb7
📒 Files selected for processing (7)
DockerfileMakefiletest/Makefiletest/cmd/main.gotest/e2e/cli.gotest/e2e/cno.gotest/e2e/util.go
🚧 Files skipped from review as they are similar to previous changes (4)
- Dockerfile
- test/cmd/main.go
- test/e2e/cli.go
- test/e2e/cno.go
anuragthehatter
force-pushed
the
cno-ote-framework
branch
from
0cc7cad to
8fd2ea4
Compare
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@anuragthehatter: This pull request references CORENET-6512 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/util.go (1)
61-65: 💤 Low value"Ready" is not a valid pod phase.
Pod phases are
Pending,Running,Succeeded,Failed, andUnknown. The value"Ready"is a condition type, not a phase, socheckPodReadywill never match it. This shouldn't cause immediate failures since"Running"is also in the list, but the dead entry is confusing.Proposed fix
func checkPodReady(oc *CLI, namespace string, podName string) (bool, error) { podOutPut, err := getPodStatus(oc, namespace, podName) - status := []string{"Running", "Ready", "Complete", "Succeeded"} + status := []string{"Running", "Succeeded"} return contains(status, podOutPut), 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/e2e/util.go` around lines 61 - 65, The status slice in checkPodReady contains the invalid phase "Ready"; update the slice in checkPodReady (which calls getPodStatus and uses contains) to use only valid Kubernetes pod phases — e.g., ["Pending","Running","Succeeded","Failed","Unknown"] — or remove "Ready" entirely; if the intent was to test the Ready condition instead of phase, adjust getPodStatus/checkPodReady to inspect pod.conditions for Type==\"Ready\" rather than matching phases.
🤖 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 `@test/e2e/util.go`:
- Around line 479-480: The code currently assumes re.FindAllString(output,
-1)[0] always returns a match which will panic if no IPv4 is present; update the
logic around regexp variable re and the assignment to nodeOVNK8sMgmtIP so you
first capture the slice (e.g., matches := re.FindAllString(output, -1)), check
len(matches) > 0, and handle the no-match case gracefully (return an error, nil,
or a clear sentinel value and log a helpful message) instead of indexing [0]
directly; ensure the calling function receives/propagates the error or handles
the sentinel so tests don’t panic when output contains no IPv4.
- Around line 429-431: The JSONPath used in the node status query assumes the
Ready condition is last; update the
oc.AsAdmin().WithoutNamespace().Run("get").Args(...) call inside the
wait.PollUntilContextTimeout lambda to select the condition by type instead of
index. Replace "-ojsonpath={.status.conditions[-1].status}" with a filter that
finds the Ready condition, e.g.
"-ojsonpath={.status.conditions[?(@.type==\"Ready\")].status}", so the code
reads the Ready condition for nodeName reliably.
---
Nitpick comments:
In `@test/e2e/util.go`:
- Around line 61-65: The status slice in checkPodReady contains the invalid
phase "Ready"; update the slice in checkPodReady (which calls getPodStatus and
uses contains) to use only valid Kubernetes pod phases — e.g.,
["Pending","Running","Succeeded","Failed","Unknown"] — or remove "Ready"
entirely; if the intent was to test the Ready condition instead of phase, adjust
getPodStatus/checkPodReady to inspect pod.conditions for Type==\"Ready\" rather
than matching phases.
🪄 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: Enterprise
Run ID: fbbca07c-b2ab-4b52-a6d3-fcc2196bab48
📒 Files selected for processing (7)
DockerfileMakefiletest/Makefiletest/cmd/main.gotest/e2e/cli.gotest/e2e/cno.gotest/e2e/util.go
🚧 Files skipped from review as they are similar to previous changes (4)
- Dockerfile
- test/cmd/main.go
- test/e2e/cno.go
- test/e2e/cli.go
anuragthehatter
force-pushed
the
cno-ote-framework
branch
from
8fd2ea4 to
6759e76
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
test/e2e/cli.go (2)
188-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound
ocexecutions with a timeout.
exec.Commandcan hang the whole suite on stuckoc debug,must-gather, or rollout calls. Useexec.CommandContextwith a shared timeout and surface deadline expiry distinctly.Suggested direction
+const defaultCommandTimeout = 2 * time.Minute + - cmd := exec.Command(c.execPath, cmdArgs...) + ctx, cancel := context.WithTimeout(context.Background(), defaultCommandTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, c.execPath, cmdArgs...)🤖 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/e2e/cli.go` around lines 188 - 195, Replace the blocking exec.Command call with exec.CommandContext using a context with the shared timeout: create ctx, cancel := context.WithTimeout(context.Background(), sharedTimeout) and defer cancel(), then use exec.CommandContext(ctx, c.execPath, cmdArgs...). Run the command the same way but after cmd.Run() check for ctx.Err() or errors.Is(err, context.DeadlineExceeded) to detect deadline expiry and surface that distinctly (include stdout/stderr in the error logging). Ensure you still assign cmd.Stdout/Stderr to the bytes.Buffers (stdout, stderr) and propagate or log non-timeout errors as before.
118-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
AsAdmin()is still a no-op.
asAdminis copied onto the cloned CLI, butoutput()never reads it, so every admin-only path here runs exactly like the non-admin path. Either honor the flag when building theocinvocation or remove the API to avoid false confidence in cluster-scoped tests.🤖 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/e2e/cli.go` around lines 118 - 123, The cloned CLI's asAdmin flag set in asAdminInternal() is never used by CLI.output(), so admin-only behavior is a no-op; update CLI.output() (or the function that composes the oc invocation, e.g., buildOCArgs/composeOCCommand) to check c.asAdmin and, when true, add the appropriate admin elevation flag (for example add "--as=system:admin" or switch to "oc adm" invocation) into the command args before executing, or alternatively remove the asAdmin/asAdminInternal API if you prefer not to support admin elevation.test/e2e/util.go (3)
316-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the hardcoded public test image.
This helper always pulls from
quay.io, so every test that usescreatePingPodOnNode()breaks in disconnected environments. Make the image configurable or resolve it from the mirrored test-image source used by the suite.
As per coding guidelines, "Flag tests requiring connections to public internet hosts, pulling images from public registries without mirror/internal registry".🤖 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/e2e/util.go` around lines 316 - 319, The helper createPingPodOnNode() embeds a hardcoded public quay.io image which breaks in disconnected environments; make the image source configurable or resolved from the suite's mirrored test-image source instead. Modify createPingPodOnNode() to accept an image parameter (or read from an existing test config/env var such as TEST_IMAGE or a suite mirror resolver function) and replace the hardcoded quay.io reference with that configurable value, and update callers to pass the configured/mirrored image so tests do not pull directly from the public registry.
420-441:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReady-condition polling is brittle for reboot transitions.
conditions[-1]is not guaranteed to be the Ready condition, and a rebooted node can legitimately report Ready=Falsebefore it reportsUnknown. This can make the reboot test wait for the wrong value even when the node is transitioning normally.Suggested fix
func checkNodeStatus(oc *CLI, nodeName string, expectedStatus string) { - var expectedValue string + var expectedValues []string if expectedStatus == "Ready" { - expectedValue = "True" + expectedValues = []string{"True"} } else if expectedStatus == "NotReady" { - expectedValue = "Unknown" + expectedValues = []string{"False", "Unknown"} } else { o.Expect(fmt.Errorf("unsupported node status: %s", expectedStatus)).NotTo(o.HaveOccurred()) } err := wait.PollUntilContextTimeout(context.Background(), 5*time.Second, 15*time.Minute, true, func(ctx context.Context) (bool, error) { - statusOutput, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", nodeName, "-ojsonpath={.status.conditions[-1].status}").Output() + statusOutput, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", nodeName, `-ojsonpath={.status.conditions[?(@.type=="Ready")].status}`).Output() if err != nil { e2e.Logf("Get node status with error : %v", err) return false, nil } e2e.Logf("Expect Node %s in state %v, kubelet status is %s", nodeName, expectedStatus, statusOutput) - if statusOutput != expectedValue { + if !contains(expectedValues, statusOutput) { return false, nil } return true, nil })🤖 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/e2e/util.go` around lines 420 - 441, The checkNodeStatus function currently reads the last condition via jsonpath {.status.conditions[-1].status}, which is brittle; change the kubectl/jsonpath call inside checkNodeStatus (the oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", nodeName, "-ojsonpath=...").Output() call) to explicitly select the Ready condition (e.g. jsonpath that filters where .type=="Ready" such as .status.conditions[?(@.type=="Ready")].status) or fetch the node JSON and parse .status.conditions to locate the condition with Type=="Ready" before comparing its status to the expectedValue so the code checks the Ready condition reliably during reboots.
135-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe rollout poll never matches the normal success output.
oc rollout statusreturns a waiting message or a success message, not both in the same response. Requiring both substrings makes this poll time out after a successful rollout.Suggested fix
- if strings.Contains(status, "rollout to finish") && strings.Contains(status, "successfully rolled out") { + if strings.Contains(status, "successfully rolled out") { e2e.Logf("ovnkube rollout was triggerred and rolled out successfully") return true, nil }🤖 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/e2e/util.go` around lines 135 - 143, The poll incorrectly requires both "rollout to finish" and "successfully rolled out" in the `status` output so it never matches a normal success response; update the predicate inside the wait.PollUntilContextTimeout callback (the code that calls oc.AsAdmin().WithoutNamespace().Run("rollout").Args(...).Output() and inspects the status variable) to treat a successful rollout as when the status contains "successfully rolled out" (i.e., check for that substring alone), and optionally handle the waiting message ("rollout to finish" or "waiting") only for logging rather than as part of the success condition.test/e2e/cno.go (1)
146-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit Machine API/topology prechecks before scaling.
Checking
platformalone is not enough. This path still assumesMachineSet/MachineAPIs and supported node scaling; on MicroShift or other unsupported topologies it will fail before it ever validates NBDB sync. Skip when those APIs are absent or the topology does not support adding nodes.
As per coding guidelines, "Flag tests referencing unavailable APIs ... MachineSet/Machine/MachineHealthCheck" and "Flag tests making unsupported assumptions about multi-node/HA ... node scaling".🤖 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/e2e/cno.go` around lines 146 - 173, Add explicit Machine API and topology prechecks before attempting to create or scale MachineSets: before calling createMachineSet/getInfrastructureName/waitForMachineSetRunning/getNodeNameFromMachineSet, detect whether the Machine/MachineSet APIs exist and the cluster topology supports node scaling (e.g., check for the Machine API resource presence and that checkPlatform(oc) indicates a topology that allows Machine objects); if the APIs are absent or topology is unsupported, call g.Skip(...) with a clear message. Place these checks immediately after determining platform (after platform := checkPlatform(oc)) and before createMachineSet(oc, machineSetName) so the test exits early rather than failing later when MachineSet/Machine APIs or topology are not available.
🤖 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 `@test/e2e/cno.go`:
- Around line 114-125: This test reboots a node but doesn't guard against
single-node (SNO) clusters; before selecting worker from workerList returned by
e2enode.GetReadySchedulableNodes in the "ovsdb-server and northd should not core
dump on node restart" g.It block, add a guard that checks len(workerList.Items)
== 1 and calls g.Skip (or o.Skip) with a message explaining SNO incompatibility;
keep the existing check for <1 nodes but change the logic to explicitly skip
when exactly one schedulable node is present to avoid rebooting the entire
cluster.
In `@test/e2e/util.go`:
- Around line 54-64: The checkPodReady function is using getPodStatus (which
reads .status.phase) and treating "Running" as ready; change checkPodReady to
instead poll the pod's Ready condition
(status.conditions[?(@.type=="Ready")].status == "True") or call the existing
framework/helper that asserts pod readiness so waitPodReady and curlPod2PodPass
don't proceed prematurely; specifically update checkPodReady(oc, namespace,
podName) to query the Ready condition via
oc.AsAdmin().WithoutNamespace().Run("get").Args("pod", "-n", namespace",
podName, "-o=jsonpath=...") or delegate to the helper that checks Ready and
return a boolean based on the Ready condition (and preserve returning an error
for failures).
---
Duplicate comments:
In `@test/e2e/cli.go`:
- Around line 188-195: Replace the blocking exec.Command call with
exec.CommandContext using a context with the shared timeout: create ctx, cancel
:= context.WithTimeout(context.Background(), sharedTimeout) and defer cancel(),
then use exec.CommandContext(ctx, c.execPath, cmdArgs...). Run the command the
same way but after cmd.Run() check for ctx.Err() or errors.Is(err,
context.DeadlineExceeded) to detect deadline expiry and surface that distinctly
(include stdout/stderr in the error logging). Ensure you still assign
cmd.Stdout/Stderr to the bytes.Buffers (stdout, stderr) and propagate or log
non-timeout errors as before.
- Around line 118-123: The cloned CLI's asAdmin flag set in asAdminInternal() is
never used by CLI.output(), so admin-only behavior is a no-op; update
CLI.output() (or the function that composes the oc invocation, e.g.,
buildOCArgs/composeOCCommand) to check c.asAdmin and, when true, add the
appropriate admin elevation flag (for example add "--as=system:admin" or switch
to "oc adm" invocation) into the command args before executing, or alternatively
remove the asAdmin/asAdminInternal API if you prefer not to support admin
elevation.
In `@test/e2e/cno.go`:
- Around line 146-173: Add explicit Machine API and topology prechecks before
attempting to create or scale MachineSets: before calling
createMachineSet/getInfrastructureName/waitForMachineSetRunning/getNodeNameFromMachineSet,
detect whether the Machine/MachineSet APIs exist and the cluster topology
supports node scaling (e.g., check for the Machine API resource presence and
that checkPlatform(oc) indicates a topology that allows Machine objects); if the
APIs are absent or topology is unsupported, call g.Skip(...) with a clear
message. Place these checks immediately after determining platform (after
platform := checkPlatform(oc)) and before createMachineSet(oc, machineSetName)
so the test exits early rather than failing later when MachineSet/Machine APIs
or topology are not available.
In `@test/e2e/util.go`:
- Around line 316-319: The helper createPingPodOnNode() embeds a hardcoded
public quay.io image which breaks in disconnected environments; make the image
source configurable or resolved from the suite's mirrored test-image source
instead. Modify createPingPodOnNode() to accept an image parameter (or read from
an existing test config/env var such as TEST_IMAGE or a suite mirror resolver
function) and replace the hardcoded quay.io reference with that configurable
value, and update callers to pass the configured/mirrored image so tests do not
pull directly from the public registry.
- Around line 420-441: The checkNodeStatus function currently reads the last
condition via jsonpath {.status.conditions[-1].status}, which is brittle; change
the kubectl/jsonpath call inside checkNodeStatus (the
oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", nodeName,
"-ojsonpath=...").Output() call) to explicitly select the Ready condition (e.g.
jsonpath that filters where .type=="Ready" such as
.status.conditions[?(@.type=="Ready")].status) or fetch the node JSON and parse
.status.conditions to locate the condition with Type=="Ready" before comparing
its status to the expectedValue so the code checks the Ready condition reliably
during reboots.
- Around line 135-143: The poll incorrectly requires both "rollout to finish"
and "successfully rolled out" in the `status` output so it never matches a
normal success response; update the predicate inside the
wait.PollUntilContextTimeout callback (the code that calls
oc.AsAdmin().WithoutNamespace().Run("rollout").Args(...).Output() and inspects
the status variable) to treat a successful rollout as when the status contains
"successfully rolled out" (i.e., check for that substring alone), and optionally
handle the waiting message ("rollout to finish" or "waiting") only for logging
rather than as part of the success condition.
🪄 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: Enterprise
Run ID: d579fbee-c1b0-40fc-9d8d-6c2279fe55d8
📒 Files selected for processing (7)
DockerfileMakefiletest/Makefiletest/cmd/main.gotest/e2e/cli.gotest/e2e/cno.gotest/e2e/util.go
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile
- test/cmd/main.go
anuragthehatter
changed the title
Contributor
|
@anuragthehatter: No Jira issue is referenced in the title of this pull request. 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. |
openshift-ci-robot
removed
the
jira/valid-reference
2 tasks
pperiyasamy
reviewed
May 27, 2026
pperiyasamy
left a comment
pperiyasamy
left a comment
Member
There was a problem hiding this comment.
@anuragthehatter can you avoid pushing vendor directory ? because this PR is too big for review. Please refer to ovnk how this can be ignored.
anuragthehatter
force-pushed
the
cno-ote-framework
branch
from
6759e76 to
3a5cf1b
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 `@go.mod`:
- Line 150: The go.mod currently pins the vulnerable dependency
"github.com/moby/spdystream v0.5.0"; update it to a patched release (or add a
replace directive) and re-resolve the module graph: pick a non-vulnerable tag
for github.com/moby/spdystream, run go get
github.com/moby/spdystream@<patched-version> (or add replace
github.com/moby/spdystream => github.com/moby/spdystream <patched-version>),
then run go mod tidy (and go mod vendor if you vendor) and verify with go mod
graph to ensure v0.5.0 is no longer present.
In `@test/e2e/cno.go`:
- Around line 151-154: The test assumes GetReadySchedulableNodes returned at
least one node and directly accesses currentNodeList.Items[0], which can panic;
update the test around GetReadySchedulableNodes, currentNodeList and oldNode to
check that currentNodeList.Items is non-empty (e.g., len(currentNodeList.Items)
> 0) and fail the test with a clear error message or skip/return if it's empty
before assigning oldNode, so you safely handle the no-ready-node case instead of
indexing into an empty slice.
🪄 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: Enterprise
Run ID: 970bdb43-8418-4dfb-9031-eedb64e3e747
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
DockerfileMakefilego.modpkg/network/mtu.gotest/Makefiletest/cmd/main.gotest/e2e/cli.gotest/e2e/cno.gotest/e2e/util.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/network/mtu.go
- Dockerfile
- test/cmd/main.go
- test/e2e/cli.go
- test/e2e/util.go
anuragthehatter
force-pushed
the
cno-ote-framework
branch
5 times, most recently
from
b545a1a to
9391dd3
Compare
anuragthehatter
force-pushed
the
cno-ote-framework
branch
from
9391dd3 to
b6fdfef
Compare
Author
|
/testwith openshift/origin#31218 |
Author
|
/testwith openshift/ovn-kubernetes/master/e2e-aws-ovn-serial openshift/origin#31218 |
Contributor
|
@anuragthehatter, |
anuragthehatter
force-pushed
the
cno-ote-framework
branch
from
b6fdfef to
75584e2
Compare
Author
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-serial openshift/origin#31218 |
Contributor
|
@anuragthehatter: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/8b7cea40-6047-11f1-8641-ad5566a4bfd8-0 |
Contributor
|
@anuragthehatter: 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. |
Contributor
|
@anuragthehatter: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
Contributor
|
@anuragthehatter: An error was encountered. No known errors were detected, please see the full error message for details. Full error message.
unable to get additional pr info from string: openshift/origin/pull#31218: string: openshift/origin/pull#31218 doesn't match expected format: org/repo#number
Please contact an administrator to resolve this issue. |
Contributor
|
@anuragthehatter: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/60210ff0-606c-11f1-9efc-61492c5af2a5-0 |
Author
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-serial openshift/origin#31218 |
Contributor
|
@anuragthehatter: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/cf346f40-607b-11f1-860f-f0487da5a3e9-0 |
Author
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial openshift/origin#31218 openshift/origin#31264 |
Contributor
|
@anuragthehatter: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/30323330-6145-11f1-8085-18a113a858ae-0 |
1 task
Author
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial openshift/origin#31218 openshift/origin#31264 |
Contributor
|
@anuragthehatter: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a0e017d0-6345-11f1-9c80-ac8ee1ba40aa-0 |
Set up the OTE framework and migrate three e2e test cases from openshift-tests-private: - OCP-72817: internalJoinSubnet/internalTransitSwitchSubnet Day 2 config - OCP-51727: ovsdb-server/northd core dump check after node restart - OCP-72028: NBDB sync of join switch/mgmt port IPs for new nodes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial openshift/origin#31218 |
Contributor
|
@anuragthehatter: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c28462b0-6377-11f1-805f-664899d5708f-0 |
anuragthehatter
force-pushed
the
cno-ote-framework
branch
from
75584e2 to
798336c
Compare
Author
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial openshift/origin#31218 |
Contributor
|
@anuragthehatter: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6a411290-63a1-11f1-8717-3fccd4afa0ed-0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
internalJoinSubnetandinternalTransitSwitchSubnetare configurable post-install as a Day 2 operationDetails
The first commit adds vendor dependencies for the OTE framework (openshift-tests-extension, OpenShift ginkgo fork, k8s.io/kubernetes e2e framework). The second commit implements the OTE framework infrastructure and the migrated test cases.
The test binary (
cluster-network-operator-tests-ext) is built and shipped as a gzipped binary in the operator image.Test plan
make build-e2e-testsbuilds successfullytest/bin/cluster-network-operator-tests-ext run-test --names "72817"test/bin/cluster-network-operator-tests-ext run-test --names "51727"test/bin/cluster-network-operator-tests-ext run-test --names "72028"🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Chores
Depends-On: openshift/origin#31218