Add dynamic provisioning support for CSI migration scenarios #253

ddebroy · 2019-03-15T04:47:36Z

NOTE 1: The CSI migration provisioning logic here is not directly feature gated. However, the feature flags on the in-tree side sufficiently protects the logic here. This is because the PV controller in-tree will only yield PVCs (for in-tree provisioners) to the external provisioner when the feature flags for CSI migration for a particular plugin is enabled in-tree which in turn will make a plugin advertise IsMigratedToCSI : kubernetes/kubernetes@23478f1#diff-3f32ad90966ceeb297f57c2a8d726348R1404

NOTE 2: Unit tests are skipped for now. We need to address kubernetes/kubernetes#74594 to be able to inject unit-tests as desired into the translation framework for unit testing purposes.

Tested e2e with GCE PD CSI driver name and ensured CSI driver got invoked for provisioning but the resulting PV pointed to GCEPersistentDisk

I0315 03:43:50.483655       1 controller.go:476] CreateVolumeRequest {Name:pvc-8bd370e9-46d4-11e9-b867-42010a8a001f CapacityRange:required_bytes:6442450944  VolumeCapabilities:[mount:<fs_type:"ext4" > access_mode:<mode:SINGLE_NODE_WRITER > ] Parameters:map[replication-type:none type:pd-standard] Secrets:map[] VolumeContentSource:<nil> AccessibilityRequirements:<nil> XXX_NoUnkeyedLiteral:{} XXX_unrecognized:[] XXX_sizecache:0}
I0315 03:43:50.483838       1 connection.go:180] GRPC call: /csi.v1.Controller/CreateVolume
I0315 03:43:50.483866       1 connection.go:181] GRPC request: {"capacity_range":{"required_bytes":6442450944},"name":"pvc-8bd370e9-46d4-11e9-b867-42010a8a001f","parameters":{"replication-type":"none","type":"pd-standard"},"volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}},"access_mode":{"mode":1}}]}
I0315 03:43:54.881857       1 connection.go:183] GRPC response: {"volume":{"accessible_topology":[{"segments":{"topology.gke.io/zone":"us-west1-a"}}],"capacity_bytes":6442450944,"volume_id":"projects/docker4x/zones/us-west1-a/disks/pvc-8bd370e9-46d4-11e9-b867-42010a8a001f"}}
I0315 03:43:54.883303       1 connection.go:184] GRPC error: <nil>
I0315 03:43:54.883320       1 controller.go:520] create volume rep: {CapacityBytes:6442450944 VolumeId:projects/docker4x/zones/us-west1-a/disks/pvc-8bd370e9-46d4-11e9-b867-42010a8a001f VolumeContext:map[] ContentSource:<nil> AccessibleTopology:[segments:<key:"topology.gke.io/zone" value:"us-west1-a" > ] XXX_NoUnkeyedLiteral:{} XXX_unrecognized:[] XXX_sizecache:0}
I0315 03:43:54.883435       1 controller.go:588] successfully created PV {GCEPersistentDisk:&GCEPersistentDiskVolumeSource{PDName:pvc-8bd370e9-46d4-11e9-b867-42010a8a001f,FSType:ext4,Partition:0,ReadOnly:false,} AWSElasticBlockStore:nil HostPath:nil Glusterfs:nil NFS:nil RBD:nil ISCSI:nil Cinder:nil CephFS:nil FC:nil Flocker:nil FlexVolume:nil AzureFile:nil VsphereVolume:nil Quobyte:nil AzureDisk:nil PhotonPersistentDisk:nil PortworxVolume:nil ScaleIO:nil Local:nil StorageOS:nil CSI:nil}
I0315 03:43:54.883573       1 controller.go:1275] provision "default/podpvc" class "slow": volume "pvc-8bd370e9-46d4-11e9-b867-42010a8a001f" provisioned
I0315 03:43:54.883594       1 controller.go:1292] provision "default/podpvc" class "slow": succeeded

> kubectl describe pv pvc-8bd370e9-46d4-11e9-b867-42010a8a001f
Name:              pvc-8bd370e9-46d4-11e9-b867-42010a8a001f
Labels:            failure-domain.beta.kubernetes.io/region=us-west1
                   failure-domain.beta.kubernetes.io/zone=us-west1-a
Annotations:       pv.kubernetes.io/provisioned-by: pd.csi.storage.gke.io
Finalizers:        [external-provisioner.volume.kubernetes.io/finalizer kubernetes.io/pv-protection]
StorageClass:      slow
Status:            Bound
Claim:             default/podpvc
Reclaim Policy:    Delete
Access Modes:      RWO
VolumeMode:        Filesystem
Capacity:          6Gi
Node Affinity:     
  Required Terms:  
    Term 0:        failure-domain.beta.kubernetes.io/zone in [us-west1-a]
                   failure-domain.beta.kubernetes.io/region in [us-west1]
Message:           
Source:
    Type:       GCEPersistentDisk (a Persistent Disk resource in Google Compute Engine)
    PDName:     pvc-8bd370e9-46d4-11e9-b867-42010a8a001f
    FSType:     ext4
    Partition:  0
    ReadOnly:   false
Events:         <none>

ddebroy · 2019-03-15T04:51:03Z

/cc @leakingtapan @msau42 @davidz627

davidz627

Thanks for the PR Deep! Just a couple small comments.

davidz627 · 2019-03-16T00:55:20Z

cmd/csi-provisioner/csi-provisioner.go

+	handlesMigrationFromInTreePlugin := false
+	handlesMigrationFromInTreePluginName := ""
+	if csitranslationlib.IsMigratedCSIDriverByName(provisionerName) {
+		handlesMigrationFromInTreePluginName, err = csitranslationlib.GetInTreeNameFromCSIName(provisionerName)


Could this instead be a parameter passed in to the provisioner? If an inTreePluginName is given then it handles that migration and if an empty string or nothing is passed in we know it doesn't handle it.

This just gives us more options. Maybe someone will want to run two of the same driver, one for migration one for native CSI. Or maybe someone wants to do a fork of a driver and use that for migration (for some crazy weird reason). Either way I think leaving it open for more options seems like a better idea unless there is a specific issue we have with doing that.

Also could we envision a scenario where we would want to pass in multiple in-tree plugin names? Maybe if in the future a single external provisioner could service multiple drivers? Or a single driver is able to handle the dynamic provisioning for multiple different in-tree plugins. This is probably not as useful to think about right now but it would be nice to leave room for it.

Maybe just a string parameter is fine for now

The suggestion to have options and keep this extensible sounds good. The main concern I have with the options is that some of the logic in https://github.com/kubernetes/csi-translation-lib/blob/master/translate.go is pretty tightly bound to names of specific CSI drivers for certain translations like https://github.com/kubernetes/csi-translation-lib/blob/master/translate.go#L72. Perhaps we can add a note for authors in the migration notes that name changes for a CSI driver needs to be reflected in the csi-translation-lib.

davidz627 · 2019-03-16T00:57:16Z

pkg/controller/controller.go

+	volumeNameUUIDLength                 int
+	config                               *rest.Config
+	driverName                           string
+	handlesMigrationFromInTreePlugin     bool


Do we need this bool since the existence of handlesMigrationFromInTreePluginName kind of implies that it is migrated right?

davidz627 · 2019-03-16T00:58:10Z

pkg/controller/controller.go

+	performInTreeTranslation := false
+	if p.handlesMigrationFromInTreePlugin {
+		storageClassName := options.PVC.Spec.StorageClassName
+		storageClass, err := p.client.StorageV1().StorageClasses().Get(*storageClassName, metav1.GetOptions{})


add a TODO(ISSUENUMBER): use informers

davidz627 · 2019-03-16T01:05:50Z

pkg/controller/controller.go

+		if err != nil {
+			return nil, fmt.Errorf("failed to get storage class named %s: %v", *storageClassName, err)
+		}
+		if storageClass.Provisioner == p.handlesMigrationFromInTreePluginName {


add the else block that can log that we are not translating the storage class parameters. Just to make this easier to understand (I got confused at first)

davidz627 · 2019-03-16T01:06:55Z

pkg/controller/controller.go

+			return nil, fmt.Errorf("failed to get storage class named %s: %v", *storageClassName, err)
+		}
+		if storageClass.Provisioner == p.handlesMigrationFromInTreePluginName {
+			klog.V(2).Infof("Perform CSI migration for intree plugin %s", storageClass.Provisioner)


be more specific here. CSI Migration: translating storage class parameters for in-tree plugin %s?

davidz627 · 2019-03-16T01:10:07Z

pkg/controller/controller.go

@@ -551,6 +576,13 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis
 		pv.Spec.PersistentVolumeSource.CSI.FSType = fsType
 	}

+	if performInTreeTranslation {


performInTreeTranslation doesn't seem like the right name.

It relates to "this specific volume is using translation logic for migration"
volumeMigrated?
volumeShimmed?
volumeTranslated?

I would go with translated probably but will defer to you

davidz627 · 2019-03-16T01:11:19Z

pkg/controller/controller.go

+		volumeNamePrefix:                     volumeNamePrefix,
+		volumeNameUUIDLength:                 volumeNameUUIDLength,
+		handlesMigrationFromInTreePlugin:     handlesMigrationFromInTreePlugin,
+		handlesMigrationFromInTreePluginName: handlesMigrationFromInTreePluginName,


maybe just supportsInTreePlugin (name is implied)?

jsafrane · 2019-03-19T09:37:33Z

cmd/csi-provisioner/csi-provisioner.go

@@ -59,12 +59,32 @@ var (
 	workerThreads        = flag.Uint("worker-threads", 100, "Number of provisioner worker threads, in other words nr. of simultaneous CSI calls.")
 	operationTimeout     = flag.Duration("timeout", 10*time.Second, "Timeout for waiting for creation or deletion of a volume")
 	provisioner          = flag.String("provisioner", "", "This option is deprecated")
+	inTreePluginName     = flag.String("supersedes-in-tree-plugin-name", "", "An in-tree plugin that is superseded by this CSI plugin")


I would appreciate "migration" somewhere in the parameter name and description. --migrated-plugin-name?

jsafrane · 2019-03-19T09:38:47Z

cmd/csi-provisioner/csi-provisioner.go

+		klog.Warningf("Provisioner name: %s not registered in CSI translation library for performing migrations for %s", csiPluginName, inTreePluginName)
+		return
+	}
+	inTreePluginNameFromLib, err := csitranslationlib.GetInTreeNameFromCSIName(csiPluginName)


If we can query in-tree plugin name from CSI driver name, why do we need --supersedes-in-tree-plugin-name option? Should be --feature-gate enough? It allows us to enable it by default during beta.

oops this is complete opposite of my comment from earlier. I didn't think about enabling it by default during beta. Sorry @ddebroy could you change it back... my bad

Will revert back to querying behavior. That will ensure a single source of truth i.e. the translation library and enablement of migration by default. As far as feature gates is concerned, this will depend on the in-tree feature gates to be enabled for individual plugins (besides migration in general) for the PV controller to pass PVCs with in-tree provisioners to external provisioners.

ddebroy · 2019-03-19T21:53:45Z

Comments above have been addressed and ready for another look @jsafrane @davidz627

Signed-off-by: Deep Debroy <ddebroy@docker.com>

ddebroy · 2019-03-20T17:54:31Z

Rebased to latest changes.

jsafrane · 2019-03-21T12:31:26Z

pkg/controller/controller.go

@@ -325,6 +329,31 @@ func getVolumeCapability(
 }

 func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.PersistentVolume, error) {
+	if options.PVC.Spec.Selector != nil {


There is already selector check below.

(this is probably result of rebase)

Thanks for catching this. Will remove.

jsafrane · 2019-03-21T12:38:46Z

pkg/controller/controller.go

+	if p.supportsMigrationFromInTreePluginName != "" {
+		storageClassName := options.PVC.Spec.StorageClassName
+		//TODO(https://github.com/kubernetes-csi/external-provisioner/issues/256): use informers
+		storageClass, err := p.client.StorageV1().StorageClasses().Get(*storageClassName, metav1.GetOptions{})


If the only reason for Get class is to get the provisioner name, there is one in PVC.Annotations["volume.beta.kubernetes.io/storage-provisioner"]. Can this be used instead? Or please add comment why it can't.

Good point. PVC.Annotations["volume.beta.kubernetes.io/storage-provisioner"] cannot be used for migration scenarios because it's set to the name of the CSI provisioner at https://github.com/kubernetes/kubernetes/blob/596a48dd64bcaa01c1d2515dc79a558a4466d463/pkg/controller/volume/persistentvolume/pv_controller.go#L1383 and https://github.com/kubernetes/kubernetes/blob/596a48dd64bcaa01c1d2515dc79a558a4466d463/pkg/controller/volume/persistentvolume/pv_controller.go#L1396 so that external CSI provisioner can detect the PVC and act on it. Will clarify with comment.

jsafrane · 2019-03-21T12:42:58Z

pkg/controller/controller.go

+	if migratedVolume {
+		pv, err = csitranslationlib.TranslateCSIPVToInTree(pv)
+		if err != nil {
+			return nil, err


This leaves the provisioned volume orphaned in the storage backend. We can either delete it (possibly resulting in other errors, oh well...) or return an error that suggests there is an orphan that must be cleaned manually. Both are quite bad, I'd prefer deleting the volume though.

Btw, some log (or even an event?) that this is a critical bug and should be reported / fixed would be helpful.

davidz627 · 2019-03-22T00:16:33Z

LGTM, will let @jsafrane do the final tagging

jsafrane · 2019-03-22T10:08:38Z

lgtm, please squash the commits (at least no "Address code review comments")

Signed-off-by: Deep Debroy <ddebroy@docker.com>

ddebroy · 2019-03-22T15:15:08Z

Commits squashed.

davidz627 · 2019-03-22T18:50:46Z

/lgtm
/approve

ddebroy · 2019-03-22T21:20:06Z

/assign @lpabon @msau42 @jsafrane for approval

k8s-ci-robot · 2019-03-22T21:20:09Z

@ddebroy: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubernetes-csi members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @lpabon @msau42 @jsafrane for approval

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.

jsafrane · 2019-03-25T16:48:33Z

/approve

k8s-ci-robot · 2019-03-25T16:48:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~OWNERS~~ [jsafrane]

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

Upgrade csi-lib-utils to 0.8.1

adb3af9df Merge pull request kubernetes-csi#252 from bells17/update-go-version b82ee3888 Merge pull request kubernetes-csi#253 from bells17/fix-typo c31745621 Fix typo 0a7850561 Bump to Go 1.22.3 edd89ad58 Merge pull request kubernetes-csi#251 from jsafrane/add-logcheck 043fd0991 Add test-logcheck target d7535ae0c Merge pull request kubernetes-csi#250 from jsafrane/go-1.22 b52e7ad35 Update go to 1.22.2 14fdb6f66 Merge pull request kubernetes-csi#247 from msau42/prow 9b4352e9f Update release playbook c7bb972cc Fix release notes script to use fixed tags 463a0e9f5 Add script to update specific go modules git-subtree-dir: release-tools git-subtree-split: adb3af9dfa3ed4d1a922cd839bb48e0b73918617

f40f0ccd4 Merge pull request kubernetes-csi#256 from solumath/master cfa92106c Instruction update 379a1bb9b Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bbbb fix typo in sidecar release process 49676850e Merge pull request kubernetes-csi#254 from bells17/add-github-actions d9bd160c2 Update skip list in codespell GitHub Action adb3af9df Merge pull request kubernetes-csi#252 from bells17/update-go-version f5aebfc9f Add GitHub Actions workflows b82ee3888 Merge pull request kubernetes-csi#253 from bells17/fix-typo c31745621 Fix typo 0a7850561 Bump to Go 1.22.3 edd89ad58 Merge pull request kubernetes-csi#251 from jsafrane/add-logcheck 043fd0991 Add test-logcheck target d7535ae0c Merge pull request kubernetes-csi#250 from jsafrane/go-1.22 b52e7ad35 Update go to 1.22.2 14fdb6f66 Merge pull request kubernetes-csi#247 from msau42/prow 9b4352e9f Update release playbook c7bb972cc Fix release notes script to use fixed tags 463a0e9f5 Add script to update specific go modules git-subtree-dir: release-tools git-subtree-split: f40f0ccd458f2d4555e3ca98d69b5a984bae0f14

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 15, 2019

k8s-ci-robot requested review from msau42 and sbezverk March 15, 2019 04:47

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 15, 2019

ddebroy requested review from jsafrane and removed request for sbezverk and msau42 March 15, 2019 04:49

k8s-ci-robot requested review from davidz627, leakingtapan and msau42 March 15, 2019 04:51

ddebroy force-pushed the csimigprov1 branch 4 times, most recently from ef09afb to ae0c72e Compare March 15, 2019 23:32

davidz627 reviewed Mar 16, 2019

View reviewed changes

ddebroy mentioned this pull request Mar 19, 2019

Use Informers for storage class lookups during provisioning and deleting #256

Closed

jsafrane reviewed Mar 19, 2019

View reviewed changes

Add Vendored files for CSI Translation library

cb67401

Signed-off-by: Deep Debroy <ddebroy@docker.com>

ddebroy force-pushed the csimigprov1 branch from de8f635 to 205b412 Compare March 20, 2019 17:50

jsafrane reviewed Mar 21, 2019

View reviewed changes

ddebroy force-pushed the csimigprov1 branch from 51909f0 to 478f3e4 Compare March 22, 2019 05:23

Add dynamic provisioning support for CSI Migration

ddfb2f5

Signed-off-by: Deep Debroy <ddebroy@docker.com>

ddebroy force-pushed the csimigprov1 branch from 478f3e4 to ddfb2f5 Compare March 22, 2019 15:11

k8s-ci-robot assigned davidz627 Mar 22, 2019

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

k8s-ci-robot assigned jsafrane, lpabon and msau42 Mar 22, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2019

k8s-ci-robot merged commit 41036d7 into kubernetes-csi:master Mar 25, 2019

ddebroy mentioned this pull request Apr 15, 2019

External provisioner migration notes #190

Closed

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023

Merge pull request kubernetes-csi#253 from Jiawei0227/csilibupgrade

0bed2c1

Upgrade csi-lib-utils to 0.8.1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic provisioning support for CSI migration scenarios #253

Add dynamic provisioning support for CSI migration scenarios #253

ddebroy commented Mar 15, 2019 •

edited

Loading

ddebroy commented Mar 15, 2019

davidz627 left a comment

davidz627 Mar 16, 2019

ddebroy Mar 18, 2019

davidz627 Mar 16, 2019

davidz627 Mar 16, 2019

davidz627 Mar 16, 2019

davidz627 Mar 16, 2019

davidz627 Mar 16, 2019

davidz627 Mar 16, 2019

jsafrane Mar 19, 2019

jsafrane Mar 19, 2019 •

edited

Loading

davidz627 Mar 19, 2019

ddebroy Mar 19, 2019

ddebroy commented Mar 19, 2019 •

edited

Loading

ddebroy commented Mar 20, 2019

jsafrane Mar 21, 2019

ddebroy Mar 21, 2019

jsafrane Mar 21, 2019

ddebroy Mar 21, 2019 •

edited

Loading

jsafrane Mar 21, 2019

jsafrane Mar 21, 2019

davidz627 commented Mar 22, 2019

jsafrane commented Mar 22, 2019

ddebroy commented Mar 22, 2019

davidz627 commented Mar 22, 2019

ddebroy commented Mar 22, 2019

k8s-ci-robot commented Mar 22, 2019

jsafrane commented Mar 25, 2019

k8s-ci-robot commented Mar 25, 2019

Add dynamic provisioning support for CSI migration scenarios #253

Add dynamic provisioning support for CSI migration scenarios #253

Conversation

ddebroy commented Mar 15, 2019 • edited Loading

ddebroy commented Mar 15, 2019

davidz627 left a 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 Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy commented Mar 19, 2019 • edited Loading

ddebroy commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Mar 22, 2019

jsafrane commented Mar 22, 2019

ddebroy commented Mar 22, 2019

davidz627 commented Mar 22, 2019

ddebroy commented Mar 22, 2019

k8s-ci-robot commented Mar 22, 2019

jsafrane commented Mar 25, 2019

k8s-ci-robot commented Mar 25, 2019

ddebroy commented Mar 15, 2019 •

edited

Loading

jsafrane Mar 19, 2019 •

edited

Loading

ddebroy commented Mar 19, 2019 •

edited

Loading

ddebroy Mar 21, 2019 •

edited

Loading