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

Start adding a KEP for csi volume resizing #780

Merged
merged 1 commit into from
Feb 2, 2019

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Jan 30, 2019

xref #556

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 30, 2019
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/pm sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 30, 2019
@gnufied gnufied changed the title {WIP} Start adding a KEP for csi volume resizing Start adding a KEP for csi volume resizing Jan 30, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2019
@childsb
Copy link
Contributor

childsb commented Jan 30, 2019

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2019
keps/sig-storage/20190129-csi-volume-resizing.md Outdated Show resolved Hide resolved
keps/sig-storage/20190129-csi-volume-resizing.md Outdated Show resolved Hide resolved
keps/sig-storage/20190129-csi-volume-resizing.md Outdated Show resolved Hide resolved
keps/sig-storage/20190129-csi-volume-resizing.md Outdated Show resolved Hide resolved
keps/sig-storage/20190129-csi-volume-resizing.md Outdated Show resolved Hide resolved
keps/sig-storage/20190129-csi-volume-resizing.md Outdated Show resolved Hide resolved
@gnufied
Copy link
Member Author

gnufied commented Jan 31, 2019

@saad-ali addressed all the open comments. PTAL. thanks.

The design of CSI volume resizing is made of two parts.


#### External resize controller
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent should be one level up


#### External resize controller

To support resizing of CSI volumes an external resize controller will monitor all PVCs. If a PVC meets following critirea for resizing, it will be added to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

criteria


#### External resize controller

To support resizing of CSI volumes an external resize controller will monitor all PVCs. If a PVC meets following critirea for resizing, it will be added to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the internal resize controller need any changes to ignore CSI volumes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no - this was resolved couple of releases back when another sig-storage member wanted to externally resize volumes.

To support resizing of CSI volumes an external resize controller will monitor all PVCs. If a PVC meets following critirea for resizing, it will be added to
controller's workqueue:

- The driver name disovered from PVC should match name of driver currently known(by querying driver info via CSI RPC call) to external resize controller.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it get the driver name? From StorageClass?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From PV.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's clarify that.

keps/sig-storage/20190129-csi-volume-resizing.md Outdated Show resolved Hide resolved
@msau42
Copy link
Member

msau42 commented Feb 1, 2019

lgtm please squash

Clarify dependency on online resizing.
@gnufied
Copy link
Member Author

gnufied commented Feb 2, 2019

@msau42 done!

Copy link
Member

@msau42 msau42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: childsb, gnufied, msau42

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

@k8s-ci-robot k8s-ci-robot merged commit 4ba2a6f into kubernetes:master Feb 2, 2019
Copy link
Member

@saad-ali saad-ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm looks like I forgot to hit submit on this.

You can address these in a follow up PR


[umbrella issues]: https://github.com/kubernetes/kubernetes/issues/62096

## Implementation History
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the Implementation History to clarify what has been implemented.


### External resize controller

To support resizing of CSI volumes an external resize controller will monitor all PVCs. If a PVC meets following criteria for resizing, it will be added to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: monitor all changes to PVCs

To support resizing of CSI volumes an external resize controller will monitor all PVCs. If a PVC meets following critirea for resizing, it will be added to
controller's workqueue:

- The driver name disovered from PVC should match name of driver currently known(by querying driver info via CSI RPC call) to external resize controller.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's clarify that.

- The driver name disovered from PVC should match name of driver currently known(by querying driver info via CSI RPC call) to external resize controller.
- Once it notices a PVC has been updated and by comparing old and new PVC object, it determines more space has been requested by the user.

Once PVC gets picked from workqueue, the controller will also compare requested PVC size with actual size of volume in `PersistentVolume`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this check do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to check PV size before actually doing controller resize because it is possible that, volume already has been resized and controller is just resyncing PVCs.

object. Once PVC passes all these checks, a CSI `ControllerExpandVolume` call will be made by the controller if CSI plugin implements `ControllerExpandVolume`
RPC call.

If `ControllerExpandVolume` call is successful and plugin implements `NodeExpandVolume`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i.e. the CSI driver has a EXPAND_VOLUME node capability)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I did not use use capabilities in this document on purpose because I thought "plugins implements NodeExpandVolume "is synonymous with "plugin has EXPAND_VOLUME node capability".

- if `ControllerExpandVolumeResponse` returns `true` in `node_expansion_required` then `FileSystemResizePending` condition will be added to PVC and `NodeExpandVolume` operation will be queued on kubelet. Also volume size reported by PV will be updated to new value.
- if `ControllerExpandVolumeResponse` returns `false` in `node_expansion_required` then volume resize operation will be marked finished and both `pvc.Status.Capacity` and `pv.Spec.Capacity` will report updated value.

If plugin does not implement `NodeExpandVolume` then volume resize operation will be marked as finished and both `pvc.Status.Capacity` and `pv.Spec.Capacity` will report updated value after successful completion of `ControllerExpandVolume` RPC call.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ControllerExpandVolume call is successful and the plugin does not implement...


#### Offline volume resizing on kubelet:

This is the default mode and in this mode `NodeExpandVolume` will only be called when volume is being mounted on the node. In other words, pod that was using the volume must be re-created for expansion on node to happen.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What causes kubelet to start this process of calling NodeExpandVolume? Based on above it seems like some component (mount operation?) watches the PVC and triggers this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - offline resizing is done as part of MountVolume operation (in operation_executor.go). The actual checks we perform are outlined below. I am going to update this document to be more explicit.

RPC call.

If `ControllerExpandVolume` call is successful and plugin implements `NodeExpandVolume`:
- if `ControllerExpandVolumeResponse` returns `true` in `node_expansion_required` then `FileSystemResizePending` condition will be added to PVC and `NodeExpandVolume` operation will be queued on kubelet. Also volume size reported by PV will be updated to new value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So kubelet will wait until the FileSystemResizePending condition is applied to PVC? If so, this smells fishy. My understanding was that conditions were informative for end user and not to be used to trigger behavior in other components?

Copy link
Member Author

@gnufied gnufied Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No kubelet does not check for FileSystemResiznePending condition saad. From proposal:

When a pod that is using the PVC is started, kubelet will compare pvc.spec.resources.requests.storage and pvc.Status.Capacity. It also compares PVC's size with pv.Spec.Capacity and if it detects PV is reporting same size as pvc's spec but PVC's status is still reporting smaller value then it determines - a volume expansion is pending on the node. At this point if plugin implements NodeExpandVolume RPC call then, kubelet will call it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants