Skip to content
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

Support local filesystem volume with block source reconstruction and add related e2e tests #84218

Merged
merged 5 commits into from Nov 1, 2019

Conversation

@cofyc
Copy link
Member

cofyc commented Oct 23, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:

  • Support local filesystem volume with block source reconstruction
    • local block volume reconstruction see #84173
  • Add e2e test to verify global volume mount paths are cleaned

Which issue(s) this PR fixes:

Fixes #74552
Fixes #74923

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

local: support local filesystem volume with block resource reconstruction

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


@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Oct 23, 2019

This PR contains commits from #84173. I'll update after it's merged.


// findMountPoints returns all mount points on given node under specified directory.
func findMountPoints(nodeIP string, dir string) []string {
result, err := e2essh.SSH(fmt.Sprintf(`find %s -type d -exec mountpoint {} \; | grep 'is a mountpoint$'`, dir), nodeIP, framework.TestContext.Provider)

This comment has been minimized.

Copy link
@jsafrane

jsafrane Oct 23, 2019

Member

Don't use ssh please, try HostExec:

type HostExec interface {
IssueCommandWithResult(cmd string, node *v1.Node) (string, error)
IssueCommand(cmd string, node *v1.Node) error
Cleanup()
}

Check its usage in other e2e tests.

This comment has been minimized.

Copy link
@cofyc

cofyc Oct 23, 2019

Author Member

sure

This comment has been minimized.

Copy link
@cofyc

cofyc Oct 23, 2019

Author Member

done

@@ -903,6 +908,9 @@ func testSubpathReconstruction(f *framework.Framework, pod *v1.Pod, forceDelete
framework.ExpectNoError(err, "while getting pod")

utils.TestVolumeUnmountsFromDeletedPodWithForceOption(f.ClientSet, f, pod, forceDelete, true)

mountPointsAfter := utils.FindVolumeGlobalMountPoints(f.ClientSet, pod)
gomega.Expect(mountPointsAfter).To(gomega.ConsistOf(mountPoints), "Global mount points leaked. Before: %v, After: %v.", mountPoints, mountPointsAfter)

This comment has been minimized.

Copy link
@jsafrane

jsafrane Oct 23, 2019

Member

I wonder if it would be easier to use sets.String and expect their difference is zero. Global directory can be quite big and noticing an extra directory could be hard.

This comment has been minimized.

Copy link
@cofyc

cofyc Oct 23, 2019

Author Member

I think it's a good idea, easier to debug flaky tests.

@cofyc cofyc force-pushed the cofyc:fix74552 branch from 49ba283 to a249aff Oct 23, 2019
@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Oct 23, 2019

I’ve verified local volume in my cluster. I will test other volumes and fix bazel files soon.

One potential performance issue is we need to cache global mount points on all schedulable nodes because we don’t know assigned node of pod before the test. This may be slow in large-scale cluster.

@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Oct 25, 2019

/test pull-kubernetes-e2e-gce-iscsi
/test pull-kubernetes-e2e-gce-iscsi-serial

@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Oct 25, 2019

/retest

@cofyc cofyc force-pushed the cofyc:fix74552 branch 2 times, most recently from d2c26b4 to 78c5f32 Oct 25, 2019
@cofyc cofyc changed the title WIP: Support local filesystem volume with block source reconstruction and add related e2e tests Support local filesystem volume with block source reconstruction and add related e2e tests Oct 27, 2019
@cofyc cofyc force-pushed the cofyc:fix74552 branch from 78c5f32 to 7a8f75d Oct 27, 2019
volumeGidValue string
devicePath string
mounter volumepkg.Mounter
deviceMounter volumepkg.DeviceMounter

This comment has been minimized.

Copy link
@cofyc

cofyc Oct 28, 2019

Author Member

I replace AttachableVolumePlugin with DeviceMounter (DeviceMountableVolumePlugin) because:

  1. AttachableVolumePlugin wraps DeviceMounter, so all plugins implementing AttachableVolumePlugin implement DeviceMounter and can get device mount path via GetDeviceMountPath() method
  2. local plugin (and only this plugin) does not implement AttachableVolumePlugin but implements DeviceMountableVolumePlugin
@cofyc cofyc force-pushed the cofyc:fix74552 branch from 7a8f75d to 8def74f Oct 28, 2019
@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Oct 28, 2019

/assign @jsafrane
/assign @msau42
PTAL.

@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Oct 28, 2019

/assign @msau42
(made a typo)

@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Oct 29, 2019

/test pull-kubernetes-e2e-gce-iscsi
/test pull-kubernetes-e2e-gce-iscsi-serial

// The resolved device path may not be the exact same as path in
// local PV object if symbolic link is used. However, it's the true
// source and can be used in reconstructed volume.
path, _, err = mount.GetDeviceNameFromMount(mounter, ref)

This comment has been minimized.

Copy link
@jsafrane

jsafrane Oct 31, 2019

Member

Can you please log the reconstructed device, just in case? Level 4 should be ok, I only want to make sure we have all the information we need if something gets wrong.

This comment has been minimized.

Copy link
@cofyc

cofyc Oct 31, 2019

Author Member

done

@jsafrane

This comment has been minimized.

Copy link
Member

jsafrane commented Oct 31, 2019

/lgtm
/approve
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 31, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, 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:

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Oct 31, 2019

/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.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Oct 31, 2019

/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.

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Oct 31, 2019

/priority important-soon

@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Nov 1, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit ed5b038 into kubernetes:master Nov 1, 2019
17 of 18 checks passed
17 of 18 checks passed
pull-kubernetes-e2e-gce Job triggered.
Details
cla/linuxfoundation cofyc authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Nov 1, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 1, 2019

@cofyc: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce 0fd4cbc link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.