A design proposal for volume capacity from node #256

gnufied · 2018-07-23T15:11:49Z

Fixes #236

saad-ali · 2018-07-23T18:35:37Z

csi.proto

+  // The number of inodes used by the volume.
+  // This field is OPTIONAL.
+  // The value of this field MUST NOT be negative.
+  int64 used_inodes = 4;


I agree with @ddebroy that we have to avoid leaking OS specific constructs in to the spec. Is there a similar concept in Windows, could we come up with something that would work for all OSes?

Windows/NTFS does not have a concept of a variable number of inodes. It just uses File IDs. As @garthy mentions below, a NTFS volume is divided into a variable number of clusters (typically 4K in size but can be configured during format) and the cluster usage might be useful on top of capacity usage but clusters and inodes are quite different.

Yeah I think our options boil down to:

a. Either come up with a name that may or may not represent inode number in a OS agnostic way. It seems hard to do and could easily cause confusion.
b. Drop the inode information altogether from stat information.
d. Keep the inode capacity information as it is and make it clear that plugin authors need not set this value when running on a platform that does not support it. These numbers will not be available when using raw block devices on *nix too. So, Windows is not alone in this.
e. @msau42 suggested we can move such platform specific information in a opaque dictionary. It could work but I am not sure if it is very different from option#d.

garthy · 2018-07-23T19:11:21Z

Cluster size maybe for the NTFS volume but personally I'd not bother exporting filesystem specific tlinformation. How will it get used? Is it useful? Export it via the metrics if needed.

…

On Mon, 23 Jul 2018, 19:35 Saad Ali, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In csi.proto <#256 (comment)> : > + // shared with the host processes and other volumes. + // The value of this field MUST NOT be negative. + int64 available_capacity = 2; + + // The total capacity in bytes. This field is REQUIRED. + // For volumes that share a file system with the host (e.g hostpath) + // this is the total size of underlying storage and it MAY + // not be equal to used_capacity + available_capacity because + // filesystem is shared. + // The value of this field MUST NOT be negative. + int64 total_capacity = 3; + + // The number of inodes used by the volume. + // This field is OPTIONAL. + // The value of this field MUST NOT be negative. + int64 used_inodes = 4; I agree with @ddebroy <https://github.com/ddebroy> that we have to avoid leaking OS specific constructs in to the spec. Is there a similar concept in Windows, could we come up with something that would work for all OSes? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#256 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABCPap5TfPLnMqfzoSBpRRuVp9vWYkXqks5uJhd-gaJpZM4VbLRR> .

jieyu · 2018-07-25T04:24:36Z

spec.md

@@ -1913,6 +1916,78 @@ The CO MUST implement the specified error recovery behavior when it encounters t
 | Operation pending for volume | 10 ABORTED | Indicates that there is a already an operation pending for the specified volume. In general the Cluster Orchestrator (CO) is responsible for ensuring that there is no more than one call "in-flight" per volume at a given time. However, in some circumstances, the CO MAY lose state (for example when the CO crashes and restarts), and MAY issue multiple calls simultaneously for the same volume. The Plugin, SHOULD handle this as gracefully as possible, and MAY return this error code to reject secondary calls. | Caller SHOULD ensure that there are no other calls pending for the specified volume, and then retry with exponential back off. |


+#### `NodeGetVolumeStats`
+
+A Node plugin MUST implement this RPC call.


Then, this is a breaking change. We try to avoid that if possible.

Why not make it optional, and control by a plugin capability.

jieyu · 2018-07-25T04:25:02Z

spec.md

+  // The path at which the volume was published. It MUST be an absolute
+  // path in the root filesystem of the process serving this request.
+  // This is a REQUIRED field.
+  string target_path = 2;


Can you explain why we need this field?

I have renamed this field to volume_path and updated the comment to reflect as per our discussion in community call.

jieyu · 2018-07-25T04:28:00Z

spec.md

+  // For volumes that share a file system with the host (e.g hostpath)
+  // this is the total size of underlying storage and it MAY
+  // not be equal to used_capacity + available_capacity because
+  // filesystem is shared.


I don't quite understand this. Can you give some examples?

I have dropped these elaborate comments, because host paths that are shared with underlying storage may or may not be a strong use case. I expect plugin implementations will use their judgement to return right values from here.

jdef · 2018-07-25T16:19:00Z

csi.proto

