Bug 952587

Nick has offered some suggestions of dealing with the systems on the scheduler side, so we don't have to make expensive database calls. This may be a viable solution.

http://gerrit.beaker-project.org/2027

So, in reviewing the patch, we (mostly dcallagh) realised that the core problem here is the fact that "update_dirty_jobs()" runs in a different thread from the main scheduling loop. This means that after calling queue_processed_recipes() in the main loop, some of the queued recipes *may not* yet be marked as queued - they're be marked as processed+dirty instead.

We have a longer term plan to make this whole arrangement more sensible (see http://beaker-project.org/dev/proposals/event-driven-scheduler.html), but we want a simpler near term fix.

Dan's suggestion (and I now agree) is that we drop the separate "update_dirty_jobs_loop" and instead change main_recipes_loop to look something like this:

    @log_traceback(log)
    def main_recipes_loop(*args, **kwargs):
        while running:
            updated = update_dirty_jobs()
            abort_dead_recipes()
            updated = update_dirty_jobs() or updated
            if _virt_enabled():
                virt = provision_virt_recipes()
            else:
                virt = False
            updated = update_dirty_jobs() or updated
            queued = schedule_queued_recipes()
            updated = update_dirty_jobs() or updated
            scheduled = provision_scheduled_recipesets()
            updated = update_dirty_jobs() or updated
            if not virt and not queued and not scheduled and not updated:
                event.wait()
        log.debug("main recipes thread exiting")


It's a clumsy fix, but it will eliminate this issue and should also eliminate the database deadlock that can occur when both threads try to update the system_recipe_map table.

(In reply to Nick Coghlan from comment #3)
> So, in reviewing the patch, we (mostly dcallagh) realised that the core
> problem here is the fact that "update_dirty_jobs()" runs in a different
> thread from the main scheduling loop. This means that after calling
> queue_processed_recipes() in the main loop, some of the queued recipes *may
> not* yet be marked as queued - they're be marked as processed+dirty instead.
> 

This has 'always' (well, since 0.12) been the case, and was by design. The need for this BZ is exactly because of this.

> We have a longer term plan to make this whole arrangement more sensible (see
> http://beaker-project.org/dev/proposals/event-driven-scheduler.html), but we
> want a simpler near term fix.
> 
> Dan's suggestion (and I now agree) is that we drop the separate
> "update_dirty_jobs_loop" and instead change main_recipes_loop to look
> something like this:
> 
>     @log_traceback(log)
>     def main_recipes_loop(*args, **kwargs):
>         while running:
>             updated = update_dirty_jobs()
>             abort_dead_recipes()
>             updated = update_dirty_jobs() or updated
>             if _virt_enabled():
>                 virt = provision_virt_recipes()
>             else:
>                 virt = False
>             updated = update_dirty_jobs() or updated
>             queued = schedule_queued_recipes()
>             updated = update_dirty_jobs() or updated
>             scheduled = provision_scheduled_recipesets()
>             updated = update_dirty_jobs() or updated
>             if not virt and not queued and not scheduled and not updated:
>                 event.wait()
>         log.debug("main recipes thread exiting")
> 
> 
> It's a clumsy fix, but it will eliminate this issue and should also
> eliminate the database deadlock that can occur when both threads try to
> update the system_recipe_map table.

I think you meant to say "when both threads try to lock the same system record".

Did you mean to include process_new_recipes() in this loop? That will need to be in there to fix the deadlocking issue as well.

I guess the obvious problem with this is that serialising it means longer processing times. This is better than the deadlocks though.

http://gerrit.beaker-project.org/#/c/2045/

FWIW I don't think this is a proper verification. However, the way that the scheduler has been changed the original conditions cannot be reproduced. So I think that it's fine for this to stat 'VERIFIED'

Beaker 0.13.2 has been released. (http://beaker-project.org/docs/whats-new/release-0.13.html#beaker-0-13-2).