Add azuredisk PV size grow feature

[andyzhangx]

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

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 #56463

Special notes for your reviewer:

• This feature is ony for azure managed disk, and if that disk is already attached to a running VM, disk resize will fail as following:

$ kubectl describe pvc pvc-azuredisk
Events:
  Type     Reason              Age               From           Message
  ----     ------              ----              ----           -------
  Warning  VolumeResizeFailed  51s (x3 over 3m)  volume_expand  Error expanding volume "default/pvc-azuredisk" of plugin kubernetes.io/azure-disk : disk.DisksClient#CreateOrUpdate: Failure responding to request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=409 Code="OperationNotAllowed" Message="Cannot resize disk andy-mg1102-dynamic-pvc-d2d00dd9-6185-11e8-a6c3-000d3a0643a8 while it is attached to running VM /subscriptions/.../resourceGroups/.../providers/Microsoft.Compute/virtualMachines/k8s-agentpool-17607330-0."

• Note: if run into such error issue, workaround is delete the pod or even Deployment/StatefulSet, make sure the disk is in unattached state and run kubectl edit pvc ... operation again.

How to use azure disk size grow feature

• In the beginning, pls make sure the azure disk PVC is created by kubernetes.io/azure-disk storage class with allowVolumeExpansion: true (default is false)

kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: hdd
provisioner: kubernetes.io/azure-disk
parameters:
  skuname: Standard_LRS
  kind: Managed
  cachingmode: None
allowVolumeExpansion: true

• Before run kubectl edit pvc pvc-azuredisk operation, pls make sure this PVC is not mounted by any pod(change the replica count to 0, this will terminate the pod and detach the disk, need wait a few minutes) otherwise there would be resize error. Now run kubectl edit pvc pvc-azuredisk to change azuredisk PVC size from 6GB to 10GB

# Please edit the object below. Lines beginning with a '#' will be ignored,
# and an empty file will abort the edit. If an error occurs while saving this file will be
# reopened with the relevant failures.
#
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  annotations:
...
...
  name: pvc-azuredisk
...
spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 6Gi
  storageClassName: hdd
  volumeMode: Filesystem
  volumeName: pvc-d2d00dd9-6185-11e8-a6c3-000d3a0643a8
status:
  accessModes:
  - ReadWriteOnce
  capacity:
    storage: 6Gi
  conditions:
  - lastProbeTime: null
    lastTransitionTime: 2018-05-27T08:14:34Z
    message: Waiting for user to (re-)start a pod to finish file system resize of
      volume on node.
    status: "True"
    type: FileSystemResizePending
  phase: Bound

• After resized, run kubectl describe pvc pvc-azuredisk to check PVC status:

$ kubectl describe pvc pvc-azuredisk
Name:          pvc-azuredisk
Namespace:     default
StorageClass:  hdd
Status:        Bound
...
Capacity:      5Gi
Access Modes:  RWO
Conditions:
  Type                      Status  LastProbeTime                     LastTransitionTime                Reason  Message
  ----                      ------  -----------------                 ------------------                ------  -------
  FileSystemResizePending   True    Mon, 01 Jan 0001 00:00:00 +0000   Wed, 29 Aug 2018 02:29:52 +0000           Waiting for user to (re-)start a pod to finish file system resize of volume on node.
Events:
  Type       Reason                 Age    From                         Message
  ----       ------                 ----   ----                         -------
  Normal     ProvisioningSucceeded  3m57s  persistentvolume-controller  Successfully provisioned volume pvc-d7d250c1-ab32-11e8-bfaf-000d3a4e76db using kubernetes.io/azure-disk
Mounted By:  <none>

• Create a pod mounting with this PVC, you will get

$ kubectl exec -it nginx-azuredisk -- bash
# df -h
Filesystem      Size  Used Avail Use% Mounted on
...
/dev/sdf        9.8G   16M  9.3G   1% /mnt/disk
...

Note: volume expansion feature is beta in v1.11

Release note:

Add azuredisk size grow feature

/sig azure
/assign @feiskyer @karataliu @gnufied
cc @khenidak

[andyzhangx]

/assign @deads2k

[feiskyer]

report error early if oldSize > newSize?

[andyzhangx]

aws & gce won't report error, let's be consistent with that

[feiskyer]

nit: Add a info log here with diskname, oldSize and newSize

[andyzhangx]

I added the logs in ManagedDiskController.ResizeDisk func

[gnufied]

How fast is resize operation on Azure btw? If it is slow and controller crashes while resize is in progress, I am wondering if there is a way to query the volume and find out if resize is in progress with cloudprovider and return early from here rather than calling resize again.

I know GCE and AWS does not implement this mechanism currently but I am going to change it for AWS at least because resize is pretty slow there.

[andyzhangx]

@gnufied azure disk resize operation could be completed in a few seconds, while Azure does not support resize when that disk is attached to a running VM, GCE and AWS supports that, right?

[gnufied]

good point, shouldn't ResizeDisk call then check if volume is attached somewhere before allowing volume resize operation? Expand controller does not have any kind of fencing to prevent volumes from being resused when mounted on nodes.

[andyzhangx]

@gnufied if that disk is already attached to a running VM, the API call will fail with error, eventually ResizeDisk will return error, that's expected case.

[gnufied]

I don't know much about Azure tbh, but I have found that it is less costly to make a GET request and return early than make a POST request and handle error and return (cloudproviders seem to have different quotas for mutable and immutable API requests).

And as far as I can tell, you are already fetching disk state from cloudprovider - https://github.com/kubernetes/kubernetes/pull/64386/files#diff-db9a5ad5d2cc7740ca2b73ad9d904fa2R144, so if we can ascertain that volume is attached, we should return error before even attempting to call resize.