+  string target_path = 2;
+}
+
+message NodeGetVolumeStatsResponse {


What about consolidating this a bit w/ units that we can extend later?

message Usage { enum Unit { UNKNOWN = 0; INODE = 1; CLUSTER = 2; BYTES = 3; } int64 free = 1; int64 total = 2; Unit unit = 3; } repeated Usage usage = 1;

gnufied · 2018-07-26T17:05:30Z

@jdef @jieyu fixed or addressed most review comments. PTAL.

gnufied · 2018-07-30T17:10:06Z

@jdef @jieyu ping again. Can you guys please take another look?

jieyu

This LGTM! ping @jdef @saad-ali @julian-hj

ddebroy · 2018-07-30T21:50:54Z

csi.proto

+
+message VolumeUsage {
+  enum Unit {
+    BYTES = 0;


Nit: Most enums typically have a UNKNOWN = 0; Can we add one here too?

Good catch @ddebroy!

thanks .. LGTM

saad-ali · 2018-07-31T00:52:39Z

LGTM

jdef · 2018-07-31T03:09:30Z

spec.md

+A Node plugin MUST implement this RPC call if it has GET_VOLUME_STATS node capability.
+`NodeGetVolumeStats` RPC call returns the volume capacity stats available for the volume.
+
+If the volume is being used in `BlockVolume` mode then `used_capacity` and `available_capacity` MAY be omitted from `NodeGetVolumeStatsResponse`.


used_capacity and available _capacity don't make sense here

jdef · 2018-07-31T03:11:02Z

spec.md

+  // The value of this field MUST NOT be negative.
+  int64 used = 3;
+
+  // The units that represent this value


"Units by which values are measured"?

jdef · 2018-07-31T03:12:38Z

spec.md

+
+| Condition | gRPC Code | Description | Recovery Behavior |
+|-----------|-----------|-------------|-------------------|
+| Volume does not exist | 5 NOT_FOUND | Indicates that a volume corresponding to the specified `volume_id` does not exist. | Caller MUST verify that the `volume_id` is correct and that the volume is accessible and has not been deleted before retrying with exponential back off. |


Vol id AND volume_path, right?

This is the generic error we return when specified volume does not exist. See NodeUnpublishVolume for example.

This seems like a different case than node-unpublish-volume. For example, if the volume_id is already node-unpublished at target_path, then target_path may not exist (this is an expected possibility) - the call is deemed idempotent.

For this RPC, a missing vol staged/published at target_path is an error because of the nature of the operation: the RPC isn't changing the mount state of the volume at target_path, it's trying to query the state of the volume at target_path. And if volume_id exists, but it not present at target_path .. well, that's an error that we should bubble up to the CO.

One might argue that NOT_FOUND isn't appropriate for the case I outlined above and that we should use a different error code for such a case - I'm fine with that too.

hmm I think the only other error code that makes sense perhaps is FAILED_PRECONDITION but in this case, it is not all that different from NOT_FOUND error because in both the cases the CO may retry.

I think I am okay with rewording the error to "Indicates that a volume corresponding to volume_id does not exist on specified volume_path"

I have updated the comment.

jdef · 2018-07-31T03:15:48Z

spec.md

+#### `NodeGetVolumeStats`
+
+A Node plugin MUST implement this RPC call if it has GET_VOLUME_STATS node capability.
+`NodeGetVolumeStats` RPC call returns the volume capacity stats available for the volume.


s/stats/statistics/

jdef · 2018-07-31T03:20:05Z

spec.md

+  // The ID of the volume. This field is REQUIRED.
+  string volume_id = 1;
+
+  // It can be any valid path where volume was previously published.


staged or published, right?

jdef

thanks, left a few more minor comments. otherwise LGTM

jdef · 2018-07-31T15:57:37Z

spec.md

+  // The value of this field MUST NOT be negative.
+  int64 used = 3;
+
+  // Units by which values are measured.


This field is REQUIRED.

jdef · 2018-07-31T15:57:57Z

spec.md

+}
+
+message NodeGetVolumeStatsResponse {
+  repeated VolumeUsage usage = 1;


this field is ... OPTIONAL?

jdef · 2018-07-31T15:58:51Z

spec.md

@@ -1968,6 +2032,10 @@ message NodeServiceCapability {
    enum Type {
      UNKNOWN = 0;
      STAGE_UNSTAGE_VOLUME = 1;
+      // If Plugin implements GET_VOLUME_STATS capability
+      // then it MUST implement NodeGetVolumeStats RPC
+      // call for fetching volume stats


s/stats/statistics/ (... and end sentences with punctuation please).

gnufied · 2018-07-31T16:14:35Z

@jdef done. thank you. I will squash when ready for merge.

Use repeated usage for representing volume stats

gnufied · 2018-07-31T18:22:12Z

@saad-ali @jdef squashed the commits. thank you!

jdef · 2018-08-01T14:19:25Z

Going forward, let's also hold ourselves to a higher standard w/ respect to descriptive PR summaries and/or commit messages. "... a design proposal ..." doesn't cut it here: this was an actual specification change not just a proposal.

saad-ali reviewed Jul 23, 2018

View reviewed changes

jieyu reviewed Jul 25, 2018

View reviewed changes

jdef reviewed Jul 25, 2018

View reviewed changes

gnufied force-pushed the volume_capacity_stats branch from d1fe31b to b5ddbe6 Compare July 27, 2018 14:17

jieyu approved these changes Jul 30, 2018

View reviewed changes

ddebroy reviewed Jul 30, 2018

View reviewed changes

gnufied force-pushed the volume_capacity_stats branch from b5ddbe6 to c6674bb Compare July 30, 2018 22:42

jdef reviewed Jul 31, 2018

View reviewed changes

jdef approved these changes Jul 31, 2018

View reviewed changes

Write a design proposal for volume capacity

1e78359

Use repeated usage for representing volume stats

gnufied force-pushed the volume_capacity_stats branch from 173553e to 1e78359 Compare July 31, 2018 18:21

jieyu merged commit aeca98c into container-storage-interface:master Aug 1, 2018

2phost mentioned this pull request Nov 10, 2020

Monitoring Kubernetes PVs linode/linode-blockstorage-csi-driver#62

Closed

mjosipovic1979 mentioned this pull request Mar 4, 2023

Monitoring Kubernetes PVs space usage metric missing IBM/ibm-block-csi-driver#661

Closed

A design proposal for volume capacity from node #256

A design proposal for volume capacity from node #256

Uh oh!

Conversation

gnufied commented Jul 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garthy commented Jul 23, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdef Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied commented Jul 26, 2018

Uh oh!

gnufied commented Jul 30, 2018

Uh oh!

jieyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saad-ali commented Jul 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied commented Jul 31, 2018

Uh oh!

jdef Jul 25, 2018 •

edited

Loading