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 concurrent reconciles, QPS, and burst configurations #91

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

zyjjay
Copy link
Contributor

@zyjjay zyjjay commented Oct 11, 2023

To allow configuration for concurrent reconciles, QPS, and burst, the following changes are proposed:

  • Each reconciler struct for all controllers will require an extra field called ConcurrentReconciles, which is exposed in main and set to the value specified in the arguments passed to the framework addon.
  • The ConcurrentReconcile field will be used when the set up between controller and manager occurs
  • QPS and burst are also parsed from controller arguments passed to the framework addon, and override default QPS and burst values from controller-runtime after the hub and managed configs are created.

ref: https://issues.redhat.com/browse/ACM-6711

main.go Outdated
Comment on lines 200 to 201
hubCfg.QPS, hubCfg.Burst = tool.Options.ClientQPS, int(tool.Options.ClientBurst)
managedCfg.QPS, managedCfg.Burst = tool.Options.ClientQPS, int(tool.Options.ClientBurst)
Copy link
Member

Choose a reason for hiding this comment

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

Using multiple assignments like this is unusual. I'd prefer each to have their own line.

I also wonder if it would be nice to be able to independently configure the QPS and Burst for the hub cluster client and the managed cluster client. I can imagine that a user would increase these settings in order to speed things up on the managed cluster, but would not want to open up the possibility of a massively increased number of requests to their hub cluster. Maybe another reviewer can provide input - we also don't want to have too many configuration options, because they could get hard to manage.

To allow configuration for concurrent reconciles, QPS, and burst,
the following changes are proposed:
- Each reconciler struct for all controllers will require an extra
field called 'ConcurrentReconciles`, which is exposed in main and set
to the value specified in the arguments passed to the framework addon.
- The `ConcurrentReconcile` field will be used when the set up between
controller and manager occurs
-  QPS and burst are also parsed from controller arguments passed to the
framework addon, and override default QPS and burst values from
controller-runtime after the hub and managed configs are created.

ref: https://issues.redhat.com/browse/ACM-6711
Signed-off-by: Jason Zhang <[email protected]>
@openshift-ci
Copy link

openshift-ci bot commented Oct 12, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, zyjjay

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

@zyjjay zyjjay marked this pull request as ready for review October 13, 2023 16:23
@openshift-ci openshift-ci bot merged commit 3ccb6f2 into open-cluster-management-io:main Oct 13, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants