Increase the QPS limits on clients slightly and use protobuf for sync #90
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Missed in a previous PR, this makes the name match `openshift-cluster-version` rather than `version`.
We are getting a fair amount of throttling during deployment and updates. Increase QPS and Burst to be 4x the default. Refactor the start client slightly to allow modifying the client in case we need to do this in the future.
For the kube apis, protobuf is more efficient on both client and server side which reduces the base load of the cluster. We can't set protobuf for custom resource clients or unstructured at the current time.
openshift-ci-robot
added
approved
|
/retest |
pkg/start/start.go
Outdated
| @@ -231,21 +231,24 @@ type ClientBuilder struct { | |||
| config *rest.Config | |||
| } | |||
|
|
|||
| func (cb *ClientBuilder) RestConfig() *rest.Config { | |||
| func (cb *ClientBuilder) RestConfig(fns ...func(*rest.Config)) *rest.Config { | |||
There was a problem hiding this comment.
$ godoc ./pkg/start
PACKAGE DOCUMENTATION
package start
import "./pkg/start"
TYPES
type ClientBuilder struct {
// contains filtered or unexported fields
}
func (cb *ClientBuilder) ClientOrDie(name string, fns ...func(*rest.Config)) clientset.Interface
func (cb *ClientBuilder) KubeClientOrDie(name string, fns ...func(*rest.Config)) kubernetes.Interface
func (cb *ClientBuilder) RestConfig(fns ...func(*rest.Config)) *rest.Configfns ... isn't the best godoc, maybe either add comments / name the variable that it is evident that these are config modifiers...
| @@ -291,7 +299,7 @@ func (o *Options) NewControllerContext(cb *ClientBuilder) *Context { | |||
| resyncPeriod(o.ResyncInterval)(), | |||
| cvInformer.Config().V1().ClusterVersions(), | |||
| sharedInformers.Config().V1().ClusterOperators(), | |||
| cb.RestConfig(), | |||
| cb.RestConfig(increaseQPS), | |||
There was a problem hiding this comment.
this will only affect the generic resource builders. what about the ClientOrDie, KubeClientOrDie atleast KubeClientOrDie ?
There was a problem hiding this comment.
I left those at the defaults, those have low qps and low actions (rest config is what is used for sync loop). I'd like to do a shared qps pool eventually though for everything that isn't status updates to CV (which is effectively what this PR is).
|
@smarterclayton few nits, lgtm otherwise. |
|
updated with better godoc |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, smarterclayton 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 |
wking
added a commit
to wking/cluster-version-operator
that referenced
this pull request
Jul 31, 2020
Protecting us from [1,2]: converting (v1beta1.Role) to (v1.Role): unknown conversion Because merging and application are also version-dependent, it's hard to paper over this in resourceread with automatic type conversion. Although from [3]: Promotes the rbac.authorization.k8s.io/v1beta1 API to v1 with no changes so all we really need is the apiVersion bump. Anyhow, with this commit, I'm doubling down on the approach from 4ee7b07 (Add apiextensions.k8s.io/v1 support for CRDs, 2019-10-22, openshift#259) and collapsing the readers into two helpers that support all of our types and return runtime.Object. From 0a255ab (cvo: Use protobuf for sending events and other basic API commands, 2019-01-18, openshift#90), protobuf is more efficient, so we should use it where possible. And because all of this is very tedious to maintain by hand, there's now a Python generator to spit out all the boilerplate. [1]: kubernetes/kubernetes#90018 [2]: openshift#420 (comment) [3]: kubernetes/kubernetes#49642
wking
added a commit
to wking/cluster-version-operator
that referenced
this pull request
Jul 31, 2020
Protecting us from [1,2]: converting (v1beta1.Role) to (v1.Role): unknown conversion Because merging and application are also version-dependent, it's hard to paper over this in resourceread with automatic type conversion. Although from [3]: Promotes the rbac.authorization.k8s.io/v1beta1 API to v1 with no changes so all we really need is the apiVersion bump. Anyhow, with this commit, I'm doubling down on the approach from 4ee7b07 (Add apiextensions.k8s.io/v1 support for CRDs, 2019-10-22, openshift#259) and collapsing the readers into two helpers that support all of our types and return runtime.Object. From 0a255ab (cvo: Use protobuf for sending events and other basic API commands, 2019-01-18, openshift#90), protobuf is more efficient, so we should use it where possible. And because all of this is very tedious to maintain by hand, there's now a Python generator to spit out all the boilerplate. [1]: kubernetes/kubernetes#90018 [2]: openshift#420 (comment) [3]: kubernetes/kubernetes#49642
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For the kube apis, protobuf is more efficient on both client and server
side which reduces the base load of the cluster. We can't set protobuf for
custom resource clients or unstructured at the current time.
We are getting a fair amount of throttling during deployment and updates.
Increase QPS and Burst to be 4x the default. Refactor the start client
slightly to allow modifying the client in case we need to do this in the
future.