Kubernetes CSI - in-tree Plugin Implementation #54529

vladimirvivien · 2017-10-25T03:01:11Z

What this PR does / why we need it:
This PR is part of the internal Kubernetes CSI Volume plugin. It implements the Attach/Detach/Mount/Unmount API.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): kubernetes/enhancements#178

Special notes for your reviewer:

Implements feature Out-of-tree CSI Volume Plugins enhancements#178
Designed CSI Volume Plugins in Kubernetes Design Doc community#1258

Other CSI Volume plugin PRs

CSI Persistent Volume Source - Kubernetes CSI - Persistent Volume Source Type #55204

Release note:

NONE

jsafrane · 2017-10-25T07:03:12Z

/assign

dims · 2017-10-26T15:23:58Z

/ok-to-test

lpabon · 2017-10-27T21:07:47Z

lgtm for this stage

vladimirvivien · 2017-10-29T19:07:08Z

⚠️ This is still WIP ⚠️

dims · 2017-10-29T19:57:31Z

@vladimirvivien you can use /hold and '/hold cancel` to mark as such

vladimirvivien · 2017-10-30T09:21:07Z

/hold

dixudx · 2017-11-09T01:39:01Z

@vladimirvivien Seems this PR is based on #55204. Can we close #55204?

I don't like to split this feature into 2 PRs, WDYT?

jsafrane

It would be nice to have a separate commit with just the plugin, now it's in the same commit as the API.

jsafrane · 2017-11-14T13:10:41Z

pkg/volume/csi/csi_client.go

