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

Updating the api chunking proposal to add remainingItemCount #3710

Closed

Conversation

caesarxuchao
Copy link
Member

/sig api-machinery

Retrospectively updating the proposal for api-chunking for the remainingItemCount API change introduced in kubernetes/kubernetes#75993.

/assign @smarterclayton
cc @lavalamp

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 14, 2019
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/design Categorizes issue or PR as related to design. labels May 14, 2019
Copy link
Contributor

@smarterclayton smarterclayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some detail in use cases of what this is for, how exact it has to be, and why we should add it would be my request.

@@ -78,6 +80,7 @@ The server **may** limit the amount of time a continue token is valid for. Clien

The server **must** support `continue` tokens that are valid across multiple API servers. The server **must** support a mechanism for rolling restart such that continue tokens are valid after one or all API servers have been restarted.

The `remainingItemCount` returned by the server is the number of subsequent items in the list which are not included in this list response. If the list request contained label or field selectors, then the number of remaining items is unknown and this field will be unset and omitted during serialization. If the list is complete (either because the list request is not a chunking one or because this is the last chunk), then there are no more remaining items and this field is unset and is omitted during serialization. Servers older than v1.15 omit this field in their response to any list request.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it required to be exact? What’s the use case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when I’m halfway through a list?

@@ -91,6 +94,8 @@ Implementations that cannot offer consistent ranging (returning a set of results

For etcd3 the continue token would contain a resource version (the snapshot that we are reading that is consistent across the entire LIST) and the start key for the next set of results. Upon receiving a valid continue token the apiserver would instruct etcd3 to retrieve the set of results at a given resource version, beginning at the provided start key, limited by the maximum number of requests provided by the continue token (or optionally, by a different limit specified by the client). If more results remain after reading up to the limit, the storage should calculate a continue token that would begin at the next possible key, and the continue token set on the returned list.

etcd3 returns the total number of keys within the range as `response.Count` if the read is a range read. The storage checks if the list request contained label or field selector. If not, the storage calculates the `remainingItemCount` as `response.Count-limit`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, what happens?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caesarxuchao
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton

If they are not already assigned, you can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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:

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

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 14, 2019
@caesarxuchao
Copy link
Member Author

@smarterclayton I added use cases and why we needed the field. PTAL.

The remainingItemCount is exact. The doc never suggests that the count is an approximation, so I think readers will get it.

@smarterclayton
Copy link
Contributor

The remainingItemCount is exact. The doc never suggests that the count is an approximation, so I think readers will get it.

That's actually my concern, that we enshrine an exact count as part of the API. I would generally want to be as vague as possible to leave open the door for either relaxed semantics (performance) or alternate backends (proxies that can't necessarily know).

@smarterclayton
Copy link
Contributor

Also add what feature flag gate this will be under and what release it will be promoted (1.16 is ok for promotion to GA if we get good data in 1.15).


For example, assuming there are 1450 pods in the cluster when the client sends a chunking list request for pods, with `limit` set to 500. The first chunked list response contains 500 pods, a `continue` token, and with `metadata.remainingItemCount` set to 950. If the client use the `continue` token to continue listing, the server returns the second chunked list response containing the next 500 pods, another `continue` token, and with `metadata.remainingItemCount` set to 450. Note that the `remainingItemCount` is calculated based on the consistent list taken at the first chunking list request, that is, no matter if pods are created or deleted between the first and the second chunking list requests, the `metadata.remainingItemCount` in the second list response is always set to 450.

The `remainingItemCount` offers a simple and efficient way to get the count of objects of a resource type. For example, to get the total count of all pods in the cluster, simply sends `GET /api/v1/pods?limit=1` to the apiserver. Similarly, one can get the total count of all pods in a namespace. Note that the `remainingItemCount` is set to 0 when the list request contains any label or field selector, so this feature cannot be used to count the number of objects matching specific label or field selectors. Without the `remainingItemCount` feature, one needs to list all pods to get a count.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since old servers do not support this, and I don't think we can require this value in all possible future implementations, you need to specifically describe to a client how they can tell when the server doesn't support this field, and instruct them how to correctly detect it (as other sections do).

Effectively the algorithm is:

  1. Make a limit request
  2. Observe whether continue is set (if not, you have only one or zero items)
  3. Observe whether remainingItemCount is set (if not, remainingItemCount isn't supported)

@smarterclayton
Copy link
Contributor

Hrm, I didn't see a description of the use cases in the form I would expect. I.e. "we want to let people estimate the size of the collection", "we want to make this part of a UI" (I hope it's the former rather than the latter, because we discouraged use of chunking for human UI viewing).

@lavalamp
Copy link
Member

lavalamp commented May 14, 2019 via email

@caesarxuchao
Copy link
Member Author

RE. concerns over the "exact count", I added "The intended use of the remainingItemCount is estimating the size of a collection. Clients should not rely on the remainingItemCount to be set or to be exact."

RE. detecting if remainingItemCount is supported, I added instructions.

RE. feature flag, I'll wait for you and Daniel to converge. See Daniel's comment at kubernetes/kubernetes#75993 (comment). Starting the field as beta sounds more useful to me.

RE. use cases, perhaps I didn't understand your request, let me know if the current wording is still too vague.

@caesarxuchao
Copy link
Member Author

Added feature flag. @smarterclayton PTAL. Thank you.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 13, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 12, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants