-
Notifications
You must be signed in to change notification settings - Fork 40k
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.
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
Update FitError as a message component into the PodConditionUpdater. #39491
Update FitError as a message component into the PodConditionUpdater. #39491
Conversation
630d7f6
to
abe8f17
Compare
@k8s-bot test this please |
abe8f17
to
ec1bfa1
Compare
ec1bfa1
to
e309d26
Compare
Alrite this is working now... here's an example of how it looks with a redis pod that asks for too much memory.
I'm just using the existing scheduler condition updater, and all I've done here is modify the update function to write the error as a message that is has the desired UX . Should be Good enough for us to keep moving forward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit.
"3": []algorithm.PredicateFailureReason{algorithmpredicates.ErrNodeUnderDiskPressure}, | ||
}, | ||
} | ||
if strings.Contains(error.Error(), "No nodes are available that match all of the following predicates") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not string match the the entire expected string?
- It makes it easier to read
- It gives a clear example of the flattening
... etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, this is to avoid Overtesting: We shouldn't be testing the order in which the predicate failures are listed, as thats neither a requirement nor a gauranteed quality of maps in golang: https://nathanleclaire.com/blog/2014/04/27/a-surprising-feature-of-golang-that-colored-me-impressed/.
We could print it multiple times and gaurantee that the exact string is eventually provided, but that would be awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your "No nodes ..." should be a const btw, otherwise subtle change in 2 locs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked the constant idea, but kept the constant unused in the unit test -- simply because it would make a "self-fulfilling pass" . The decoupling of unit test from the code is necessary for this feature since otherwise, regression in the readability of the message could go unchecked. just updated and pushed
e309d26
to
7873e48
Compare
Jenkins Bazel Build failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins verification failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins GCE e2e failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins GCE etcd3 e2e failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins GKE smoke e2e failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins kops AWS e2e failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins GCI GCE e2e failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins GCE Node e2e failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins CRI GCE Node e2e failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins GCI GKE smoke e2e failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins unit/integration failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
Jenkins Kubemark GCE e2e failed for commit 7873e48122304d9395789efa0535e9a743eb4d79. Full PR test history. The magic incantation to run this job again is 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. |
7873e48
to
9cdc4ae
Compare
This is great, capturing the last predicate failed predicate histogram in the Reason is highly useful. |
Unfortunately I don't have time to review this. @wojtek-t could you take a look? |
Automatic merge from submit-queue (batch tested with PRs 34488, 39511, 39619, 38342, 39491) |
Fixes #20064 , after a roundabout volley of ideas, we ended up digging into existing Conditions for this, rather then a first class API object. This is just a quick sketch of the skeleton minimal implementation, it should pretty much "just work". I'll test it more later today.
Release Note: