data store: wipe workflow data on reload. #1479

oliver-sanders · 2023-09-15T14:10:01Z

The UI side of cylc/cylc-flow#5715

In combination with cylc/cylc-flow#5715, this closes #1480

When a workflow is reloaded or restarted (node restart infers reload), there may be objects left behind in the store which have been wiped out in the scheduler by the configuration change.
To handle this, we must wipe all objects stored on the workflow (e.g. TaskProxies, TaskDefs, Jobs, etc), and rebuild from scratch.
Note, we don't need to wipe the data on the Workflow object itself (e.g. port, stateTotals, etc), these things will be refreshed by this or subsequent deltas.

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.

src/graphql/queries.js

src/views/SimpleTree.vue

CHANGES.md

MetRonnie

Something is going wrong here when you open the graph view at the same time as the tree view. Both stop updating.

oliver-sanders · 2023-10-10T16:06:11Z

src/services/treeCallback.js

+      store.commit('workflows/REMOVE_CHILDREN', (deltas.updated.workflow.id))
+    }
+    if (deltas.added?.workflow?.reloaded) {
+      store.commit('workflows/REMOVE_CHILDREN', (deltas.added.workflow.id))


To test, try jamming in console.log statements or whatever here to ensure that the REMOVE_CHILDREN store method is being called.

oliver-sanders · 2023-10-10T16:07:18Z

src/views/README.md

@@ -25,3 +25,61 @@ E.G. the Tree *view* displays Cylc cycles/tasks/jobs in a collapsible hierarchy.
 It uses the Tree *component* which provides the generic tree logic e.g.
 indentation, expand/collapse, etc. This Tree *component* is also used by
 the Workflows *view*.
+
+
+## Subscriptions


Added some details here as the rules for subscription writing are getting a little complex.

See also #862 for context, and a possible way to simplify things for view writers.

oliver-sanders · 2023-10-10T16:14:46Z

Started this again from scratch:

I've moved the subscriptions out of the common module into the views to which they pertain.
The idea of the subscription merging interface is that each view only requests the fields it needs so code reuse is not desirable here.
I've been through the subscriptions and pulled out a number of fields which weren't required by the views which were requesting them.
I've also added a couple of things in which the views weren't requesting but should have been.
The tree and table now have different subscriptions containing only the things they need rather than the superset of the two.
I've added the reloaded field for each of the workflow query views (i.e. the views which get their data from the "workflow" subscription). The other subscriptions don't need this field:
- Application subscription is used for Workflows, Dashboard & WorkflowsTable, since these only request information about the workflow, not about tasks, etc, they don't need to be wiped on reload.
- Log subscriptions are reload agnostic.
- The analysis view uses queries not subscriptions so doesn't require this logic.

The tree and graph seem to be playing together nicely.

MetRonnie

Seems to work well. Spotted a couple of minor issues after a reload which may or may not be specific to this branch

Succeeded tasks without jobs (I have a feeling we came across this before)
The Graph View arrow placement went a bit errant for a moment and I got this error and couldn't pan or zoom anymore (though the graph kept updating afterwards)

src/views/Dashboard.vue

src/views/README.md

src/views/Table.vue

oliver-sanders · 2023-10-16T14:31:12Z

Succeeded tasks without jobs (I have a feeling we came across this before)

I haven't seen that one in testing, though I have seen cylc/cylc-flow#5215

If you're able to reproduce, could you check whether the job info was sent through in the deltas?

oliver-sanders · 2023-10-16T15:05:34Z

I can't reproduce either reported issue?

Though the graph issue you showed wouldn't be entirely surprising, as I don't imagine svgPanZoom would appreciate a document with no dimensions.

I have now seen the absence of graph arrows following a firstParent null error though which is useful.

oliver-sanders · 2023-10-16T15:59:05Z

Turns out that repeatably reloading the workflow with this branch is a really good way of triggering the firstParent bug.

Tried testing with #1513 merged in and everything is looking good after 25 reloads:

diff --git a/src/services/treeCallback.js b/src/services/treeCallback.js
index 570c3ac4..1ab9308c 100644
--- a/src/services/treeCallback.js
+++ b/src/services/treeCallback.js
@@ -38,9 +38,11 @@ class CylcTreeCallback extends DeltasCallback {
     // do this, we can end up with nodes in the store which aren't meant to be
     // there and won't get pruned.
     if (deltas.updated?.workflow?.reloaded) {
+      console.warn('REMOVE_CHILDREN')
       store.commit('workflows/REMOVE_CHILDREN', (deltas.updated.workflow.id))
     }
     if (deltas.added?.workflow?.reloaded) {
+      console.warn('REMOVE_CHILDREN')
       store.commit('workflows/REMOVE_CHILDREN', (deltas.added.workflow.id))
     }
   }

* Each view must register a subscription requesting the data the view requires. * Currently some of these queries are colocated and have not been kept in sync with the requirements of the views. * This commit moves the queries into the views they are used by.

* Remove fields from the query which are not used by the view. * Fill in missing field requests.

* When a workflow is reloaded or restarted (node restart infers reload), there may be objects left behind in the store which have been wiped out in the scheduler by the configuration change. * To handle this, we must wipe all objects stored on the workflow (e.g. TaskProxies, TaskDefs, Jobs, etc), and rebuild from scratch. * Note, we don't need to wipe the data on the Workflow object itself (e.g. port, stateTotals, etc), these things will be refreshed by this or subsequent deltas.

oliver-sanders · 2023-10-18T12:29:14Z

@MetRonnie

Applied suggestions
Removed delta fragments from all queries

MetRonnie

I've tested out again, and the only problem I can see is the jobs disappear on reload (includes running and succeeded jobs) (seems to be at least latestJob goes missing?). This does not happen on master / #1513 presumably because the data was not wiped.

Update: turns out this job wiping was only happening in simulation mode, which is forgiveable

hjoliver

LGTM, tested as working 👍

oliver-sanders added the bug Something isn't working label Sep 15, 2023

oliver-sanders added this to the 2.2.0 milestone Sep 15, 2023

oliver-sanders self-assigned this Sep 15, 2023

oliver-sanders requested a review from dwsutherland September 15, 2023 14:11

oliver-sanders mentioned this pull request Sep 18, 2023

Fix data-store reloaded flag cylc/cylc-flow#5715

Merged

7 tasks

oliver-sanders force-pushed the reloaded-deltas branch from 2a56276 to 86122aa Compare September 18, 2023 11:05

oliver-sanders commented Sep 21, 2023

View reviewed changes

src/graphql/queries.js Show resolved Hide resolved

MetRonnie reviewed Oct 3, 2023

View reviewed changes

src/views/SimpleTree.vue Outdated Show resolved Hide resolved

CHANGES.md Outdated Show resolved Hide resolved

MetRonnie requested changes Oct 4, 2023

View reviewed changes

oliver-sanders marked this pull request as draft October 10, 2023 10:01

oliver-sanders force-pushed the reloaded-deltas branch from f29b5ee to febc956 Compare October 10, 2023 16:05

oliver-sanders commented Oct 10, 2023

View reviewed changes

oliver-sanders requested a review from MetRonnie October 11, 2023 08:29

oliver-sanders marked this pull request as ready for review October 11, 2023 08:29

MetRonnie reviewed Oct 16, 2023

View reviewed changes

src/views/Dashboard.vue Outdated Show resolved Hide resolved

src/views/README.md Outdated Show resolved Hide resolved

src/views/Table.vue Outdated Show resolved Hide resolved

src/views/Table.vue Outdated Show resolved Hide resolved

src/views/Table.vue Outdated Show resolved Hide resolved

oliver-sanders added 3 commits October 18, 2023 13:25

views: rationalise view queries

42ce1c1

* Remove fields from the query which are not used by the view. * Fill in missing field requests.

oliver-sanders force-pushed the reloaded-deltas branch from febc956 to fceb26a Compare October 18, 2023 12:28

oliver-sanders requested a review from MetRonnie October 18, 2023 12:29

MetRonnie reviewed Oct 18, 2023

View reviewed changes

MetRonnie approved these changes Oct 19, 2023

View reviewed changes

hjoliver approved these changes Oct 19, 2023

View reviewed changes

hjoliver merged commit ccba831 into cylc:master Oct 19, 2023

oliver-sanders deleted the reloaded-deltas branch October 23, 2023 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data store: wipe workflow data on reload. #1479

data store: wipe workflow data on reload. #1479

oliver-sanders commented Sep 15, 2023 •

edited

Loading

MetRonnie left a comment

oliver-sanders Oct 10, 2023

oliver-sanders Oct 10, 2023

oliver-sanders commented Oct 10, 2023

MetRonnie left a comment •

edited

Loading

oliver-sanders commented Oct 16, 2023

oliver-sanders commented Oct 16, 2023 •

edited

Loading

oliver-sanders commented Oct 16, 2023

oliver-sanders commented Oct 18, 2023

MetRonnie left a comment •

edited

Loading

hjoliver left a comment

data store: wipe workflow data on reload. #1479

data store: wipe workflow data on reload. #1479

Conversation

oliver-sanders commented Sep 15, 2023 • edited Loading

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders Oct 10, 2023

Choose a reason for hiding this comment

oliver-sanders Oct 10, 2023

Choose a reason for hiding this comment

oliver-sanders commented Oct 10, 2023

MetRonnie left a comment • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Oct 16, 2023

oliver-sanders commented Oct 16, 2023 • edited Loading

oliver-sanders commented Oct 16, 2023

oliver-sanders commented Oct 18, 2023

MetRonnie left a comment • edited Loading

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

oliver-sanders commented Sep 15, 2023 •

edited

Loading

MetRonnie left a comment •

edited

Loading

oliver-sanders commented Oct 16, 2023 •

edited

Loading

MetRonnie left a comment •

edited

Loading