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.

Sign up for GitHub

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

Jump to bottom

Don't fail making mount dirs if they already exist #272

Merged
merged 1 commit into from
Dec 4, 2020
Merged

Don't fail making mount dirs if they already exist #272

merged 1 commit into from
Dec 4, 2020

Conversation

chrishenzie
Copy link
Contributor

@chrishenzie chrishenzie commented Oct 31, 2020 •
edited
Loading

Is this a bug fix or adding new feature?
Bug fix.

What is this PR about? / Why do we need it?
When the should mount different paths on same volume on same node e2e test is re-ran more than once against the same pre-provisioned EFS file system, the test fails because the first pod in the test repeatedly crashloops. The pod crashloops because mkdir fails if the directory already exists. This fixes the issue by ignoring if the directory already exists.

A followup to this might include adding a cleanup step that deletes the files / directories placed on the EFS file system in these tests. Thoughts on this?

What testing is done?

# Pre-provision an EFS file system

# Make the binary.
make test-e2e-bin

# Run test against pre-provisioned EFS file system.
bin/test-e2e \
	-kubeconfig=$HOME/.kube/config \
	-ginkgo.focus="should mount different paths on same volume on same node" \
	-ginkgo.skip="\[Disruptive\]" \
	-ginkgo.dryRun \
	-file-system-id fs-0c7a3009 \
	-region us-west-2

# Run it a second time and observe the test not fail.
bin/test-e2e \
	-kubeconfig=$HOME/.kube/config \
	-ginkgo.focus="should mount different paths on same volume on same node" \
	-ginkgo.skip="\[Disruptive\]" \
	-ginkgo.dryRun \
	-file-system-id fs-0c7a3009 \
	-region us-west-2

@msau42 @wongma7

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 31, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @chrishenzie. Thanks for your PR.

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

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 31, 2020
msau42
msau42 reviewed Oct 31, 2020
View reviewed changes
test/e2e/e2e.go
@@ -189,7 +189,7 @@ var _ = ginkgo.Describe("[efs-csi] EFS CSI", func() {
defer func() { _ = f.ClientSet.CoreV1().PersistentVolumes().Delete(pvRoot.Name, &metav1.DeleteOptions{}) }()

ginkgo.By(fmt.Sprintf("Creating pod to make subpaths /a and /b"))
pod := e2epod.MakePod(f.Namespace.Name, nil, []*v1.PersistentVolumeClaim{pvcRoot}, false, "mkdir /mnt/volume1/a && mkdir /mnt/volume1/b")
pod := e2epod.MakePod(f.Namespace.Name, nil, []*v1.PersistentVolumeClaim{pvcRoot}, false, "mkdir -p /mnt/volume1/a && mkdir -p /mnt/volume1/b")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the test should write to a randomly generated directory name so that we can validate in the test that read/write works?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also how similiar is this test case to https://github.com/kubernetes/kubernetes/blob/5ead4974e0133286b801915b2d8de24bb64e36aa/test/e2e/storage/testsuites/subpath.go#L291?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I guess this test case is creating two separate pvs for each subpath

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way I think the test should clean up after itself, i.e. not have weird sideffects like this. I am fine with current form of PR since this part is basically just setting up for the "real " test and if write is failing here then another test should cover that.

Copy link
Contributor

@wongma7 wongma7 Oct 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is testing the workaround to issue #100, where due to how kubernetes treats volumeHandle , two volumemounts w/ same* volumeHandle won't be mounted, so we create pvs with different volumehandles like fs:/a and fs:/b (probably I should have linked it in comment of test)

@msau42
Copy link

msau42 commented Oct 31, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 31, 2020
@chrishenzie chrishenzie mentioned this pull request Nov 11, 2020
Closed
6 tasks
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2020
@wongma7
Copy link
Contributor

wongma7 commented Dec 4, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrishenzie, wongma7

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit c82831c into kubernetes-sigs:master Dec 4, 2020
@chrishenzie chrishenzie deleted the e2e-mount-dirs branch February 17, 2021 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@wongma7 wongma7 wongma7 left review comments

@leakingtapan leakingtapan Awaiting requested review from leakingtapan

@nckturner nckturner Awaiting requested review from nckturner

Assignees

@wongma7 wongma7

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chrishenzie @k8s-ci-robot @msau42 @wongma7