Skip to content

selector: make sure value of GT and LT is integer#28474

Merged
k8s-github-robot merged 1 commit intokubernetes:masterfrom
hongchaodeng:lb
Jul 6, 2016
Merged

selector: make sure value of GT and LT is integer#28474
k8s-github-robot merged 1 commit intokubernetes:masterfrom
hongchaodeng:lb

Conversation

@hongchaodeng
Copy link
Copy Markdown
Contributor

@hongchaodeng hongchaodeng commented Jul 4, 2016

GT and LT in selector has been introduced in Node Affinity feature: #19758, #18261

According to the API:

If the operator is Gt or Lt, the values array must have a single element, which will be interpreted as an integer.

But the implementation has parsed it as float64:

kubernetes/pkg/labels/selector.go

Line 183 in ef0c9f0

lsValue, err := strconv.ParseFloat(ls.Get(r.key), 64)

Modeling integer as float is dangerous. We don't even have fixed precision guarantee when doing comparison.

This PR is to get rid of this pre-optimization and convert integer to int64.

@hongchaodeng
Copy link
Copy Markdown
Contributor Author

hongchaodeng commented Jul 4, 2016

@xiang90 @kubernetes/sig-scheduling @timothysc

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 4, 2016
@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 4, 2016

GCE e2e build/test passed for commit eb981a8770852cd7d49847b4c40da1344d46537a.

@kevin-wangzefeng
Copy link
Copy Markdown
Contributor

kevin-wangzefeng commented Jul 5, 2016

There's no special motivation to use float instead of int here (my mistake), to make it integer would be a better choice and this is also what the API defines.

@kevin-wangzefeng
Copy link
Copy Markdown
Contributor

kevin-wangzefeng commented Jul 5, 2016

The test TestNodeSelectorRequirementsAsSelector in pkg/api/helpers_test.go also needs update :)

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 5, 2016

GCE e2e build/test passed for commit 09b02a5e64aec73b3d9090a7c71e3eaa98dee6ed.

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 5, 2016

GCE e2e build/test passed for commit 7127915.

@kevin-wangzefeng
Copy link
Copy Markdown
Contributor

kevin-wangzefeng commented Jul 5, 2016

LGTM, thanks.

@davidopp davidopp assigned davidopp and unassigned thockin Jul 5, 2016
@hongchaodeng
Copy link
Copy Markdown
Contributor Author

hongchaodeng commented Jul 5, 2016

@davidopp
Kevin (original author) already gives LGTM.
Can you help review this?

@davidopp davidopp added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 5, 2016
@davidopp
Copy link
Copy Markdown
Contributor

davidopp commented Jul 5, 2016

LGTM

Thanks

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2016
@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 5, 2016

GCE e2e build/test failed for commit 7127915.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@hongchaodeng
Copy link
Copy Markdown
Contributor Author

hongchaodeng commented Jul 5, 2016

@k8s-bot e2e test this issue: #27335

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 6, 2016

GCE e2e build/test passed for commit 7127915.

@ixdy
Copy link
Copy Markdown
Contributor

ixdy commented Jul 6, 2016

@k8s-bot test this please, issue #IGNORE (probably broke this while restarting Jenkins)

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 6, 2016

Build finished. 3357 tests run, 14 skipped, 0 failed.

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 6, 2016

Build finished. 330 tests run, 148 skipped, 0 failed.
GCE e2e build/test passed for commit 7127915.

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 6, 2016

GCE e2e build/test passed for commit 7127915.

@ixdy
Copy link
Copy Markdown
Contributor

ixdy commented Jul 6, 2016

@k8s-bot test this please, issue #27462

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 6, 2016

GCE e2e build/test passed for commit 7127915.

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 6, 2016

GCE e2e build/test passed for commit 7127915.

@wojtek-t
Copy link
Copy Markdown
Member

wojtek-t commented Jul 6, 2016

@k8s-bot node e2e test this please, issue #27462

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 6, 2016

GCE e2e build/test passed for commit 7127915.

@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Jul 6, 2016

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 1ff33c5 into kubernetes:master Jul 6, 2016
@hongchaodeng hongchaodeng deleted the lb branch July 8, 2016 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@davidopp davidopp

Labels

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@hongchaodeng @k8s-bot @kevin-wangzefeng @davidopp @ixdy @wojtek-t @k8s-github-robot @googlebot @thockin