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

✨ Add configurable workers for individual controllers, defaults to 10 #3093

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ Add configurable workers for individual controllers, defaults to 10 #3093

wants to merge 1 commit into from

Conversation

dilipmighty245
Copy link

Summary

To make the worker count configurable for individual controllers which can be leveraged in scale environment

Related issue(s)
Fixes #

Release Notes

Set worker count configurable, defaults to 10

@kcp-ci-bot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@kcp-ci-bot kcp-ci-bot added dco-signoff: yes Indicates the PR's author has signed the DCO. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 15, 2024
@kcp-ci-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mjudeikis for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@kcp-ci-bot kcp-ci-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 15, 2024
@kcp-ci-bot
Copy link
Contributor

Hi @dilipmighty245. Thanks for your PR.

I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@dilipmighty245 dilipmighty245 changed the title Add configurable workers for individual controllers, defaults to 10 ✨ Add configurable workers for individual controllers, defaults to 10 Mar 15, 2024
@palnabarun
Copy link
Member

/ok-to-test

@kcp-ci-bot kcp-ci-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 15, 2024
@dilipmighty245
Copy link
Author

/ok-to-test

@dilipmighty245
Copy link
Author

/test pull-kcp-test-e2e-shared

@dilipmighty245 dilipmighty245 mentioned this pull request Mar 16, 2024
Closed
@kcp-ci-bot
Copy link
Contributor

@dilipmighty245: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kcp-test-e2e-shared c401669 link true /test pull-kcp-test-e2e-shared

Full PR test history

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. I understand the commands that are listed here.

@mjudeikis
Copy link
Contributor

Can we do this more generic:
https://kubernetes.slack.com/archives/C021U8WSAFK/p1710515852080189?thread_ts=1710423745.848959&cid=C021U8WSAFK ?

sttts
sttts reviewed Mar 16, 2024
View reviewed changes
cmd/kcp/help_test.go
@@ -0,0 +1,74 @@
/*
Copyright 2022 The KCP Authors.
Copy link
Member

Choose a reason for hiding this comment

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

2024

sttts
sttts reviewed Mar 16, 2024
View reviewed changes
cmd/kcp/kcp.go
@@ -76,6 +77,10 @@ func main() {
} else if strings.HasPrefix(f, "--root-directory=") {
rootDir = strings.TrimPrefix(f, "--root-directory=")
}

if strings.HasPrefix(f, "--controller-threads=") {
Copy link
Member

Choose a reason for hiding this comment

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

workers is the normal wording, not threads.

sttts
sttts reviewed Mar 16, 2024
View reviewed changes
pkg/server/options/options.go
// add flags that are filtered out from upstream, but overridden here with our own version
fs.Var(kcpfeatures.NewFlagValue(), "controller-settings", ""+
"A set of key=value pairs that manage controller settings for alpha/experimental features. "+
"Options are:\n"+strings.Join(kcpfeatures.KnownFeatures(), "\n")) // hide kube-only gates
Copy link
Member

Choose a reason for hiding this comment

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

not sure I understand from the description and go doc what this is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

Assignees
No one assigned
Labels
dco-signoff: yes Indicates the PR's author has signed the DCO. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dilipmighty245 @kcp-ci-bot @palnabarun @mjudeikis @sttts