This is not a blocker though, I know very little about Azure - so take my comment with a pinch of salt. :-)

[andyzhangx]

@gnufied thanks for the check, while according to https://docs.microsoft.com/en-us/rest/api/compute/disks/get, the returned value DiskProperties.ProvisioningState is not sufficient to tell whether disk is in Attached state or not, we need another GET request to get diskState. So for this PR, I think it's ok to do resize directly.

[feiskyer]

After user run sudo resize2fs /dev/sdf in agent node, /mnt/disk becomes 10GB now:

Is this user operation required? why not resizeFilesystem being called?

[andyzhangx]

@feiskyer addressed your comments, PTAL

[andyzhangx]

aws & gce won't report error, let's be consistent with that

[andyzhangx]

I added the logs in ManagedDiskController.ResizeDisk func

[andyzhangx]

@feiskyer little change about pointer operation in golang, PTAL, thanks.

[andyzhangx]

@gnufied thanks for the check, while according to https://docs.microsoft.com/en-us/rest/api/compute/disks/get, the returned value DiskProperties.ProvisioningState is not sufficient to tell whether disk is in Attached state or not, we need another GET request to get diskState. So for this PR, I think it's ok to do resize directly.

[gnufied]

/lgtm

[feiskyer]

/lgtm
/approve

[andyzhangx]

@deads2k PTAL, thanks.
You may only take a look at plugin/pkg/admission/storage/persistentvolume/resize/admission.go if you don't have time, thanks.

[andyzhangx]

just rebased.
@deads2k @brendandburns
PTAL, thanks.
You may only take a look at plugin/pkg/admission/storage/persistentvolume/resize/admission.go if you don't have time, thanks.

@feiskyer would you mark as v1.11 milestone, thanks.

[feiskyer]

/milestone v1.11

[feiskyer]

/status approved-for-milestone

[gnufied]

/lgtm

[andyzhangx]

/assign @brendandburns

[k8s-github-robot]

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@andyzhangx @brendandburns @deads2k @feiskyer @gnufied @karataliu

Pull Request Labels

• sig/azure: 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/bug: Fixes a bug discovered during the current release.

Help

• Additional instructions
• Commands for setting labels

[brendandburns]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

• pkg/cloudprovider/providers/azure/OWNERS [andyzhangx,brendandburns,feiskyer]
• pkg/volume/azure_dd/OWNERS [andyzhangx,brendandburns,feiskyer,gnufied]
• plugin/pkg/admission/OWNERS [brendandburns]

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

[k8s-github-robot]

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

[k8s-github-robot]

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

[echo-devnull]

I'm sorry to say that this does not fully work when combined with operators who take control of pods.

For instance: Strimzi kafka operator does not allow me to scale down the pods of 1 statefulset to 0. It will immediately start a new pod because it's trying to keep the cluster "up" . That's it's job as an operator.

So seamless expansion of volumes perhaps should be next on the list ?

Or at least change ALLOWVOLUMEEXPANSION to false? That will keep people from making wrong assumptions in the future ;-)

[lebenitza]

@markmaas , I have the exactly same problem with strimzi, you can essentially do all that by scaling down the operator to 0 too but that's not the workaround I want to do from a platform like Azure... So essentially the thing you guys implemented creates downtime even if you like it or not... I cannot take my pods one by one and rollout the new PVC size.

If a pod is killed and the underlying PVC has a resize pending, postpone the attachment, make sure to detach the volume, resize and let the pod attach it again...

Like @markmaas is saying, if we knew clearly PVC resize is not supported by not allowing the storage class the allowVolumeExpansion flag and what is more, defaulting it on true, we would have not deploy such a thing in AKS or Azure for that matter.

But I guess Azure is known for creating more problems than actual fixing so that's on us.

[shan0809]

@markmaas , I have the exactly same problem with strimzi, you can essentially do all that by scaling down the operator to 0 too but that's not the workaround I want to do from a platform like Azure... So essentially the thing you guys implemented creates downtime even if you like it or not... I cannot take my pods one by one and rollout the new PVC size.

If a pod is killed and the underlying PVC has a resize pending, postpone the attachment, make sure to detach the volume, resize and let the pod attach it again...

Like @markmaas is saying, if we knew clearly PVC resize is not supported by not allowing the storage class the allowVolumeExpansion flag and what is more, defaulting it on true, we would have not deploy such a thing in AKS or Azure for that matter.

But I guess Azure is known for creating more problems than actual fixing so that's on us.

also i think expansion feature is pretty smooth in GCP and AWS. Cummon Azure, you can do better :)

[Morriz]

Why was this accepted? This PR does not satisfy k8s supported workflow that supports disk resize on a Running VM.

[AlexGoris-KasparSolutions]

Ran into the same issue as the last 2-3 comments have pointed out, having to scale down any deployments/statefulsets before the disk resize will go through is really a shame and defeats the purpose of having an orchestrator which is supposed to manage these things for me. In case an operator is involved (in my case Elastic ECK operator), like @lebenitza has pointed out (thanks for the tip!), you also have to scale down the operator to avoid it scaling the deployment/stateful set up again before the resize has completed.

Essentially this means that resizing a PVC is not possible without complete downtime and unavailability of involved services, which strictly is not stated as a requirement for having `` set to true (docs), but I believe that is what the kubernetes devs intended to communicate with this flag, and I'll open an issue on the kubernetes github to ask whether this is the case and should be updated in the docs.

[andyzhangx]

https://docs.microsoft.com/en-us/azure/aks/azure-disk-csi#resize-a-persistent-volume-without-downtime

Currently, Azure disk CSI driver supports resizing PVCs without downtime on specific regions. Follow this link to register the disk online resize feature. If your cluster is not in the supported region list, you need to delete application first to detach disk on the node before expanding PVC.