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

Increase default value of apps/v1beta2 DeploymentSpec.RevisionHistoryLimit to 10 #49924

Merged
merged 2 commits into from
Aug 3, 2017
Merged

Increase default value of apps/v1beta2 DeploymentSpec.RevisionHistoryLimit to 10 #49924

merged 2 commits into from
Aug 3, 2017

Conversation

dixudx
Copy link
Member

@dixudx dixudx commented Aug 1, 2017 •
edited
Loading

What this PR does / why we need it:

All controllers that use the RevisionHistoryLimit field have a default value of 10 for the field, except for Deployment, which has a default of 2. We should increase it to 10 for consistency on its default value across controllers.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #49913

Special notes for your reviewer:
/cc @janetkuo @foxish @liyinan926

Release note:

Increase default value of apps/v1beta2 DeploymentSpec.RevisionHistoryLimit to 10

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 1, 2017
@k8s-ci-robot k8s-ci-robot requested a review from janetkuo August 1, 2017 02:55
@k8s-ci-robot
Copy link
Contributor

@dixudx: GitHub didn't allow me to request PR reviews from the following users: liyinan926.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it:

All controllers that use the RevisionHistoryLimit field have a default value of 10 for the field, except for Deployment, which has a default of 2. We should increase it to 10 for consistency on its default value across controllers.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #49913

Special notes for your reviewer:
/cc @janetkuo @foxish @liyinan926

Release note:

Increase default value of DeploymentSpec.RevisionHistoryLimit to 10

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 k8s-ci-robot requested a review from foxish August 1, 2017 02:55
@k8s-ci-robot
Copy link
Contributor

Hi @dixudx. Thanks for your PR.

I'm waiting for a kubernetes 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.

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

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 1, 2017
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 1, 2017
@liyinan926
Copy link
Contributor

@dixudx We only plan to do this for apps/v1beta2. You also need to update defaults_test.go.

@dixudx dixudx force-pushed the increase_deployment_default_RevisionHistoryLimit branch from 8434913 to ebd8ac7 Compare August 1, 2017 04:52
@dixudx
Copy link
Member Author

dixudx commented Aug 1, 2017

@liyinan926 Updated. PTAL. Thanks.

@liyinan926
Copy link
Contributor

LGTM.

@sttts sttts assigned tsmetana and unassigned sttts Aug 1, 2017
@sttts
Copy link
Contributor

sttts commented Aug 1, 2017

@tsmetana can you take a look here?

@sttts
Copy link
Contributor

sttts commented Aug 1, 2017

Wrong Tomas it seems. @tnozicka ptal

@bgrant0607
Copy link
Member

Ref kubernetes/enhancements#353

@dixudx @liyinan926 I don't see this mentioned in #49135

Also, default values should be documented in the API documentation in the field comments in v1beta2/types.go.

Both the API documentation and the release note should be written for Kubernetes users, not just for Kubernetes contributors, which means they should use the json names, not Go type/field names.

@liyinan926
Copy link
Contributor

@bgrant0607 this is part of the work on "ensure that the defaults and validation are consistent and appropriate across all controllers." mentioned in #49135. #49913 has more details on inconsistency of the default value of RevisionHistoryLimit.

@kow3ns
Copy link
Member

kow3ns commented Aug 1, 2017

/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 Aug 1, 2017
@liyinan926
Copy link
Contributor

/lgtm

@k8s-ci-robot
Copy link
Contributor

@liyinan926: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues.

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.

@mml
Copy link
Contributor

mml commented Aug 2, 2017

/assign mml janetkuo saad-ali kow3ns
/unassign pmorie wojtekt

@k8s-ci-robot k8s-ci-robot assigned janetkuo, kow3ns, mml and saad-ali and unassigned pmorie Aug 2, 2017
@mml
Copy link
Contributor

mml commented Aug 2, 2017

/unassign wojtekt

@mml
Copy link
Contributor

mml commented Aug 2, 2017

👎

/unassign wojtek-t

@mml
Copy link
Contributor

mml commented Aug 2, 2017

OK, after all those gymnastics, it looks like @kow3ns has this under control.

/unassign

@dixudx
Copy link
Member Author

dixudx commented Aug 3, 2017

@thockin @smarterclayton Needs your approval. Thanks.

@thockin
Copy link
Member

thockin commented Aug 3, 2017

/approve

@thockin
Copy link
Member

thockin commented Aug 3, 2017

FWIW the corr3ct protocol would be to /assign thockin which makes it WAY more visible to me. :)

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, kow3ns, liyinan926, thockin

Associated issue: 49913

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2017
@kow3ns
Copy link
Member

kow3ns commented Aug 3, 2017

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 66bfab8 into kubernetes:master Aug 3, 2017
@dixudx dixudx deleted the increase_deployment_default_RevisionHistoryLimit branch August 4, 2017 01:31
@tnozicka tnozicka mentioned this pull request Aug 4, 2017
Merged
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Aug 25, 2017
@openshift-merge-robot
Merge pull request #15642 from tnozicka/add-default-for-deploymentcon…
a6abbaf 
…figs

Automatic merge from submit-queue

Set default for DeploymentConfigSpec.RevisionHistoryLimit in apps/v1

At this point we don't default DeploymentConfigSpec.RevisionHistoryLimit in apps/v1 which is causing the deployments to have unlimited history which will lead to performance issues at scale and when deploying regularly. 

Note that upstream also defaulted RevisionHistoryLimit to 10 for Deployments in kubernetes/kubernetes#49924

Needs to be backported to 3.6.1 as well.

cc: @smarterclayton @mfojtik @Kargakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kow3ns kow3ns kow3ns approved these changes

@janetkuo janetkuo Awaiting requested review from janetkuo

@foxish foxish Awaiting requested review from foxish

Assignees

@janetkuo janetkuo

@saad-ali saad-ali

@kow3ns kow3ns

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Increase default value of DeploymentSpec.RevisionHistoryLimit to 10 in apps/v1beta2