Use pod + nsenter instead of SSH in mount propagation tests #82424

jsafrane · 2019-09-06T15:15:23Z

Not all Kubernetes clusters allow ssh to nodes during test.

What type of PR is this?
/kind cleanup

Does this PR introduce a user-facing change?:

NONE

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

/sig storage
/sig node

cc @msau42 @derekwaynecarr, PTAL

mattjmcnaughton

/assign @tallclair
/test pull-kubernetes-integration

/lgtm

This looks good to me, but would love an additional pair of eyes from the approver.

mattjmcnaughton · 2019-09-06T18:48:58Z

test/e2e/node/mount_propagation.go

@@ -102,8 +105,8 @@ var _ = SIGDescribe("Mount propagation", func() {
 		// running in parallel.
 		hostDir := "/var/lib/kubelet/" + f.Namespace.Name
 		defer func() {
-			cleanCmd := fmt.Sprintf("sudo rm -rf %q", hostDir)
-			e2essh.IssueSSHCommand(cleanCmd, framework.TestContext.Provider, node)
+			cleanCmd := fmt.Sprintf("rm -rf %q", hostDir)


For my own understanding, can you share why we no longer need to use sudo?

we're definitely root now (the pod is root)

aojea · 2019-09-06T20:57:20Z

/cc

tallclair

A couple questions about generalizing this, but otherwise lgtm

tallclair · 2019-09-11T22:33:39Z

test/e2e/node/mount_propagation.go

@@ -85,6 +85,9 @@ var _ = SIGDescribe("Mount propagation", func() {
 		// tmpfs to a subdirectory there. We check that these mounts are
 		// propagated to the right places.

+		hostExec := utils.NewHostExec(f)


Just an observation for generalizing this - I think the hostexec pod should set hostNetwork, hostIPC, and hostPID to true

It depends how far do we want to go.

For storage, we're fine if the host exec pod does nsenter to host mount namespace, so we can check mounts and what's in /var/lib/kubelet without potential mount propagation hiccups.

Do we have tests that actually need host pid or network namespace? Would it be better to add them to nsenter command line instead?

kubernetes/test/e2e/storage/utils/host_exec.go

Lines 96 to 106 in 5262633

args := []string{

"exec",

fmt.Sprintf("--namespace=%v", pod.Namespace),

pod.Name,

"--",

"nsenter",

"--mount=/rootfs/proc/1/ns/mnt",

"--",

"sh",

"-c",

cmd,

[I think we can leave generalizing for a follow up] ?

tallclair · 2019-09-11T22:34:49Z

test/e2e/node/mount_propagation.go

@@ -102,8 +105,8 @@ var _ = SIGDescribe("Mount propagation", func() {
 		// running in parallel.
 		hostDir := "/var/lib/kubelet/" + f.Namespace.Name
 		defer func() {
-			cleanCmd := fmt.Sprintf("sudo rm -rf %q", hostDir)
-			e2essh.IssueSSHCommand(cleanCmd, framework.TestContext.Provider, node)


Should we remove e2essh.IssueSSHCommand, or just have it call hostexec under the hood?

I think that most tests that use ssh should move to hostexec, however, there may be some that need real ssh.

What does kubectl exec <hostesxec pod> nsenter [...] systemctl restart kubelet do when kubelet is restarted in the middle of kubectl exec? Our disruptive tests do that. systemctl restart (in host mount namespace) should work, it writes to a socket and it reaches the real systemd on the host. I am not sure about the kubectl exec part and interrupted connection to kubelet.

I'd assume just like real SSH we will need to handle re-connect?

Yeah, there are solutions, but they are far more risky than good old ssh. I'd prefer gradual move to hostexec.

BenTheElder · 2019-09-13T06:35:28Z

/priority important-longterm
needs rebase

thanks for doing this

jsafrane · 2019-09-13T10:35:00Z

rebased

BenTheElder · 2019-09-16T23:04:10Z

/lgtm

BenTheElder · 2019-09-18T20:44:23Z

/approve
/hold

k8s-ci-robot · 2019-09-18T20:44:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, jsafrane

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~~ [BenTheElder]

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

jsafrane · 2019-09-23T12:13:10Z

@BenTheElder @tallclair, can ve move forward with this PR? Or do you insist on generalization?

BenTheElder · 2019-09-23T19:04:15Z

I would like to move forward and follow on with generalization later. If this pattern works well for some tests it would be great to move everything over but we shouldn't block on that.

BenTheElder · 2019-09-30T23:27:48Z

/hold cancel
/label needs-rebase

k8s-ci-robot · 2019-09-30T23:27:49Z

@BenTheElder: The label(s) /label needs-rebase cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

/hold cancel
/label needs-rebase

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.

fejta-bot · 2019-10-01T05:34:59Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-10-01T07:40:59Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

Not all Kubernetes clusters allow ssh to nodes during test.

jsafrane · 2019-10-01T08:23:16Z

rebased

BenTheElder · 2019-10-01T20:10:52Z

/lgtm

k8s-ci-robot requested review from dchen1107 and sjenning September 6, 2019 15:16

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 6, 2019

jsafrane force-pushed the mount-propagation-remove-ssh branch from 0fcade4 to 633bcea Compare September 6, 2019 15:56

k8s-ci-robot assigned tallclair Sep 6, 2019

mattjmcnaughton approved these changes Sep 6, 2019

View reviewed changes

k8s-ci-robot assigned mattjmcnaughton Sep 6, 2019

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

k8s-ci-robot requested a review from aojea September 6, 2019 20:57

tallclair reviewed Sep 11, 2019

View reviewed changes

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 13, 2019

jsafrane force-pushed the mount-propagation-remove-ssh branch from 633bcea to d659e11 Compare September 13, 2019 10:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2019

k8s-ci-robot assigned BenTheElder Sep 16, 2019

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2019

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

jsafrane mentioned this pull request Sep 25, 2019

Use pod + nsenter instead of SSH in block volume tests #83108

Merged

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2019

Use pod + nsenter instead of SSH in mount propagation tests

0c3293a

Not all Kubernetes clusters allow ssh to nodes during test.

jsafrane force-pushed the mount-propagation-remove-ssh branch from d659e11 to 5dba3cb Compare October 1, 2019 08:21

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2019

jsafrane force-pushed the mount-propagation-remove-ssh branch from 5dba3cb to 0c3293a Compare October 1, 2019 08:31

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2019

k8s-ci-robot merged commit 867fef5 into kubernetes:master Oct 1, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 1, 2019

BenTheElder mentioned this pull request Oct 2, 2019

update kind presubmit kubernetes/test-infra#14553

Merged

jsafrane mentioned this pull request Oct 2, 2019

Bug 1711600: UPSTREAM: 82424:Use pod + nsenter instead of SSH in mount propagation tests openshift/origin#23897

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pod + nsenter instead of SSH in mount propagation tests #82424

Use pod + nsenter instead of SSH in mount propagation tests #82424

jsafrane commented Sep 6, 2019

mattjmcnaughton left a comment

mattjmcnaughton Sep 6, 2019

BenTheElder Sep 6, 2019

aojea commented Sep 6, 2019

tallclair left a comment

tallclair Sep 11, 2019

jsafrane Sep 12, 2019

BenTheElder Sep 16, 2019

tallclair Sep 11, 2019

jsafrane Sep 12, 2019

BenTheElder Sep 16, 2019

jsafrane Sep 23, 2019

BenTheElder commented Sep 13, 2019

jsafrane commented Sep 13, 2019

BenTheElder commented Sep 16, 2019

BenTheElder commented Sep 18, 2019

k8s-ci-robot commented Sep 18, 2019

jsafrane commented Sep 23, 2019

BenTheElder commented Sep 23, 2019

BenTheElder commented Sep 30, 2019

k8s-ci-robot commented Sep 30, 2019

fejta-bot commented Oct 1, 2019

fejta-bot commented Oct 1, 2019

jsafrane commented Oct 1, 2019

BenTheElder commented Oct 1, 2019

	args := []string{
	"exec",
	fmt.Sprintf("--namespace=%v", pod.Namespace),
	pod.Name,
	"--",
	"nsenter",
	"--mount=/rootfs/proc/1/ns/mnt",
	"--",
	"sh",
	"-c",
	cmd,

Use pod + nsenter instead of SSH in mount propagation tests #82424

Use pod + nsenter instead of SSH in mount propagation tests #82424

Conversation

jsafrane commented Sep 6, 2019

mattjmcnaughton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Sep 6, 2019

tallclair 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

BenTheElder commented Sep 13, 2019

jsafrane commented Sep 13, 2019

BenTheElder commented Sep 16, 2019

BenTheElder commented Sep 18, 2019

k8s-ci-robot commented Sep 18, 2019

jsafrane commented Sep 23, 2019

BenTheElder commented Sep 23, 2019

BenTheElder commented Sep 30, 2019

k8s-ci-robot commented Sep 30, 2019

fejta-bot commented Oct 1, 2019

fejta-bot commented Oct 1, 2019

jsafrane commented Oct 1, 2019

BenTheElder commented Oct 1, 2019