API changes to support migration of inline in-tree volumes to CSI #77703

ddebroy · 2019-05-09T22:51:08Z

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 brings in the API changes needed to support migration of inline in-tree volumes to CSI. It introduces a new field InlineVolumeSpec *v1.PersistentVolumeSpec in VolumeAttachmentSource.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Supersedes https://github.com/kubernetes/kubernetes/pull/77364/files

Design (referred from KEP): https://github.com/kubernetes/community/blob/master/contributors/design-proposals/storage/csi-migration.md#in-line-volumes

Does this PR introduce a user-facing change?:

NONE

ddebroy · 2019-05-09T22:52:23Z

/sig storage
/cc @davidz627 @msau42 @leakingtapan

davidz627 · 2019-05-09T23:20:25Z

/assign @davidz627 @liggitt @msau42
/label api-review

davidz627 · 2019-05-09T23:21:40Z

/priority important-critical
/kind feature

davidz627 · 2019-05-09T23:23:40Z

/remove-priority critical-urgent
/priority important-soon

pkg/apis/storage/types.go

pkg/apis/storage/validation/validation.go

pkg/registry/storage/volumeattachment/strategy.go

pkg/apis/storage/validation/validation.go

ddebroy · 2019-05-10T05:23:17Z

/retest

ddebroy · 2019-05-29T20:30:56Z

/retest

davidz627 · 2019-05-29T23:56:01Z

/retest

fejta-bot · 2019-05-30T03:46:15Z

/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.

dstrebel · 2019-05-30T03:52:40Z

Hello! We are starting Code Freeze for 1.15. Is this PR still planned to be included in 1.15?

liggitt · 2019-05-30T03:59:12Z

yes

/retest

ddebroy · 2019-05-30T06:53:33Z

/test pull-kubernetes-kubemark-e2e-gce-big

ddebroy · 2019-05-30T07:02:06Z

So looks like while this PR was making it's way through, the Azure file CSI translator also landed in master. Will reconcile the changes here with the required interface in Azure file and resubmit

I0530 06:49:56.791] /bazel-scratch/.cache/bazel/_bazel_prow/48d5366022b4e3197674c8d6e2bee219/sandbox/linux-sandbox/1591/execroot/io_k8s_kubernetes/staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go:45:5: cannot use &azureDiskCSITranslator literal (type *azureDiskCSITranslator) as type InTreePlugin in assignment:
I0530 06:49:56.792] *azureDiskCSITranslator does not implement InTreePlugin (missing CanSupportInline method)
I0530 06:49:56.793] /bazel-scratch/.cache/bazel/_bazel_prow/48d5366022b4e3197674c8d6e2bee219/sandbox/linux-sandbox/1591/execroot/io_k8s_kubernetes/staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go:53:9: cannot use &azureDiskCSITranslator literal (type *azureDiskCSITranslator) as type InTreePlugin in return argument:
I0530 06:49:56.793] *azureDiskCSITranslator does not implement InTreePlugin (missing CanSupportInline method)
I0530 06:49:56.794] /bazel-scratch/.cache/bazel/_bazel_prow/48d5366022b4e3197674c8d6e2bee219/sandbox/linux-sandbox/1591/execroot/io_k8s_kubernetes/staging/src/k8s.io/csi-translation-lib/plugins/azure_file.go:40:5: cannot use &azureFileCSITranslator literal (type *azureFileCSITranslator) as type InTreePlugin in assignment:
I0530 06:49:56.794] *azureFileCSITranslator does not implement InTreePlugin (missing CanSupportInline method)
I0530 06:49:56.796] /bazel-scratch/.cache/bazel/_bazel_prow/48d5366022b4e3197674c8d6e2bee219/sandbox/linux-sandbox/1591/execroot/io_k8s_kubernetes/staging/src/k8s.io/csi-translation-lib/plugins/azure_file.go:48:9: cannot use &azureFileCSITranslator literal (type *azureFileCSITranslator) as type InTreePlugin in return argument:
I0530 06:49:56.796] *azureFileCSITranslator does not implement InTreePlugin (missing CanSupportInline method)

fejta-bot · 2019-05-30T09:22:05Z

/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.

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

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

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

ddebroy · 2019-05-30T09:38:38Z

@dstrebel please note that this PR was approved and LGTMed before code freeze. However as it was going through the testing as part of final merge to master, a new PR landed in master that added AzureFile and AzureDisk support for CSI translation and those two plugins needed to be enhanced to add inline volume support interfaces.

So this PR now needed to be updated to enhance AzureFile and AzureDisk with the CSI translation support for inline volumes to resolve the build failures detected during the second round testing before merging to master: #77703 (comment)

ddebroy · 2019-05-30T09:45:42Z

/cc @andyzhangx

andyzhangx

/lgtm
LGTM for azure part, thanks

ddebroy · 2019-05-30T11:41:15Z

/retest

liggitt · 2019-05-30T13:34:37Z

staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go

+		},
+	}
+
+	if *azureSource.CachingMode != "" {


should this be if azureSource.CachingMode != nil && *azureSource.CachingMode {?

good catch! I think we should just check for if azureSource.CachingMode != nil rather than something along the lines of if azureSource.CachingMode != nil && *azureSource.CachingMode != "" So:

if azureSource.CachingMode != nil { pv.Spec.PersistentVolumeSource.CSI.VolumeAttributes[azureDiskCachingMode] = string(*azureSource.CachingMode) }

Seems like this is something applicable to the above code as well for the regular TranslateInTreePVToCSI already there for AzureDisk at https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go#L79.

#78524 to fix it it both places.

liggitt · 2019-05-30T13:35:01Z

staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go

+	if *azureSource.CachingMode != "" {
+		pv.Spec.PersistentVolumeSource.CSI.VolumeAttributes[azureDiskCachingMode] = string(*azureSource.CachingMode)
+	}
+	if *azureSource.FSType != "" {


should this be if azureSource.FSType != nil && *azureSource.FSType {?

Thinking of changing this block to:

if azureSource.FSType != nil { pv.Spec.PersistentVolumeSource.CSI.FSType = *azureSource.FSType pv.Spec.PersistentVolumeSource.CSI.VolumeAttributes[azureDiskFSType] = *azureSource.FSType }

Applies to both inline portion in this PR as well as https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go#L83 .

#78524 to fix it in both places.

Not quite sure @andyzhangx why the FSType needs to be passed as a VolumeAttribute parameter on top of the field?

liggitt · 2019-05-30T13:35:32Z

staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go

+					Driver:           AzureDiskDriverName,
+					VolumeHandle:     azureSource.DataDiskURI,
+					ReadOnly:         *azureSource.ReadOnly,
+					FSType:           *azureSource.FSType,


similar comments about nil checks on these fields before dereferencing them

#78524 to fix this both above and in TranslateInTreePVToCSI

tedyu · 2019-05-31T21:34:58Z

pkg/registry/storage/volumeattachment/strategy.go

@@ -53,13 +55,20 @@ func (volumeAttachmentStrategy) PrepareForCreate(ctx context.Context, obj runtim
 		groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
 	}

+	volumeAttachment := obj.(*storage.VolumeAttachment)


shouldn't we check that the cast was successful ?

no, this is well tested and is a programmer error if any other type comes through here

What do you think of the following ?

diff --git a/pkg/registry/storage/volumeattachment/strategy.go b/pkg/registry/storage/volumeattachment/strategy.go index 183fe02d95..b3468d9370 100644 --- a/pkg/registry/storage/volumeattachment/strategy.go +++ b/pkg/registry/storage/volumeattachment/strategy.go @@ -48,20 +48,17 @@ func (volumeAttachmentStrategy) NamespaceScoped() bool { } // ResetBeforeCreate clears the Status field which is not allowed to be set by end users on creation. -func (volumeAttachmentStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { +func (volumeAttachmentStrategy) PrepareForCreate(ctx context.Context, volumeAttachment *storage.VolumeAttachment) { var groupVersion schema.GroupVersion if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} } - volumeAttachment := obj.(*storage.VolumeAttachment) - switch groupVersion { case storageapiv1beta1.SchemeGroupVersion: // allow modification of status for v1beta1 default: - volumeAttachment := obj.(*storage.VolumeAttachment) volumeAttachment.Status = storage.VolumeAttachmentStatus{} }

This would make the parameter type more visible.

thanks

No, PrepareForCreate is an interface all strategies implement

thanks for the explanation.

tedyu · 2019-05-31T23:38:05Z

pkg/registry/storage/volumeattachment/strategy.go

@@ -53,13 +55,20 @@ func (volumeAttachmentStrategy) PrepareForCreate(ctx context.Context, obj runtim
 		groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
 	}

+	volumeAttachment := obj.(*storage.VolumeAttachment)
+
 	switch groupVersion {
 	case storageapiv1beta1.SchemeGroupVersion:
 		// allow modification of status for v1beta1
 	default:
 		volumeAttachment := obj.(*storage.VolumeAttachment)


This line can be removed.

The assignment happens on line 58

Hi @tedyu, thank you for the suggestions. This PR has already merged. Could you please submit a cleanup PR for the nit you mentioned

k8s-ci-robot requested review from davidz627, leakingtapan and msau42 May 9, 2019 22:52

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label May 9, 2019

k8s-ci-robot requested review from dchen1107 and deads2k May 9, 2019 22:52

k8s-ci-robot assigned davidz627, liggitt and msau42 May 9, 2019

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label May 9, 2019

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 9, 2019

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels May 9, 2019

msau42 reviewed May 10, 2019

View reviewed changes

pkg/apis/storage/types.go Outdated Show resolved Hide resolved

pkg/apis/storage/validation/validation.go Outdated Show resolved Hide resolved

pkg/registry/storage/volumeattachment/strategy.go Show resolved Hide resolved

pkg/apis/storage/validation/validation.go Show resolved Hide resolved

ddebroy added 3 commits May 30, 2019 09:34

API changes to support CSI migration of inline volumes

c34309a

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

Generated code for API changes to support CSI migration of inline vol…

df19622

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

Populate InlineVolumeSpec in CSI attacher and translation library

de7be9d

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

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 30, 2019

k8s-ci-robot requested a review from andyzhangx May 30, 2019 09:45

k8s-ci-robot assigned andyzhangx May 30, 2019

andyzhangx approved these changes May 30, 2019

View reviewed changes

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

liggitt reviewed May 30, 2019

View reviewed changes

ddebroy mentioned this pull request May 30, 2019

Need nil checks for Azure CSI translation checks #78524

Closed

k8s-ci-robot merged commit b7fa33e into kubernetes:master May 31, 2019

tedyu reviewed May 31, 2019

View reviewed changes

tedyu mentioned this pull request Jun 3, 2019

Remove redundant assignment to volumeAttachment #78659

Merged

ddebroy mentioned this pull request Jun 8, 2019

Add support for attachment of inline volumes migrated to CSI kubernetes-csi/external-attacher#154

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API changes to support migration of inline in-tree volumes to CSI #77703

API changes to support migration of inline in-tree volumes to CSI #77703

ddebroy commented May 9, 2019 •

edited by jsafrane

Loading

ddebroy commented May 9, 2019

davidz627 commented May 9, 2019

davidz627 commented May 9, 2019

davidz627 commented May 9, 2019

ddebroy commented May 10, 2019

ddebroy commented May 29, 2019

davidz627 commented May 29, 2019

fejta-bot commented May 30, 2019

dstrebel commented May 30, 2019

liggitt commented May 30, 2019

ddebroy commented May 30, 2019

ddebroy commented May 30, 2019

fejta-bot commented May 30, 2019

ddebroy commented May 30, 2019 •

edited

Loading

ddebroy commented May 30, 2019

andyzhangx left a comment

ddebroy commented May 30, 2019

liggitt May 30, 2019 •

edited

Loading

ddebroy May 30, 2019 •

edited

Loading

liggitt May 30, 2019

ddebroy May 30, 2019 •

edited

Loading

liggitt May 30, 2019

ddebroy May 30, 2019

tedyu May 31, 2019

liggitt May 31, 2019

tedyu May 31, 2019

liggitt May 31, 2019

tedyu Jun 1, 2019

tedyu May 31, 2019

davidz627 Jun 3, 2019

tedyu Jun 3, 2019

API changes to support migration of inline in-tree volumes to CSI #77703

API changes to support migration of inline in-tree volumes to CSI #77703

Conversation

ddebroy commented May 9, 2019 • edited by jsafrane Loading

ddebroy commented May 9, 2019

davidz627 commented May 9, 2019

davidz627 commented May 9, 2019

davidz627 commented May 9, 2019

ddebroy commented May 10, 2019

ddebroy commented May 29, 2019

davidz627 commented May 29, 2019

fejta-bot commented May 30, 2019

dstrebel commented May 30, 2019

liggitt commented May 30, 2019

ddebroy commented May 30, 2019

ddebroy commented May 30, 2019

fejta-bot commented May 30, 2019

ddebroy commented May 30, 2019 • edited Loading

ddebroy commented May 30, 2019

andyzhangx left a comment

Choose a reason for hiding this comment

ddebroy commented May 30, 2019

liggitt May 30, 2019 • edited Loading

Choose a reason for hiding this comment

ddebroy May 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy May 30, 2019 • 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

ddebroy commented May 9, 2019 •

edited by jsafrane

Loading

ddebroy commented May 30, 2019 •

edited

Loading

liggitt May 30, 2019 •

edited

Loading

ddebroy May 30, 2019 •

edited

Loading

ddebroy May 30, 2019 •

edited

Loading