Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

feat: make concurrency of the worker configurable #1400

Merged

Conversation

zqzten
Copy link
Contributor

@zqzten zqzten commented Apr 11, 2021

What this PR does / why we need it:
As to gain better performance on different reconciliations, the concurrency of worker should be made configurable for different controllers.

In short, this PR does two things below:

  • Introduce a new MaxConcurrentReconciles option to worker for inner controller concurrency settings.
  • Introduce a new Concurrency section to KubeFedConfg to expose the concurrency settings to users.

Which issue(s) this PR fixes:
Fixes #1392

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 11, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @zqzten!

It looks like this is your first PR to kubernetes-sigs/kubefed 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubefed has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 11, 2021
@zqzten
Copy link
Contributor Author

zqzten commented Apr 13, 2021

@hectorj2f Mind take a look? Advice on further design is needed.

@swiftslee
Copy link
Contributor

I'd prefer the first piece of advice. How do we define the kubefedconfig custom resource if we choose the second advice?Cause we don't know what sync controllers we will run.

@zqzten
Copy link
Contributor Author

zqzten commented Apr 14, 2021

I'd prefer the first piece of advice. How do we define the kubefedconfig custom resource if we choose the second advice?Cause we don't know what sync controllers we will run.

I agree that configuring sync controller for each type in KubeFedConfig is not applicable, but what if we can configure that in FTC? In addition, there's still a choice between one config for all of the core controllers (sync & status) and seperated config for each core controller and higher level controller (like RSP) of kubefed.

@zqzten zqzten force-pushed the feat/concurrent_reconciles branch from 6225d40 to f53825e Compare May 3, 2021 08:59
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 3, 2021
@zqzten zqzten force-pushed the feat/concurrent_reconciles branch 6 times, most recently from 4affcc7 to 9308c18 Compare May 3, 2021 12:12
@zqzten
Copy link
Contributor Author

zqzten commented May 3, 2021

To keep it simple as well as extendable, currently I exposed the MaxConcurrentReconciles option of the two core controllers' workers to user, because they do the most work of KubeFed and their performance is subject to the scale of clusters KubeFed managed.

Other concurrency settings can alse be exposed in the Concurrency section of KubeFedConfg in the future when needed.

@zqzten zqzten changed the title [WIP] feat: make concurrency of the worker configurable (needs advice before proceeding) feat: make concurrency of the worker configurable May 3, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 3, 2021
@zqzten
Copy link
Contributor Author

zqzten commented May 3, 2021

@jimmidyson @makkes This PR is ready for review.

@zqzten
Copy link
Contributor Author

zqzten commented May 3, 2021

/cc @hectorj2f

@hectorj2f
Copy link
Contributor

@zqzten Could you highlight the performance benefits of this solution ? and why would you propose these changes ?

@zqzten
Copy link
Contributor Author

zqzten commented May 4, 2021

@hectorj2f We are using KubeFed in some large-scale cluster management cases, such as a public cloud service. It's common that operations on some clusters may take a long time (or even timeout) and block the controller processing resources belong to other users' clusters. Increasing the concurrency of sync controller mitigated this problem because now it has chance to process other resources on the fly.

In addition, it's apparent that properly increase the concurrency of a controller can improve its performance because it can simply process more events at the same time. The MaxConcurrentReconciles is a commonly used option in controller-runtime (as its name comes from the controller-runtime library too) to improve the controller's performance.

Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

@zqzten Thanks for the explanation. @irfanurrehman @jimmidyson Do you have any concerns about these properties ?

@zqzten zqzten force-pushed the feat/concurrent_reconciles branch from 9308c18 to ba81f5a Compare May 4, 2021 11:43
@zqzten zqzten force-pushed the feat/concurrent_reconciles branch from ba81f5a to 661df5d Compare May 4, 2021 12:36
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2021
Copy link
Contributor

@irfanurrehman irfanurrehman left a comment

Choose a reason for hiding this comment

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

Thanks @zqzten for doing this.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irfanurrehman, jimmidyson, zqzten

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:
  • OWNERS [irfanurrehman,jimmidyson]

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 merged commit 0541f43 into kubernetes-retired:master May 5, 2021
@zqzten zqzten deleted the feat/concurrent_reconciles branch May 5, 2021 14:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make worker count configurable for different controllers
6 participants