Target GA for recover from Volume Expansion failure feature

[gnufied]

#1790

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gnufied
Once this PR has been reviewed and has the lgtm label, please assign jpbetz, xing-yang for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS
• keps/sig-storage/OWNERS

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

[jsafrane]

(starting a new "thread") I would also expect https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1790-recover-resize-failure#graduation-criteria having GA graduation criteria.

[jsafrane]

(starting another unrelated "thread")
Should we mention the new annotation added in kubernetes/kubernetes#131907 ? The enhancement itself does not aim at RWX volumes, still, an API (annotation is an API) deserves some note somewhere. I guess that can be solved in the annotation "API review".

[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]

@jsafrane section about annotation stuff.

[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.