Skip to content

Validate that existing mount point is compatible with volume and capabilities when it already exists for Stage and Publish #479

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
Closed
wants to merge 1 commit into from
Closed

Validate that existing mount point is compatible with volume and capabilities when it already exists for Stage and Publish #479

wants to merge 1 commit into from

Conversation

davidz627
Copy link
Contributor

/kind feature
/assign @msau42

Fixes #253

Staging or Publishing to existing mountpoint's are now return an AlreadyExists error if the volume or capabilities don't match, success otherwise

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 26, 2020
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 26, 2020
@davidz627 davidz627 force-pushed the fix/validateNode branch 2 times, most recently from 49d5ced to 6aecabb Compare February 26, 2020 00:17
msau42
msau42 reviewed Feb 26, 2020
View reviewed changes
Copy link
Contributor

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

Is this unit testable?

pkg/gce-pd-csi-driver/node.go Outdated
return nil, status.Error(codes.Internal, fmt.Sprintf("Error when getting device path: %v", err))
}

if err := ns.volumeCompatible(stagingTargetPath, devicePath, volumeCapability); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that targetPath is bind mounted correctly to devicePath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean by this? We are checking whether it is bind mounted correctly. If it is not it will throw an AlreadyExists error which is the expected behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that it is checking that devicePath (/dev/blah) is mounted to stagingTargetPath (NodeStage path), but not necessarily that stagingTargetPath is bind mounted to targetPath (NodePublish path).

Regardless, this is still a good check to have, it's just not actually verifying the Publish path.

Copy link
Contributor Author

@davidz627 davidz627 Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I see, I think in NodePublish we check that /dev/sdx is mounted to publishTargetPath. But you're right in that we never explicitly check that stagingTargetPath is bindmounted to publishTargetPath.

Edit: I just noted we check stagingTargetPath in NodePublish as well, it should actually be targetPath to make my above statement true.

test/e2e/tests/single_zone_e2e_test.go
}
}()

// Assert: Stage Disk B to Disk A position and fail even both as EXT4
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we run each scenario as a separate test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a lot of boilerplate setup for "later assertions" as asserting things for publish depends on doing a attach and stage. I actually think this is a lot cleaner especially since i've documented it quite thoroughly. In the passing case we're happy, in the failing case I'm not really concerned about "knock on errors" since a failure is a failure and you can debug based on the first error seen.

@davidz627
Copy link
Contributor Author

davidz627 commented Mar 6, 2020 •
edited
Loading

@msau42 resolved comments. This is going to be fairly difficult to unit test given its almost complete dependence on actual filesystem behavior. I will leave it as an exercise to the reader :)

@davidz627 davidz627 force-pushed the fix/validateNode branch 3 times, most recently from cd62ccd to e480300 Compare March 6, 2020 21:51
@davidz627
Validate that existing mount point is compatible with volume and capa…
eb512b7 
…bilities when it already exists for Stage and Publish
msau42
msau42 reviewed Mar 6, 2020
View reviewed changes
pkg/gce-pd-csi-driver/node.go
// location of the mount we're looking for and printing out the backing
// disk. the resulting output should be a path to a device such as
// "/dev/sda"
mountedPathDevBytes, err := ns.Mounter.Exec.Command("sh", "-c", fmt.Sprintf("df -P | awk '$6==\"%s\"{print $1}'", mountedPath)).CombinedOutput()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if df works for bind mounts. Mounter I believe has GetMountRefs() which I think handles bind mounts

@k8s-ci-robot
Copy link
Contributor

@davidz627: PR 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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2020
@davidz627
Copy link
Contributor Author

/close

@k8s-ci-robot
Copy link
Contributor

@davidz627: Closed this PR.

In response to this:

/close

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.

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

@verult verult Awaiting requested review from verult

Assignees

@msau42 msau42

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. kind/feature Categorizes issue or PR as related to a new feature. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate VolumeCapability matches with existing volume in Publish/Stage calls
3 participants
@davidz627 @k8s-ci-robot @msau42