test: support storage tests with non-standard kubelet root directory #108253

pohly · 2022-02-21T14:52:24Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Some storage tests deploy DaemonSets which hard-code /var/lib/kubelet as root
directory for kubelet registration and pod directory. To enable those tests in
clusters with some other directory, a new global parameter gets added and the
value defined there gets patched into the relevant places of a CSI driver's
YAML files.

Which issue(s) this PR fixes:

Fixes #92664

Special notes for your reviewer:

A global parameter is used because other tests might also need the same
information.

There are currently no Prow jobs which use a non-standard path. I verified manually that the new flag has the intended behavior:

# Without flags.
$ kubectl get pods --all-namespaces -o yaml | grep /var/lib/kubelet
                k:{"mountPath":"/var/lib/kubelet/plugins"}:
                k:{"mountPath":"/var/lib/kubelet/pods"}:
      - mountPath: /var/lib/kubelet/pods
      - mountPath: /var/lib/kubelet/plugins
      - --kubelet-registration-path=/var/lib/kubelet/plugins/csi-hostpath-provisioning-8787/csi.sock
        path: /var/lib/kubelet/plugins/csi-hostpath-provisioning-8787
        path: /var/lib/kubelet/pods
        path: /var/lib/kubelet/plugins_registry
        path: /var/lib/kubelet/plugins

# With -kubelet-root=/var/lib/kubeletxxx
$ kubectl get pods --all-namespaces -o yaml | grep /var/lib/kubelet
                k:{"mountPath":"/var/lib/kubeletxxx/plugins"}:
                k:{"mountPath":"/var/lib/kubeletxxx/pods"}:
      - mountPath: /var/lib/kubeletxxx/pods
      - mountPath: /var/lib/kubeletxxx/plugins
      - --kubelet-registration-path=/var/lib/kubeletxxx/plugins/csi-hostpath-provisioning-4758/csi.sock
        path: /var/lib/kubeletxxx/plugins/csi-hostpath-provisioning-4758
        path: /var/lib/kubeletxxx/pods
        path: /var/lib/kubeletxxx/plugins_registry
        path: /var/lib/kubeletxxx/plugins

Does this PR introduce a user-facing change?

The e2e.test binary supports a new `--kubelet-root` parameter to override the default `/var/lib/kubelet` path. CSI storage tests use this.

pohly · 2022-02-21T14:53:26Z

/cc @jonesbr17

k8s-ci-robot · 2022-02-21T14:53:27Z

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

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

In response to this:

/cc @jonesbr17

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.

pohly · 2022-02-21T19:12:44Z

/retest

/hold

I'm working on a Prow job with non-standard kubelet root. Let's see whether I can get that working before merging this PR.

pohly · 2022-02-23T12:28:32Z

The idea with using a unique path in pull-kubernetes-e2e-kind and then using that path to find leaked loop devices didn't work out. Therefore running pull-kubernetes-e2e-kind with a non-standard path is probably not worth it. But that is debatable: on the one hand it ensures that tests don't make assumptions about the kubelet path, on the other hand it creates additional friction for the upstream maintainers.

This PR currently also doesn't change where csi-driver-host-path puts its persistent state. That is still /var/lib/csi-data-dir. @jonesbr17: do you know whether that might be a problem in the cluster where you encountered issue #92664?

For future reference, here's a patch which also changes csi-data-dir:

