@wojtek-t I assume #22835 isn't actually a fix for this, just a way to get faster logs?

@davidopp Actually #22835 is only where the test failed. To be honest, I didn't have time to look into it at all.

@mml any progress on this?

http://pr-test.k8s.io/23415/kubernetes-pull-test-unit-integration/19839/ is another failure.

scheduler_test.go:370: Test MultiScheduler: pod-with-no-annotation Pod scheduled
scheduler_test.go:377: Test MultiScheduler: pod-with-annotation-fits-default Pod scheduled
scheduler_test.go:384: Test MultiScheduler: pod-with-annotation-fits-foo Pod not scheduled
scheduler_test.go:408: Test MultiScheduler: pod-with-annotation-fits-foo Pod scheduled
scheduler_test.go:439: Test MultiScheduler: pod-with-no-annotation2 Pod got scheduled, <nil>
scheduler_test.go:447: Test MultiScheduler: pod-with-annotation-fits-default2 Pod scheduled

Only this one is an actual error.

    scheduler_test.go:439: Test MultiScheduler: pod-with-no-annotation2 Pod got scheduled, <nil>

And... the logging logic doesn't make much sense. It logs "err", but it only gets there if err was nil.

I can't see any other logs besides the junit logs, which don't provide any extra info. Am I missing something?

There is a concurrency bug in the test. We signal the scheduler to stop by closing a channel but we don't synchronize at this point to verify that the scheduler has stopped. Indeed, since scheduleOne blocks on NextPod in the absence of any work, we could sit around a very long time, schedule one more thing, and only then get the signal to exit.

We need to synchronize here, but once we do that, we may need to prevent the deadlock that occurs while scheduleOne blocks forever waiting for NextPod().

I don't know that this is the bug, but the bug is there and it fits the symptom.

This is similar to the bug I fixed with #22727. The Until() function seems susceptible to this by design. Not only does it provide no way to verify that the goroutine has been stopped, it makes it hard to avoid deadlock because the provided f may block forever.

We should probably re-do this interface to avoid the problem, although in practice I bet it only pops up in tests. @davidopp I see two choices: invest in fixing this now, or comment out all the test that come after the assumption "now we've stopped this scheduler" and fix it later. WDYT?

When you say

comment out all the test that come after the assumption "now we've stopped this scheduler"

Are you talking about everything after step 7?

@davidopp Yes.

I guess it is fine to comment out everything after step 7 for now, if you don't see any easy alternative. The key is that we test that the right scheduler is scheduling the pod, and while steps 8/9 do that, earlier parts of the test also do it and I think they're adequate.

Please file an issue related to the problem you described, so we can fix it eventually.