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

Enable API resource paging by default, by feature gate in sample-apiserver #77278

Merged
merged 1 commit into from May 3, 2019

Conversation

@liggitt
Copy link
Member

commented Apr 30, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
Enable paging and quorum reads by default in etcd storage, and by feature gate in sample-apiserver

Does this PR introduce a user-facing change?:

API paging is now enabled by default in k8s.io/apiserver recommended options, and in k8s.io/sample-apiserver

/cc MikeSpreitzer
/sig api-machinery

@MikeSpreitzer

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Yikes, I thought quorum reads were the norm in all servers!
Do I understand correctly that before this change, sample-apiserver does not do quorum reads, and after this change, sample-apiserver does do quorum reads --- and no command line flags affect whether sample-apiserver does quorum reads?

@liggitt liggitt force-pushed the liggitt:enable-etcd-paging branch from 60eaabc to eacda79 Apr 30, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

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

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Yikes, I thought quorum reads were the norm in all servers!
Do I understand correctly that before this change, sample-apiserver does not do quorum reads, and after this change, sample-apiserver does do quorum reads --- and no command line flags affect whether sample-apiserver does quorum reads?

that is mostly for completeness... the etcd3 driver ignores whatever you put in that options struct and does quorum reads (since https://github.com/kubernetes/kubernetes/pull/69527/files#diff-0f11327dd3daafdfe8e326e895616f66L109)

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

would actually be good to drop that options field in a follow up

@liggitt liggitt force-pushed the liggitt:enable-etcd-paging branch from eacda79 to f996587 Apr 30, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

on second thought, I just dropped the quorum change (since it is a dummy field anyway) and limited this to the paging bit

@liggitt liggitt changed the title Enable paging and quorum reads by default, by feature gate in sample-apiserver Enable API resource paging by default, by feature gate in sample-apiserver Apr 30, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

dropped the Quorum field from that config struct in #77281

@MikeSpreitzer
Copy link
Member

left a comment

On second thought, maybe not. Doesn't this read the feature gate before the command line flags are parsed?

@liggitt liggitt force-pushed the liggitt:enable-etcd-paging branch from f996587 to 90cd672 Apr 30, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

On second thought, maybe not. Doesn't this read the feature gate before the command line flags are parsed?

you're right, good catch.

@MikeSpreitzer
Copy link
Member

left a comment

This time, for sure

@liggitt

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

/priority important-soon

@apelisse

This comment has been minimized.

Copy link
Member

commented May 2, 2019

/assign @jpbetz

@jpbetz

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Looks right.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 2, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 005eb53 into kubernetes:master May 3, 2019

20 checks passed

cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@liggitt liggitt deleted the liggitt:enable-etcd-paging branch May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.