GetCapacityResponse: add maximum_volume_size #470

pohly · 2021-02-04T13:33:34Z

This new field has a more precisely defined semantic than the existing
available_capacity. It is intended for checking in advance in which
topology a CreateVolume call might succeed. Checking that based
on available_capacity is less precise, for example because
fragmentation might prevent creating a single volume that uses
all available capacity.

The new value has to be returned in a message to allow the caller to
determine whether the value was provided.

Fixes: #432

pohly · 2021-02-04T13:35:31Z

As discussed in the community meeting on 2021-02-03, the semantic of available_capacity is left as ambiguous as it was before. If there ever is an actual need to use it for some specific purpose and that ambiguity becomes a problem, then another change will have to address that.

For issue #432, adding this new field and letting a CO use it when available is sufficient.

pohly · 2021-02-04T13:36:21Z

@bswartz : perhaps you can have a first look whether this reflects what we discussed?

bswartz · 2021-02-05T15:54:30Z

csi.proto

@@ -947,6 +947,26 @@ message GetCapacityResponse {
  // storage. This field is REQUIRED.
  // The value of this field MUST NOT be negative.
  int64 available_capacity = 1;
+
+  // The largest size that could have been used in a CreateVolume call


Reword: The largest size that may used in a CreateVolume call

bswartz · 2021-02-05T15:57:44Z

csi.proto

+  // This field is OPTIONAL. If a Plugin has no restrictions for the
+  // size of volumes (for example, linear storage with no fragmentation
+  // issues and no technical limitation for individual volumes), then
+  // can leave this field unset and only return available_capacity.


Given that we've defined available_capacity very vaguely, and a CO will probably ignore that field, I think a better recommendation here would be to not leave the field blank, but the return the full available capacity, even if that means both fields of this struct have the same value.

Since we don't have a capability bit to go with this feature, I think the presence/absence of this field tells the CO if the SP supports this feature, and therefore we never want to leave it blank, unless the SP doesn't support the feature at all.

Given that we've defined available_capacity very vaguely, and a CO will probably ignore that field,

No, Kubernetes won't ignore it if that's only field that is set. Some vague value is still better than none.

I think a better recommendation here would be to not leave the field blank, but the return the full available capacity,

But then we need to define what "full capacity" is, right?

We can say "return the same vague value as in available_capacity", but benefit does that provide given that a CO must be prepared to fall back to using available_capacity when maximum_volume_size is unset?

No I think we just document that this field should always be the largest possible volume size, and it should never be blank, unless the SP don't support the feature. If the largest possible volume size and the available capacity are the same, that's fine. This allows COs that are only interested in whether a given volume will fit on a given node to answer that question by looking at a single field. The value of the other field could be for sorting the list of possible candidate nodes, if the CO was trying to spread out space consumption, for example.

Perhaps I should just leave out the sentence with "If a Plugin has no..."? That the field can be unset is already covered by "is OPTIONAL".

We could say "A Plugin SHOULD set this value to the available capacity if it has no additional limitations (for example, fragmentation) that lead to a smaller maximum volume size."

I say keep it simple and stick to what the field means and how it will be interpreted by COs. We should not speculate about the reasons why volume size might be limited to a value smaller than the available capacity in the spec. Just say it means the largest volume size which should be successfully created. COs MAY use this value to make decisions about where to schedule volumes,.

Makes sense. Updated.

This new field has a more precisely defined semantic than the existing available_capacity. It is intended for checking in advance in which topology a CreateVolume call might succeed. Checking that based on available_capacity is less precise, for example because fragmentation might prevent creating a single volume that uses all available capacity. The new value has to be returned in a message to allow the caller to determine whether the value was provided.

bswartz

/lgtm

pohly · 2021-02-10T21:11:09Z

/assign @saad-ali

These commands don't do anything here, though, do they? 😅

bswartz · 2021-02-11T01:32:59Z

Oh IDK what our review process is in this repo. I just do both methods to cover my bases

jdef · 2021-02-13T17:32:19Z

spec.md

@@ -1593,6 +1593,24 @@ message GetCapacityResponse {
  // storage. This field is REQUIRED.
  // The value of this field MUST NOT be negative.
  int64 available_capacity = 1;
+
+  // The largest size that may be used in a CreateVolume call


This seems to be communicating constraints related to the size of a "contiguous segment" of storage (terms vary across backends, of course) that may be used in a create volume call. Nice idea, and it seems very applicable to a plugin managing e.g. GPT partitions on a block device. I wonder if it could be taken further w/ respect to minimum size requirements -- e.g. perhaps it's not possible to create a storage volume of 1 byte on some backend.

Instead of capturing just the max requirement of such storage segments here, perhaps we should reuse the CapacityRange type to express both min and max requirements. This also would allow the plugin to express that it only has the ability to create volumes of a certain fixed/static size (given the requested parameters, of course), which also might be interesting for some use cases.

Thoughts?

I'm not sure about the exact use case where the minimum size is useful because CreateVolume is allowed to create a larger volume than requested, so in contrast to the maximum size it is a "softer" constraint.

But I think it is worthwhile adding nonetheless to minimize churn in the spec and CSI drivers if this turns out to be useful later.

I've added another commit which changes Capacity maximum_volume_size into CapacityRange volume_size. The naming of the CapacityRange is not a perfect fit (required_bytes, limit_bytes), but it is close enough to make sense. I like it slightly better than before, but don't care too much.

@bswartz can you take another look?

I don't agree that CreateVolume is allowed to create larger volumes than requested. There is a size range, and the upper limit may be larger than the lower limit, or it may be zero (unlimited). If the upper limit is set, though, it would be a spec violation to return a larger size volume. Kubernetes never sets the upper limit, so it's not a problem we frequently notice.

I can see the value of reporting a size range for supported volume size. If a plugin were to report minimum of 1 GiB, and I requested a volume with a max size of 100 MiB, then I could reasonably expect a failure. I also don't like the field names for the exiting CapacityRange structure, and I'm also not sure what to do about it. My best idea is to not attempt to reuse that structure but instead to define two simple Capacity fields. One for max, and one for min. This way we can name them appropriately and the functionality is equivalent to one CapacityRange field.

It's probably worth calling out specifically what we expect to see from the minimum. In some cases it could be a single block (such as 4K). In other cases it might be the minimum the system will allow, which is often hardcoded into the filesystem. And in the case @jdef mentioned it might be the smallest size pre-provisioned chunk of space that's sitting on the shelf.

COs should take care never to set the maximum volume size for CreateVolume() to a size smaller than the minimum. For Kubernetes this will never be an issue because the maximum is always infinity, but for other COs I can see it mattering.

I don't agree that CreateVolume is allowed to create larger volumes than requested. There is a size range, and the upper limit may be larger than the lower limit, or it may be zero (unlimited). If the upper limit is set, though, it would be a spec violation to return a larger size volume. Kubernetes never sets the upper limit, so it's not a problem we frequently notice.

Indeed, I was thinking only of how Kubernetes uses this. I agree, other COs may do things differently.

It's probably worth calling out specifically what we expect to see from the minimum.

Hmm, isn't it the same abstract rule as for maximum size? "If your CreateVolume call uses a required size smaller than the maximum volume size and a limit larger than the mimimum volume size, then a volume can be created". We decided against mentioning fragmentation as something that affects maximum volume size. Following the same logic we also shouldn't describe in the spec why a driver might report a non-zero minimum size.

My best idea is to not attempt to reuse that structure but instead to define two simple Capacity fields. One for max, and one for min. This way we can name them appropriately and the functionality is equivalent to one CapacityRange field.

Besides naming, an additional benefit is that a driver can leave the minimum size unspecified which may be more natural than "zero" when it has no (known?) minimum size.

I've added YAC (yet another commit 😁 ) with that third alternative. I used the opportunity to explicitly mention the CreateVolumeRequest.capacity_range field.

Yeah I didn't mean to include in the spec the reasons why a minimum might exist, just be specific about how the minimum should be interpreted and perhaps offer examples of valid values. In particular, we should explain if zero is a valid value, because zero has special meaning for the maximum. I might expect that values less than 1 are not okay, and even a 1 byte volume seems strange to me, but perhaps some provider can support it.

These are nitpicks though, I'm actually okay merging the change as-is, if others agree.

The minimum volume size may also be relevant. Another advantage is that the existing CapacityRange type can be used.

msau42 · 2021-02-23T01:22:16Z

csi.proto

@@ -947,6 +947,39 @@ message GetCapacityResponse {
  // storage. This field is REQUIRED.
  // The value of this field MUST NOT be negative.
  int64 available_capacity = 1;
+
+  // The largest size that may be used in a


For volume plugins that don't need to deal with fragmentation issues, I think defining a stricter available_capacity would be beneficial to help us do batch processing of multiple volumes. Right now, we're limited to processing a single volume at a time, which is a major issue for supporting a single workload that requests multiple volumes.

Improving the definition of available_capacity was intentionally left out because it seemed to need further discussion.

I meant should we define a new available_capacity field that has a more strict definition?

Yes, probably. But not in this PR 😅

What you want is tracked in #301

Not quite. So we said available_capacity could be interpreted 2 ways: 1) available capacity to provision a single volume and 2) total available capacity as reported by the storage system. Why do we only want to clarify one of the methods that this could be interpreted?

In the last CSI community meeting we talked about whether available_capacity should be clarified and concluded that the wording should be left as-is until there is a specific use-case that needs a stricter definition than the one that is currently in the spec. See (or rather, listen): https://www.youtube.com/watch?list=PLypWp79Fch09Sv7Vo-mHgEo20cGU7oNBc&t=1917&v=ZB0Y05jo7-M&feature=youtu.be

That link points to my summary of the discussion; both @bswartz and @saad-ali confirmed it.

pohly · 2021-02-24T16:19:40Z

@jdef is this now ready to be merged?

When is the CSI 1.4.0 release going to be created? This API change is something that I'd like to use in Kubernetes 1.21, which has a code freeze on Tuesday, March 9th.

xing-yang · 2021-03-03T15:59:05Z

csi.proto

@@ -947,6 +947,39 @@ message GetCapacityResponse {
  // storage. This field is REQUIRED.
  // The value of this field MUST NOT be negative.
  int64 available_capacity = 1;


Now that we have added the new capacity fields for max/min volume size, can we also add comments to this existing field to explain how CO should be using this field?

I think that is not clear yet. If you have a proposal, then please put it into a PR and we can discuss it.

saad-ali · 2021-03-03T22:05:28Z

/lgtm
/approve

@jdef Please speak up soon if you have any objections. I want to merge this and cut 1.4 by EOD.

jdef · 2021-03-03T23:37:51Z

spec.md

+  //
+  // If `volume_capabilities` or `parameters` is
+  // specified in the request, the Plugin SHALL take those into
+  // consideration when calculating the maximum volume size of the


s/maximum/minimum/ ?

It's actually correct here and wrong for maximum_volume_size: the requested minimum volume size must be <= maximum_volume_size and the requested maximum volume size (this case here, aka capacity_range.limit_bytes) >= minimum_volume_size.

I fixed the maximum_volume_size comment.

jdef · 2021-03-03T23:43:32Z

Nit: I'm not crazy about a message type that wraps a single int value. There are WKT messages for this. Otherwise lgtm

saad-ali · 2021-03-04T00:25:56Z

Ack, @pohly can you please make the changes requested by @jdef?

Remove message type that wraps a single int value
s/maximum/minimum/ ?

Once that is done, I'll go ahead and merge this PR and cut a 1.4.0 release

pohly · 2021-03-04T07:45:58Z

csi.proto

-  CapacityRange volume_size = 2 [(alpha_field) = true];
+  // where to create volumes. MUST NOT be negative.
+  google.protobuf.Int64Value maximum_volume_size = 2
+    [(alpha_field) = true];


The line break is necessary here to pass the length > 72 check.

pohly · 2021-03-04T07:52:05Z

spec.md

+  //
+  // If `volume_capabilities` or `parameters` is
+  // specified in the request, the Plugin SHALL take those into
+  // consideration when calculating the maximum volume size of the


It's actually correct here and wrong for maximum_volume_size: the requested minimum volume size must be <= maximum_volume_size and the requested maximum volume size (this case here, aka capacity_range.limit_bytes) >= minimum_volume_size.

I fixed the maximum_volume_size comment.

pohly

Nit: I'm not crazy about a message type that wraps a single int value. There are WKT messages for this. Otherwise lgtm

Done.

@saad-ali please take another look and merge.

jdef

thanks!

jdef · 2021-03-04T11:51:08Z

So I was about to merge this, and I realized that we probably don't even need a WKT here. Can't the default value of 0 for either of these new fields translate to "this is field is unset"? We do that elsewhere in the spec, why not here? Or maybe I'm missing something? I'd prefer the simpler form if possible, but if there's a good reason to keep the WKT then OK

pohly · 2021-03-04T12:05:06Z

We do that elsewhere in the spec, why not here?

minimum_volume_size = 0 might be a valid value when the CSI driver has no size restriction. It's probably not a very useful volume if it cannot store data, but still...

It's more obvious for maximum_volume_size = 0: that indicates that the CSI driver completely ran out of capacity, which has to be different from "CSI driver did not provide the information".

jdef · 2021-03-04T12:35:57Z

Makes sense. Can you add wording to the spec that describes this intent, phrased as a recommendation (i.e. MAY, SHOULD)?

…

On Thu, Mar 4, 2021, 7:05 AM Patrick Ohly ***@***.***> wrote: We do that elsewhere in the spec, why not here? minimum_volume_size = 0 might be a valid value when the CSI driver has no size restriction. It's probably not a very useful volume if it cannot store data, but still... It's more obvious for maximum_volume_size = 0: that indicates that the CSI driver completely ran out of capacity, which has to be different from "CSI driver did not provide the information". — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#470 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLHEYYGCPAFDHWZHZY3TB5ZQHANCNFSM4XC2ZFRQ> .

Using CapacityRange in GetCapacityResponse is not a good fit because the names of its fields is a bit awkward. By adding two fields in GetCapacityResponse we support the same functionality, plus they can be left unset separately from each other.

pohly · 2021-03-04T14:07:04Z

We already had a comment about "COs MAY...". I added "The Plugin SHOULD..." so now it reads:

  // This field is OPTIONAL. MUST NOT be negative.
  // The Plugin SHOULD provide a value for this field if it has
  // a maximum size for individual volumes and leave it unset
  // otherwise. COs MAY use it to make decision about
  // where to create volumes.
   google.protobuf.Int64Value maximum_volume_size = 2
     [(alpha_field) = true];

jdef · 2021-03-04T14:24:13Z

We already had a comment about "COs MAY...". I added "The Plugin SHOULD..." so now it reads:

  // This field is OPTIONAL. MUST NOT be negative.
  // The Plugin SHOULD provide a value for this field if it has
  // a maximum size for individual volumes and leave it unset
  // otherwise. COs MAY use it to make decision about
  // where to create volumes.
   google.protobuf.Int64Value maximum_volume_size = 2
     [(alpha_field) = true];

Thanks.
still LGTM

saad-ali · 2021-03-04T21:51:55Z

Thank you both @jdef and @pohly. Merging!

bswartz requested changes Feb 5, 2021

View reviewed changes

pohly force-pushed the maximum-volume-size branch from b842b28 to e09188b Compare February 8, 2021 09:47

bswartz approved these changes Feb 10, 2021

View reviewed changes

jdef reviewed Feb 13, 2021

View reviewed changes

GetCapacityResponse: volume size range

0600cb2

The minimum volume size may also be relevant. Another advantage is that the existing CapacityRange type can be used.

msau42 reviewed Feb 23, 2021

View reviewed changes

xing-yang reviewed Mar 3, 2021

View reviewed changes

jdef reviewed Mar 3, 2021

View reviewed changes

pohly force-pushed the maximum-volume-size branch 3 times, most recently from 17acd3c to 66b486d Compare March 4, 2021 07:51

pohly commented Mar 4, 2021

View reviewed changes

jdef approved these changes Mar 4, 2021

View reviewed changes

GetCapacityResponse: minimum volume size

7a2fcdd

Using CapacityRange in GetCapacityResponse is not a good fit because the names of its fields is a bit awkward. By adding two fields in GetCapacityResponse we support the same functionality, plus they can be left unset separately from each other.

pohly force-pushed the maximum-volume-size branch from 66b486d to 7a2fcdd Compare March 4, 2021 14:05

saad-ali merged commit 2816e42 into container-storage-interface:master Mar 4, 2021

pohly mentioned this pull request Mar 4, 2022

KEP-1472: storage capacity: update drawbacks kubernetes/enhancements#3233

Merged

GetCapacityResponse: add maximum_volume_size #470

GetCapacityResponse: add maximum_volume_size #470

Conversation

pohly commented Feb 4, 2021

pohly commented Feb 4, 2021

pohly commented Feb 4, 2021

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

bswartz left a comment

Choose a reason for hiding this comment

pohly commented Feb 10, 2021

bswartz commented Feb 11, 2021

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

pohly Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

pohly commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdef commented Mar 3, 2021

saad-ali commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly left a comment

Choose a reason for hiding this comment

jdef left a comment

Choose a reason for hiding this comment

jdef commented Mar 4, 2021

pohly commented Mar 4, 2021 • edited Loading

jdef commented Mar 4, 2021 via email

pohly commented Mar 4, 2021

jdef commented Mar 4, 2021

saad-ali commented Mar 4, 2021

pohly Mar 1, 2021 •

edited

Loading

pohly commented Mar 4, 2021 •

edited

Loading