USHIFT-2226: Test Multus - Bridge CNI #3137

pmtk · 2024-03-11T16:15:25Z

No description provided.

openshift-ci-robot · 2024-03-11T16:15:29Z

@pmtk: This pull request references USHIFT-2226 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 task to target the "4.16.0" version, but no target version was set.

In response to this:

/cc

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 · 2024-03-11T16:15:44Z

@pmtk: GitHub didn't allow me to request PR reviews from the following users: pmtk.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc

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/test-infra repository.

pmtk · 2024-03-12T06:41:26Z

/test ?

openshift-ci · 2024-03-12T06:41:30Z

@pmtk: The following commands are available to trigger required jobs:

/test images
/test metal-periodic-test
/test metal-periodic-test-arm
/test microshift-metal-cache
/test microshift-metal-cache-arm
/test microshift-metal-tests
/test microshift-metal-tests-arm
/test ocp-conformance-rhel-eus
/test ocp-conformance-rhel-eus-arm
/test test-rpm
/test test-unit
/test verify

The following commands are available to trigger optional jobs:

/test test-rebase

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-microshift-main-images
pull-ci-openshift-microshift-main-microshift-metal-tests
pull-ci-openshift-microshift-main-microshift-metal-tests-arm
pull-ci-openshift-microshift-main-test-unit
pull-ci-openshift-microshift-main-verify

In response to this:

/test ?

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/test-infra repository.

pmtk · 2024-03-12T06:41:43Z

/test metal-periodic-test metal-periodic-test-arm

openshift-ci-robot · 2024-03-12T09:08:32Z

@pmtk: This pull request references USHIFT-2226 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 task to target the "4.16.0" version, but no target version was set.

In 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.

pmtk · 2024-03-12T09:11:07Z

/cc @pliurh @s1061123 @zshi-redhat @dhensel-rh @jogeo

dhellmann

This looks good. One suggestion about a potential race condition, but I see you're trying to work around that so I'll leave it up to you and other reviewers to decide how risky it is.

dhellmann · 2024-03-13T13:58:00Z

test/suites/optional/multus.robot

+    ...    work correctly if there is no pre-existing bridge
+    ...    interface - it needs to be created.
+    [Setup]    Run Keywords
+    ...    Make Sure Interface Does Not Exist    ${BRIDGE_INTERFACE}


Using the same interface for both tests might be a little brittle, especially if we find a race condition. Can we use separate interfaces to avoid that?

Yes, though it'll need two (or single templated) NetworkAttachmentDefinition and Pod manifests.
We'll have similar problem with ipvlan / macvlan tests, because:

A single master interface can not be enslaved by both macvlan and ipvlan. (src)

Perhaps we want to add more interface to the VM to avoid that?

Would we need more than 2? The bridge test doesn't need a new one, right?

No, bridge plugin doesn't have this exclusivity rule. Other solution is to (probably) reboot the host after the test, so the other *vlan can enslave main interface

Which approach seems like it will be most reliable?

I don't have data on that, but adding two nics will be faster (if we keep random order of the tests, then we'll have two reboots).

Adding the extra NICs sounds like a good place to start, then.

Made changes to the bridge tests. I decided to duplicate the files as we don't have any templating of such sort yet in RF, so it felt like over-engineering and waste of time.

Regarding the two interfaces for macvlan/ipvlan - that's for another PR(s).

dhellmann

You've addressed my comments, so I'm happy with this version. I had one philosophical question inline, but it shouldn't block merging this.

You have listed other reviewers, so I'll wait to lgtm to give them a chance to review it.

dhellmann · 2024-03-15T14:23:56Z

test/suites/optional/multus.robot

+*** Test Cases ***
+Pre-Existing Bridge Interface
+    [Documentation]    Test verifies if Bridge CNI plugin will work correctly with pre-existing interface.
+    [Setup]    Run Keywords


You don't need to change it, but I'm curious why there are so many individual keywords in the setup for this case. Why not include them in the test body, or write a separate keyword?

Why not include them in the test body

I was being literal - test setup in [Setup]

or write a separate keyword?

I didn't like how this requires a jump to different location in the file - this way everything is single place.

So I think I just felt like it :D It's not perfect though, this keyword (in this or future PR) is maxing out the length, but separate keyword was "context jump" and writing general function for both Bridge tests was quite ugly and required some mental processing on what happens with which arguments.

It can definitely be hard to find a balance between inline and separate code blocks. That max keyword aspect to the linter is a nudge towards what the linter author thought we should be doing, but there have definitely been cases where we overrode the rule. Maybe we should have written more of those cases in Python? I don't know. As I said, there's no need to change anything here, I was just curious about the evolution of the code.

Maybe we should have written more of those cases in Python?

Perhaps, but python function don't support so great support for log.html. We'll see in the future

s1061123 · 2024-03-26T05:33:12Z

Regarding of CNI config in net-attach-def, it looks good to me.

pliurh · 2024-03-26T07:48:14Z

LGTM

ggiguash · 2024-03-26T09:44:10Z

