add PV size grow feature for azure file #57017

andyzhangx · 2017-12-11T07:00:31Z

What this PR does / why we need it:
According to kubernetes/enhancements#284, add size grow feature for azure file

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #56462

Special notes for your reviewer:
Since azure file is using SMB 3.0 protocal, there is no necessary to resize filesystem on agent side, the agent node will detect the changed size automatically.

Release note:

add size grow feature for azure file

/sig azure
@gnufied @rootfs @brendandburns

andyzhangx · 2017-12-11T07:04:26Z

/assign @gnufied @rootfs

andyzhangx · 2017-12-11T07:52:20Z

/test pull-kubernetes-unit

rootfs · 2017-12-11T20:36:37Z

pkg/cloudprovider/providers/azure/azure_storage.go

@@ -68,3 +68,19 @@ func (az *Cloud) DeleteFileShare(accountName, key, name string) error {
 	return nil

 }
+
+// ResizeFileShare resize a file share
+func (az *Cloud) ResizeFileShare(accountName, accountKey, name string, sizeGB int) error {


cannot comment on unchanged lines but createFileShare looks buggy to me: in event of SetProperties() failure, the share should be deleted, otherwise a quota-less share is left unaccounted...

createFileShare will delete share when hit SetProperties() failure, it's already implemented, see code:
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/azure/azure_file.go#L46-L50

andyzhangx · 2017-12-14T12:34:00Z

@gnufied PTAL

andyzhangx · 2017-12-27T02:36:00Z

@gnufied PTAL, thanks

gnufied

Mostly looks okay. needs some minor changes

gnufied · 2018-01-02T22:10:11Z

pkg/volume/azure_file/azure_file.go

+	shareName := spec.PersistentVolume.Spec.AzureFile.ShareName
+	azure, err := getAzureCloudProvider(plugin.host.GetCloudProvider())
+	if err != nil {
+		glog.V(2).Infof("failed to get azure provider")


Doesn't caller log the returned error anyways? so - rather than logging the similar error from two places, it may be better to augment the original and return a modified value via fmt.Errorf ?

good catch, actually it's not necessary, I have removed those info logs

gnufied · 2018-01-02T22:10:25Z

pkg/volume/azure_file/azure_file.go

+
+	secretName, secretNamespace, err := getSecretNameAndNamespace(spec, spec.PersistentVolume.Spec.ClaimRef.Namespace)
+	if err != nil {
+		glog.V(2).Infof("failed to get secret")


same as before here..

gnufied · 2018-01-02T22:11:04Z

pkg/volume/azure_file/azure_file.go

+	util := &azureSvc{}
+	accountName, accountKey, err := util.GetAzureCredentials(plugin.host, secretNamespace, secretName)
+	if err != nil {
+		glog.V(2).Infof("failed to get account key for secret: %s in namespace: %s", secretName, secretNamespace)


gnufied · 2018-01-02T22:11:57Z

pkg/volume/azure_file/azure_file.go

+		return oldSize, err
+	}
+
+	if err := azure.ResizeFileShare(accountName, accountKey, shareName, int(newSize.Value()/1024/1024/1024)); err != nil {


Can you use - https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util.go#L303 rather than dividing by 1024**3 ?

gnufied · 2018-01-02T22:12:55Z

pkg/cloudprovider/providers/azure/azure_storage.go

+	}
+
+	share := fileClient.GetShareReference(name)
+	share.Properties.Quota = sizeGB


Is it GB or GiB? It appears that you are passing GiB value from other place.

thanks for checking, it's GiB, have changed the var name

andyzhangx

@gnufied thanks for the review. PTAL.
Next I will work on azure disk resize.

andyzhangx · 2018-01-12T07:55:05Z

pkg/cloudprovider/providers/azure/azure_storage.go

+	}
+
+	share := fileClient.GetShareReference(name)
+	share.Properties.Quota = sizeGB


thanks for checking, it's GiB, have changed the var name

andyzhangx · 2018-01-12T07:57:44Z

pkg/volume/azure_file/azure_file.go

+	shareName := spec.PersistentVolume.Spec.AzureFile.ShareName
+	azure, err := getAzureCloudProvider(plugin.host.GetCloudProvider())
+	if err != nil {
+		glog.V(2).Infof("failed to get azure provider")


good catch, actually it's not necessary, I have removed those info logs

andyzhangx · 2018-01-12T07:57:51Z

pkg/volume/azure_file/azure_file.go

+
+	secretName, secretNamespace, err := getSecretNameAndNamespace(spec, spec.PersistentVolume.Spec.ClaimRef.Namespace)
+	if err != nil {
+		glog.V(2).Infof("failed to get secret")


andyzhangx · 2018-01-12T07:57:58Z

pkg/volume/azure_file/azure_file.go

+	util := &azureSvc{}
+	accountName, accountKey, err := util.GetAzureCredentials(plugin.host, secretNamespace, secretName)
+	if err != nil {
+		glog.V(2).Infof("failed to get account key for secret: %s in namespace: %s", secretName, secretNamespace)


andyzhangx · 2018-01-12T07:58:11Z

pkg/volume/azure_file/azure_file.go

+		return oldSize, err
+	}
+
+	if err := azure.ResizeFileShare(accountName, accountKey, shareName, int(newSize.Value()/1024/1024/1024)); err != nil {


andyzhangx · 2018-01-22T02:41:06Z

@karataliu @feiskyer PTAL, thx

feiskyer · 2018-01-22T02:47:26Z

pkg/volume/azure_file/azure_file.go

+	if err != nil {
+		return oldSize, err
+	}
+


Add a check of whether newSize is greater than oldSize?

not sure what @feiskyer meant - but this function should be idempotent and if existing size already satisfies requested new size, no API call should be made. I forgot to mention that.

If you notice corresponding functions in GCE/AWS etc they are all idempotent.

https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L2409

I add a check in ResizeFileShare func

feiskyer · 2018-01-22T02:48:07Z

pkg/volume/azure_file/azure_file.go

@@ -21,17 +21,19 @@ import (
 	"os"
 	"runtime"

+	"k8s.io/apimachinery/pkg/api/resource"


nit: group import with prefix, e.g. k8s.io and others

it's made by IDE automatically...

if remove the empty lines, gofmt will auto do the sort.

karataliu · 2018-01-22T03:02:46Z

pkg/cloudprovider/providers/azure/azure_storage.go

@@ -72,3 +72,19 @@ func (az *Cloud) DeleteFileShare(accountName, key, name string) error {
 	return nil

 }
+
+// ResizeFileShare resize a file share


karataliu · 2018-01-22T03:04:55Z

pkg/volume/azure_file/azure_file.go

 	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/kubernetes/pkg/util/mount"
 	kstrings "k8s.io/kubernetes/pkg/util/strings"
 	"k8s.io/kubernetes/pkg/volume"
+	"k8s.io/kubernetes/pkg/volume/util"


can we sort this import? literals k8s.io and github.com are mixed now

karataliu · 2018-01-22T03:08:56Z

pkg/volume/azure_file/azure_file.go

@@ -139,6 +142,42 @@ func (plugin *azureFilePlugin) newUnmounterInternal(volName string, podUID types
 	}}, nil
 }

+func (plugin *azureFilePlugin) RequiresFSResize() bool {
+	return false


Will not call resize if set to false?

kubernetes/pkg/volume/util/operationexecutor/operation_generator.go

Line 601 in 81b4340

expandableVolumePlugin.RequiresFSResize() &&

RequiresFSResize means require file system resize, azure file don't need this. this value of Azure disk should be true.

karataliu · 2018-01-22T03:11:05Z

pkg/volume/azure_file/azure_file.go

+	newSize resource.Quantity,
+	oldSize resource.Quantity) (resource.Quantity, error) {
+
+	if spec.PersistentVolume != nil && spec.PersistentVolume.Spec.AzureFile == nil {


better be spec.PersistentVolume==nil || ...

Otherwise if spec.PersistentVolume is nil, Line#157 will raise null reference exception.

good catch, fixed.

karataliu · 2018-01-22T03:16:45Z

pkg/volume/azure_file/azure_provision.go

@@ -41,6 +41,8 @@ type azureCloudProvider interface {
 	CreateFileShare(name, storageAccount, storageType, location string, requestGB int) (string, string, error)
 	// delete a file share
 	DeleteFileShare(accountName, key, name string) error
+	// resize a file share
+	ResizeFileShare(accountName, accountKey, name string, sizeGiB int64) error


ResizeFileShare uses sizeGiB here, CreateFileShare uses 'requestGB', that may be confusing. Since both use 'GiB' in in reality, consider renaming requestGB to sizeGiB.

andyzhangx · 2018-01-22T08:19:17Z

@gnufied @feiskyer @karataliu PTAL, thanks.

feiskyer

/lgtm
/approve

andyzhangx · 2018-01-22T13:30:27Z

/assign @brendandburns PTAL, some code is out of Azure code

gnufied · 2018-01-22T14:52:56Z

/lgtm

jdumars · 2018-01-22T22:56:42Z

/cc @brendandburns

andyzhangx · 2018-01-26T10:49:35Z

/test pull-kubernetes-e2e-gce

andyzhangx · 2018-01-26T10:49:55Z

@brendandburns PTAL, some code is out of Azure code

andyzhangx · 2018-01-30T02:03:15Z

/assign @brendandburns PTAL

enable azure file grow size fix according to comments fix comments fix review comments fix comments

andyzhangx · 2018-01-31T02:18:28Z

/test pull-kubernetes-bazel-test

fix test build failure

andyzhangx · 2018-02-01T02:34:01Z

@feiskyer @brendandburns rebased, PTAL.

feiskyer · 2018-02-01T06:18:41Z

/lgtm
/approve

rootfs · 2018-02-05T16:31:20Z

/assign @deads2k for approval

deads2k · 2018-02-05T18:12:11Z

/approve

k8s-ci-robot · 2018-02-05T18:12:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, deads2k, feiskyer, gnufied

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

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

~~cmd/kube-controller-manager/OWNERS~~ [deads2k]
~~pkg/cloudprovider/providers/azure/OWNERS~~ [andyzhangx,feiskyer]
~~pkg/volume/azure_file/OWNERS~~ [andyzhangx,feiskyer,gnufied]
~~plugin/pkg/admission/OWNERS~~ [deads2k]

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

k8s-github-robot · 2018-02-05T18:48:10Z

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

k8s-github-robot · 2018-02-05T19:25:47Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/azure size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 11, 2017

k8s-github-robot assigned thockin and nikhiljindal Dec 11, 2017

k8s-ci-robot assigned gnufied and rootfs Dec 11, 2017

rootfs reviewed Dec 11, 2017

View reviewed changes

thockin removed their assignment Dec 15, 2017

andyzhangx mentioned this pull request Dec 27, 2017

add PV resize support for azure disk #56463

Closed

andyzhangx changed the title ~~add size grow feature for azure file~~ add PV size grow feature for azure file Jan 2, 2018

gnufied reviewed Jan 2, 2018

View reviewed changes

andyzhangx commented Jan 12, 2018

View reviewed changes

andyzhangx force-pushed the azurefile-growsize branch 3 times, most recently from 78ecdbf to fe6a549 Compare January 12, 2018 08:31

feiskyer reviewed Jan 22, 2018

View reviewed changes

karataliu reviewed Jan 22, 2018

View reviewed changes

feiskyer self-assigned this Jan 22, 2018

karataliu reviewed Jan 22, 2018

View reviewed changes

feiskyer reviewed Jan 22, 2018

View reviewed changes

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

k8s-ci-robot requested a review from brendandburns January 22, 2018 22:56

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

initial work for azure file grow size implementation

fc988d4

enable azure file grow size fix according to comments fix comments fix review comments fix comments

andyzhangx force-pushed the azurefile-growsize branch from 76ad030 to 8a645f2 Compare January 30, 2018 14:03

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 30, 2018

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 30, 2018

fix rebase error

bf0c6d8

fix test build failure

andyzhangx force-pushed the azurefile-growsize branch from 8a645f2 to bf0c6d8 Compare January 31, 2018 03:02

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2018

k8s-github-robot merged commit ffda1e2 into kubernetes:master Feb 5, 2018

add PV size grow feature for azure file #57017

add PV size grow feature for azure file #57017

Conversation

andyzhangx commented Dec 11, 2017

andyzhangx commented Dec 11, 2017

andyzhangx commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Dec 14, 2017

andyzhangx commented Dec 27, 2017

gnufied 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

Choose a reason for hiding this comment

andyzhangx 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

andyzhangx commented Jan 22, 2018

Choose a reason for hiding this comment

gnufied Jan 22, 2018 • 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

karataliu Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

andyzhangx commented Jan 22, 2018

feiskyer left a comment

Choose a reason for hiding this comment

andyzhangx commented Jan 22, 2018

gnufied commented Jan 22, 2018

jdumars commented Jan 22, 2018

andyzhangx commented Jan 26, 2018

andyzhangx commented Jan 26, 2018

andyzhangx commented Jan 30, 2018

andyzhangx commented Jan 31, 2018

andyzhangx commented Feb 1, 2018

feiskyer commented Feb 1, 2018

rootfs commented Feb 5, 2018

deads2k commented Feb 5, 2018

k8s-ci-robot commented Feb 5, 2018

k8s-github-robot commented Feb 5, 2018

k8s-github-robot commented Feb 5, 2018

gnufied Jan 22, 2018 •

edited

Loading

karataliu Jan 22, 2018 •

edited

Loading