+		if v.GetMajor() == ver.GetMajor() &&
+			v.GetMinor() == ver.GetMinor() &&
+			v.GetPatch() == ver.GetPatch() {
+			supported = true


break here?

Test will only be true (if any) one time. Once true, no other condition can turn it off (which is what a break would avoid).. ok take that back.. i'll add to avoid completing loop.

jsafrane · 2017-11-14T13:11:04Z

pkg/volume/csi/csi_attacher.go

@@ -0,0 +1,62 @@
+/*
+Copyright 2014 The Kubernetes Authors.


wrong year (applies to all new files)

updated all files.

I still see 2014. Did you push the updates?

jsafrane · 2017-11-14T13:13:01Z

pkg/volume/csi/csi_client.go

+		}
+	}
+	if !supported {
+		return fmt.Errorf("version %d.%d.%d not supported", ver.GetMajor(), ver.GetMinor(), ver.GetPatch())


It would be nice to log also version(s) supported by the driver.

updated, cleaned

jsafrane · 2017-11-14T13:19:22Z

pkg/volume/csi/csi_client.go

+		VolumeId:          volId,
+		TargetPath:        targetPath,
+		Readonly:          readOnly,
+		PublishVolumeInfo: map[string]string{"device": "/dev/null"}, //TODO where is this from


ad TODO: you get this from AttachedVolume.Status.AttachmentMetadata. For non-attachable drivers it should be IMO empty.

So, you could call ControllerGetCapabilities to find out if the driver is attachable (once, if possible) and then do API Get request for VolumeAttachment instance - VolumeHost can give you kube client.

OTOH, doing Get with every Setup call may be too offensive... can the plugin create an informer that gets updated with events?

+1 This info is in AttachedVolume.Status.AttachmentMetadata. I would fetch and cache it at the beginning of SetupAt if ControllerGetCapabilities says the driver is attachable.

Done. Required a few new things, but done.

jsafrane · 2017-11-14T13:47:37Z

pkg/volume/csi/csi_client_test.go

+		{name: "test ok", volID: "vol-test", targetPath: "/test/path"},
+		{name: "missing volID", targetPath: "/test/path", mustFail: true},
+		{name: "missing target path", volID: "vol-test", mustFail: true},
+		{name: "bad fs", volID: "vol-test", targetPath: "/test/path", fsType: "badfs", mustFail: true},


I admit I have troubles reviewing this. The test is correct, just the fake client is "far away", i.e. in a different file in a separate directory.

<pedantic>
In addition, the fake client does not check that it got correct NodePublishVolumeRequest - it checks that the values were filled, but it does not check that the values have expected value.
</pedantic>

@jsafrane the distance of the fake client was sort of necessary since it is it's own thing with lots of code. There are some validation rules in the aggregation client type, csiDriverClient, which façades all three grpc clients. Adding more expectations would make the fake csi driver not so fake.

jsafrane · 2017-11-14T14:19:01Z

pkg/volume/csi/csi_mounter.go

+		return err
+	}
+
+	_, volID := path.Split(dir)


This implies that CSI volume IDs don't contain slashes. IMO it's very dangerous assumption.

If we do proper sanitization of the name in GetPath, as mentioned above, it should be fine. The CSI spec has also been modified to limit allowed characters (/ is not allowed).

Will apply slash-escape logic indicated earlier.

jsafrane · 2017-11-14T14:46:46Z

pkg/volume/csi/csi_plugin.go

+}
+
+// ProbeVolumePlugins returns implemented plugins
+func ProbeVolumePlugins() []volume.VolumePlugin {


ProbeVolumePlugins is ok for now (with hardcoded socket), but it should be called dynamically when kubelet discovers a new CSI driver on the machine. Some TODO comment would be nice.

I assume in future there will be multiple instances of csiPlugin, one for each driver (following the Flex model).

multiple instances of csiPlugin, one for each driver

Well... this is going to be very interesting. There will be multiple instances of csiPlugin, one for each driver only on nodes. Controller-manager (provisioner, a/d controller) will have one instance of the plugin for all CSI drivers - there is no way how to enumerate drivers on master.

It should work, however we need to test it carefuly.

@jsafrane agreed, will add TODO. Discovery is still not clear and we definitely need to come back and careful tests. Solution can be as simple as a known path for socket (a la flexvolume) or resource registry.

jsafrane · 2017-11-14T14:50:02Z

pkg/volume/csi/csi_plugin.go

+}
+
+func (p *csiPlugin) CanSupport(spec *volume.Spec) bool {
+	return spec.PersistentVolume != nil && spec.PersistentVolume.Spec.CSI != nil


Especially this one must be different on nodes (where you should compare also Driver name) and on controllers (where presence of Spec.CSI is enough).

@jsafrane so you are saying CanSupport should take into consideration the Driver the spec is configured for? If so, agreed. I will add TODO here.

yes, CanSupport in kubelet it should check the driver, on the controller-manager it should not.

jsafrane · 2017-11-14T14:56:25Z

pkg/volume/csi/csi_mounter_test.go

+
+	// Mounter.SetUp()
+	if err := csiMounter.SetUp(nil); err != nil {
+		t.Fatal("mounter.Setup failed: %v", err)


jsafrane · 2017-11-14T14:58:46Z

pkg/volume/csi/csi_plugin.go

+)
+
+const (
+	csiName       = "csi"


csiName does not seem to be used anywhere.

saad-ali · 2017-11-21T22:53:12Z

/test pull-kubernetes-node-e2e

saad-ali · 2017-11-21T22:53:33Z

@thockin for approval of vendored code

k8s-github-robot · 2017-11-21T22:54:07Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@derekwaynecarr @jsafrane @saad-ali @thockin @vladimirvivien @yifan-gu

Note: If this pull request is not resolved or labeled as priority/critical-urgent by Wed, Nov 22 it will be moved out of the v1.9 milestone.

Pull Request Labels

sig/storage: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/feature: New functionality.

Help

thockin · 2017-11-21T23:22:04Z

cmd/kubelet/app/plugins.go

@@ -96,6 +97,7 @@ func ProbeVolumePlugins() []volume.VolumePlugin {
 	allPlugins = append(allPlugins, scaleio.ProbeVolumePlugins()...)
 	allPlugins = append(allPlugins, local.ProbeVolumePlugins()...)
 	allPlugins = append(allPlugins, storageos.ProbeVolumePlugins()...)
+	allPlugins = append(allPlugins, csi.ProbeVolumePlugins()...)


Shouldn't this be alpha-gated?

thockin · 2017-11-21T23:23:14Z

Approving, you can add the feature gate.

/approve

saad-ali · 2017-11-21T23:24:08Z

Adding do-not-merge until flag gate is added around probe.

This commit tracks source code update for the CSI volume plugin implementation.

This commit tracks all auto-generated sources.

saad-ali · 2017-11-22T01:31:48Z

/lgtm
/approve

Thanks again @vladimirvivien!

k8s-github-robot · 2017-11-22T01:32:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese, saad-ali, thockin, vladimirvivien

Associated issue: 178

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

vladimirvivien · 2017-11-22T02:34:34Z

/test pull-kubernetes-bazel-test

k8s-github-robot · 2017-11-22T04:42:22Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2017-11-22T05:14:14Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e	`e489629`	link	`/test pull-kubernetes-node-e2e`

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.

k8s-github-robot · 2017-11-22T05:38:55Z

Automatic merge from submit-queue (batch tested with PRs 54529, 53765). If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Kubernetes CSI - Persistent Volume Source Type **What this PR does / why we need it**: This PR is to track the addition of new API type `CSIPersistentVolumeSource` that will be used as PersistentVolume for storage sources managed by CSI drivers. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: xref kubernetes/enhancements#178 **Special notes for your reviewer**: - Implements API `PersistentVolume` type `CSIPersistentVolumeSource` - Part of implementation for kubernetes/enhancements#178 - Designed at kubernetes/community#1258 Other CSI Volume Plugin PRs: - Plugin Mounter/Unmounter kubernetes/kubernetes#54529 - Plugin Attacher/Detacher kubernetes/kubernetes#55809 **Release note**: ```release-note NONE ``` Kubernetes-commit: 928c85fc997a49e607a94960f1680d9a87b8934f

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 25, 2017

k8s-github-robot assigned derekwaynecarr Oct 25, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 25, 2017

k8s-github-robot assigned yifan-gu Oct 25, 2017

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 25, 2017

vladimirvivien changed the title ~~K8s csi intree plugin~~ Kubernetes in-tree volume plugin for CSI Oct 25, 2017

clintkitson mentioned this pull request Oct 25, 2017

Kubernetes CSI implementation thecodeteam/roadmap#200

Closed

6 tasks

k8s-ci-robot assigned jsafrane Oct 25, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 26, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2017

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2017

vladimirvivien changed the title ~~Kubernetes in-tree volume plugin for CSI~~ Kubernetes CSI - in-tree volume plugin Nov 7, 2017

vladimirvivien mentioned this pull request Nov 7, 2017

Kubernetes CSI - Persistent Volume Source Type #55204

Merged

clintkitson mentioned this pull request Nov 9, 2017

REX-Ray 4Q17 releases thecodeteam/roadmap#197

Closed

17 tasks

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 9, 2017

vladimirvivien changed the title ~~Kubernetes CSI - in-tree volume plugin~~ Kubernetes CSI - in-tree Mounter/Unmounter Nov 14, 2017

jsafrane reviewed Nov 14, 2017

View reviewed changes

saad-ali assigned thockin Nov 21, 2017

thockin reviewed Nov 21, 2017

View reviewed changes

saad-ali added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 21, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2017

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017

vladimirvivien added 3 commits November 21, 2017 20:20

CSI - API source code implementation

dd08d1b

This commit tracks source code update for the CSI volume plugin implementation.

CSI - Generated source code

c90a68c

This commit tracks all auto-generated sources.

CSI - GoDeps dependency updates

e489629

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017

saad-ali removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 22, 2017

k8s-github-robot merged commit 1bdb61f into kubernetes:master Nov 22, 2017

lpabon mentioned this pull request Nov 28, 2017

e2e: CSI Tests #56509

Merged

Kubernetes CSI - in-tree Plugin Implementation #54529

Kubernetes CSI - in-tree Plugin Implementation #54529

Conversation

vladimirvivien commented Oct 25, 2017 • edited by saad-ali Loading

jsafrane commented Oct 25, 2017

dims commented Oct 26, 2017

lpabon commented Oct 27, 2017

vladimirvivien commented Oct 29, 2017

dims commented Oct 29, 2017

vladimirvivien commented Oct 30, 2017

dixudx commented Nov 9, 2017

jsafrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Nov 21, 2017

saad-ali commented Nov 21, 2017

k8s-github-robot commented Nov 21, 2017

Choose a reason for hiding this comment

thockin commented Nov 21, 2017

saad-ali commented Nov 21, 2017

saad-ali commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

vladimirvivien commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-ci-robot commented Nov 22, 2017 • edited Loading

k8s-github-robot commented Nov 22, 2017

vladimirvivien commented Oct 25, 2017 •

edited by saad-ali

Loading

vladimirvivien Nov 16, 2017 •

edited

Loading

jsafrane Nov 14, 2017 •

edited

Loading

k8s-ci-robot commented Nov 22, 2017 •

edited

Loading