reload: wait for pending tasks to submit and pause the workflow #5592

oliver-sanders · 2023-06-19T12:25:08Z

Closes Making reload safer #5107
Reload now waits for pending tasks to submit before attempting to reload the config itself.
Reload now also puts the workflow into the paused state during the reload process. This doesn't actually achieve anything as the reload command is blocking in the main loop, but it does help to communicate that the workflow will not de-queue or submit and new tasks during this process.
The workflow status message is now updated to reflect the reload progress.

This should also close #5579 by removing any potential for interaction between reload and preparing tasks. But note, I've not actually reproduced #5579.

Needs extensive functional testing.

The second commit straightens out the reload logic which is currently split between four routines:

Scheduler.command_reload_workflow
Scheduler.main_loop
Pool.set_do_reload
Pool.reload_taskdefs

With command_reload_workflow calling set_do_reload somewhere in the middle of the routine, then handing back over to the main loop which does its part before calling reload_taskdefs. I think this is just Cylc 7 hangover, removing the main-loop interaction from reload should reduce the chance of unexpected state changes occurring during the process.

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 if present).
Tests are included (or explain why tests are not needed).
CHANGES.md entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

cylc/flow/task_pool.py

oliver-sanders · 2023-06-19T17:12:28Z

cylc/flow/scheduler.py

+            self.process_workflow_db_queue()  # see #5593
+
+        # flush out preparing tasks before attempting reload
+        self.reload_pending = 'waiting for pending tasks to submit'


This gets used in the workflow status so progress will be indicated in the GUI.

oliver-sanders · 2023-06-19T17:17:45Z

tests/functional/logging/01-basic.t

