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

Remove FailedResourceType and return custom error #19296

Merged
merged 1 commit into from
Jan 11, 2016
Merged

Remove FailedResourceType and return custom error #19296

merged 1 commit into from
Jan 11, 2016
+59 −15

Conversation

hongchaodeng
Copy link
Contributor

Currently in Fit predicate it uses a global variable FailedResourceType to pass information:

            predicates.FailedResourceType = ""
            fit, err := predicate(pod, machineToPods[node.Name], node.Name)
            if !fit {
                ...
                if predicates.FailedResourceType != "" {
                    failedPredicateMap[node.Name].Insert(predicates.FailedResourceType)
                    break
                }
            }

We should use custom error to pass it instead:

            fit, err := predicate(pod, machineToPods[node.Name], node.Name)
            if err != nil {
                return api.NodeList{}, FailedPredicateMap{}, err
            }
            if !fit {
                ...
                if re, ok := err.(*predicates.ErrResourceUnfit); ok {
                    failedPredicateMap[node.Name].Insert(re.Reason())
                    break
                }
            }

This PR adds a new custom type of error to predicates

@xiang90
Copy link
Contributor

xiang90 commented Jan 5, 2016

@hongchaodeng Thanks! I wanted to remove this a while ago.

@k8s-bot
Copy link

k8s-bot commented Jan 5, 2016

GCE e2e build/test failed for commit f23ad8329c2124dab404e4bbbe434bbfe99bb9d8.

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 5, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 5, 2016
@k8s-bot
Copy link

k8s-bot commented Jan 5, 2016

GCE e2e test build/test passed for commit da61b4d026fa824a277c60fc4847c0f4a777b8c0.

@hongchaodeng
Copy link
Contributor Author

@lavalamp
Can you take a look at this PR?

@hongchaodeng hongchaodeng reopened this Jan 6, 2016
@lavalamp
Copy link
Member

lavalamp commented Jan 6, 2016

If I understand what you're doing with this--please make a custom error type and encode this information in the error. Don't add an additional return parameter for this.

@hongchaodeng hongchaodeng changed the title Remove predicates.FailedResourceType and use returned value Remove FailedResourceType and return custom error Jan 6, 2016
@hongchaodeng
Copy link
Contributor Author

@lavalamp
Thanks for the suggestion. Addressed.

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 6, 2016
xiang90
xiang90 reviewed Jan 6, 2016
View reviewed changes
plugin/pkg/scheduler/algorithm/predicates/error.go

import "fmt"

type ResourceUnFitError struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

type ErrResourceUnfit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@hongchaodeng hongchaodeng force-pushed the pred branch 3 times, most recently from 0735b16 to 4f6cf41 Compare January 6, 2016 18:00
@k8s-bot
Copy link

k8s-bot commented Jan 6, 2016

GCE e2e test build/test passed for commit bb6492bee7b60f789b4963bd5dcbfac47d268164.

@k8s-bot
Copy link

k8s-bot commented Jan 6, 2016

GCE e2e test build/test passed for commit 3234379a524df929e69a883a203ba95b72146bb4.

@k8s-bot
Copy link

k8s-bot commented Jan 6, 2016

GCE e2e test build/test passed for commit fbb9f07ee184d51ea136463109a0d0168c44507d.

@k8s-bot
Copy link

k8s-bot commented Jan 6, 2016

GCE e2e test build/test passed for commit 4f6cf41bd83c361ab9ede13f90521e3525d75282.

@k8s-bot
Copy link

k8s-bot commented Jan 6, 2016

GCE e2e test build/test passed for commit a91c1696dd615ea5f093f664bdde3dab55002fa4.

@lavalamp
Copy link
Member

lavalamp commented Jan 8, 2016

LGTM, thanks

@lavalamp lavalamp added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Jan 8, 2016
@mwielgus
Copy link
Contributor

mwielgus commented Jan 8, 2016

k8s bot test this please

@davidopp
Copy link
Member

@k8s-bot unit test this please

@gmarek
Copy link
Contributor

gmarek commented Jan 11, 2016

@k8s-bot unit test this

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Jan 11, 2016

GCE e2e test build/test passed for commit 40e5e68.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jan 11, 2016

GCE e2e test build/test passed for commit 40e5e68.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Jan 11, 2016
Merge pull request #19296 from hongchaodeng/pred
f580569 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit f580569 into kubernetes:master Jan 11, 2016
@gmarek
Copy link
Contributor

gmarek commented Jan 11, 2016

I believe this broke our tests:

SchedulerPredicates [Serial] validates resource limits of pods that are allowed to run [Conformance]

/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/scheduler_predicates.go:355 Expected : 19 to equal : 21

I'll try reverting it locally to confirm.

@hongchaodeng hongchaodeng deleted the pred branch January 11, 2016 17:17
@hongchaodeng
Copy link
Contributor Author

@gmarek
I'm trying to understand what error it brings. But the error message is so cryptic (who brought up the idea to use ginkgo?) My guess is that this line failed:

kubernetes/test/e2e/scheduler_predicates.go

Line 86 in 14d9a0f

Expect(currentlyScheduledPods).To(Equal(len(podsScheduledBefore) + replicas))

If so, it looks like there is terrible logical error in pods (listing, whatever)..

@yujuhong
Copy link
Contributor

Can we revert the PR to unblock the builds if the test/pr cannot be fixed immediately?

@saad-ali saad-ali mentioned this pull request Jan 11, 2016
Closed
@hongchaodeng
Copy link
Contributor Author

Can we wait for @gmarek 's confirmation?

@saad-ali
Copy link
Member

I too suspect that #19487 is caused by this PR. This is blocking the merge queue and is a P0 issue, so I will roll back this PR (it can be re-merged if it is not problematic).

@gmarek
Copy link
Contributor

gmarek commented Jan 11, 2016

After revert it was still failing. Sorry - I had to run home:/

@gmarek
Copy link
Contributor

gmarek commented Jan 11, 2016

It's better if @saad-ali would take it from here.

@saad-ali
Copy link
Member

It's better if @saad-ali would take it from here.

Thanks @gmarek. I'll take over.

@saad-ali
Copy link
Member

After revert it was still failing.

Ok, I'll hold off on rolling back this PR then. I'll look into some of the other PRs that went in at the same time, if nothing looks promising I'll roll back anyway.

@saad-ali
Copy link
Member

if nothing looks promising I'll roll back anyway

This PR still looks the most suspicious to me. I'm going to roll it back to be safe. If it is not the cause of the issue we can re-merge when everything is green again.

@saad-ali saad-ali mentioned this pull request Jan 11, 2016
Merged
@hongchaodeng hongchaodeng restored the pred branch January 11, 2016 21:48
@saad-ali
Copy link
Member

Confirmed revert of this PR fixed #19487

@hongchaodeng hongchaodeng mentioned this pull request Jan 12, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.