No problem, really glad to have additional eyes on the code! I'm sure there are other bugs waiting for your discovery :-)
-=Bill On Mon, Sep 15, 2014 at 2:46 PM, Stephan Erb <m...@stephanerb.eu> wrote: > > You are right. I totally misread what is returned there. Sorry for the > fuss! :-) > > > > On So, 2014-09-14 at 16:48 -0700, Bill Farner wrote: > > > > > > assumes the latter > > > > > > I think i see what you're saying, but i believe this is a case of > > performing unnecessary work rather than unnecessarily evicting tasks. > > > > Consider the case you describe, where there are tasks on a host and a > > pending task that can preempt them, but due to constraints may not run on > > the host. We iterate through the possible victim tasks [1], however each > > time the vetos [sic] List is non-empty [2]. As a result, we exit the > loop > > and return Optional.absent() [3], thus not actually flagging anything to > > evict. > > > > I suppose what's confusing here is the variable name toPreemptTasks, > which > > suggests that we *are* going to preempt these tasks. However, these are > > actually preemption candidates, and the pending task's constraints still > > must be satisfied. > > > > I do agree to something you alluded — if Veto differentiated between a > > constraint mismatch and insufficient resources, we could exit this > function > > sooner. > > > > Does that make sense and match your read of the code? > > > > [1] > > > https://github.com/apache/incubator-aurora/blob/091bd6decbb9d75962a55a4e5cff82ce58a2a064/src/main/java/org/apache/aurora/scheduler/async/Preemptor.java#L262 > > [2] > > > https://github.com/apache/incubator-aurora/blob/091bd6decbb9d75962a55a4e5cff82ce58a2a064/src/main/java/org/apache/aurora/scheduler/async/Preemptor.java#L276 > > [3] > > > https://github.com/apache/incubator-aurora/blob/091bd6decbb9d75962a55a4e5cff82ce58a2a064/src/main/java/org/apache/aurora/scheduler/async/Preemptor.java#L280 > > > > where the scheduler does unnecessary work by collecting preemptable > tasks > > on a host in the hopes that the SchedulingFilter is satisfied and no > Vetos > > are returned. > > > > -=Bill > > > > On Sun, Sep 14, 2014 at 12:40 PM, Stephan Erb < > step...@dev.static-void.de> > > wrote: > > > > > Hi Bill, > > > > > > you are right that the SchedulingFilter deals with constraints. > However, > > > the preemptor seems to be reacting poorly when a job cannot be > scheduled > > > due to such a constaint. Let me explain. > > > > > > The preemptor cannot distinguish whether he sees a veto because of a > > > mismatched constraint or due to a lack of free resources on a slave. He > > > assumes the latter and therefore begins preempting tasks until the > whole > > > slave has been drained. He returns happily, even though the task can > > > still not be scheduled. > > > > > > At least that is the way I do read the code... > > > > > > Best, > > > Stephan > > > > > > On So, 2014-09-14 at 08:49 -0700, Bill Farner wrote: > > > > Thanks for raising the concern! > > > > > > > > > Unfortunately, it seems the preemptor cannot handle constraints > > > > > > > > Sounds like you are saying the Preemptor is only matching resources > and > > > not > > > > constraints? > > > > > > > > I'm on a phone, so I may have missed something, but my read is that > the > > > > preemptor lets SchedulingFilter.filter deal with satisfying > constraints. > > > > > > > > > > > On Sunday, September 14, 2014, Stephan Erb < > step...@dev.static-void.de> > > > > wrote: > > > > > > > > > Hi together, > > > > > > > > > > I was scrolling through Preemptor.java and noticed that tasks > might be > > > > > preempted unnecessarily. > > > > > > > > > > If I am not mistaken, the code works as follows: > > > > > > > > > > > > > > > for slave in slaves: > > > > > while SchedulingFilter.filter is still yielding vetos: > > > > > add preemptable task of slave to list toPreemptTasks > > > > > > > > > > if toPreemptTasks has some tasks: > > > > > preempt > > > > > return > > > > > > > > > > > > > > > SchedulingFilter is checking for available resources and > constraints. > > > > > Unfortunately, it seems the preemptor cannot handle constraints: > It may > > > > > drain an entire slave trying to make room for a new task. However, > if > > > > > this task has a constraint which prevents it from being scheduled > on > > > > > this slave at all, then the other tasks are preempted > unnecessarily. > > > > > > > > > > Is my analysis correct here? > > > > > > > > > > Best Regards, > > > > > Stephan > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
