CORENET-7206: Multus OTE: add OpenShift Tests Extension framework infrastructure - #303
CORENET-7206: Multus OTE: add OpenShift Tests Extension framework infrastructure#303anuragthehatter wants to merge 11 commits into
Conversation
|
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:
WalkthroughThis PR adds a multus-cni e2e extension, new build targets, OTP test coverage, and Docker packaging for the compressed test binary. ChangesMultus e2e extension
Sequence Diagram(s)sequenceDiagram
participant Makefile
participant TestMakefile
participant Main
participant OpenShiftTestsExtension
participant Cobra
participant Dockerfile
Makefile->>TestMakefile: build-e2e-tests
TestMakefile->>Main: build multus-cni-tests-ext
Main->>OpenShiftTestsExtension: create and register extension specs
Main->>Cobra: execute root command
Dockerfile->>TestMakefile: run build-e2e-tests in rhel9 stage
Dockerfile->>Dockerfile: gzip multus-cni-tests-ext and copy artifact
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
anuragthehatter
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/cmd/main.go`:
- Around line 31-33: The main() function currently uses
panic(fmt.Sprintf("couldn't build extension test specs from ginkgo: %+v",
err.Error())) on a recoverable setup error; replace this panic with a controlled
exit: write a clear error message including err (use err or err.Error()) to
stderr via fmt.Fprintln(os.Stderr, ...) and call os.Exit(nonZeroCode) to
terminate deterministically. Update imports to include "os" if missing and
remove the panic call in test/cmd/main.go so the code path uses the controlled
stderr+os.Exit behavior when the err variable is non-nil.
🪄 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: 6d4d4554-adef-4a1b-b1ba-b8e2f8a3eb43
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
Dockerfile.openshiftMakefilego.modtest/Makefiletest/cmd/main.go
anuragthehatter
changed the title
anuragthehatter
changed the title
1713ca4 to
31bd7e6
Compare
|
@anuragthehatter: This pull request references CORENET-7206 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. |
openshift-ci-robot
added
the
jira/valid-reference
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/otp/multus.go`:
- Around line 68-70: The cleanup paths in the test helpers are ignoring errors,
which can leave stale namespaces or active tcpdump processes; update the
deferred cleanup and exec handling in the relevant test functions (including the
namespace delete and pkill tcpdump calls) to check returned errors instead of
discarding them. Only suppress expected cleanup cases like NotFound, and make
sure the same fix is applied across the referenced cleanup blocks in the test
file so no Go error return is ignored.
- Around line 599-637: The macvlan test pods created in the
ReplicationController are not guaranteed to land on the same node as the
sniffer, which can make the observation flaky on multi-node clusters. Update the
test setup around the `rc`/`ReplicationController` template to label the
`sniff-pod` and add pod affinity or node affinity on the six test pods so they
are scheduled onto the same `kubernetes.io/hostname` as `sniff-pod`. Keep the
fix localized to the `Creating 6 test pods with macvlan secondary network` block
and the pod template used by `Create`.
- Around line 720-721: The tcpdump count assertions are comparing the trimmed
stdout as a string, so unexpected non-numeric output can still pass; update the
Neighbor Advertisement and the other affected count checks to parse the result
numerically after strings.TrimSpace using strconv.Atoi, then assert the parsed
count is greater than zero. Keep the fix localized around the existing
stdout.String() handling and the o.Expect checks so the logic validates actual
packet counts instead of string content.
- Line 27: The Ginkgo suite name for the OTP Multus specs is missing the suite
qualifier token used by test/cmd/main.go selection. Update the g.Describe
declaration for OTP Multus to include the “[Suite:openshift/multus-cni]”
qualifier in the name so these specs are picked up by the new extension suite
selector.
- Around line 34-35: The test setup is using context.Background() for
Kubernetes-related API and exec calls, which prevents cancellation and can let a
stuck request outlive the surrounding Eventually timeout. Update the test
helpers in multus setup to use a bounded context instead of a background
context, and thread that context through the Kubernetes call sites so
cancellations/timeouts are respected; focus on the ctx initialization and any
functions in this flow that consume it.
🪄 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: 889abbbd-989e-44df-8adb-08b3f83f47c0
📒 Files selected for processing (2)
test/cmd/main.gotest/otp/multus.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cmd/main.go
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/otp/multus.go`:
- Line 927: The ping command construction in the multus test helpers uses
/bin/sh -c with pod-derived input, which should be removed. Update the ping
command setup at the referenced pingCmd assignments in multus.go to pass ping
and its flags as direct argv entries with the target IP as a separate argument,
and apply the same fix to the other matching call sites so no shell
interpolation is used for pod annotation values.
- Line 758: The new Ginkgo specs in multus test suite are missing the standard
OTP informing metadata in their It names. Update the affected It declarations to
include the same [OTP][informing] prefix used by the rest of this suite so these
LifecycleInforming tests are tagged consistently; use the existing test names in
the multus spec block to locate the two new cases.
- Around line 811-817: The node selection logic in the Multus OTP tests is too
restrictive because it assumes a worker-labeled node exists. Update the node
lookup in the affected test helpers (for example the block that uses
clientset.CoreV1().Nodes().List in multus.go and the related spots noted in the
review) to select any Ready, schedulable node instead of filtering on
node-role.kubernetes.io/worker, then continue using that chosen node for
pinning/affinity. Keep the existing assertions, but base them on readiness and
schedulability so the tests still validate bridge isolation on SNO and other
non-HA topologies.
- Line 895: The json.Unmarshal calls in multus.go cannot typecheck because
encoding/json is not imported. Add the encoding/json import alongside the other
imports in test/otp/multus.go, making sure the import is present for all four
unmarshaling sites that use json.Unmarshal.
🪄 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: 88083e4a-8848-4dc7-bea8-a7720bc96ed4
📒 Files selected for processing (1)
test/otp/multus.go
…IPv6 exclude ranges - [76652]: Dummy CNI plugin support - [66876]: Whereabouts dual-stack IPAM - [69947]: Macvlan Unsolicited Neighbor Advertisements - [80524]: Basic port isolation with bridge CNI - [80525]: Mixed port isolation networks These tests were moved as they are pure Multus CNI tests without ovn-kubernetes dependencies. Total OTP tests now: 9 (2 security + 5 networking tools + 2 port isolation) Related: openshift/multus-cni#303
|
@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. |
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn openshift/origin#31231 |
|
@anuragthehatter: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
/test-with openshift/origin#31231 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn openshift/origin#31231 |
|
@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/da8e4da0-7036-11f1-8f14-4a9942044fcb-0 |
|
@SachinNinganure lets check above /payload-job-with-prs job results and debug if you see any failures in any of your cases going in via this PR |
|
@anuragthehatter The payload job build failures look like a CI infrastructure issue, similar to the recent tests-private:4.22 build failures that was fixed (Go version mismatch in builder images). |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn openshift/origin#31231 |
|
@SachinNinganure: 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. |
|
/retest |
1 similar comment
|
/retest |
| "k8s.io/client-go/tools/remotecommand" | ||
| ) | ||
|
|
||
| var _ = g.Describe("[JIRA:Networking][OTP][sig-network] OTP Multus", func() { |
There was a problem hiding this comment.
Lets add openshift/conformance.serial or parallel to your g.Describe like
https://github.com/openshift/cluster-network-operator/pull/3015/changes#diff-30af132467d7a9df230f52564f78a0256952bbca5c96d190650b7cf6610280caR18
|
@SachinNinganure Your tests are not being running in any jobs IIUC. Your tests are not picked up due to |
16a9333 to
6cb94b3
Compare
|
/test all |
Signed-off-by: Sachin Ninganure <sninganu@sninganu.pnq.csb>
Create test/go.mod as an independent module so test dependencies (OTE, cobra, remotecommand) don't pollute the top-level vendor directory. Vendor is generated at build time via go mod vendor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…IPv6 exclude ranges - [76652]: Dummy CNI plugin support - [66876]: Whereabouts dual-stack IPAM - [69947]: Macvlan Unsolicited Neighbor Advertisements - [80524]: Basic port isolation with bridge CNI - [80525]: Mixed port isolation networks These tests were moved as they are pure Multus CNI tests without ovn-kubernetes dependencies. Total OTP tests now: 9 (2 security + 5 networking tools + 2 port isolation) Related: openshift/multus-cni#303
2226e2e to
eaf804a
Compare
openshift-ci
Bot
removed
the
needs-rebase
Hello @anuragthehatter
should we open a que |
I guess nothing we can do at this point. Not due to our PR fixes. When it comes for final approval, maintainers can decide. The issues seems very at LOW level |
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-ovn-ipv6 openshift/origin#31231 |
|
@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/e0a1cc50-9041-11f1-9060-caa2412bec4a-0 |
| // Load kubeconfig | ||
| loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() | ||
| configOverrides := &clientcmd.ConfigOverrides{} | ||
| kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides) |
There was a problem hiding this comment.
Use exutil.CLI which handles kubeconfig loading, namespace lifecycle, and cleanup instead of using kubernetes.Clientset and rest.Config manually via clientcmd.NewDefaultClientConfigLoadingRules()
There was a problem hiding this comment.
Thank you. Adding the changes! While c_lientcmd.NewDefaultClientConfigLoadingRules()_ light weight.
Also added github.com/openshift/origin as a dependency in test/go.mod and all the dependencies....
|
@SachinNinganure We decided on using ote instead of otp. Rename the folder and package accordingly. |
| build-e2e-tests: | ||
| @echo "Building multus-cni-tests-ext binary..." | ||
| $(MAKE) -C test build | ||
|
No newline at end of file |
There was a problem hiding this comment.
Could you get rid of the extra character below.
|
|
||
| // Give tcpdump time to start capturing | ||
| // tcpdump needs extra time after pod reaches Running to initialize and start listening | ||
| time.Sleep(20 * time.Second) |
There was a problem hiding this comment.
Could use wait/polling pattern instead of hard coded sleep here and other places in the test.
|
|
||
| // High-57589: Whereabouts CNI Timeout with Large Exclude Range | ||
| g.It("[JIRA:Networking][OTP] 57589-should handle large IPv6 exclude ranges without timeout", func() { | ||
| const testNS = "test-whereabouts-57589" |
There was a problem hiding this comment.
append a random suffix to namespace name for all the tests to avoid failures or collisions
| ext.AddSuite(e.Suite{ | ||
| Name: "openshift/multus-cni", | ||
| Parents: []string{ | ||
| "openshift/conformance/parallel", |
There was a problem hiding this comment.
74933 is marked [Serial][Disruptive] but registered as parallel. It should be serial as it is Disruptive.
There was a problem hiding this comment.
The [Serial] in the test name isn't being picked up as a ginkgo label — labels are empty for all tests. Added g.Label("Serial") to the test 74933 declaration
| time.Sleep(20 * time.Second) | ||
|
|
||
| g.By("Creating 6 test pods with macvlan secondary network") | ||
| rc := &corev1.ReplicationController{ |
There was a problem hiding this comment.
Instead of replicationcontroller use deployment or replicaset. rc is legacy API. Test 66876 uses deployment.
| } | ||
| o.Expect(targetNode).NotTo(o.BeEmpty(), "Should have at least one Ready, schedulable node") | ||
|
|
||
| rc := &corev1.ReplicationController{ |
There was a problem hiding this comment.
Instead of replicationcontroller use deployment or replicaset. rc is legacy API.
Test 66876 uses deployment.
| Name: "green-test-pod", | ||
| Namespace: testNS, | ||
| }, | ||
| Spec: corev1.ReplicationControllerSpec{ |
There was a problem hiding this comment.
Same use deployment or replicaset instead of rc
| o.Expect(newIP).To(o.Equal(originalIP), | ||
| "Pod %s IP changed after reboot: was %s, now %s (whereabouts reconciliation failed)", | ||
| podName, originalIP, newIP) | ||
| g.By(fmt.Sprintf("✓ Pod %s kept the same IP: %s", podName, newIP)) |
There was a problem hiding this comment.
| g.By(fmt.Sprintf("✓ Pod %s kept the same IP: %s", podName, newIP)) | |
| g.By(fmt.Sprintf("Pod %s kept the same IP: %s", podName, newIP)) |
Avoid using unicde character as it may create issue in logs
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(len(nodes.Items)).To(o.BeNumerically(">", 0), "Should have at least one node") | ||
|
|
||
| // Find first Ready, schedulable node (no taints blocking scheduling) |
There was a problem hiding this comment.
Could this be a function as this code is repeated in tests
…olling
- Switch from manual clientcmd kubeconfig to exutil.NewCLI with
AdminConfig/AdminKubeClient (adds openshift/origin dependency)
- Add framework initialization (provider.go, types.go, WithCleanup)
required for exutil.CLI to work with OTE run-suite
- Rename package and folder from otp to ote
- Convert 3 ReplicationControllers to Deployments (legacy API)
- Replace time.Sleep with polling/Eventually patterns
- Add random suffix to all test namespace names
- Add Serial label and serial suite for test 74933
- Extract getSchedulableNode helper to reduce duplication
- Remove unicode character from log output
- Fix Makefile trailing whitespace
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-ovn-ipv6 openshift/origin#31231 |
|
@SachinNinganure: 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: #31231: string: #31231 doesn't match expected format: org/repo#number
Please contact an administrator to resolve this issue. |
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-ovn-ipv6 openshift/origin#31231 |
|
@SachinNinganure: 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/66f10b40-90a6-11f1-8fd1-0b1541d37ab6-0 |
|
@SachinNinganure: 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/8a475ea0-90a6-11f1-9160-9254f9821791-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn openshift/origin #31231 |
|
@SachinNinganure: 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: string: openshift/origin doesn't match expected format: org/repo#number
Please contact an administrator to resolve this issue. |
|
/test all |
|
@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. |
5 participants
Summary
test/cmd/main.go), build Makefile (test/Makefile), and Dockerfile integrationLifecycleInformingTest plan
multus-cni-tests-extbinary successfully/usr/bin/multus-cni-tests-ext.gz🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores