CSIDriver: allow "StorageCapacity" to be modified #101789

pohly · 2021-05-07T13:18:59Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

When originally introduced, the field was made immutable to be
consistent with the other fields. But in practice allowing it to be
toggled makes more sense, in particular when considering the rollout
of a CSI driver (let it run without using the published
CSIStorageCapacity object, then flip the field, or upgrading from a
driver without support to one which supports it).

The only consumer of this field, the kube-scheduler, can handle
mutation without problems because it always consults the informer
cache to get the current value.

Which issue(s) this PR fixes:

Fixes #100089

Does this PR introduce a user-facing change?

CSIDriver.Spec.StorageCapacity can now be modified.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1472-storage-capacity-tracking

k8s-ci-robot · 2021-05-07T13:19:06Z

@pohly: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fejta-bot · 2021-05-07T13:55:38Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

pohly · 2021-05-26T07:32:22Z

/assign @msau42

msau42

Regarding safety of relaxing the mutability of this check:

There are no system or csi controllers that create/update/delete this object
The scheduler looks up the cached informer object every time
There are no other consumers of this field

msau42 · 2021-06-02T01:20:38Z

pkg/apis/storage/validation/validation.go

@@ -430,7 +430,6 @@ func ValidateCSIDriverUpdate(new, old *storage.CSIDriver) field.ErrorList {
 	allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(new.Spec.FSGroupPolicy, old.Spec.FSGroupPolicy, field.NewPath("spec", "fsGroupPolicy"))...)
 	allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(new.Spec.PodInfoOnMount, old.Spec.PodInfoOnMount, field.NewPath("spec", "podInfoOnMount"))...)
 	allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(new.Spec.VolumeLifecycleModes, old.Spec.VolumeLifecycleModes, field.NewPath("spec", "volumeLifecycleModes"))...)
-	allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(new.Spec.StorageCapacity, old.Spec.StorageCapacity, field.NewPath("spec", "storageCapacity"))...)


The comment in types.go should also be updated: https://github.com/kubernetes/kubernetes/blob/1f8b098e5af29392f1cb9af808a8bf915877b67f/pkg/apis/storage/types.go#L360

This comment is still outstanding

Oops, sorry for that. Instead of simply dropping the line, I decided to include historic information to avoid the mistake mentioned by Jordan (developer develops and tests with 1.23, then updating the field fails with older Kubernetes):

This field was immutable in Kubernetes <= 1.22 and now is mutable.

msau42 · 2021-06-02T01:44:37Z

pkg/apis/storage/validation/validation.go

@@ -430,7 +430,6 @@ func ValidateCSIDriverUpdate(new, old *storage.CSIDriver) field.ErrorList {
 	allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(new.Spec.FSGroupPolicy, old.Spec.FSGroupPolicy, field.NewPath("spec", "fsGroupPolicy"))...)
 	allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(new.Spec.PodInfoOnMount, old.Spec.PodInfoOnMount, field.NewPath("spec", "podInfoOnMount"))...)
 	allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(new.Spec.VolumeLifecycleModes, old.Spec.VolumeLifecycleModes, field.NewPath("spec", "volumeLifecycleModes"))...)
-	allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(new.Spec.StorageCapacity, old.Spec.StorageCapacity, field.NewPath("spec", "storageCapacity"))...)


The scheduler may also need updating similar to #100026 (comment). Would a pod that failed scheduling be schedulable after this object is added/removed/updated?

The scheduler may also need updating similar to #100026 (comment).

That link just takes me to the PR. Are you suggesting that the scheduler needs to watch CSIDriver objects and attempt to reschedule pods when those objects change?

Would a pod that failed scheduling be schedulable after this object is added/removed/updated?

Yes. Scheduling might be blocked when the value was "true" and there is no capacity. When the value becomes "false", scheduling could proceed.

But this seems a bit unlikely. The main use case for mutating CSIDriver was to start with the "false" and later switch to "true".

My expectation was that pod scheduling is going to be retried. That seems sufficient for the less common other direction.

Even though it may not be common, it would be good to handle for cases such as rollback/disablement. Similarly, the scheduler probably needs to handle CSIStorageCapacity objects as well. Anyway, I think this is orthogonal to the api change and can be investigated separately.

But pod scheduling for true -> false is handled, isn't it? It just doesn't happen immediately after the change.

cc @ahg-g

My old understanding is that we would move the pod back to the scheduling queue after some period of time, but I'm not sure if that's still the case anymore and if it's all event based.

It might make sense to fold the results of this discussion/PR into an update in https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/1472-storage-capacity-tracking and CSIDriver docs to clarify the rollout of an update to a driver that adds support for this feature. If this field becomes mutable in 1.23, I suspect people will test their driver update against latest/1.23+ versions and be surprised when their apply/update fails on 1.21 and 1.22

We are working towards scheduling be purely event based. But we still have a routine that puts Pods back into the scheduling queue after 1min.
Either way, the volumebinding plugin already knows to react to CSIDriver updates https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go#L116

if this is dropping the immutability validation, it needs to start validating the new value is valid (calling validateStorageCapacity instead), right?

Shouldn't we just call ValidateCSIDriver on the new object to be future proof against other mutability changes?

PR for the KEP: kubernetes/enhancements#2854

I'm not sure whether we have specific documentation for CSIDriver besides the auto-generated API documentation which will be updated as part of this PR.

