-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1887488: e2e: node: fix ping tester pod #25688
Conversation
/retest |
2 similar comments
/retest |
/retest |
this
can't be caused by this PR changing how we use an image on an unrelated test. Will report as flake. |
@fromanirh: This pull request references Bugzilla bug 1887488, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
/retest |
/assign @adambkaplan |
@fromanirh: This pull request references Bugzilla bug 1887488, which is valid. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
/retest |
/lgtm |
/cherry-pick release-4.6 |
@fromanirh: once the present PR merges, I will cherry-pick it on top of release-4.6 in a new PR and assign it to you. 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 kubernetes/test-infra repository. |
@@ -24,6 +24,7 @@ import ( | |||
const ( | |||
networkAttachmentAnnotation string = "k8s.v1.cni.cncf.io/networks" | |||
sriovInterfaceName string = "sriov1" | |||
busyboxImage string = "busybox:1.31.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is pulling from DockerHub without a pull secret, and thus can fail to deploy due to a rate limit.
Can registry.redhat.io/ubi8/ubi-minimal
be used instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything which contains ping and it is maintained is fine for this task. I'll research for alternatives.
if cp.Privileged { | ||
isTrue := true | ||
cnt.SecurityContext = &corev1.SecurityContext{ | ||
Privileged: &isTrue, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you need to run a pod that's fully privileged? This bypasses every baked in security check.
If the issue is that on busybox you now need root, you can use runAsUser: 0
in the security context. Not awesome, but better than running a privileged container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. Let me narrow down the privileges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So RunAsUser: 0
is not enough, it seems I need at least CAP_NET_RAW
(which is still too much?)
if I remove Privileged: true
and I replace with RunAsUser: 0
I get:
Nov 24 17:52:36.585: INFO: Error running /usr/local/bin/oc --namespace=e2e-test-topology-manager-xhjcv --kubeconfig=/home/fromani/clusters/cnflab/kubeconfig rsh -n default -c test-0 test-29pj7 ping -c 3 10.56.217.171:
StdOut>
bind: Permission denied
PING 10.56.217.171 (10.56.217.171): 56 data bytes
command terminated with exit code 2
StdErr>
bind: Permission denied
PING 10.56.217.171 (10.56.217.171): 56 data bytes
command terminated with exit code 2
681a143
to
6a964fd
Compare
/retest |
1 similar comment
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash commits, otherwise looks good.
Pin the busybox image setting a known-good tag. It was not done previously because of an oversight, no other reason. Generalize the code which creates the testing pod to make the code more flexible and a bit easier to read. Any public-available, reasonably recent (>= 1.21) busybox image is fine. We use the same image other extended tests (DNS) are already using. This solves the issues for all but one tests: the connectivity test. One of the e2e topology manager tests wants to run a basic network check between two NUMA-aligned pods, to get a basic signal if the network is working. This is done to test that resource alignment is not preventing the network to work. This test is intentionally minimal: proper network check is demanded to other testsuites (e.g. the SRIOV testsuite), but we need some basic coverage in this testsuite and we have this intentional, minimal, overlap. To do the minimal testing, the test what to ping two resource-aligned pod from each other. To do so we need a container image which contains the ping tool and which not require any special privilege. So, this patch requires the pull URL of the network-check image to be supplied using a (documented) environment variable. This way: 1. users can use existing images not part of openshift (quay.io/openshift-kni/cnf-tests) which they may want to use to validate their cluster anyway 2. users can easily opt out from this test (do not supply the env variable to skip the test) 3. in the future we can provide a very simple base image in from openshift itself to fill this role. Bug-Url: https://bugzilla.redhat.com/1887488 Signed-off-by: Francesco Romani <fromani@redhat.com>
6a964fd
to
af3023d
Compare
/retest |
1 similar comment
/retest |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, fromanirh, rphillips 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:
Approvers can indicate their approval by writing |
/test e2e-gcp |
/retest Please review the full test history for this PR and help us cut down flakes. |
13 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
@fromanirh: All pull requests linked via external trackers have merged: Bugzilla bug 1887488 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
@fromanirh: new pull request created: #25729 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 kubernetes/test-infra repository. |
Pin the busybox image setting a known-good tag.
It was not done previously because of an oversight, no other reason.
Generalize the code which creates the testing pod to make the
code more flexible and a bit easier to read.
Last, use privileged pods for ping tests. This is needed because of the busybox images used
by the pinger pods.
Another approach could be to change base image (e.g. using centos images) but considering
the nature of the test and the fact that the busybox image is tiny, and much smaller than the closest
good alternative (centos), this approach seems good enough.
Bug-Url: https://bugzilla.redhat.com/1887488
Signed-off-by: Francesco Romani fromani@redhat.com