+        script = """
+            cylc reload "${CYLC_WORKFLOW_ID}"
+            # wait for the command to complete
+            cylc__job__poll_grep_workflow_log 'Reload completed'


Orthogonal change: This test was timing dependent because it didn't wait for reload to complete.

tests/functional/logging/01-basic.t

oliver-sanders · 2023-06-19T17:19:01Z

tests/integration/test_scheduler.py

-        )
-    ]
-)
-async def test_illegal_config_load(


Covered by the new test.

oliver-sanders · 2023-06-20T16:58:18Z

To test, you can simulate a slow remote-init and a slow config load by jamming in sleeps like so:

diff --git a/cylc/flow/scheduler.py b/cylc/flow/scheduler.py
index d3cab5225..28bd79ac8 100644
--- a/cylc/flow/scheduler.py
+++ b/cylc/flow/scheduler.py
@@ -1096,6 +1096,7 @@ class Scheduler:
         LOG.info("Reloading the workflow definition.")
         try:
             config = self.load_flow_file(is_reload=True)
+            sleep(20)
         except (ParsecError, CylcConfigError) as exc:
             if cylc.flow.flags.verbosity > 1:
                 # log full traceback in debug mode
diff --git a/cylc/flow/scripts/remote_init.py b/cylc/flow/scripts/remote_init.py
index 7706cecfa..9c52ae8af 100755
--- a/cylc/flow/scripts/remote_init.py
+++ b/cylc/flow/scripts/remote_init.py
@@ -60,7 +60,8 @@ def get_option_parser() -> COP:
 
 @cli_function(get_option_parser)
 def main(parser, options, install_target, rund, *dirs_to_be_symlinked):
-
+    from time import sleep
+    sleep(20)
     remote_init(
         install_target,
         rund,

In the GUI you should see the following when you reload:

Workflow will enter the paused state.
Status message (in toolbar) will change to reloading: waiting for pending tasks to submit.
Status message will change to reloading: loading the workflow configuration.
Workflow will unpause and continue.

oliver-sanders · 2023-06-20T17:28:13Z

Got a couple of functional tests to fix, but otherwise the functionality should be good.

One question, in order to keep the scheduler responsive whilst we're waiting for pending tasks to submit, I added process_command_queue into the mini event-loop in command_reload_workflow. This means any arbitrary command could be actioned during this period.

This is probably fine, the point of this loop is just to wait for the workflow to enter a safe state for reload to be performed. It's probably even possible to trigger tasks at this stage if you want them to be submitted with the pre-reload config! But there's the potential for unplanned interactions so we may want to consider whitelisting this somehow.

wxtim

Code read

Functional Review

Try this with a variety of workflows
Check that this works the same way with cylc vr and cylc reinstall
Check what reloading with these changes looks like in the GUI.
Try reloading a change which breaks the workflow.
Try to get tasks in horrible states to break the logic at line 1418 of scheduler.py

cylc/flow/scheduler.py

oliver-sanders · 2023-06-22T16:32:50Z

The two test failures have flagged a legitimate issue with reload causing duplicate stall events which I'll sort out soon, will require a line or two of change.

hjoliver

Some trivial, and possibly annoying, comments. Code LGTM. I'll do some more brutal functional testing on Monday, but I'm pretty confident it's all good 🎉

cylc/flow/scheduler.py

hjoliver · 2023-06-22T22:52:45Z

cylc/flow/scheduler.py

+                A user-facing string explaining why the workflow was paused if
+                helpful.


Suggested change

A user-facing string explaining why the workflow was paused if

helpful.

A user-facing string explaining why the workflow was paused.

Yeah, so I'm not going to do that!

Why not? Isn't the "if helpful" redundant? (We wouldn't put it there if it not helpful!)

hjoliver · 2023-06-22T22:53:27Z

cylc/flow/scheduler.py

+                Whether to log anything in the event the workflow is not
+                paused.


Suggested change

Whether to log anything in the event the workflow is not

paused.

Whether to log anything if the workflow is not paused.

cylc/flow/scheduler.py

cylc/flow/task_pool.py

tests/conftest.py

wxtim · 2023-06-23T09:51:28Z

Using a super-basic workflow I tried:

$ cylc vip workflow
$ sed 's@allow implicit tasks = True@allow implicit tasks = False@' -i ~/cylc-src/workflow/flow.cylc
$ cylc vr workflow
#  ... Fails horribly - it's now broken
$ cylc reinstall workflow
#  ... succeeds, does no validation checks
$ cylc reload workflow
Done
$ cylc log workflow 
.... CRITICAL Reload failed - WorkflowConfigError ...
# Contains the evidence that the reload did not in fact work.

Is that what you want? I think it's dangerous/misleading...And this behaviour seems to be matched in the GUI.

oliver-sanders · 2023-06-26T09:49:01Z

Yes:

cylc vr protects us from configuration issues.
If users decide to bypass cylc vr and a config error is present then reload will log the error and leave the workflow paused.
Cylc commands are async so don't return their completion status.

oliver-sanders · 2023-06-27T14:53:10Z

Fixed the test failures, the problem was that update_workflow_structure has a role to play in stall event detection, so I've come up with a cut-down version of the method which is enough to update the workflow status and status-message so that users can see what's going on, but doesn't try to update the task pool or mess with stall detection.

https://github.com/cylc/cylc-flow/compare/d5f331fba83b67acd5e12613e786051020859aa7..e7545d86ee27c0f6e51a226297299bbf5b1656d5

* Closes cylc#5107 * Reload now waits for pending tasks to submit before attempting to reload the config itself. * Reload now also puts the workflow into the paused state during the reload process. This doesn't actually achieve anything as the reload command is blocking in the main loop, but it does help to communicate that the workflow will not de-queue or submit and new tasks during this process. * The workflow status message is now updated to reflect the reload progress.

* The reload code was spread around four places: * Scheduler.command_reload_workflow * Scheduler.main_loop * Pool.set_do_reload * Pool.reload_taskdefs * This commit co-locates the Scheduler/Pool parts and turns them into a single synchronous operation (no main-loop moving parts) to simplify the pathway. * This removes the need for the `do_reload` pool flag.

cylc/flow/scheduler.py

CHANGES.md

cylc/flow/scheduler.py

Co-authored-by: Tim Pillinger <[email protected]>

wxtim

Happy

oliver-sanders added this to the cylc-8.2.0 milestone Jun 19, 2023

oliver-sanders self-assigned this Jun 19, 2023

oliver-sanders requested a review from hjoliver June 19, 2023 12:47

oliver-sanders mentioned this pull request Jun 19, 2023

workflow_params: potential bug when rapidly toggling parameters #5593

Closed

oliver-sanders removed the request for review from hjoliver June 19, 2023 16:47

oliver-sanders force-pushed the safer-reload branch 2 times, most recently from ebe5dd7 to acdb441 Compare June 19, 2023 17:07

oliver-sanders commented Jun 19, 2023

View reviewed changes

cylc/flow/task_pool.py Show resolved Hide resolved

oliver-sanders commented Jun 19, 2023

View reviewed changes

tests/functional/logging/01-basic.t Outdated Show resolved Hide resolved

oliver-sanders commented Jun 19, 2023

View reviewed changes

tests/integration/test_scheduler.py

)

]

)

async def test_illegal_config_load(

Copy link

Member Author

oliver-sanders Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered by the new test.

oliver-sanders marked this pull request as ready for review June 19, 2023 17:19

oliver-sanders requested review from hjoliver and wxtim June 19, 2023 17:19

oliver-sanders added the bug Something is wrong :( label Jun 20, 2023

oliver-sanders force-pushed the safer-reload branch from acdb441 to d5f331f Compare June 20, 2023 17:23

wxtim reviewed Jun 22, 2023

View reviewed changes

cylc/flow/scheduler.py Outdated Show resolved Hide resolved

hjoliver approved these changes Jun 23, 2023

View reviewed changes

oliver-sanders force-pushed the safer-reload branch from d5f331f to e7545d8 Compare June 27, 2023 14:49

oliver-sanders force-pushed the safer-reload branch from e7545d8 to bfebb85 Compare June 27, 2023 14:53

oliver-sanders added 2 commits June 27, 2023 15:59

oliver-sanders force-pushed the safer-reload branch from bfebb85 to 820d337 Compare June 27, 2023 14:59

oliver-sanders commented Jun 28, 2023

View reviewed changes

cylc/flow/scheduler.py Outdated Show resolved Hide resolved

Update cylc/flow/scheduler.py

9eb0d4d

oliver-sanders requested a review from wxtim June 29, 2023 15:48

Merge branch 'master' into safer-reload

a002938

wxtim reviewed Jul 5, 2023

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

wxtim reviewed Jul 5, 2023

View reviewed changes

cylc/flow/scheduler.py Show resolved Hide resolved

MetRonnie mentioned this pull request Jul 5, 2023

DB workflow_params table: store None instead of deleting/not storing params #5618

Merged

7 tasks

Update CHANGES.md

9262b1b

Co-authored-by: Tim Pillinger <[email protected]>

wxtim self-requested a review July 6, 2023 07:48

wxtim approved these changes Jul 6, 2023

View reviewed changes

wxtim merged commit 5108f25 into cylc:master Jul 6, 2023

oliver-sanders deleted the safer-reload branch July 7, 2023 08:16

oliver-sanders mentioned this pull request Jul 18, 2023

Timeout on restarting completed workflows #5231

Merged

8 tasks

hjoliver mentioned this pull request Dec 6, 2023

Double-reload if command in job script #5865

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reload: wait for pending tasks to submit and pause the workflow #5592

reload: wait for pending tasks to submit and pause the workflow #5592

oliver-sanders commented Jun 19, 2023 •

edited

Loading

oliver-sanders Jun 19, 2023

oliver-sanders Jun 19, 2023

oliver-sanders Jun 19, 2023

oliver-sanders commented Jun 20, 2023 •

edited

Loading

oliver-sanders commented Jun 20, 2023

wxtim left a comment •

edited

Loading

oliver-sanders commented Jun 22, 2023

hjoliver left a comment

hjoliver Jun 22, 2023

oliver-sanders Jun 27, 2023

hjoliver Jun 29, 2023 •

edited

Loading

hjoliver Jun 22, 2023

wxtim commented Jun 23, 2023 •

edited

Loading

oliver-sanders commented Jun 26, 2023

oliver-sanders commented Jun 27, 2023

wxtim left a comment

		A user-facing string explaining why the workflow was paused if
		helpful.

		Whether to log anything in the event the workflow is not
		paused.

	Whether to log anything in the event the workflow is not
	paused.
	Whether to log anything if the workflow is not paused.

reload: wait for pending tasks to submit and pause the workflow #5592

reload: wait for pending tasks to submit and pause the workflow #5592

Conversation

oliver-sanders commented Jun 19, 2023 • edited Loading

oliver-sanders Jun 19, 2023

Choose a reason for hiding this comment

oliver-sanders Jun 19, 2023

Choose a reason for hiding this comment

oliver-sanders Jun 19, 2023

Choose a reason for hiding this comment

oliver-sanders commented Jun 20, 2023 • edited Loading

oliver-sanders commented Jun 20, 2023

wxtim left a comment • edited Loading

Choose a reason for hiding this comment

Functional Review

oliver-sanders commented Jun 22, 2023

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver Jun 22, 2023

Choose a reason for hiding this comment

oliver-sanders Jun 27, 2023

Choose a reason for hiding this comment

hjoliver Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

hjoliver Jun 22, 2023

Choose a reason for hiding this comment

wxtim commented Jun 23, 2023 • edited Loading

oliver-sanders commented Jun 26, 2023

oliver-sanders commented Jun 27, 2023

wxtim left a comment

Choose a reason for hiding this comment

oliver-sanders commented Jun 19, 2023 •

edited

Loading

oliver-sanders commented Jun 20, 2023 •

edited

Loading

wxtim left a comment •

edited

Loading

hjoliver Jun 29, 2023 •

edited

Loading

wxtim commented Jun 23, 2023 •

edited

Loading