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

Fix cross-build by having active deadline seconds use maxInt32 #46711

Merged
merged 2 commits into from Jun 2, 2017
Merged

Fix cross-build by having active deadline seconds use maxInt32 #46711

merged 2 commits into from Jun 2, 2017

Conversation

derekwaynecarr
Copy link
Member

@derekwaynecarr derekwaynecarr commented May 31, 2017 •
edited

What this PR does / why we need it:
Fixes cross build by having active deadline seconds use maxInt32 in validation.
In effect, this means the largest active deadline on a pod is ~68 years. Long enough.
Removes a redundant if-block in the validation code path.

We need this to fix cross build.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2017
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels May 31, 2017
@derekwaynecarr derekwaynecarr added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 31, 2017
@derekwaynecarr derekwaynecarr mentioned this pull request May 31, 2017
Merged
@alexandercampbell
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 31, 2017
@derekwaynecarr derekwaynecarr changed the title redudant if-block in validation code Fix cross-build by having active deadline seconds use maxInt32 May 31, 2017
@derekwaynecarr
Copy link
Member Author

/cc @ncdc @smarterclayton - this should fix the cross build error introduced in #46640

@derekwaynecarr derekwaynecarr added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 31, 2017
@ncdc
Copy link
Member

ncdc commented May 31, 2017

@k8s-bot pull-kubernetes-cross test this

@luxas
Copy link
Member

luxas commented May 31, 2017

Does this need a release note?

@derekwaynecarr
Copy link
Member Author

@luxas - it has one.

janetkuo
janetkuo reviewed May 31, 2017
View reviewed changes
pkg/api/validation/validation.go
}
value := *spec.ActiveDeadlineSeconds
if value < 1 || value > math.MaxInt32 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("activeDeadlineSeconds"), value, validation.InclusiveRangeError(1, math.MaxInt32)))
Copy link
Member

@janetkuo janetkuo May 31, 2017 •
edited

Choose a reason for hiding this comment

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

What's the range of valid pod .spec.ActiveDeadlineSeconds? 0 - MaxInt32 or 1 - MaxInt32?

Copy link
Member Author

Choose a reason for hiding this comment

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

0 is allowed on update of pod to mean "kill it now".

on create its 1-MaxInt32

Copy link
Member

Choose a reason for hiding this comment

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

@derekwaynecarr , can we just update InclusiveRangeError to use int64?

Copy link
Member

Choose a reason for hiding this comment

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

no, int64 values cannot be reliably represented in JSON

Copy link
Member

Choose a reason for hiding this comment

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

InclusiveRangeError is an internal error message builder which will return string. I think it's not related with JSON. Refer to #46807 for detail :).

Copy link
Member

Choose a reason for hiding this comment

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

ah, misread... I thought you wanted the valid range expanded to int64

@cblecker
Copy link
Member

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@cblecker cblecker mentioned this pull request May 31, 2017
Closed
@luxas
Copy link
Member

luxas commented Jun 1, 2017

@derekwaynecarr Ah, I meant the other way, does this really need a release note when it's a bugfix 😄? Just don't know whether someone will be interested in this Uint -> Int shift in relnotes...

@dhilipkumars dhilipkumars mentioned this pull request Jun 1, 2017
Merged
@caesarxuchao
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017
@luxas luxas mentioned this pull request Jun 1, 2017
Closed
@luxas
Copy link
Member

luxas commented Jun 1, 2017

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@caesarxuchao
Copy link
Member

Fixes #46793

@luxas luxas added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2017
@caesarxuchao
Copy link
Member

Adding a priority label since it's blocking cutting 1.7.0-beta.0.

@caesarxuchao
Copy link
Member

cc @dchen1107 @kubernetes/kubernetes-release-managers

@caesarxuchao caesarxuchao added the kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. label Jun 1, 2017
@derekwaynecarr derekwaynecarr added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 1, 2017
@derekwaynecarr
Copy link
Member Author

@luxas - struck release note.

@cblecker
Copy link
Member

cblecker commented Jun 1, 2017

@caesarxuchao were you looking to change queue order? If so you need a queue/fix or queue/critical-fix label. the latter skips e2e tests, the former just pushes it up in the queue

@derekwaynecarr
Copy link
Member Author

I think this should skip to top of queue if its blocking, added label.

@dchen1107
Copy link
Member

This pr is fixing a regression which blocks 1.7 beta.0 release. I am going to manually merge it once pull-kubernetes-e2e-gce-etcd3 is green. cc/ @pwittrock the build cop.

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alexandercampbell, caesarxuchao, derekwaynecarr
We suggest the following additional approver: smarterclayton

Assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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

@caesarxuchao
Copy link
Member 

error during ./hack/e2e-internal/e2e-up.sh: exit status 1

The test suite is not working again?

@caesarxuchao
Copy link
Member

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

This was referenced Jun 2, 2017
Closed
Closed
@dchen1107 dchen1107 merged commit b9e8d2a into kubernetes:master Jun 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@janetkuo janetkuo janetkuo left review comments

@k82cn k82cn k82cn left review comments

Assignees

@caesarxuchao caesarxuchao

@janetkuo janetkuo

@alexandercampbell alexandercampbell

@piosz piosz

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. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. 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.

None yet