Target GA for recover from Volume Expansion failure feature

[gnufied]

#1790

[kannon92]

Shadowing @deads2k on this. I had one item that jumped at me reading the PRR.

Was this metric added?

Are there any missing metrics that would be useful to have to improve observability if this feature?
We are planning to add new counter metrics that will record success and failure of recovery operations. In cases where recovery fails, the counter will forever be increasing until an admin action resolves the error.

Tentative name of metric is - operation_operation_volume_recovery_total{state='success', volume_name='pvc-abce'}

The reason of using PV name as a label is - we do not expect this feature to be used in a cluster very often and hence it should be okay to use name of PVs that were recovered this way.

[kannon92]

It would be worth expanding on https://github.com/kubernetes/enhancements/blob/d52c7d43a67a3f337e9c8a1869b820e8204af5dd/keps/sig-storage/1790-recover-resize-failure/README.md#scalability as part of the GA graduation.

The answers were a bit vague so we should revisit that section and comment on the scalability questions with more information.

[gnufied]

Was this metric added?

No, we didn't add the metric. I was just discussing with storage folks and while the idea sounds nice on paper, there is very little practical benefit from this particular metric. From operational point of view, I can't see a reason why will an admin be interested in this metric. There are already other resizing related metrics, which work well enough.

The metric item was tentative, so not sure if blocker for GA.

[gnufied]

It would be worth expanding on https://github.com/kubernetes/enhancements/blob/d52c7d43a67a3f337e9c8a1869b820e8204af5dd/keps/sig-storage/1790-recover-resize-failure/README.md#scalability as part of the GA graduation.

I am adding some more information for GA graduation criteria but scalability requirements as such has not changed between beta and GA. Which bits you found vague? I answered couple of questions with - "Potentially Yes", because answer depends on whether a recovery was attempted.

[kannon92]

Was this metric added?

No, we didn't add the metric. I was just discussing with storage folks and while the idea sounds nice on paper, there is very little practical benefit from this particular metric. From operational point of view, I can't see a reason why will an admin be interested in this metric. There are already other resizing related metrics, which work well enough.

The metric item was tentative, so not sure if blocker for GA.

It would be worth cleaning up the KEP then. I think you mention using that metric in a few places in the PRR.

[kannon92]

It would be worth expanding on https://github.com/kubernetes/enhancements/blob/d52c7d43a67a3f337e9c8a1869b820e8204af5dd/keps/sig-storage/1790-recover-resize-failure/README.md#scalability as part of the GA graduation.

I am adding some more information for GA graduation criteria but scalability requirements as such has not changed between beta and GA. Which bits you found vague? I answered couple of questions with - "Potentially Yes", because answer depends on whether a recovery was attempted.

The potentially yes jumped out at me because it sounded like we have not yet investigated if this would occur.

Maybe you can expand on what you mean by recovery.

[deads2k]

No, we didn't add the metric. I was just discussing with storage folks and while the idea sounds nice on paper, there is very little practical benefit from this particular metric. From operational point of view, I can't see a reason why will an admin be interested in this metric. There are already other resizing related metrics, which work well enough.

What is the alternative method we recommend to a cluster-admin to determine when resize operations are failing on a particular node and/or failing across all nodes.

[gnufied]

What is the alternative method we recommend to a cluster-admin to determine when resize operations are failing on a particular node and/or failing across all nodes.

So there is csi_sidecar_operations_seconds{method_name="/csi.v1.Controller/ControllerExpandVolume}, csi_operations_seconds{method_name="/csi.v1.Node/NodeExpandVolume"} and storage_operation_duration_seconds{operation_name="volume_fs_resize"}, which track these operations already on both controller and node side. The csi_xxx metrics track grpc error codes, so if either Controller or node side expansion is failing, it will be recorded appropriately and a metric emitted.

Some of these metrics have evolved since KEP (This KEP is now 5 years old) was originally written, I will update the KEP with newer metric names.

[gnufied]

The potentially yes jumped out at me because it sounded like we have not yet investigated if this would occur.

I have reworded those, ptal.

[kannon92]

Do we have both presubmits and periodics executing this feature?

[jsafrane]

The tests are still running only in alpha jobs, despite the feature is already in beta. I am going to LGTM the KEP, but I will block feature gate graduation until the test run in regular e2e jobs.

[gnufied]

err I misread the text. I will move the test for normal non-featuregated default.

[kannon92]

• Were upgrade and rollback tested? Was upgrade->downgrade->upgrade path tested?
  We have not fully tested upgrade and rollback but as part of beta process we will have it tested.

For stable, this needs to be explained and documented how this was done.

[msau42]

Can you update the version skew strategy section with the fixes we made?

[gnufied]

Added.

[kannon92]

Reading the Version skew section, this feature seems to have a really complicated rollout plan. Is this documented anywhere? What order do you recommend turning on these features gates? Should someone enable this in external-resizer first? And then enable it in kas, kubelet, kcm?

What is the recommendation for rolling this feature out for operators?

external-resizer must have the feature gate enabled for this to work. The feature gate is still beta. Could we consider GAing this feature first and then promote the other feature gates?

edit: NVM. You call this out that this is actually bad..

So the recommendation here would be to turn this feature gate on in all kubernetes components first and then you promote the feature gate in external-resizer after skew is satisfied?

[gnufied]

So the recommendation here would be to turn this feature gate on in all kubernetes components first and then you promote the feature gate in external-resizer after skew is satisfied?

Yes that is generally recommended and I have added this in the latest commit. d6f5b11

Having said that, there is some fail-safety builtinto this. Older kubelets can work with newer resizer upto a certain version and external-resizer can work with newer api-server etc. Please see version skew section for more details.

[kannon92]

LGTM from PRR shadow standpoint. Thank you for the updates!

/assign @deads2k

[deads2k]

PRR lgtm

/approve

[gnufied]

/assign @jsafrane @xing-yang @msau42

[jmickey]

Small nit, I believe this should be nested under the Graduation Criteria

[msau42]

Now that we support 3 version node skew, does that mean we realistically cannot enable the feature until 3 releases past GA?

[gnufied]

Doesn't versions upto 1.31 (which have kubelet fix) cover this? Even if version skew with node is 3 versions? This does mean - users must upgrade their older nodes to latest z-streams, if they want to upgrade control-plane and use newer resizer.

[msau42]

I guess for the kubelet case, it's not about enabling the feature. It's about being a node version that has the fix.

[gnufied]

We can leave external-resizer beta in this release while moving k/k components GA? Or are you thinking we should move everything GA in 1.35 ?

[msau42]

I think we can go ahead and GA external-resizer. My comment is just a wording nitpick because it currently says don't enable resizer until kubelet has the feature enabled, which would imply kubelets have to be on 1.32 for default feature gate settings.

[jsafrane]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

• keps/prod-readiness/OWNERS [deads2k]
• keps/sig-storage/OWNERS [jsafrane]

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