Add a system modeler to scheduler #5446
Conversation
|
Updated with one more test and some logging output in case this ever needs to be debugged. |
|
...and found a spec.host/status.host bug. OK, this should be ready to go now. Running e2e again but I expect it to pass again. |
|
e2e passes. Travis is a flake, rerunning. |
| @@ -52,6 +52,15 @@ func (s *StoreToPodLister) List(selector labels.Selector) (pods []api.Pod, err e | |||
| return pods, nil | |||
| } | |||
|
|
|||
| // Returns true if a pod matching the namespace/name of the given pod exists in the store. | |||
| func (s *StoreToPodLister) Exists(pod *api.Pod) bool { | |||
There was a problem hiding this comment.
I thought you said the linter barfs if this isn't phrased as "StoreToPodLister returns true if ..." ? (BTW I think that policy is overly verbose and presumably optimizing for the uncommon case, which is that people are reading the godoc-processed code rather than the source code)
On a more concrete note, I'm not sure how I feel about this function converting the error into "false." It would probably be better if you propagated it up to the caller by returning bool, err.
There was a problem hiding this comment.
Fixed, and fixed. It's go vet that barfs, and we don't run it on precommit.
|
LGTM |
davidopp
added
the
lgtm
| @@ -72,5 +72,13 @@ func TestStoreToPodLister(t *testing.T) { | |||
| t.Errorf("Expected %v, got %v", e, a) | |||
| continue | |||
| } | |||
|
|
|||
| if !spl.Exists(&api.Pod{ObjectMeta: api.ObjectMeta{Name: id}}) { | |||
| t.Errorf("exists returnef false for %v", id) | |||
|
Ready to merge? |
|
Yeah if it's OK I'd like to fix these comments in a follow-up? |
So it can try to predict the effect its bindings will have.
|
OK actually I've fixed the comments here. |
|
LGTM Oncall, please merge on Monday in case I forget. |
Add a system modeler to scheduler
|
|
||
| // SimpleModeler implements the SystemModeler interface with a timed pod cache. | ||
| type SimpleModeler struct { | ||
| queuedPods ExtendedPodLister |
There was a problem hiding this comment.
Hi @lavalamp
Sorry for disturbing on this old commit.
I'm wondering why it adds queuePods here? It looks like it's the queue for pods to schedule. But if a pod is assumed, it's already made upon a schedule decision. If it succeed, it will go into scheduledPods; otw, it will still get deleted. Right?
There was a problem hiding this comment.
To be more specific, I am pointing to this code block:
c.ScheduledPodLister.Store, c.scheduledPodPopulator = framework.NewInformer(
c.createAssignedPodLW(),
...
DeleteFunc: func(obj interface{}) {
c.modeler.LockedAction(func() {
switch t := obj.(type) {
case *api.Pod:
c.modeler.ForgetPod(t)
case cache.DeletedFinalStateUnknown:
c.modeler.ForgetPodByKey(t.Key)
}
})If a pod is rejected or removed, it's gonna be deleted and notified with delete event. Right?
There was a problem hiding this comment.
@hongchaodeng IIRC, at the time there was a race where a rejected pod could show up in the watch before the binding failure propagated back? Note that this code was written before the informer was created. So this race might not be possible any more.
|
@lavalamp is out for a while, but I believe @bprashanth is familiar with this code. |
So it can try to predict the effect its bindings will have.
Fixes #5336
I'm slightly concerned about the TODO. But hopefully we can fix it in a followup PR as I'm out next week and want to get this in before I go.