Timeout on restarting completed workflows

[hjoliver]

Close #5078

• a new configurable restart timer (default PT2M) starts if the task pool is found to be empty at restart
• the timer stops if any task gets triggered before timeout
• on timeout the scheduler shuts down again
• does not need associated event handlers - if it times out, nothing has changed
• (also tried to disentangle some touched code a little, mainly scheduler.update_data_structure())

Check List

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Applied any dependency changes to both setup.cfg and conda-environment.yml.
• Tests are included (or explain why tests are not needed).
• CHANGES.md entry included if this is a change that can affect users
• Documentation issue - How to "continue" a completed workflow cylc-doc#525
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[oliver-sanders]

It might be worth prising apart the restart timer work from the update data store changes included on this branch so that we can get this in sooner. I think the data store changes will require more work, reviewing & testing.

[hjoliver]

It might be worth prising apart the restart timer work from the update data store changes included on this branch so that we can get this in sooner. I think the data store changes will require more work, reviewing & testing.

I must have made those changes for a reason, I don't think they were entirely incidental to the main change. Unfortunately, I've forgotten by now. I'll take a look ...

[hjoliver]

(Rebased)

[hjoliver]

It might be worth prising apart the restart timer work from the update data store changes included on this branch so that we can get this in sooner. I think the data store changes will require more work, reviewing & testing.

I must have made those changes for a reason, I don't think they were entirely incidental to the main change. Unfortunately, I've forgotten by now. I'll take a look ...

@oliver-sanders - the new timeout required tweaking the scheduler's datastore-update call slightly. The problem was, the datastore update method itself contained code to figure out if the datastore actually needs updating. It was easier to accommodate the new change (and it makes more sense too) to have the method just update the datastore, and decide whether or not to call it one level up in the stack. So it's a small change - I just moved that code out into the caller.

[oliver-sanders]

LGTM.

One minor issue is that after the "this workflow already ran to completion" message, we get a subsequent "not active tasks" message which buries this more important information.

[hjoliver]

One minor issue is that after the "this workflow already ran to completion" message, we get a subsequent "not active tasks" message which buries this more important information.

Fixed - by not attempting to poll tasks on restart if the task pool is empty.

[hjoliver]

(Temporarily reverting to DRAFT while I investigate something ...)

[oliver-sanders]

(Temporarily reverting to DRAFT while I investigate something ...)

Find anything?

[oliver-sanders]

Added an integration test here - hjoliver#33

[hjoliver]

(Temporarily reverting to DRAFT while I investigate something ...)

Sorry, I no longer recall what in hell I was talking about there. I have a vague feeling I was testing something else that I thought might affect this branch, but I guess I got distracted and didn't get back to this. I took another quick scan through this change though and I can't see anything wrong with it. Plus, it works.

Merged your side PR @oliver-sanders - plz take over this PR while I'm away...

[wxtim]

cancelled checks on dfde64f because changes to CHANGES.md only.

Previous test run 512e96b

[oliver-sanders]

This has the potential to interact with #5592 which was merged after this branch was forked. Likely test failures, should do a manual test jamming in sleeps to make the reload appear slow (see PR for details) to ensure that is still working.

[oliver-sanders]

Nope, went through fine, I've tested reload with sleeps jammed in and it appears to have worked well.

[oliver-sanders]

(plz squash merge)

[MetRonnie]

1 unresolved comment: #5231 (comment)

[oliver-sanders]

(Pressed the commit button)

[MetRonnie]

LGTM

[wxtim]

I have carried out the tests described, and also run the integration and functional/reload tests locally.