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.

Sign up for GitHub

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

Jump to bottom

✨ Enable concurrency in BMO controllers through cmdLine flags #1247

Merged
merged 1 commit into from
Sep 26, 2023
Merged

✨ Enable concurrency in BMO controllers through cmdLine flags #1247

merged 1 commit into from
Sep 26, 2023

Conversation

kashifest
Copy link
Member

This PR adds concurrency in controllers through command line flags

@metal3-io-bot metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 5, 2023
@kashifest
Copy link
Member Author

Opening a new PR since while fixing #1235 the branch got corrupted while fixing rebasing.

@kashifest
Copy link
Member Author

Comments on previous PR
#1235 (comment)
#1235 (comment)
#1235 (comment)

@kashifest
Copy link
Member Author

I'll put @zaneb's latest comment here in writing as well to keep the conversation going:

"I'm not opposed to changing the numbers, but I'd expect the new ones to be at least as "properly explained" as the previous ones were in #905.

Scaling by the number of vcpus so that we don't use proportionally more resources on small boxes made sense to me, but maybe there's a reason that's mistaken? (Certainly this was somewhat informed by tuning a lot of Python deployments, where the GIL makes it impossible to effectively utilise more threads than there are vcpus.)

I also don't really care whether it is configured by an env var or CLI arg. If there's a standard way to do the latter, that would be ideal. However, I don't see any reason to break existing deployers who may be relying on the env var. (Also, having different concurrency values for each controller seem pointless to me, and can't ever be standardised.)"

@kashifest kashifest force-pushed the add/concurrency branch 2 times, most recently from 68059d0 to b9b2fab Compare April 6, 2023 08:55
@kashifest
Copy link
Member Author

/test-ubuntu-integration-main
/test-centos-e2e-integration-main

@kashifest
Copy link
Member Author

@zaneb thanks for your review. IMO these numbers highly depend on the infrastructure the deployment would be running as well as resource consumption. Anyways, I dont want to do any simulation at this point since my goal is to only standardize this configuration through command line flags. I can still keep the number to 8 as you have done a simulation to support it. I think configuring it through controller flags is more standard but since you think this would break existing deploys, I have kept it as it is but added a check if the value is provided in the command line flag or not. PTAL again if it looks ok to you.

@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main

@kashifest
Copy link
Member Author

/cc @zaneb @honza @dtantsur

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2023
@kashifest
Copy link
Member Author

Any thoughts on this @zaneb @honza ?

@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2023
zaneb
zaneb reviewed Apr 27, 2023
View reviewed changes
main.go Outdated Show resolved Hide resolved
controllers/metal3.io/baremetalhost_controller.go Outdated Show resolved Hide resolved
controllers/metal3.io/baremetalhost_controller.go Outdated Show resolved Hide resolved
@metal3-io-bot metal3-io-bot 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 2, 2023
@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main

@kashifest
Copy link
Member Author

/test-ubuntu-integration-main

@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2023
@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main
@zaneb @honza can I have some review on this before it needs rebase again?

@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2023
@kashifest
Copy link
Member Author

/test-ubuntu-integration-main
/test-centos-e2e-integration-main

@kashifest
Copy link
Member Author

@zaneb lets decide the fate of this PR, I have rebased it multiple times now.

@kashifest
Copy link
Member Author

@dtantsur @zaneb PTAL again

@kashifest kashifest force-pushed the add/concurrency branch 2 times, most recently from 923fcc7 to d6e1ef6 Compare August 25, 2023 09:20
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 25, 2023
@kashifest
Copy link
Member Author

Still waiting for the reviews @zaneb @dtantsur

@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

@kashifest
Copy link
Member Author

/test-ubuntu-integration-main

1 similar comment
@kashifest
Copy link
Member Author

/test-ubuntu-integration-main

@kashifest
Copy link
Member Author

ping @zaneb @dtantsur

zaneb
zaneb reviewed Sep 26, 2023
View reviewed changes
Copy link
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

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

Happy with this. Thanks Kashif!
/approve

main.go Outdated Show resolved Hide resolved
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zaneb

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2023
@kashifest
Copy link
Member Author

/test-ubuntu-integration-main
/test-centos-e2e-integration-main

@Rozzii
Copy link
Member

Rozzii commented Sep 26, 2023

/lgtm

@metal3-io-bot
Copy link
Contributor

@Rozzii: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lentzi90
lentzi90 reviewed Sep 26, 2023
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2023
@metal3-io-bot metal3-io-bot merged commit 137006a into metal3-io:main Sep 26, 2023
10 checks passed
@tuminoid tuminoid changed the title 🌱 Enable concurrency in BMO controllers through cmdLine flags ✨ Enable concurrency in BMO controllers through cmdLine flags Sep 26, 2023
@kashifest kashifest deleted the add/concurrency branch September 26, 2023 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtantsur dtantsur dtantsur left review comments

@zaneb zaneb zaneb left review comments

@lentzi90 lentzi90 lentzi90 left review comments

@honza honza Awaiting requested review from honza

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kashifest @metal3-io-bot @Rozzii @dtantsur @zaneb @lentzi90