Check if target path is mounted before unmounting

[sreis]

Is this a bug fix or adding new feature?

Bug fix. Closes #175

What is this PR about? / Why do we need it?

NodeUnstageVolume doesn't check if target path was already unmounted.

What testing is done?

We don't have unit tests for the node service so testing was done manually.

[k8s-ci-robot]

Hi @sreis. Thanks for your PR.

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

[coveralls]

Pull Request Test Coverage Report for Build 377

• 10 of 13 (76.92%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 63.14%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/driver/node.go	10	13	76.92%

Totals
Change from base Build 375:	0.1%
Covered Lines:	764
Relevant Lines:	1210

---

💛 - Coveralls

[leakingtapan]

/ok-to-test

[sreis]

Test failed with "msg":"failed to dial (instance might not be ready yet)".

[sreis]

/retest

[leakingtapan]

/retest

[leakingtapan]

@sreis did you do any manual testing to mimic the situation?

[leakingtapan]

/retest

[leakingtapan]

Its weird that the integration test is keep failing in this PR but not in others

[leakingtapan]

/retest

[leakingtapan]

/retest

[leakingtapan]

/lgtm

Checking on the failing integration test

[leakingtapan]

This is caused by: kubernetes/test-infra#10934

[leakingtapan]

/retest

[leakingtapan]

/retest

[leakingtapan]

@sreis testing on EC2 instance the test is actually stuck on detaching:

6056 ± ./hack/test-integration.sh                                                                                                                                                ⏎ ✹
=== RUN   TestIntegration
Running Suite: AWS EBS CSI Driver Integration Tests
===================================================
Random Seed: 1548394228
Will run 1 of 1 specs

EBS CSI Driver
  Should create, attach, stage and mount volume, check if it's writable, unmount, unstage, detach, delete, and check if it's deleted
  /home/ubuntu/ebs-csi/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/tests/integration/integration_test.go:52
Creating volume with name "volume-name-integration-test-14653969182178890307"
Waiting for 1 volumes with ID "vol-04bc17a570a8d694e"
Attaching volume "vol-04bc17a570a8d694e" to node "i-0c5166f6e3363507d"
Checking if attachment state of volume "vol-04bc17a570a8d694e" is "attached"
Staging volume "vol-04bc17a570a8d694e" to path "/tmp/volume-name-integration-test-14653969182178890307/stage"
E0125 05:30:39.062107    3575 mount_linux.go:151] Mount failed: exit status 32
Mounting command: mount
Mounting arguments: -t ext4 -o defaults /dev/xvdbl /tmp/volume-name-integration-test-14653969182178890307/stage
Output: mount: wrong fs type, bad option, bad superblock on /dev/xvdbl,
       missing codepage or helper program, or other error
       In some cases useful info is found in syslog - try
       dmesg | tail  or so


Publishing volume "vol-04bc17a570a8d694e" to path "/tmp/volume-name-integration-test-14653969182178890307/mount"
Writing and reading a file
Unpublishing volume "vol-04bc17a570a8d694e" from path "/tmp/volume-name-integration-test-14653969182178890307/mount"
Unstaging volume "vol-04bc17a570a8d694e" from path "/tmp/volume-name-integration-test-14653969182178890307/stage"
Detaching volume "vol-04bc17a570a8d694e" from node "i-0c5166f6e3363507d"

The volume is not unstaged correctly.

[leakingtapan]

/lgtm cancel

The GetMountRefs is not doing its job. Or its not used in the right way. It returns empty list while the target is still mounted. Have you tested on your environment?

[sreis]

I'll try to debug this.

[sreis]

I spent most of the time trying to run the integration tests locally and couldn't figure out why GetMountRefs works when I deployed it and not in the integration tests.

So, I changed the approach and used mount.GetDeviceNameFromMount instead. This function checks if there's a device mounted at a target path and the number of mount points pointing to said device. On paper GetMountRefs should be doing the same thing but it seems to be excluding the target path from the search.

[leakingtapan]

/lgtm
/approve

[leakingtapan]

@sreis you will need to add some test once #187 is merged to get coverage check happy

[sreis]

@leakingtapan Added the test. PTAL

[leakingtapan]

Thanks for the quick turn around. Will take a look

[leakingtapan]

/retest

[leakingtapan]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leakingtapan, sreis

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:

• OWNERS [leakingtapan]

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