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 max-requests-inflight parameter #4395

Merged
merged 1 commit into from
Feb 14, 2018
Merged

Add max-requests-inflight parameter #4395

merged 1 commit into from
Feb 14, 2018

Conversation

ihoegen
Copy link
Contributor

@ihoegen ihoegen commented Feb 6, 2018

Adding the field as described in #4322

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 6, 2018
@chrislovecnm
Copy link
Contributor

/assign @mikesplain

@mikesplain
Copy link
Contributor

Hey @ihoegen thanks so much for the contribution!

This looks pretty good but we need a few things.

  • Can you regenerate with make apimachinery, my generation has more in it than yours did. You may need to run make clean first.
  • Can you add docs so others can use this as well?
  • Squash the above changes all into one nice commit

Let me know if you have any questions! Thanks again!

@mikesplain
Copy link
Contributor

/ok-to-test

@k8s-ci-robot 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 Feb 6, 2018
@mikesplain
Copy link
Contributor

/retest

@chrislovecnm
Copy link
Contributor

Also which versions of k8s does this run on?

@ihoegen
Copy link
Contributor Author

ihoegen commented Feb 7, 2018

Earliest occurrence of the flag I could find on the kube apiserver was on 1.0

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 7, 2018
@mikesplain
Copy link
Contributor

@ihoegen Great! Looks like you have some test issues though, when I run locally I get

nodeup/pkg/model/kube_apiserver_test.go:44:23: unknown field 'MaxRequestInflight' in struct literal of type "k8s.io/kops/pkg/apis/kops".KubeAPIServerConfig
nodeup/pkg/model/kube_apiserver_test.go:59:23: unknown field 'MaxRequestInflight' in struct literal of type "k8s.io/kops/pkg/apis/kops".KubeAPIServerConfig

@mikesplain
Copy link
Contributor

/retest

@mikesplain
Copy link
Contributor

/lgtm thanks so much!

@mikesplain
Copy link
Contributor

/assign @chrislovecnm

@ihoegen ihoegen changed the title Add max-request-inflight parameter Add max-requests-inflight parameter Feb 8, 2018
@chrislovecnm
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2018
justinsb
justinsb reviewed Feb 13, 2018
View reviewed changes
nodeup/pkg/model/kube_apiserver_test.go Outdated
@@ -31,38 +31,52 @@ func Test_KubeAPIServer_BuildFlags(t *testing.T) {
}{
{
kops.KubeAPIServerConfig{},
"--insecure-port=0 --secure-port=0",
"--insecure-port=0 --max-requests-inflight=0 --secure-port=0",
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I think this might actually be honored - i.e. we would actually set the limit as 0.

I actually thought that int=0 defaulted to a omitted flag, but I guess not.

I think you need to add flag-empty:"0"

pkg/apis/kops/componentconfig.go Outdated
@@ -293,6 +293,8 @@ type KubeAPIServerConfig struct {
RequestheaderAllowedNames []string `json:"requestheaderAllowedNames,omitempty" flag:"requestheader-allowed-names"`
// FeatureGates is set of key=value pairs that describe feature gates for alpha/experimental features.
FeatureGates map[string]string `json:"featureGates,omitempty" flag:"feature-gates"`
// MaxRequestsInflight The maximum number of non-mutating requests in flight at a given time.
MaxRequestsInflight int `json:"maxRequestsInflight,omitempty" flag:"max-requests-inflight"`
Copy link
Member

Choose a reason for hiding this comment

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

So you need to add flag-empty:"0" here (although separately we might want to figure out if 0 should be omitted I guess!)

Also technically this should be int32 (or int64), because it's part of our API and we shouldn't have architecture dependent types in our API. Not that it matters in practice :-)

pkg/apis/kops/v1alpha1/componentconfig.go Outdated
@@ -293,6 +293,8 @@ type KubeAPIServerConfig struct {
RequestheaderAllowedNames []string `json:"requestheaderAllowedNames,omitempty" flag:"requestheader-allowed-names"`
// FeatureGates is set of key=value pairs that describe feature gates for alpha/experimental features.
FeatureGates map[string]string `json:"featureGates,omitempty" flag:"feature-gates"`
// MaxRequestsInflight The maximum number of non-mutating requests in flight at a given time.
MaxRequestsInflight int `json:"maxRequestsInflight,omitempty" flag:"max-requests-inflight"`
Copy link
Member

Choose a reason for hiding this comment

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

int32 and flag-empty:"0" here as well

pkg/apis/kops/v1alpha2/componentconfig.go Outdated
@@ -293,6 +293,8 @@ type KubeAPIServerConfig struct {
RequestheaderAllowedNames []string `json:"requestheaderAllowedNames,omitempty" flag:"requestheader-allowed-names"`
// FeatureGates is set of key=value pairs that describe feature gates for alpha/experimental features.
FeatureGates map[string]string `json:"featureGates,omitempty" flag:"feature-gates"`
// MaxRequestsInflight The maximum number of non-mutating requests in flight at a given time.
MaxRequestsInflight int `json:"maxRequestsInflight,omitempty" flag:"max-requests-inflight"`
Copy link
Member

Choose a reason for hiding this comment

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

And don't forget here either :-) int32 and flag-empty:"0"

@justinsb
Copy link
Member

Generally looks good but I think we want int32 and flag-empty:"0", as otherwise I think we will actually set the limit to 0 (?)

@chrislovecnm
Copy link
Contributor

/approve cancel
/assign @justinsb

@chrislovecnm chrislovecnm removed their assignment Feb 13, 2018
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2018
@justinsb
Copy link
Member

Many thanks for updating @ihoegen

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ihoegen, justinsb, mikesplain

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 OWNERS Files:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2018
@k8s-ci-robot k8s-ci-robot merged commit 2b1ecba into kubernetes:master Feb 14, 2018
@captainkerk captainkerk mentioned this pull request Sep 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

Assignees

@justinsb justinsb

@mikesplain mikesplain

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 "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ihoegen @chrislovecnm @mikesplain @justinsb @k8s-ci-robot