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

CSI volume expansion #74863

Merged
merged 5 commits into from Mar 8, 2019

Conversation

@gnufied
Copy link
Member

gnufied commented Mar 3, 2019

Add support for CSI volume expansion.

Added tests for both online and offline expansion of CSI volumes.

Note to reviewers

/sig storage
cc @msau42 @jsafrane @mlmhl

xref kubernetes/enhancements#556

Add support for node side CSI volume expansion
Show resolved Hide resolved pkg/volume/csi/expander.go Outdated
Show resolved Hide resolved pkg/volume/csi/expander.go Outdated
@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Mar 4, 2019

/kind feature
/priority important-soon

@gnufied gnufied changed the title Csi volume expansion CSI volume expansion Mar 4, 2019

@gnufied gnufied force-pushed the gnufied:csi-volume-expansion branch from 5c502ee to 159b6b9 Mar 4, 2019

Show resolved Hide resolved pkg/volume/util/operationexecutor/operation_generator.go
Show resolved Hide resolved pkg/volume/util/operationexecutor/operation_generator.go
)

func (c *csiPlugin) RequiresFSResize() bool {
// We could check plugin's node capability but we instead are going to rely on

This comment has been minimized.

@msau42

msau42 Mar 4, 2019

Member

This is different from how we implement skip attach. Can we be consistent in how we check a plugin's capability?

This comment has been minimized.

@gnufied

gnufied Mar 5, 2019

Author Member

But CanAttach check depends on CSIDriver field too right? Whereas this check is more static. I do not like changing the behaviour of FindByXXX function fwiw depending on feature tags and other dynamic things.

This comment has been minimized.

@msau42

msau42 Mar 5, 2019

Member

maybe we should consider changing skipAttach then (separately) cc @jsafrane

Show resolved Hide resolved pkg/volume/csi/expander.go Outdated
Show resolved Hide resolved pkg/volume/csi/expander.go Outdated
Show resolved Hide resolved pkg/volume/csi/expander.go

fsResizeFunc := func() (error, error) {
_, resizeError := og.resizeFileSystem(volumeToMount, volumeToMount.DevicePath, deviceMountPath, volumePlugin.GetPluginName())
if attachableVolumePlugin != nil {

This comment has been minimized.

@msau42

msau42 Mar 4, 2019

Member

Are device mountable plugins supported here? (doesn't support attach but does support global device mount)

This comment has been minimized.

@msau42

msau42 Mar 4, 2019

Member

could use some unit tests here

This comment has been minimized.

@gnufied

gnufied Mar 5, 2019

Author Member

The only volume type that is not attachable but implements device mountable interface is local storage. So for in-tree volumes this check should be good.

This comment has been minimized.

@gnufied

gnufied Mar 5, 2019

Author Member

Also if CSI plugin is not attachable but supports node expansion, it should be resized after node publish and hence we should be covered there too.

name: external-resizer-cfg
rules:
- apiGroups: [""]
resources: ["endpoints"]

This comment has been minimized.

@msau42

msau42 Mar 4, 2019

Member

we're trying to get all new components to use the Lease mechanism for leader election, not endpoints. cc @andrewsykim

This comment has been minimized.

@gnufied

gnufied Mar 5, 2019

Author Member

We don't use this rbac permission in e2e btw because our e2es run without leader election in external-resizer. The same applies to other sidecar containers too. I have kept it here just to have same version between here and external-resizer repo.

Show resolved Hide resolved test/e2e/storage/csi_mock_volume.go Outdated
Show resolved Hide resolved test/e2e/storage/csi_mock_volume.go
@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Mar 7, 2019

@spiffxp I am not sure I entirely agree (but I could be biased.. :-)).

PR has been under review since Monday, has extensive e2e and falls in code path that should not break any of existing feature. A lot of code change here is refactoring existing code to be cleaner such as renaming FSResize to NodeExpand. I have kept commits that deal with refactoring in separate commits.
In fact about 80% of the code is either refactoring or tests. The code also does not include any API changes fwiw.

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Mar 7, 2019

This may be fair, but keep in mind, large refactors that are going to cause merge conflicts for others will slow us all down. I'm not trying to mandate a hard no here, just asking that you pause and consider, and /hold cancel if you think this is wise and worth it

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Mar 7, 2019

Point noted. I still think we should be fine. This is the only PR that touches volume expansion code path and afaik should not conflict with any other PRs trying to get in for 1.14. I am also running e2es introduced here in a loop to ensure we don't introduce any flakes in the code.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Mar 7, 2019

The vast majority of changes here are pkg/volume or test/e2e/storage - i think it's flyable.

vendor change LGTM

@saad-ali
Copy link
Member

saad-ali left a comment

There are few changes to common code. The majority of functional changes are adding functionality to the CSI package and that functionality flag gated and off by default since this is an alpha feature.

/lgtm
/approve

@@ -676,15 +700,15 @@ func (og *operationGenerator) GenerateMountVolumeFunc(
}
}

func (og *operationGenerator) resizeFileSystem(volumeToMount VolumeToMount, devicePath, deviceMountPath, pluginName string) (simpleErr, detailedErr error) {
func (og *operationGenerator) resizeFileSystem(volumeToMount VolumeToMount, rsOpts volume.NodeResizeOptions, pluginName string) (bool, error) {

This comment has been minimized.

@saad-ali

saad-ali Mar 7, 2019

Member

nit: put a comment explaining what the expected values are that are returned here.

This comment has been minimized.

@gnufied

gnufied Mar 7, 2019

Author Member

I will follow up this in a follow up.

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Mar 7, 2019

/hold cancel
This feature is already planned and tracked for 1.14

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Mar 7, 2019

/assign @smarterclayton

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Mar 7, 2019

approving vendor change as per sig and other leads

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, msau42, saad-ali, smarterclayton

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

gnufied added some commits Feb 11, 2019

Introduce feature gate for volume expansion
Update CSI library version
Rename ExandFS to NodeExpand
Handle resize error in online resizing
Use NodeExpandable plugin to mark volumes that require node expansion
Move resize function parameters to a new ResizeOptions type
This enables us to pass CSI volume phase
Add CSI volume resizing tests
Add some tests for checking node expansion
Add new tests for expander

@gnufied gnufied force-pushed the gnufied:csi-volume-expansion branch from 4582623 to 1bd9ed0 Mar 8, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 8, 2019

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Mar 8, 2019

I had to rebase because of a conflict. @jsafrane @tsmetana can you add lgtm again?

@jsafrane

This comment has been minimized.

Copy link
Member

jsafrane commented Mar 8, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 8, 2019

@k8s-ci-robot k8s-ci-robot merged commit 3624c74 into kubernetes:master Mar 8, 2019

17 checks passed

cla/linuxfoundation gnufied authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
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 Job succeeded.
Details
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

@gnufied gnufied referenced this pull request Mar 8, 2019

Closed

Add support for NodeExpand #10

}

if newSize.Value() < 0 {
return newSize, errors.New("size can not be less than 0")

This comment has been minimized.

@tedyu

tedyu Mar 8, 2019

Contributor

It would be helpful to include the negative value in the message

if err != nil {
return newSize, err
}
updatedQuantity := resource.NewQuantity(resp.CapacityBytes, resource.BinarySI)

This comment has been minimized.

@tedyu

tedyu Mar 8, 2019

Contributor

Do we need to check that updatedQuantity is not nil?

}
for _, capability := range capabilities {
if capability.GetRpc().GetType() == csipbv1.NodeServiceCapability_RPC_EXPAND_VOLUME {
nodeExpandSet = true

This comment has been minimized.

@tedyu

tedyu Mar 8, 2019

Contributor

We can break out of the loop here

@tedyu

This comment has been minimized.

Copy link
Contributor

tedyu commented Mar 8, 2019

Created #75203 for the last comment above.

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.