diff --git a/test/e2e/storage/drivers/csi.go b/test/e2e/storage/drivers/csi.go
index 10af124fc4c..9ca76bb0cfa 100644
--- a/test/e2e/storage/drivers/csi.go
+++ b/test/e2e/storage/drivers/csi.go
@@ -45,6 +45,7 @@ import (
 	"strings"
 	"sync"
 	"time"
+	"path"
 
 	"github.com/onsi/ginkgo"
 	spb "google.golang.org/genproto/googleapis/rpc/status"
@@ -73,6 +74,7 @@ import (
 	"k8s.io/kubernetes/test/e2e/storage/drivers/proxy"
 	storageframework "k8s.io/kubernetes/test/e2e/storage/framework"
 	"k8s.io/kubernetes/test/e2e/storage/utils"
+	e2eframework "k8s.io/kubernetes/test/e2e/framework"
 
 	"google.golang.org/grpc"
 )
@@ -227,6 +229,20 @@ func (h *hostpathCSIDriver) PrepareTest(f *framework.Framework) (*storageframewo
 		DriverNamespace:     driverNamespace,
 	}
 
+	// We put the state directory alongside the kubelet state directory:
+	// /var/lib/kubelet -> /var/lib/csi-hostpath-data.
+	//
+	// If kubelet is told to use something like /var/lib/<test specific ID>/kubelet,
+	// then our directory also automatically uses that test specific directory.
+	// We also make that available inside the container under the same path as on
+	// the host, in contrast to the default deployment file, which uses /csi-data-dir.
+	//
+	// This is relevant for loop device cleanup under kind Prow jobs: when a loop
+	// device shows up with /var/lib/<test specific ID> after the cluster has been
+	// torn down, https://github.com/kubernetes-sigs/kind/blob/main/hack/ci/e2e-k8s.sh
+	// knows that it can delete the loop device.
+	statedir := path.Join(path.Dir(strings.TrimRight(e2eframework.TestContext.KubeletRoot, "/")), "csi-hostpath-data")
+
 	o := utils.PatchCSIOptions{
 		OldDriverName:       h.driverInfo.Name,
 		NewDriverName:       config.GetUniqueDriverName(),
@@ -239,6 +255,8 @@ func (h *hostpathCSIDriver) PrepareTest(f *framework.Framework) (*storageframewo
 			// Enable volume lifecycle checks, to report failure if
 			// the volume is not unpublished / unstaged correctly.
 			"--check-volume-lifecycle=true",
+
+			"--statedir", statedir,
 		},
 		ProvisionerContainerName: "csi-provisioner",
 		SnapshotterContainerName: "csi-snapshotter",
@@ -272,12 +290,28 @@ func (h *hostpathCSIDriver) PrepareTest(f *framework.Framework) (*storageframewo
 				switch container.Name {
 				case "csi-external-health-monitor-agent", "csi-external-health-monitor-controller":
 					// Remove these containers.
+				case "hostpath":
+					for i, volmount := range container.VolumeMounts {
+						if volmount.Name == "csi-data-dir" {
+							volmount.MountPath = statedir
+							container.VolumeMounts[i] = volmount
+							break
+						}
+					}
+					containers = append(containers, container)
 				default:
 					// Keep the others.
 					containers = append(containers, container)
 				}
 			}
 			item.Spec.Template.Spec.Containers = containers
+			for i, volume := range item.Spec.Template.Spec.Volumes {
+				if volume.Name == "csi-data-dir" {
+					volume.HostPath.Path = statedir
+					item.Spec.Template.Spec.Volumes[i] = volume
+					break
+				}
+			}
 		}
 		return nil
 	}, h.manifests...)

I tested this PR here locally with a modified version of https://github.com/kubernetes-sigs/kind/blob/main/hack/ci/e2e-k8s.sh:

diff --git a/hack/ci/e2e-k8s.sh b/hack/ci/e2e-k8s.sh
index 9d91b21c..c42bbd3c 100755
--- a/hack/ci/e2e-k8s.sh
+++ b/hack/ci/e2e-k8s.sh
@@ -26,6 +26,30 @@ set -o errexit -o nounset -o xtrace
 #          false - (default) APIs and features left at defaults
 # 
 
+# This unique ID will be used to run kubelet with a --root-dir that is specific
+# to this test run. Loop devices created for raw block volumes will have a path
+# that is inside that directory and will show up on the host with that path (no
+# namespacing for loop devices!). While cleaning up we can delete those.
+#
+# Only some Kubernetes test/e2e binaries have support for running with a
+# non-standard kubelet root directory (added in
+# https://github.com/kubernetes/kubernetes/pull/108253). We leave this empty
+# if not available and skip loop device cleanup.
+TEST_UUID=
+TEST_ROOT_DIR=
+KUBELET_ROOT_DIR=
+if grep -q kubelet-root test/e2e/framework/test_context.go; then
+    TEST_UUID=$(dd if=/dev/random count=16 bs=1 status=none | base64)
+    TEST_ROOT_DIR="/var/lib/$TEST_UUID"
+    KUBELET_ROOT_DIR="${TEST_ROOT_DIR}/kubelet"
+fi
+
+# Root privileges are required for cleaning up loop devices.
+SUDO=
+if [ $(id -u) -ne 0 ]; then
+    SUDO=sudo
+fi
+
 # cleanup logic for cleanup on exit
 CLEANED_UP=false
 cleanup() {
@@ -42,6 +66,17 @@ cleanup() {
   if [ -n "${TMP_DIR:-}" ]; then
     rm -rf "${TMP_DIR:?}"
   fi
+
+  # Clean up leaked loop devices?
+  if [ "$TEST_UUID" ]; then
+    ${SUDO} losetup --noheadings --output NAME,BACK-FILE | while read -r dev path; do
+      if echo "$path" | grep -q "^${TEST_ROOT_DIR}/"; then
+        echo "Cleaning up leaked ${dev} for ${path}."
+        ${SUDO} losetup -d "${dev}"
+      fi
+    done
+  fi
+
   CLEANED_UP=true
 }
 
@@ -86,6 +121,10 @@ create_cluster() {
     esac
   fi
 
+  # NOTE: the indendation on the next line is meaningful!
+  kubelet_extra_args="${kubelet_extra_args}
+      \"root-dir\": \"${KUBELET_ROOT_DIR}\""
+
   # JSON map injected into featureGates config
   feature_gates="{}"
   # --runtime-config argument value passed to the API server
@@ -229,6 +268,7 @@ run_tests() {
   # running a builtin like `wait`, saving the PID also allows us to forward the
   # interrupt
   ./hack/ginkgo-e2e.sh \
+    $(if [ "${KUBELET_ROOT_DIR}" ]; then echo "--kubelet-root=${KUBELET_ROOT_DIR}"; fi) \
     '--provider=skeleton' "--num-nodes=${NUM_NODES}" \
     "--ginkgo.focus=${FOCUS}" "--ginkgo.skip=${SKIP}" \
     "--report-dir=${ARTIFACTS}" '--disable-log-dump=true' &

Tests passed, just the loop device cleanup didn't work out as intended.

/hold cancel

pacoxu · 2022-03-01T09:48:36Z

It is a little dup with KubeVolumeDir.

kubernetes/test/e2e/framework/test_context.go

Line 340 in 77f6476

    
           flags.StringVar(&TestContext.KubeVolumeDir, "volume-dir", "/var/lib/kubelet", "Path to the directory containing the kubelet volumes.")

EDITED: rename it to KubeRootDir and use for both scenarios. (But it may affect some volume test cases.) Or can we reuse it directly?

pohly · 2022-03-01T10:56:44Z

It is a little dup with KubeVolumeDir.

Good catch, I hadn't seen that one.

rename it to KubeRootDir and use for both scenarios. (But it may affect some volume test cases.) Or can we reuse it directly?

You want me to rename KubeletRoot to KubeRootDir, right? KubeletRootDir makes sense to me, but not so much KubeRootDir - what would Kube stand for, i.e. which component of Kubernetes?

I'm not sure what you mean with Or can we reuse it directly? - what is it here? KubeVolumeDir? Using that and going up one level feels hacky to me.

pacoxu · 2022-03-01T11:03:10Z

I mean can we reuse KubeVolumeDir? If the name is not valid for this purpose, we may rename it.

pohly · 2022-03-01T13:08:43Z

KubeVolumeDir is too specific. We can of course assume that KubeletRootDir = KubeVolumeDir/.. but that looks backwards to me. I prefer to specify KubeletRootDir and then derived the other directories from that.

pacoxu · 2022-03-02T02:01:16Z

I prefer to specify KubeletRootDir and then derived the other directories from that.

I agree.

pohly · 2022-03-03T10:41:47Z

I must have been confused by the KubeVolumeDir name: that parameter already is the kubelet root dir, not the pods/*/volumes directory underneath that.

I think the parameter was badly named when it was introduced. I'm now torn between renaming the parameter (may cause problems for anyone already using it) and just updating the parameter's description (less intrusive, my still harder to discover).

@pacoxu : any preference?

pacoxu · 2022-03-03T10:47:01Z

@pacoxu : any preference?

😂 either is ok for me.

Some storage tests deploy DaemonSets which hard-code /var/lib/kubelet as root directory for kubelet registration and pod directory. There was already a parameter which allowed specifying the root directory, just with a very confusing name ("--volume-dir") and matching field name. A --kubelet-root-dir parameters gets added because this may make it easier to find the parameter, with the old name preserved as an alias for the same field for backwards compatibility.

pacoxu · 2022-03-05T06:51:09Z

/assign @SergeyKanzhelev @ehashman

pohly · 2022-03-05T13:09:14Z

@SergeyKanzhelev @ehashman: there are some comments in node tests about "framework.TestContext.KubeVolumeDir is currently not populated for node e2e", for example here:

kubernetes/test/e2e_node/node_problem_detector_linux.go

Lines 297 to 299 in 5b9910a

    
           // TODO: remove hardcoded kubelet volume directory path 
        
           // framework.TestContext.KubeVolumeDir is currently not populated for node e2e 
        
           hostLogFile = "/var/lib/kubelet/pods/" + string(pod.UID) + "/volumes/kubernetes.io~empty-dir" + logFile

I don't know what is meant with "not populated" and therefore didn't touch those parts.

The default /var/lib/kubelet should always be set, so using it would be better than hard-coding (in that example) hostLogFile = "/var/lib/kubelet/pods/". Let me know whether you want me update also those code lines.

pohly · 2022-03-06T15:21:10Z

/retest

klueska · 2022-03-14T15:38:50Z

/approve

ehashman

/approve

for node test

ehashman · 2022-03-14T17:58:39Z

test/e2e/storage/utils/deployment.go

+				// Inject non-standard kubelet path.
+				*p = substKubeletRootDir(*p)


This seems a little weird to me. But since it looks like @pohly is a storage test approver I trust you to do the right thing here...

ehashman · 2022-03-14T17:58:53Z

test/e2e/node/security_context.go

-	volumeHostPath := fmt.Sprintf("%s/pods/%s/volumes/kubernetes.io~empty-dir/%s", framework.TestContext.KubeVolumeDir, foundPod.UID, volumeName)
-	ginkgo.By(fmt.Sprintf("confirming a container with the same label can read the file under --volume-dir=%s", framework.TestContext.KubeVolumeDir))
+	volumeHostPath := fmt.Sprintf("%s/pods/%s/volumes/kubernetes.io~empty-dir/%s", framework.TestContext.KubeletRootDir, foundPod.UID, volumeName)
+	ginkgo.By(fmt.Sprintf("confirming a container with the same label can read the file under --kubelet-root-dir=%s", framework.TestContext.KubeletRootDir))


This seems fine.

ehashman · 2022-03-14T18:01:11Z

/retest

k8s-ci-robot · 2022-03-14T18:01:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ehashman, klueska, pacoxu, pohly

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ehashman · 2022-03-14T18:01:57Z

/triage accepted
/priority important-longterm

pohly · 2022-03-14T19:09:02Z

/retest

pohly · 2022-03-14T21:35:45Z

/retest

pacoxu · 2022-03-15T01:52:03Z

/test pull-kubernetes-integration

k8s-triage-robot · 2022-03-15T05:54:19Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot requested review from msau42 and neolit123 February 21, 2022 14:54

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

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

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

pohly force-pushed the kubelet-root branch from d7b0eb2 to ff5558e Compare March 3, 2022 11:37

k8s-ci-robot assigned pacoxu Mar 5, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2022

k8s-ci-robot assigned ehashman and SergeyKanzhelev Mar 5, 2022

pohly closed this Mar 5, 2022

pohly reopened this Mar 5, 2022

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board Mar 8, 2022

SergeyKanzhelev added this to Triage in SIG Node PR Triage Mar 8, 2022

ehashman reviewed Mar 14, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2022

pacoxu moved this from Triage to Done in SIG Node CI/Test Board Mar 15, 2022

pacoxu moved this from Triage to Done in SIG Node PR Triage Mar 15, 2022

k8s-ci-robot merged commit 7cc4fcd into kubernetes:master Mar 15, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 15, 2022

github-actions bot mentioned this pull request Mar 22, 2022

Week Ending March 20, 2022 dev-obs/actus#403

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: support storage tests with non-standard kubelet root directory #108253

test: support storage tests with non-standard kubelet root directory #108253

pohly commented Feb 21, 2022

pohly commented Feb 21, 2022

k8s-ci-robot commented Feb 21, 2022

pohly commented Feb 21, 2022

pohly commented Feb 23, 2022

pacoxu commented Mar 1, 2022 •

edited

pohly commented Mar 1, 2022

pacoxu commented Mar 1, 2022

pohly commented Mar 1, 2022

pacoxu commented Mar 2, 2022

pohly commented Mar 3, 2022

pacoxu commented Mar 3, 2022

pacoxu commented Mar 5, 2022

pohly commented Mar 5, 2022

pohly commented Mar 6, 2022

klueska commented Mar 14, 2022

ehashman left a comment

ehashman Mar 14, 2022

ehashman Mar 14, 2022

ehashman commented Mar 14, 2022

k8s-ci-robot commented Mar 14, 2022

ehashman commented Mar 14, 2022

pohly commented Mar 14, 2022

pohly commented Mar 14, 2022

pacoxu commented Mar 15, 2022

k8s-triage-robot commented Mar 15, 2022

		// Inject non-standard kubelet path.
		p = substKubeletRootDir(p)

test: support storage tests with non-standard kubelet root directory #108253

test: support storage tests with non-standard kubelet root directory #108253

Conversation

pohly commented Feb 21, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

pohly commented Feb 21, 2022

k8s-ci-robot commented Feb 21, 2022

pohly commented Feb 21, 2022

pohly commented Feb 23, 2022

pacoxu commented Mar 1, 2022 • edited

pohly commented Mar 1, 2022

pacoxu commented Mar 1, 2022

pohly commented Mar 1, 2022

pacoxu commented Mar 2, 2022

pohly commented Mar 3, 2022

pacoxu commented Mar 3, 2022

pacoxu commented Mar 5, 2022

pohly commented Mar 5, 2022

pohly commented Mar 6, 2022

klueska commented Mar 14, 2022

ehashman left a comment

Choose a reason for hiding this comment

ehashman Mar 14, 2022

Choose a reason for hiding this comment

ehashman Mar 14, 2022

Choose a reason for hiding this comment

ehashman commented Mar 14, 2022

k8s-ci-robot commented Mar 14, 2022

ehashman commented Mar 14, 2022

pohly commented Mar 14, 2022

pohly commented Mar 14, 2022

pacoxu commented Mar 15, 2022

k8s-triage-robot commented Mar 15, 2022

pacoxu commented Mar 1, 2022 •

edited