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

Alpha list paging implementation #48921

Merged
merged 6 commits into from
Sep 3, 2017

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jul 14, 2017

Design in kubernetes/community#896

Support ?limit=NUMBER, ?continue=CONTINUATIONTOKEN, and a continue field
on ListMeta and pass through to etcd. Perform minor validation as an example.

# first out of three
$ curl http://127.0.0.1:8080/api/v1/namespaces?limit=1
{
  "kind": "NamespaceList",
  "apiVersion": "v1",
  "metadata": {
    "selfLink": "/api/v1/namespaces",
    "resourceVersion": "146",
    "next": "ZGVmYXVsdA"
  },
  "items": [
    {
      "metadata": {
        "name": "default",
        "selfLink": "/api/v1/namespaces/default",
        "uid": "f95e1390-6852-11e7-ab03-7831c1b76042",
        "resourceVersion": "4",
        "creationTimestamp": "2017-07-14T05:12:03Z"
      },
      "spec": {
        "finalizers": [
          "kubernetes"
        ]
      },
      "status": {
        "phase": "Active"
      }
    }
  ]
}
...
# last
$ curl "http://127.0.0.1:8080/api/v1/namespaces?limit=1&continue=a3ViZS1wdWJsaWM"
{
  "kind": "NamespaceList",
  "apiVersion": "v1",
  "metadata": {
    "selfLink": "/api/v1/namespaces",
    "resourceVersion": "145"
  },
  "items": [
    {
      "metadata": {
        "name": "kube-system",
        "selfLink": "/api/v1/namespaces/kube-system",
        "uid": "f95e9484-6852-11e7-ab03-7831c1b76042",
        "resourceVersion": "5",
        "creationTimestamp": "2017-07-14T05:12:03Z"
      },
      "spec": {
        "finalizers": [
          "kubernetes"
        ]
      },
      "status": {
        "phase": "Active"
      }
    }
  ]
}
The Kubernetes API server now supports the ability to break large LIST calls into multiple smaller chunks.  A client can specify a limit to the number of results to return, and if more results exist a token will be returned that allows the client to continue the previous list call repeatedly until all results are retrieved.  The resulting list is identical to a list call that does not perform chunking thanks to capabilities provided by etcd3.  This allows the server to use less memory and CPU responding with very large lists.  This feature is gated as APIListChunking and is not enabled by default.  The 1.9 release will begin using this by default from all informers.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 14, 2017
@smarterclayton smarterclayton added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 14, 2017
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Jul 14, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2017
@smarterclayton
Copy link
Contributor Author

@xiang90 @wojtek-t @lavalamp @liggitt just a very rough sketch of paging in Kubernetes API

var options []clientv3.OpOption
switch {
case len(fromKey) > 0 && pred.Limit > 0:
options = []clientv3.OpOption{clientv3.WithRev(fromRV), clientv3.WithRange(key + "\xFF"), clientv3.WithLimit(pred.Limit)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xiang90 if I want to prevent seeing updates newer than fromRV, what do I filter by?

@davidopp davidopp removed their assignment Jul 15, 2017
@smarterclayton smarterclayton force-pushed the paging_prototype branch 2 times, most recently from 28ccce1 to 7bf2504 Compare July 16, 2017 03:52
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2017
@smarterclayton smarterclayton force-pushed the paging_prototype branch 2 times, most recently from 97f5102 to adac573 Compare July 16, 2017 21:03
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 16, 2017
@lavalamp lavalamp assigned wojtek-t, ncdc and gmarek and unassigned ncdc and gmarek Jul 17, 2017
@lavalamp
Copy link
Member

cc @jpbetz

@smarterclayton
Copy link
Contributor Author

I'm still working up to testing this against our mega clusters to see if it can help bring in long tail latency. It also potentially lets us do flyweight caches more effectively (like for GC)

@smarterclayton smarterclayton added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 18, 2017
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 31, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2017
@wojtek-t
Copy link
Member

My only comment is about the comment saying that it's in order of fifteen minutes or so, I would like to get rid of it. But I'm not going to block on it.

@smarterclayton please fix the verify script and I'm happy to lgtm it then.

@smarterclayton
Copy link
Contributor Author

I really really want to have a rough bound on time, we can always adjust it up or make it differently. But it's going to be 5 ootb and I don't want people assuming it is forever.

@wojtek-t
Copy link
Member

and I don't want people assuming it is forever.

That one I completely agree with. I just didn't want to mention "five to fifteen minutes" in the comment.

But well, let's not waste too much time on this discussion.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017
@wojtek-t wojtek-t added this to the v1.8 milestone Aug 31, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, wojtek-t

Associated issue: 896

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@smarterclayton
Copy link
Contributor Author

/retest

1 similar comment
@smarterclayton
Copy link
Contributor Author

/retest

@wojtek-t
Copy link
Member

wojtek-t commented Sep 1, 2017

@smarterclayton - this requires a rebase (not sure why bot didn't mention it).

Add a feature gate in the apiserver to control whether paging can be
used. Add controls to the storage factory that allow it to be disabled
per resource. Use a JSON encoded continuation token that can be
versioned. Create a 410 error if the continuation token is expired.

Adds GetContinue() to ListMeta.
Adds a `continue` and `limit` parameter to ListOptions
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017
@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017
@smarterclayton
Copy link
Contributor Author

Trivial rebase

@smarterclayton
Copy link
Contributor Author

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 50832, 51119, 51636, 48921, 51712)

@k8s-github-robot k8s-github-robot merged commit 35ffb5c into kubernetes:master Sep 3, 2017
k8s-github-robot pushed a commit that referenced this pull request Sep 8, 2017
Automatic merge from submit-queue (batch tested with PRs 48552, 51876)

Disable default paging in list watches

For 1.8 this will be off by default. In 1.9 it will be on by default.
Add tests and rename some fields to use the `chunking` terminology.

Note that the pager may be used for other things besides chunking.

Follow on to #48921, we left the field on to get some exercise in the normal code paths, but needs to be disabled for 1.8.

@liggitt let's merge on wednesday.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.