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.

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

Support dynamic provisioning for CSI migration scenarios #73653

Merged
merged 1 commit into from Feb 28, 2019

Conversation

@ddebroy
Copy link
Member

ddebroy commented Feb 2, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
This PR introduces a set of changes needed in in-tree PV Controller to call out to the external provisioning controller for in-tree plugins if they are migratable to CSI. The basic steps are:

  1. Check if in-tree plugin is migratable to CSI. If so:
  2. Set annStorageProvisioner in the PVC to CSI plugin name that superseded the specified in-tree plugin.
  3. Do not proceed with in-tree provisioning and return so that external provisioner can act on the claim.

In addition to the above, the PR also introduces a new API TranslateInTreeStorageClassParametersToCSI in the CSI translation staging repo. This will be used by the external provisioner to translate from in-tree storage class parameters to CSI plugin parameters.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Part of enhancements for kubernetes/enhancements#625

For the changes in this PR to take an effect, we need a separate set of changes in https://github.com/kubernetes-sigs/sig-storage-lib-external-provisioner.git which does the following:

  1. Enhances getStorageClassFields to check (through GetCSINameFromIntreeName) if the storageclass provisioner has a corresponding CSI plugin that supersedes it. If so:
  2. Makes getStorageClassFields aware that it's handling dynamic volume provisioning for an in-tree plugin that has been migrated to CSI because [a] For regular in-tree provisioning path, external provisioning will never get invoked [b] For regular CSI provisioning path, GetCSINameFromIntreeName would not return anything for a CSI plugin name.
  3. Enhances getStorageClassFields to [a] translate in-tree storage class parameters to the corresponding CSI plugin parameters using TranslateInTreeStorageClassParametersToCSI [b] return the CSI plugin name (instead of the storage class in-tree provisioner) as the provisioner name (so that it aligns with ctrl.provisionerName) and [c] return a boolean to provisionClaimOperation indicating dynamic provisioning in a CSI migration context
  4. Enhances provisionClaimOperation to invoke TranslateCSIPVToInTree for dynamic provisioning in a CSI migration context.

The above PR depends on TranslateInTreeStorageClassParametersToCSI from this PR to be staged first.

Does this PR introduce a user-facing change?:

Introduce dynamic volume provisioning shim for CSI migration

/sig-storage
/assign @jsafrane @saad-ali
/cc @msau42 @leakingtapan @davidz627

@k8s-ci-robot k8s-ci-robot requested review from davidz627 , leakingtapan and msau42 Feb 2, 2019

@ddebroy ddebroy changed the title Provisioning changes for CSI migration Support dynamic provisioning for CSI migration scenarios Feb 2, 2019

@ddebroy

This comment has been minimized.

Copy link
Member Author

ddebroy commented Feb 2, 2019

/sig storage

@davidz627

This comment has been minimized.

Copy link
Contributor

davidz627 commented Feb 4, 2019

@ddebroy thanks for this PR! I will review shortly.

Would recommend that you create a separate PR for the translation library staging change. Since the external provisioner depends on it lets fast track that and merge it separately.

// Set provisionerName to CSI plugin name for setClaimProvisioner
provisionerName, err = csitranslation.GetCSINameFromIntreeName(storageClass.Provisioner)
if err != nil {
strerr := fmt.Sprintf("Failed to get CSI name for In tree plugin: %s", storageClass.Provisioner)

This comment has been minimized.

@dims

dims Feb 5, 2019

Member
  • can we print the err so it's in strerr itself?
  • also we can drop the klog as we are returning the err as well as logging it.

This comment has been minimized.

@ddebroy

ddebroy Feb 7, 2019

Author Member

The convention in provisionClaimOperation appears to be to klog.V(2|3).Infof(...) all errors besides returning them as well as reporting the event with ctrl.eventRecorder.Event. So I was following the convention elsewhere in provisionClaimOperation. @jsafrane please confirm the convention.

I will change the error message to include the whole error as suggested.

@ddebroy

This comment has been minimized.

Copy link
Member Author

ddebroy commented Feb 5, 2019

PR #73734 supersedes the csi-translation-lib bits of of this PR.

if plugin != nil {
pluginName = plugin.GetPluginName()
if plugin.IsMigratedToCSI() {

This comment has been minimized.

@jsafrane

jsafrane Feb 5, 2019

Member

Please add an unit test for this. "11-17 - external provisioner" could be a good start. You need to inject a volume plugin that's migrated to CSI, some variant of wrapTestWithPluginCalls might be sufficient.

This comment has been minimized.

@ddebroy

ddebroy Feb 25, 2019

Author Member

Updated PR with 11-21 - external provisioner for CSI migration testcase to make sure for CSI migration scenarios, provisioning ends up waiting for a claim with annotation vendor.com/MockCSIPlugin and event Normal ExternalProvisioning.

@@ -40,6 +40,11 @@ func NewAWSElasticBlockStoreCSITranslator() InTreePlugin {
return &awsElasticBlockStoreCSITranslator{}
}

// TranslateInTreeStorageClassParametersToCSI translates InTree EBS storage class parameters to CSI storage class

This comment has been minimized.

@jsafrane

jsafrane Feb 5, 2019

Member

IMO, this can be removed now.

@ddebroy ddebroy force-pushed the ddebroy:migprov1 branch 2 times, most recently from a9eaaac to ea4980e Feb 5, 2019

@ddebroy ddebroy force-pushed the ddebroy:migprov1 branch from ea4980e to 37c0250 Feb 25, 2019

@k8s-ci-robot k8s-ci-robot added size/M and removed needs-rebase labels Feb 25, 2019

@jsafrane
Copy link
Member

jsafrane left a comment

Thanks for the test! Please remove one commented-out line and I think we're good to go.


// For testing only: hook to intercept CSI driver name <=> Intree plugin name mapping
// Not used when set to nil
csiNameFromIntreeNameHook func(pluginName string) (string, error)

This comment has been minimized.

@jsafrane

jsafrane Feb 26, 2019

Member

In long term it would be better if CSI translation library provided an interface instead of function calls so its callers can create fake implementations for unit tests. As a separate PR, of course, no need to fix it here.

This comment has been minimized.

@ddebroy

ddebroy Feb 26, 2019

Author Member

Agree.

This comment has been minimized.

@davidz627

davidz627 Feb 26, 2019

Contributor

xref" #74594

// of not setting pluginName for external provisioners (including CSI)
// Set provisionerName to CSI plugin name for setClaimProvisioner
provisionerName, err = ctrl.getCSINameFromIntreeName(storageClass.Provisioner)
// provisionerName, err = csitranslation.GetCSINameFromIntreeName(storageClass.Provisioner)

This comment has been minimized.

@jsafrane

jsafrane Feb 26, 2019

Member

nit: please remove

This comment has been minimized.

@ddebroy

ddebroy Feb 26, 2019

Author Member

done

PV Controller changes to support provisioning in CSI Migration scenarios
Signed-off-by: Deep Debroy <ddebroy@docker.com>

@ddebroy ddebroy force-pushed the ddebroy:migprov1 branch from 37c0250 to 23478f1 Feb 26, 2019

@ddebroy

This comment has been minimized.

Copy link
Member Author

ddebroy commented Feb 26, 2019

Addressed code review comment and squashed commits

@davidz627

This comment has been minimized.

Copy link
Contributor

davidz627 commented Feb 26, 2019

/lgtm

@jsafrane

This comment has been minimized.

Copy link
Member

jsafrane commented Feb 27, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 27, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 27, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 28, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 28, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 3678704 into kubernetes:master Feb 28, 2019

16 checks passed

cla/linuxfoundation ddebroy authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.