fix duplicate bind issues #832
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 0c9c03e 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/619dbe6d295ab20008ad767e |
|
Not ready yet, need to look into test failures. |
| for _, pod := range pods { | ||
| candidate := pod | ||
| if err := p.kubeClient.Get(ctx, types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}, candidate); err != nil { | ||
| logging.FromContext(ctx).Errorf("Checking if pod \"%s\" is provisionable", pod) |
There was a problem hiding this comment.
This error log line seems a bit odd to me. Also, what if the pod is deleted at this point? Perhaps we should continue if errors.IsNotFound, but log an error otherwise? "Unexpected error filtering provisionable pods, %s"
There was a problem hiding this comment.
tbh I think a lot of our error logging is really weird like this and doesn't read well in the actual log. I've tried to keep with our convention for the most part, but I definitely like explicitly saying "Unexpected error" rather than relying on the log level to convey it's an error. Maybe we should clean up the other error logging.
There was a problem hiding this comment.
I love this comment! No need to say failed if it's an error!
bwagner5
force-pushed
the
fix-bind-issues
branch
from
d56ee46 to
356c82b
Compare
bwagner5
force-pushed
the
fix-bind-issues
branch
from
356c82b to
562d4e8
Compare
1. Issue, if available:
N/A
2. Description of changes:
IsUnschedulabletoisSchedulableIsUnschedulabledoesn't make sense to describe what the function does. Although the func does do one check to determine if kube-scheduler is failed to schedule the pod, it also does a number of other checks such as if the pod is not already scheduled to a node. I think it makes more sense to name the func in the context of Karpenter's view of schedulable, hence,isSchedulable.3. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.