Add a check for crio service before starting node e2e tests #94760

harche · 2020-09-14T06:26:44Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:
None

Does this PR introduce a user-facing change?:

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

k8s-ci-robot · 2020-09-14T06:26:52Z

Hi @harche. Thanks for your PR.

I'm waiting for a kubernetes 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.

harche · 2020-09-14T06:27:57Z

/assign @ConnorDoyle

oomichi

/cc @oomichi

oomichi · 2020-09-14T21:40:27Z

test/e2e_node/remote/ssh.go

@@ -70,7 +70,8 @@ func GetHostnameOrIP(hostname string) string {
 		host = ip
 	}

-	if *sshUser == "" {
+	// Do no ignore the KUBE_SSH_USER if it's set explicitly
+	if os.Getenv("KUBE_SSH_USER") != "" {


I think the specified sshUser should be used instead of the env variable if specifying --ssh-user option.

@oomichi Thanks for your reply. For the Fedora CoreOS host, the only user available on host is core. We have seen failures like,

prow@104.199.120.168: Permission denied (publickey,gssapi-keyex,gssapi-with-mic)

The prow user doesn't exist on the Fedora CoreOS. In fact, KUBE_SSH_USER was introduced to handle Fedora CoreOS hosts (which will be used extensively for CRI-O testing as well as in future RuntimeClass tests)

This PR doesn't prevent the existing flow, it's just that for Fedora CoreOS we want to make sure we use the username that we know works.

cc @dims

@harche Thanks for your response. I see your point.
I guess that comes from https://github.com/kubernetes/test-infra/blob/master/kubetest/e2e.go#L575 which gets the specified username on --ssh-user option from the env value USER.
I feel it is better to get the username from the env value KUBE_SSH-USER if existing. If not existing, get the username from the env value USER as current implementation.

I'd like to get help from @BenTheElder for the above idea.

I agree that if --ssh-user is set we should respect that.
It also seems like https://github.com/kubernetes/test-infra/blob/feccbf60454db394d955e00977a9e2829712f050/kubetest/e2e.go#L575 could handle KUBE_SSH_USER overridding USER

kubernetes/test-infra#19273 making kubetest/e2e.go handle KUBE_SSH_USER overridding USER.

cc @oomichi @BenTheElder

dims · 2020-09-17T12:56:10Z

/ok-to-test

dims · 2020-09-17T12:57:14Z

@oomichi let's unblock the CRI-O folks as we know this works. We can log a follow up issue to tweak if needed. How does that sound?

harche · 2020-09-17T13:57:02Z

/test pull-kubernetes-conformance-kind-ipv6-parallel

oomichi · 2020-09-17T22:11:08Z

@oomichi let's unblock the CRI-O folks as we know this works. We can log a follow up issue to tweak if needed. How does that sound?

As you know, for example kubectl tries reading the specified kubeconfig at first. If not specified, the env value KUBECONFIG is read. Such behavior seems pretty straightforward and that is the same as the existing one.
And current PR changes the above behavior like makes the env value stronger than the specified option value.
I'd like to see opinions from other people.

neolit123 · 2020-09-18T02:03:13Z

test/e2e_node/remote/ssh.go

@@ -70,7 +70,8 @@ func GetHostnameOrIP(hostname string) string {
 		host = ip
 	}

-	if *sshUser == "" {
+	// Do no ignore the KUBE_SSH_USER if it's set explicitly
+	if os.Getenv("KUBE_SSH_USER") != "" {
 		*sshUser = os.Getenv("KUBE_SSH_USER")


as discussed above the flag should take precedence over the environment variable.

please change the logic to something like:

var user string if *sshUser != "" { user = *sshUser goto add_user } if os.Getenv("KUBE_SSH_USER") != "" { user = os.Getenv("KUBE_SSH_USER") goto add_user } return host add_user: return fmt.Sprintf("%s@%s", user, host)

or ....

if *sshUser != "" { return fmt.Sprintf("%s@%s", *sshUser, host) } user := os.Getenv("KUBE_SSH_USER") if user != "" { return fmt.Sprintf("%s@%s", user, host) } return host

Signed-off-by: Harshal Patil <harpatil@redhat.com>

harche · 2020-09-18T12:44:25Z

@oomichi @BenTheElder @dims based on the feedback, I have created kubernetes/test-infra#19273 to set the ssh username.

The only change in this PR now is just to add the check to see if the remote node is running crio.service or not.

Thanks everyone for taking time out for reviewing this PR.

neolit123

/lgtm

neolit123 · 2020-09-18T13:11:42Z

i've commented on the test-infra PR.
/priority important-longterm

oomichi · 2020-09-18T16:06:12Z

Thanks for your work @harche

/lgtm

dims · 2020-09-20T03:04:13Z

/approve
/lgtm
/milestone v1.20

harche · 2020-09-20T11:34:34Z

/assign @ConnorDoyle

harche · 2020-09-20T11:35:31Z

/assign @ConnorDoyle

harche · 2020-09-20T11:38:41Z

/approve
/lgtm
/milestone v1.20

Thanks @dims, it seem for some reason the approve label is not getting assigned.

dims · 2020-09-20T15:24:42Z

/assign @sjenning @dashpole

dims · 2020-09-20T15:25:16Z

@harche i don't have approval rights for this directory :)

harche · 2020-09-21T05:17:50Z

@harche i don't have approval rights for this directory :)

oh ok. @sjenning do you have the approval rights?

neolit123 · 2020-09-21T12:02:27Z

Ideally someone from #sig-node (k8s slack)should approve.

sjenning · 2020-09-21T14:48:30Z

/approve

k8s-ci-robot · 2020-09-21T14:49:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, harche, sjenning

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/e2e_node/OWNERS~~ [sjenning]

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 14, 2020

k8s-ci-robot requested review from dchen1107 and dims September 14, 2020 06:27

k8s-ci-robot added area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 14, 2020

k8s-ci-robot assigned ConnorDoyle Sep 14, 2020

harche changed the title ~~Crio tests fix~~ Crio node e2e test fix Sep 14, 2020

harche mentioned this pull request Sep 14, 2020

Change CRIO systemd service type to exec kubernetes/test-infra#19189

Closed

oomichi reviewed Sep 14, 2020

View reviewed changes

k8s-ci-robot requested a review from oomichi September 14, 2020 21:43

harche mentioned this pull request Sep 17, 2020

Add secondary runtime handler and cgroup v2 config to CRIO tests kubernetes/test-infra#19259

Merged

k8s-ci-robot 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 Sep 17, 2020

neolit123 reviewed Sep 18, 2020

View reviewed changes

harche mentioned this pull request Sep 18, 2020

Set ssh username with KUBE_SSH_USER if available kubernetes/test-infra#19273

Merged

Add a check for crio service before starting node e2e tests

2ad5f01

Signed-off-by: Harshal Patil <harpatil@redhat.com>

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2020

harche changed the title ~~Crio node e2e test fix~~ Add a check for crio service before starting node e2e tests Sep 18, 2020

neolit123 reviewed Sep 18, 2020

View reviewed changes

k8s-ci-robot assigned neolit123 Sep 18, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2020

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 18, 2020

k8s-ci-robot assigned oomichi Sep 18, 2020

k8s-ci-robot added this to the v1.20 milestone Sep 20, 2020

k8s-ci-robot assigned dims Sep 20, 2020

dims removed their assignment Sep 20, 2020

k8s-ci-robot assigned dashpole and sjenning Sep 20, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2020

k8s-ci-robot merged commit cdc7a29 into kubernetes:master Sep 21, 2020

harche deleted the crio_tests_fix branch September 21, 2020 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a check for crio service before starting node e2e tests #94760

Add a check for crio service before starting node e2e tests #94760

harche commented Sep 14, 2020

k8s-ci-robot commented Sep 14, 2020

harche commented Sep 14, 2020

oomichi left a comment

oomichi Sep 14, 2020

harche Sep 15, 2020

oomichi Sep 15, 2020

BenTheElder Sep 17, 2020

harche Sep 18, 2020

dims commented Sep 17, 2020

dims commented Sep 17, 2020

harche commented Sep 17, 2020

oomichi commented Sep 17, 2020

neolit123 Sep 18, 2020 •

edited

harche commented Sep 18, 2020

neolit123 left a comment

neolit123 commented Sep 18, 2020

oomichi commented Sep 18, 2020

dims commented Sep 20, 2020

harche commented Sep 20, 2020

harche commented Sep 20, 2020

harche commented Sep 20, 2020

dims commented Sep 20, 2020

dims commented Sep 20, 2020

harche commented Sep 21, 2020

neolit123 commented Sep 21, 2020 via email

sjenning commented Sep 21, 2020

k8s-ci-robot commented Sep 21, 2020

Add a check for crio service before starting node e2e tests #94760

Add a check for crio service before starting node e2e tests #94760

Conversation

harche commented Sep 14, 2020

k8s-ci-robot commented Sep 14, 2020

harche commented Sep 14, 2020

oomichi left a comment

Choose a reason for hiding this comment

oomichi Sep 14, 2020

Choose a reason for hiding this comment

harche Sep 15, 2020

Choose a reason for hiding this comment

oomichi Sep 15, 2020

Choose a reason for hiding this comment

BenTheElder Sep 17, 2020

Choose a reason for hiding this comment

harche Sep 18, 2020

Choose a reason for hiding this comment

dims commented Sep 17, 2020

dims commented Sep 17, 2020

harche commented Sep 17, 2020

oomichi commented Sep 17, 2020

neolit123 Sep 18, 2020 • edited

Choose a reason for hiding this comment

harche commented Sep 18, 2020

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Sep 18, 2020

oomichi commented Sep 18, 2020

dims commented Sep 20, 2020

harche commented Sep 20, 2020

harche commented Sep 20, 2020

harche commented Sep 20, 2020

dims commented Sep 20, 2020

dims commented Sep 20, 2020

harche commented Sep 21, 2020

neolit123 commented Sep 21, 2020 via email

sjenning commented Sep 21, 2020

k8s-ci-robot commented Sep 21, 2020

neolit123 Sep 18, 2020 •

edited