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

validate activeDeadlineSeconds in rs/rc #38741

Merged
merged 1 commit into from
Mar 29, 2017
Merged

validate activeDeadlineSeconds in rs/rc #38741

merged 1 commit into from
Mar 29, 2017

Conversation

FANNG1
Copy link
Contributor

@FANNG1 FANNG1 commented Dec 14, 2016 •
edited by 0xmichalis
Loading

What this PR does / why we need it:
if setting activeDeadlineSeconds, deployment will continuously created new pods after old pod dies.
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #38684

Special notes for your reviewer:

Release note:

ActiveDeadlineSeconds is validated in workload controllers now, make sure it's not set anywhere (it shouldn't be set by default and having it set means your controller will restart the Pods at some point)

@k8s-ci-robot
Copy link
Contributor

Hi @sandflee. 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 @k8s-bot 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.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 14, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@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 Dec 14, 2016
@dims
Copy link
Member

dims commented Dec 14, 2016

@k8s-bot ok to test

@0xmichalis
Copy link
Contributor

@kubernetes/sig-apps this is not backwards-compatible but I guess people who specify activeDeadlineSeconds in their controllers have already figured out it's wrong.

@0xmichalis
Copy link
Contributor

If we are going to do this, we should also update the validation for deployments, daemonsets, and statefulsets.

@FANNG1
Copy link
Contributor Author

FANNG1 commented Dec 15, 2016

the validation of deployment is through rs, we may add the validation of daemonset and statefulsets

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 15, 2016
@k8s-github-robot
Copy link

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

We suggest the following people:
cc @smarterclayton
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 assigned thockin and unassigned erictune Jan 30, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2017
thockin
thockin reviewed Feb 28, 2017
View reviewed changes
pkg/api/validation/validation.go Outdated
@@ -2752,6 +2752,10 @@ func ValidatePodTemplateSpecForRC(template *api.PodTemplateSpec, selectorMap map
if template.Spec.RestartPolicy != api.RestartPolicyAlways {
allErrs = append(allErrs, field.NotSupported(fldPath.Child("spec", "restartPolicy"), template.Spec.RestartPolicy, []string{string(api.RestartPolicyAlways)}))
}

if template.Spec.ActiveDeadlineSeconds != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("spec", "activeDeadlineSeconds"), template.Spec.ActiveDeadlineSeconds, "must be nil"))
Copy link
Member

Choose a reason for hiding this comment

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

nil is a Go ism, not part of the API. "must not be specified", same all over

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2017
@FANNG1
Copy link
Contributor Author

FANNG1 commented Mar 1, 2017

update the patch to use "must not be specified"

@thockin thockin added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 1, 2017
@FANNG1
Copy link
Contributor Author

FANNG1 commented Mar 6, 2017

test failure seems not related, see #41419

@0xmichalis 0xmichalis self-assigned this Mar 21, 2017
@0xmichalis
Copy link
Contributor

0xmichalis commented Mar 29, 2017 •
edited
Loading

@k8s-bot unit test this (confimap controller unit test flake)

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit a4f6cae into kubernetes:master Mar 29, 2017
@k8s-github-robot
Copy link

@sandflee PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thockin thockin thockin left review comments

Assignees

@0xmichalis 0xmichalis

@thockin thockin

@erictune erictune

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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

Disallow setting activeDeadlineSeconds for restartAlways pods/controllers
8 participants
@FANNG1 @k8s-ci-robot @k8s-reviewable @dims @0xmichalis @k8s-github-robot @thockin @erictune