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

Implement Cloning for csi-hostpath driver #58

Merged
merged 1 commit into from
Jun 14, 2019
Merged

Implement Cloning for csi-hostpath driver #58

merged 1 commit into from
Jun 14, 2019

Conversation

j-griffith
Copy link
Contributor

This PR adds support for cloning of hostpath volumes.

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds a simple implementation for clone support to the csi-hostpath driver.

Which issue(s) this PR fixes:

Fixes #57

Special notes for your reviewer:

Kubernetes changes have merged in to current master, target for Alpha in 1.15
Sidecar changes are here: kubernetes-csi/external-provisioner#220

Does this PR introduce a user-facing change?:

Adds support for cloning of hostpath volumes

@k8s-ci-robot
Copy link
Contributor

Welcome @j-griffith!

It looks like this is your first PR to kubernetes-csi/csi-driver-host-path 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/csi-driver-host-path has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 11, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 11, 2019
@j-griffith
Copy link
Contributor Author

/assign @jsafrane

@j-griffith
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2019
jsafrane
jsafrane reviewed Jun 12, 2019
View reviewed changes
pkg/hostpath/controllerserver.go Outdated
return nil, status.Error(codes.Internal, "source volumeID does not exist, are source/destination in the same storage class?")
}
srcPath := hostPathVolume.VolPath
args := []string{"-r", srcPath + "/*", path + "/"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be -a more appropriate for 1:1 clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! The one other thing I wanted to check on is handling block mode. Thanks for pointing this out.

pkg/hostpath/controllerserver.go Outdated
executor := utilexec.New()
out, err := executor.Command("cp", args...).CombinedOutput()
if err != nil {
return nil, status.Error(codes.Internal, fmt.Sprintf("failed pre-populate data (clone) for volume: %v: %s", err, out))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the driver at least try to delete half-cloned volume on error? Fixing it in the snapshot restore case above would be nice too :-).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure.. I'll update both.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 12, 2019
@jsafrane
Copy link
Contributor

/lgtm
/approve

why there is hold?

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

[APPROVALNOTIFIER] This PR is APPROVED

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2019
@jsafrane
Copy link
Contributor

@j-griffith, it seems that csi-sanity fails for cloning.

@jsafrane
Copy link
Contributor

/lgtm cancel
until tests are green

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2019
@j-griffith
Copy link
Contributor Author

@j-griffith, it seems that csi-sanity fails for cloning.

There are probably some updates needed there, but we also seem to only have 1.13/14 enabled there; since the code in kubernetes just merged and will be 1.15 I'm not sure (yet) how to pull that in.

@pohly
Copy link
Contributor

pohly commented Jun 13, 2019

There are probably some updates needed there, but we also seem to only have 1.13/14 enabled there; since the code in kubernetes just merged and will be 1.15 I'm not sure (yet) how to pull that in.

csi-sanity testing is independent of the Kubernetes version. It merely runs against the hostpath driver that got deployed into a Kubernetes cluster.

What we have here are simply some failing tests: "can't stat '/csi-data-dir/1a9fcb59-8d35-11e9-aa7d-024285e8710b/*': No such file or directory" and an unexpected error code.

It might very well be that the snapshot test is broken. I don't know whether it ever has been run against a driver that implements snapshotting. The error code mismatch looks like something that can be fixed in the hostpath implementation.

To reproduce the failure:

  • create a clean $GOPATH because .prow.sh will mess with it (it's meant to be run in CI)
  • check out your branch in $GOPATH/github.com/kubernetes-csi/csi-driver-host-path
  • CSI_PROW_TESTS=sanity ./.prow.sh

Once that has failed, you still have a running cluster in KinD and you can debug manually (re-run the test, update the driver, etc.)

To clean up, kind delete cluster --name=csi-prow (if you don't have it, a binary was built by the script) and delete that $GOPATH.

@j-griffith
Copy link
Contributor Author

There are probably some updates needed there, but we also seem to only have 1.13/14 enabled there; since the code in kubernetes just merged and will be 1.15 I'm not sure (yet) how to pull that in.

csi-sanity testing is independent of the Kubernetes version. It merely runs against the hostpath driver that got deployed into a Kubernetes cluster.

What we have here are simply some failing tests: "can't stat '/csi-data-dir/1a9fcb59-8d35-11e9-aa7d-024285e8710b/*': No such file or directory" and an unexpected error code.

It might very well be that the snapshot test is broken. I don't know whether it ever has been run against a driver that implements snapshotting. The error code mismatch looks like something that can be fixed in the hostpath implementation.

To reproduce the failure:

  • create a clean $GOPATH because .prow.sh will mess with it (it's meant to be run in CI)
  • check out your branch in $GOPATH/github.com/kubernetes-csi/csi-driver-host-path
  • CSI_PROW_TESTS=sanity ./.prow.sh

Once that has failed, you still have a running cluster in KinD and you can debug manually (re-run the test, update the driver, etc.)

To clean up, kind delete cluster --name=csi-prow (if you don't have it, a binary was built by the script) and delete that $GOPATH.

Thanks @pohly I'm looking into it

@j-griffith
Copy link
Contributor Author

@pohly thanks for the hints, the problem is that in the test case the src directory is empty; so the cp -a/* call fails. now that I know what's going on I can get it fixed up, thanks again for the pointers.

@j-griffith
Copy link
Contributor Author

Missed one because I was using focused test.

@j-griffith
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2019
@j-griffith
Copy link
Contributor Author

/retest

@j-griffith
Copy link
Contributor Author

@jsafrane @pohly Should be good to go now, thanks for the help.

@j-griffith
Implement Cloning for csi-hostpath driver
959c467 
This PR adds support for cloning of hostpath volumes.
@jsafrane
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2019
@k8s-ci-robot k8s-ci-robot merged commit 1f26dd2 into kubernetes-csi:master Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsafrane jsafrane jsafrane left review comments

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@jsafrane jsafrane

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clone support
4 participants
@j-griffith @k8s-ci-robot @jsafrane @pohly