Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ListVolumesResponse should return an ID #138

Closed
lpabon opened this issue Nov 3, 2017 · 8 comments
Closed

ListVolumesResponse should return an ID #138

lpabon opened this issue Nov 3, 2017 · 8 comments

Comments

@lpabon
Copy link

lpabon commented Nov 3, 2017

The current model which provides a StartToken on ListVolumesRequest does not provide enough information if there are multiple clients to the CSI driver. For this reason ListVolumesResponse should return an ID that is passed in with StartToken to identify the caller.

Unless grpc provides a method to get some type of id from the connection?

@jdef
Copy link
Member

jdef commented Nov 4, 2017 via email

@akutz
Copy link
Contributor

akutz commented Nov 5, 2017

Hi @jdef,

Isaw @lpabon's post and started considering this situation myself. First of all, gRPC does not provide a unique connection identifier. The reasoning, IMO, is faulty:

gRPC does not provide this level of functionality, because it isn't valid when proxies are used and even sometimes when they're not.

This is also true for HTTP and does not stop people from using it when it is available. Too much of Go and gRPC seems to be based around what works 100% of the time and not providing tools that work in a large number of cases. I digress.

The reason @lpabon likely brings this issue up is due to concern that paging data might require knowledge of the client requesting the next page so that not any client can request another's paged data mid-query. I can see the point, but I'd argue that a token that's effectively a pointer to a cursor should act like a bearer token -- he who has the token gets the data.

The token could also be made to look like whatever the SP requires. For example, the following JSON object has two fields:

  1. i - a unique ID
  2. c - a cursor
{
  "i": "bda5c39bdd22448d9c8c6ae211a7265e",
  "c": 800
}

A minified version of the above JSON would be:

{"i":"bda5c39bdd22448d9c8c6ae211a7265e","c":800}

The above could even be base-64 encoded:

eyJpIjoiYmRhNWMzOWJkZDIyNDQ4ZDljOGM2YWUyMTFhNzI2NWUiLCJjIjo4MDB9

However, @jdef, this is why I think you're confused as to the need for the client ID. I think @lpabon believes the token should include a unique ID and the cursor, and I think you, @jdef, probably think the token only needs to be a unique ID.

I think I agree with @jdef here (if I'm even half correct in my assumptions).

While I can see how a token could be nothing more than a simple integer cursor, or could be enhanced to be a JSON object that includes a unique ID and a cursor, as well as any other useful information, I believe ultimately an SP should simply return a unique ID. That ID can be the same for the duration of the paged data, or the ID could change every time a new page is requested. Regardless, the token should be something the SP uses to query its own cache/datastore/storage provider for the next page.

I do not think it's the job of the token to persist information about the client/cursor, but rather the SP uses the token to look up that information in a manner completely opaque to the CO and user.

@jdef
Copy link
Member

jdef commented Nov 5, 2017 via email

@lpabon
Copy link
Author

lpabon commented Nov 8, 2017

Here is an example:

[ client 1 ] --> |
                      | --> [ csi driver ]
[ client 2 ] --> |

If two clients are connected to the CSI driver and both clients send a ListVolumes( maxEntries = M ) for client 1 at time t1 and ListVolumes( maxEntries = N ) for client 2 at time t2, then how does the driver maintain state to be able to return the correct token for that point-in-time view of volumes?

Currently there is no model to uniquely identify each of these clients.

@codenrhoden
Copy link

This seems related to #104 as well

@akutz
Copy link
Contributor

akutz commented Nov 8, 2017

Hi @codenrhoden,

Maybe.

Hi @lpabon,

I'm not sure I understand. The token returned by ListVolumes is a bearer token used by a client to ask for the subsequent data page. Your use case is already handled by the spec.

@lpabon
Copy link
Author

lpabon commented Nov 8, 2017

Ah, I see @akutz . By a bearer token you mean that it is up to the driver to marshal/unmarshal data into the token to be able to continue with pagination from a specific client. I am ok with this model as long as we describe this on the comments for next_token

@lpabon lpabon closed this as completed Nov 8, 2017
@lpabon
Copy link
Author

lpabon commented Nov 8, 2017

I should leave open to add this documentation to the spec.

@lpabon lpabon reopened this Nov 8, 2017
lpabon added a commit to lpabon/spec that referenced this issue Nov 8, 2017
Closes container-storage-interface#138

Signed-off-by: Luis Pabón <luis@portworx.com>
@lpabon lpabon closed this as completed Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants