Add flag for setting number of service account workers #69937

andybradshaw · 2018-10-17T17:46:27Z

What this PR does / why we need it: This PR makes the number of workers for the Service Account controller configurable. We are running into some issues with this controller keeping up, and needed a way of updating the currently hard-coded worker value of 1.

Special notes for your reviewer: The documentation for the --concurrent-serviceaccount-token-syncs flag seems to indicate that it defaults to 5, but was unable to find anything that handled the default values for the SAControllerConfiguration struct. Please let me know if defaults for that struct are handled in a different way.

Release note:

Added a configuration flag to controller-manager for number of service account sync workers.

k8s-ci-robot · 2018-10-17T17:46:30Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

andybradshaw · 2018-10-17T17:53:27Z

I believe I've associated my github account to my CNCF now... let me know if something still needs to be done there

andybradshaw · 2018-10-17T17:54:49Z

/check-cla

andybradshaw · 2018-10-17T20:56:26Z

/check-cla

idealhack · 2018-10-18T15:34:11Z

/ok-to-test

fedebongio · 2018-10-18T20:50:43Z

/assign @cheftako

cheftako · 2018-10-20T01:03:02Z

pkg/controller/apis/config/v1alpha1/defaults.go

@@ -239,6 +239,15 @@ func SetDefaults_ResourceQuotaControllerConfiguration(obj *kubectrlmgrconfigv1al
 	}
 }

+func SetDefaults_SAControllerConfiguration(obj *kubectrlmgrconfigv1alpha1.SAControllerConfiguration) {
+	if obj.ConcurrentSATokenSyncs == 0 {


It seems this is already being set in SetDefaults_KubeControllerManagerConfiguration above.While the defaults seem to be the same do we want to allow for competing defaults?

Ah, sorry... missed that. It probably makes more sense to set it with the service account controller specific default function.

cheftako

/lgtm

andybradshaw · 2018-10-25T21:22:32Z

@cheftako anything more I need to do here? Wasn't sure if I needed to address the needs-kind label.

cheftako · 2018-10-26T21:57:01Z

/kind feature

cheftako · 2018-10-26T21:58:46Z

@andybradshaw At this point you need an approver. I would check the OWNERS files detailed above.

lavalamp · 2018-11-01T20:43:25Z

staging/src/k8s.io/kube-controller-manager/config/v1alpha1/types.go

@@ -426,6 +426,9 @@ type SAControllerConfiguration struct {
 	// serviceAccountKeyFile is the filename containing a PEM-encoded private RSA key
 	// used to sign service account tokens.
 	ServiceAccountKeyFile string
+	// concurrentServiceAccountSyncs is the number of service account syncing operations
+	// that will be done concurrently.
+	ConcurrentServiceAccountSyncs int32


@liggitt Ugh, do we do names wrong (as it's done here) in literally every config type?

do you mean not having json tags? this component doesn't yet read config from file, so we are free to rename fields, move them around, etc, prior to adding json tags.

that's on the roadmap to getting the components using external versioned config

This is showing up as an API violation. I'd like to not let anything in that adds to the violation file. Reasonable or excessive request?

that requires adding json tags to the file prior to actually thinking through what the serialized names should be. we didn't want to do that. a file with no json tags is clearly not intended to be serialized yet. a mix of fields, some with tags and some without, looks like a serializable struct with mistakes.

a reasonable requirement would be to have no violations on the types prior to supporting loading these config types from files

The tool is complaining that concurrentServiceAccountSyncs != ConcurrentServiceAccountSyncs; if the json tag were added, the tool would be happy. I honestly think the tool is kind of right.

understood, but we did not want to add json tags before we intended the types to be serialized to files, because it implies they are serializable.

Yeah, that makes sense. It seems the choices are to somehow exclude these types from the tool (which must currently believe they are serializable) or make the comment match the variable name. I don't care which choice is made, I think.

k8s-ci-robot · 2018-11-01T21:59:51Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2018-11-01T22:00:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andybradshaw
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp

If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready.

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

andybradshaw · 2018-11-02T04:43:46Z

@lavalamp @liggitt Happy working on whatever you guys decide on for the path forward here. That being said, I'm not sure what that is at the moment...

I'm not sure making the comment name and the struct name match will fix the error, the linter is complaining about fields which are missing the json tag. Note the somewhat confusing example, which is meant to show that using a tag of json:"" will satisfy this rule. However, I do find the naming inconsistency annoying ( Update controller manager configuration field comments for consistency #70565).
Making the tool skip these types might get a little wonky because of how coupled generation and validation are... If we split those up, maybe adding a tag (k8s:openapi-gen-lint=false?) to skip the structs, either individually or at the package level, would work.
Would adding a placeholder tag of - be enough to indicate to consumers that attempting to do serialization stuff probably isn't going to do what you want? The presence of a tag, even if the field is ignored, could be confusing.

lavalamp · 2018-11-02T05:12:35Z

If a placeholder of '-' makes the linter happy, I think that makes it pretty clear to others that the type doesn't serialize.

…

On Thu, Nov 1, 2018 at 9:45 PM Andy Bradshaw ***@***.***> wrote: @lavalamp <https://github.com/lavalamp> @liggitt <https://github.com/liggitt> Happy working on whatever you guys decide on for the path forward here. That being said, I'm not sure what that is at the moment... 1. I'm not sure making the comment name and the struct name match will fix the error, the linter is complaining about fields which are missing the json tag <https://github.com/kubernetes/kube-openapi/blob/72693cb1fadd73ae2742f6fe29af77d1aecdd8cd/pkg/generators/rules/names_match.go#L97>. Note the somewhat confusing example <https://github.com/kubernetes/kube-openapi/blob/72693cb1fadd73ae2742f6fe29af77d1aecdd8cd/pkg/generators/rules/names_match.go#L74>, which is meant to show that using a tag of json:"" will satisfy this rule. However, I do find the naming inconsistency annoying ( #70565 <#70565>). 2. Making the tool skip these types might get a little wonky because of how coupled generation and validation <https://github.com/kubernetes/kube-openapi/blob/72693cb1fadd73ae2742f6fe29af77d1aecdd8cd/pkg/generators/rules/names_match.go#L74> are... If we split those up, maybe adding a tag ( k8s:openapi-gen-lint=false?) to skip the structs, either individually or at the package level, would work. 3. Would adding a placeholder tag of - be enough to indicate to consumers that attempting to do serialization stuff probably isn't going to do what you want? The presence of a tag, even if the field is ignored, could be confusing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#69937 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglgZ4SdWyD9uSgaDbe_HlsD1X8Tsmks5uq83hgaJpZM4Xk1Lt> .

andybradshaw · 2018-11-02T18:15:25Z

I don't really have a notion of who owns what, so I'm going to keep @'ing people... let me know if I should stop.

@liggitt would the proposed change (json tag of -) be acceptable to you?

@lavalamp are we holding that change as blocking this one?

lavalamp · 2018-11-02T19:53:50Z

I want to take a pretty hard line about not adding violations starting asap; I'm willing to let this one in as long as we (i.e. @liggitt) all agree that the next change has to fix this.

liggitt · 2018-11-02T20:33:44Z

@liggitt would the proposed change (json tag of -) be acceptable to you?

sure, that's fine as well

andybradshaw · 2018-11-02T20:36:08Z

@liggitt okay, I can make that change as part of the comment clean up PR or a separate PR if that would be preferable

lavalamp · 2018-11-02T22:20:36Z

Either way works for me, thanks!

…

On Fri, Nov 2, 2018 at 1:57 PM Andy Bradshaw ***@***.***> wrote: @liggitt <https://github.com/liggitt> okay, I can make that change as part of the comment clean up PR <#70565> or a separate PR if that would be preferable — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#69937 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglrVPH6svDm-NsQW1yTO3psycirkqks5urLHUgaJpZM4Xk1Lt> .

k8s-ci-robot · 2019-01-18T20:12:16Z

@andybradshaw: PR needs rebase.

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.

fejta-bot · 2019-04-18T20:29:38Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-05-18T21:13:01Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2019-06-17T22:03:11Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2019-06-17T22:03:21Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 17, 2018

k8s-ci-robot requested review from justinsb and mtaufen October 17, 2018 17:46

andybradshaw closed this Oct 17, 2018

andybradshaw reopened this Oct 17, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 17, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 18, 2018

k8s-ci-robot assigned cheftako Oct 18, 2018

cheftako reviewed Oct 20, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2018

cheftako approved these changes Oct 23, 2018

View reviewed changes

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 26, 2018

lavalamp reviewed Nov 1, 2018

View reviewed changes

andybradshaw added 4 commits November 1, 2018 14:54

Add flag for setting number of service account workers

59fd3c3

Fix incorrect option field

bea4fea

Add test for new flag

2f6dd09

Remove duplicate default configuration for service account token syncs

77ca61d

andybradshaw force-pushed the ab/add-service-account-worker-configuration branch from 7aec01a to 77ca61d Compare November 1, 2018 21:59

Fix violation exceptions list

ac9a9d0

andybradshaw mentioned this pull request Nov 6, 2018

Update controller manager configuration field comments for consistency #70565

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2019

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 18, 2019

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 18, 2019

k8s-ci-robot closed this Jun 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag for setting number of service account workers #69937

Add flag for setting number of service account workers #69937

andybradshaw commented Oct 17, 2018

k8s-ci-robot commented Oct 17, 2018

andybradshaw commented Oct 17, 2018

andybradshaw commented Oct 17, 2018

andybradshaw commented Oct 17, 2018

idealhack commented Oct 18, 2018

fedebongio commented Oct 18, 2018

cheftako Oct 20, 2018

andybradshaw Oct 23, 2018

cheftako left a comment

andybradshaw commented Oct 25, 2018

cheftako commented Oct 26, 2018

cheftako commented Oct 26, 2018

lavalamp Nov 1, 2018

liggitt Nov 1, 2018

lavalamp Nov 1, 2018

liggitt Nov 1, 2018 •

edited

liggitt Nov 1, 2018

lavalamp Nov 1, 2018

liggitt Nov 1, 2018

lavalamp Nov 1, 2018

k8s-ci-robot commented Nov 1, 2018

k8s-ci-robot commented Nov 1, 2018

andybradshaw commented Nov 2, 2018

lavalamp commented Nov 2, 2018 via email

andybradshaw commented Nov 2, 2018

lavalamp commented Nov 2, 2018

liggitt commented Nov 2, 2018

andybradshaw commented Nov 2, 2018

lavalamp commented Nov 2, 2018 via email

k8s-ci-robot commented Jan 18, 2019

fejta-bot commented Apr 18, 2019

fejta-bot commented May 18, 2019

fejta-bot commented Jun 17, 2019

k8s-ci-robot commented Jun 17, 2019

Add flag for setting number of service account workers #69937

Add flag for setting number of service account workers #69937

Conversation

andybradshaw commented Oct 17, 2018

k8s-ci-robot commented Oct 17, 2018

andybradshaw commented Oct 17, 2018

andybradshaw commented Oct 17, 2018

andybradshaw commented Oct 17, 2018

idealhack commented Oct 18, 2018

fedebongio commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheftako left a comment

Choose a reason for hiding this comment

andybradshaw commented Oct 25, 2018

cheftako commented Oct 26, 2018

cheftako commented Oct 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Nov 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 1, 2018

k8s-ci-robot commented Nov 1, 2018

andybradshaw commented Nov 2, 2018

lavalamp commented Nov 2, 2018 via email

andybradshaw commented Nov 2, 2018

lavalamp commented Nov 2, 2018

liggitt commented Nov 2, 2018

andybradshaw commented Nov 2, 2018

lavalamp commented Nov 2, 2018 via email

k8s-ci-robot commented Jan 18, 2019

fejta-bot commented Apr 18, 2019

fejta-bot commented May 18, 2019

fejta-bot commented Jun 17, 2019

k8s-ci-robot commented Jun 17, 2019

liggitt Nov 1, 2018 •

edited