It seems like speeding up scheduler requeuing of unschedulable pods if a relevant reason they were unschedulable changes could be done separately from this PR (but would be worth tracking in an issue)

#104250

It turned out that #100003 already took care of this.

msau42 · 2021-06-02T01:49:11Z

/label api-review

liggitt · 2021-08-05T15:18:38Z

pkg/apis/storage/validation/validation_test.go

@@ -2063,12 +2069,6 @@ func TestCSIDriverValidationUpdate(t *testing.T) {
 				new.Spec.FSGroupPolicy = &fileFSGroupPolicy
 			},
 		},


add a test for changing storage capacity to an invalid value and make sure it fails (it looks like nil with CSIStorageCapacity enabled is invalid, even though we don't expect to ever encounter that because the field is defaulted to false if the feature is enabled)

Added. To make it work, I had to add a call to validateCSIDriverSpec in ValidateCSIDriverUpdate. That wasn't necessary before when all fields were immutable but now is good to have, albeit a bit redundant because there is not much that is going to be caught by it. As you said, nil should not occur in practice.

The field is meant to be mutable in 1.23 (kubernetes/kubernetes#101789). While at it, document the pending issue with cluster autoscaler as a gating item for GA graduation.

When originally introduced, the field was made immutable to be consistent with the other fields. But in practice allowing it to be toggled makes more sense, in particular when considering the rollout of a CSI driver (let it run without using the published CSIStorageCapacity object, then flip the field, or upgrading from a driver without support to one which supports it). The only consumer of this field, the kube-scheduler, can handle mutation without problems because it always consults the informer cache to get the current value.

alculquicondor · 2021-08-11T14:10:16Z

/lgtm

liggitt · 2021-08-11T14:17:30Z

/lgtm
/approve

k8s-ci-robot · 2021-08-11T14:19:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pohly

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:

~~api/OWNERS~~ [liggitt]
~~pkg/apis/storage/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/storage/OWNERS~~ [liggitt]

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

k8s-triage-robot · 2021-08-11T16:35:19Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2021-08-11T22:19:52Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 7, 2021

k8s-ci-robot requested review from msau42 and saad-ali May 7, 2021 13:19

k8s-ci-robot assigned msau42 May 26, 2021

msau42 reviewed Jun 2, 2021

View reviewed changes

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jun 2, 2021

liggitt added this to Backlog in API Reviews Aug 5, 2021

liggitt self-assigned this Aug 5, 2021

liggitt reviewed Aug 5, 2021

View reviewed changes

liggitt moved this from Backlog to Changes requested in API Reviews Aug 5, 2021

pohly force-pushed the mutable-csi-driver branch from 1f8b098 to d662cc0 Compare August 9, 2021 17:34

pohly mentioned this pull request Aug 9, 2021

scheduler: trigger pod scheduling when CSIStorageCapacity or CSIDriver.Spec.StorageCapacity change #104250

Closed

pohly force-pushed the mutable-csi-driver branch from d662cc0 to 8662a07 Compare August 11, 2021 07:04

pohly force-pushed the mutable-csi-driver branch from 8662a07 to ca245d9 Compare August 11, 2021 11:50

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2021

pohly added 2 commits August 11, 2021 15:10

generated swagger docs

11ed96b

pohly force-pushed the mutable-csi-driver branch from ca245d9 to 11ed96b Compare August 11, 2021 13:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2021

xing-yang mentioned this pull request Aug 11, 2021

KEP-1472: storage capacity: mutable CSIDriverSpec field kubernetes/enhancements#2854

Merged

k8s-ci-robot assigned alculquicondor Aug 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2021

liggitt moved this from Changes requested to API review completed, 1.23 in API Reviews Aug 11, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2021

k8s-ci-robot merged commit 7ee94f2 into kubernetes:master Aug 12, 2021

k8s-ci-robot added this to the v1.23 milestone Aug 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSIDriver: allow "StorageCapacity" to be modified #101789

CSIDriver: allow "StorageCapacity" to be modified #101789

pohly commented May 7, 2021

k8s-ci-robot commented May 7, 2021

fejta-bot commented May 7, 2021

pohly commented May 26, 2021

msau42 left a comment

msau42 Jun 2, 2021

msau42 Aug 11, 2021

pohly Aug 11, 2021

msau42 Jun 2, 2021

pohly Jun 2, 2021

msau42 Jun 2, 2021

pohly Jun 4, 2021

msau42 Jun 4, 2021

liggitt Aug 5, 2021

alculquicondor Aug 9, 2021

pohly Aug 9, 2021

pohly Aug 9, 2021

pohly Aug 11, 2021

msau42 commented Jun 2, 2021

liggitt Aug 5, 2021

pohly Aug 9, 2021

alculquicondor commented Aug 11, 2021

liggitt commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

k8s-triage-robot commented Aug 11, 2021

k8s-triage-robot commented Aug 11, 2021

CSIDriver: allow "StorageCapacity" to be modified #101789

CSIDriver: allow "StorageCapacity" to be modified #101789

Conversation

pohly commented May 7, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented May 7, 2021

fejta-bot commented May 7, 2021

pohly commented May 26, 2021

msau42 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jun 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Aug 11, 2021

liggitt commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

k8s-triage-robot commented Aug 11, 2021

k8s-triage-robot commented Aug 11, 2021