Add tap plugin test #27737

mlguerrero12 · 2023-02-20T10:24:14Z

Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros marguerr@redhat.com

openshift-ci · 2023-02-20T10:29:27Z

Hi @mlguerrero12. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

mmirecki · 2023-02-20T11:34:06Z

test/extended/networking/tap.go

+			ns,
+			nadName,
+			fmt.Sprintf(`{"cniVersion":"0.4.0","name":"%s","type": "tap",
+"selinuxcontext": "system_u:system_r:container_t:s0", "ipam": {}}`, nadName),


Maybe make a variable out of the config,move to line 25, and format it for better readability?

mmirecki · 2023-02-20T12:08:40Z

lgtm

mmirecki · 2023-02-21T11:00:15Z

/lgtm

mmirecki · 2023-02-21T11:01:19Z

/ok-to-test

cgoncalves · 2023-02-23T11:09:02Z

test/extended/networking/tap.go

+	})
+
+	g.AfterEach(func() {
+		// Disable container_use_devices selinux boolean.


In BeforeEach, the tun module is loaded (if not already) but here it is not unloaded. Should it be?
I guess it depends if something else on the system is using it.

It is not loaded. The test always fails without it. I didn't unload it because it always complains that the module is in use even after removing the tap interface. So, I figured it is safe to leave it there.

It should not be that hard to implement.
The BeforEach DS should mount a dir on the node, and note whether the bool has been set:
sh -c 'getsebool container_use_devices > /host/tmp/initial_state

and the AfterEach DS would then revert the value only if the content of this file is 0.

cgoncalves · 2023-02-23T11:12:53Z

test/extended/networking/tap.go

+	g.AfterEach(func() {
+		// Disable container_use_devices selinux boolean.
+		_, err := exutil.ExecCommandOnMachineConfigDaemon(f.ClientSet, oc, worker, []string{
+			"sh", "-c", "nsenter --mount=/proc/1/ns/mnt -- sh -c 'setsebool container_use_devices 0'",


This SELinux boolean may have been already set as a dependency of some service/app on the host. Blindly disabling it might have unintended consequences. Perhaps check if it's already set (in BeforeEach) and only disable it (in AfterEach) if it was not.

Test always fails without it. I know that it is a good practice to do what you're suggesting but I wonder if it is worth doing it in this case. No other tests modify selinux booleans.

I don't have a strong opinion here so either way is fine.

Will it be wrong to leave it on just to see if it collides with other services/apps? This way, we would be able to detect what cannot be run together with the tap plugin.

That's a good point.

done, let's see what we get

@cgoncalves, I decided to follow your suggestion. Please review again.

cgoncalves · 2023-02-23T13:44:35Z

/lgtm

mlguerrero12 · 2023-02-23T17:45:53Z

/retest

Test for the following PR: containernetworking/plugins#832 Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>

mlguerrero12 · 2023-02-27T15:11:55Z

/retest

mlguerrero12 · 2023-02-27T19:40:04Z

/retest-required

mlguerrero12 · 2023-02-27T22:30:19Z

/retest

mlguerrero12 · 2023-02-28T11:46:28Z

@deads2k, @bparees, could you please review/approve this pr?

bparees · 2023-02-28T18:52:02Z

/approve

cgoncalves · 2023-03-01T08:42:16Z

/lgtm

openshift-ci · 2023-03-01T08:43:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, cgoncalves, mlguerrero12, mmirecki

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:

~~test/OWNERS~~ [bparees]

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

openshift-ci · 2023-03-01T11:17:20Z

@mlguerrero12: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-ovn-etcd-scaling	`b461281`	link	false	`/test e2e-gcp-ovn-etcd-scaling`
ci/prow/e2e-aws-ovn-upgrade	`b461281`	link	false	`/test e2e-aws-ovn-upgrade`
ci/prow/e2e-azure-ovn-etcd-scaling	`b461281`	link	false	`/test e2e-azure-ovn-etcd-scaling`
ci/prow/e2e-aws-ovn-single-node-upgrade	`b461281`	link	false	`/test e2e-aws-ovn-single-node-upgrade`
ci/prow/e2e-vsphere-ovn-etcd-scaling	`b461281`	link	false	`/test e2e-vsphere-ovn-etcd-scaling`

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.

mlguerrero12 · 2023-03-01T11:30:13Z

/retest-required

mlguerrero12 force-pushed the taptests branch from a64c5bf to f1a8bc9 Compare February 20, 2023 10:26

mlguerrero12 changed the title ~~Add tap test~~ Add tap plugin test Feb 20, 2023

openshift-ci bot requested review from dcbw and kevinrizza February 20, 2023 10:29

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 20, 2023

mmirecki reviewed Feb 20, 2023

View reviewed changes

mlguerrero12 force-pushed the taptests branch from f1a8bc9 to d83eb5d Compare February 20, 2023 11:47

openshift-ci bot assigned mmirecki Feb 21, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2023

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 21, 2023

mlguerrero12 force-pushed the taptests branch from d83eb5d to 95f7e41 Compare February 22, 2023 16:43

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2023

mlguerrero12 force-pushed the taptests branch 3 times, most recently from 67c2551 to 12114da Compare February 23, 2023 02:20

cgoncalves reviewed Feb 23, 2023

View reviewed changes

openshift-ci bot assigned cgoncalves Feb 23, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023

mlguerrero12 force-pushed the taptests branch from 12114da to e37ba3f Compare February 23, 2023 13:47

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023

Add tap plugin test

b461281

Test for the following PR: containernetworking/plugins#832 Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>

mlguerrero12 force-pushed the taptests branch from e37ba3f to b461281 Compare February 27, 2023 14:53

cgoncalves approved these changes Feb 28, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2023

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

openshift-merge-robot merged commit 6d69467 into openshift:master Mar 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tap plugin test #27737

Add tap plugin test #27737

mlguerrero12 commented Feb 20, 2023 •

edited

openshift-ci bot commented Feb 20, 2023

mmirecki Feb 20, 2023

mlguerrero12 Feb 20, 2023

mmirecki commented Feb 20, 2023

mmirecki commented Feb 21, 2023

mmirecki commented Feb 21, 2023

cgoncalves Feb 23, 2023

mlguerrero12 Feb 23, 2023

mmirecki Feb 23, 2023

cgoncalves Feb 23, 2023

mlguerrero12 Feb 23, 2023

cgoncalves Feb 23, 2023

mlguerrero12 Feb 23, 2023

cgoncalves Feb 23, 2023

mlguerrero12 Feb 23, 2023

mlguerrero12 Feb 27, 2023

cgoncalves commented Feb 23, 2023

mlguerrero12 commented Feb 23, 2023

mlguerrero12 commented Feb 27, 2023

mlguerrero12 commented Feb 27, 2023

mlguerrero12 commented Feb 27, 2023

mlguerrero12 commented Feb 28, 2023

bparees commented Feb 28, 2023

cgoncalves commented Mar 1, 2023

openshift-ci bot commented Mar 1, 2023

openshift-ci bot commented Mar 1, 2023 •

edited

mlguerrero12 commented Mar 1, 2023

Add tap plugin test #27737

Add tap plugin test #27737

Conversation

mlguerrero12 commented Feb 20, 2023 • edited

openshift-ci bot commented Feb 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmirecki commented Feb 20, 2023

mmirecki commented Feb 21, 2023

mmirecki commented Feb 21, 2023

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

cgoncalves commented Feb 23, 2023

mlguerrero12 commented Feb 23, 2023

mlguerrero12 commented Feb 27, 2023

mlguerrero12 commented Feb 27, 2023

mlguerrero12 commented Feb 27, 2023

mlguerrero12 commented Feb 28, 2023

bparees commented Feb 28, 2023

cgoncalves commented Mar 1, 2023

openshift-ci bot commented Mar 1, 2023

openshift-ci bot commented Mar 1, 2023 • edited

mlguerrero12 commented Mar 1, 2023

mlguerrero12 commented Feb 20, 2023 •

edited

openshift-ci bot commented Mar 1, 2023 •

edited