test/assets/multus/bridge-preexisting-pod.yaml

+  - name: hello-microshift
+    image: quay.io/microshift/busybox:1.36
+    command: ["/bin/sh"]
+    args: ["-c", "while true; do echo -ne \"HTTP/1.0 200 OK\r\nContent-Length: 16\r\n\r\nHello MicroShift\" | nc -l -p 8080 ; done"]


Do we want to add "sleep" here not to burn CPU?

This doesn't spin furiously - nc blocks until a client arrives, so the loop has a many iterations as many GETs.

ggiguash · 2024-03-26T09:44:43Z

test/assets/multus/bridge-pod.yaml

+  - name: hello-microshift
+    image: quay.io/microshift/busybox:1.36
+    command: ["/bin/sh"]
+    args: ["-c", "while true; do echo -ne \"HTTP/1.0 200 OK\r\nContent-Length: 16\r\n\r\nHello MicroShift\" | nc -l -p 8080 ; done"]


Do we want to add "sleep" here not to burn CPU?

This doesn't spin furiously - nc blocks until a client arrives, so the loop has a many iterations as many GETs.

ggiguash · 2024-03-26T09:47:07Z

test/suites/optional/multus.robot

+${PREEXISTING_BRIDGE_POD_NAME}      test-bridge-preexisting
+${PREEXISTING_BRIDGE_IP}            10.10.1.10/24
+
+${NS}                               default


Is there a special reason we're not using a dedicated namespace for this test?

Not really, let me have a look

ggiguash · 2024-03-26T10:04:40Z

test/suites/optional/multus.robot

+    ...    Oc Delete    -f ${nad}
+    Command Should Work    ip link delete ${if}
+
+Curl Pod Over Local Interface


Nit: Curl is an implementation detail. Should we call it Connect Pod or something?

Connect To Pod Over Local Interface? Because in future PRs there's Curl Pod From The Hypervisor so we need to be more specific

ggiguash · 2024-03-26T10:07:20Z

test/suites/optional/multus.robot

+Interface Should Not Exist
+    [Documentation]    Verifies that network interface does not exist.
+    [Arguments]    ${if}
+    ${stdout}    ${rc}=    Execute Command
+    ...    ip link show ${if}
+    ...    sudo=True    return_rc=True
+    Should Be Equal As Integers    ${rc}    1


Feels as if we would benefit from Command Should Not Work keyword.

ggiguash · 2024-03-26T10:47:27Z

/lgtm

openshift-ci · 2024-03-26T10:47:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggiguash, pmtk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ggiguash,pmtk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-03-26T11:47:14Z

@pmtk: all tests passed!

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 11, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2024

pmtk changed the title ~~WIP USHIFT-2226: Test Multus - Bridge CNI~~ USHIFT-2226: Test Multus - Bridge CNI Mar 12, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 12, 2024

openshift-ci bot requested review from dhensel-rh, jogeo, pliurh, s1061123 and zshi-redhat March 12, 2024 09:11

dhellmann reviewed Mar 13, 2024

View reviewed changes

pmtk force-pushed the multus/test/bridge branch from c5e9038 to 7cd55fb Compare March 14, 2024 09:24

USHIFT-2226: Test Multus - Bridge CNI

dd38d86

pmtk force-pushed the multus/test/bridge branch from 7cd55fb to dd38d86 Compare March 14, 2024 09:33

pmtk mentioned this pull request Mar 14, 2024

USHIFT-2228: Test Multus - macvlan CNI #3155

Merged

dhellmann reviewed Mar 15, 2024

View reviewed changes

ggiguash reviewed Mar 26, 2024

View reviewed changes

pmtk added 3 commits March 26, 2024 11:23

introduce Command Should Fail keyword

5dac32a

rename 'Curl Pod...' to 'Connecto To Pod...'

2a95a68

use dynamic namespace

f1cd4f1

openshift-ci bot assigned ggiguash Mar 26, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2024

openshift-merge-bot bot merged commit ec8258e into openshift:main Mar 26, 2024
6 checks passed

pmtk deleted the multus/test/bridge branch March 26, 2024 14:30

USHIFT-2226: Test Multus - Bridge CNI #3137

USHIFT-2226: Test Multus - Bridge CNI #3137

Conversation

pmtk commented Mar 11, 2024 • edited

openshift-ci-robot commented Mar 11, 2024 • edited by openshift-ci bot

openshift-ci bot commented Mar 11, 2024

pmtk commented Mar 12, 2024

openshift-ci bot commented Mar 12, 2024

pmtk commented Mar 12, 2024

openshift-ci-robot commented Mar 12, 2024 • edited by openshift-ci bot

pmtk commented Mar 12, 2024

dhellmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhellmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1061123 commented Mar 26, 2024

pliurh commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggiguash commented Mar 26, 2024

openshift-ci bot commented Mar 26, 2024

openshift-ci bot commented Mar 26, 2024

pmtk commented Mar 11, 2024 •

edited

openshift-ci-robot commented Mar 11, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Mar 12, 2024 •

edited by openshift-ci bot