Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Datastore fix for removed taskdef. #5067

Merged
merged 5 commits into from
Aug 18, 2022
Merged

Datastore fix for removed taskdef. #5067

merged 5 commits into from
Aug 18, 2022

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Aug 14, 2022 •
edited
Loading

Close #5057

Fix datastore bug on restart after removing a task from the graph.

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
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PRs raised to both master and the relevant maintenance branch.

@hjoliver hjoliver added the bug Something is wrong :( label Aug 14, 2022
@hjoliver hjoliver self-assigned this Aug 14, 2022
@hjoliver hjoliver added this to the cylc-8.0.1 milestone Aug 14, 2022
@oliver-sanders oliver-sanders changed the base branch from master to 8.0.x August 15, 2022 08:30
@oliver-sanders
Copy link
Member

Bumped onto the 8.0.x branch, @hjoliver can you rebase this one.

@oliver-sanders oliver-sanders modified the milestones: cylc-8.0.1, 8.0.2 Aug 16, 2022
datamel
datamel reviewed Aug 16, 2022
View reviewed changes
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

I've checked this out and run the bug recreation steps from the issue. The traceback appearing on master is fixed, and looking at the gui, the removed task is not present, which is super.

I have a small query about the logs after restart, removed task b is appearing in the restart logs as failed. Is this by design? Is this confusing for users if their removed task appears in the logs (perhaps apart from maybe being warned that it was once there but now removed). I'm probably missing something!

image

dwsutherland
dwsutherland approved these changes Aug 17, 2022
View reviewed changes
Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

Reproduced, and fix confirmed working.

I suppose we have to filter out DB loads somewhere.

cylc/flow/data_store_mgr.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member Author

hjoliver commented Aug 17, 2022 •
edited
Loading

I have a small query about the logs after restart, removed task b is appearing in the restart logs as failed.

Good spotting. It's not exactly wrong - if the removed task was still running at shutdown, the scheduler should poll it and log its status and progress (but there would be no further instances of it). However, I'm not sure why it is being polled here, as it failed before shutdown. Investigating ... on second look at the log, both tasks are getting polled, so that's fine. We're only polling failed tasks in the task pool (i.e., incomplete tasks) not all failed tasks.

@hjoliver
Copy link
Member Author

@datamel - I've rebased to remove all the spurious commits that appeared after changing the PR base branch (not sure why it did that)... and added a new test.

I'll cherry-pick to master once this is approved.

@oliver-sanders
Copy link
Member

I'll cherry-pick to master once this is approved.

FYI: we've been merging 8.0.x into master (you can create a PR from the GH web app).

hjoliver and others added 2 commits August 18, 2022 10:06
@hjoliver @datamel
Update tests/functional/restart/58-removed-task.t [skip ci]
d133b2d 
Co-authored-by: Melanie Hall <37735232+datamel@users.noreply.github.com>
@hjoliver
Reformulate new func test.
da6a946
@hjoliver
Copy link
Member Author

@datamel I've reformulated the functional test to not rely on timing.

datamel
datamel approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

All working smoothly for me. Thanks!

@datamel datamel merged commit 7dfbf9d into cylc:8.0.x Aug 18, 2022
@datamel datamel mentioned this pull request Aug 21, 2022
Closed
@hjoliver hjoliver deleted the 5057 branch August 22, 2022 01:57
wxtim added a commit to wxtim/cylc that referenced this pull request Aug 24, 2022
@wxtim
Merge remote-tracking branch 'upstream/8.0.x' into bugfix.c8.reload_p…
5f9072d 
…olling_bug

* upstream/8.0.x:
  fix install w/ .cylcignore (cylc#5066)
  Datastore fix for removed taskdef. (cylc#5067)
  Update PULL_REQUEST_TEMPLATE.md
@hjoliver hjoliver mentioned this pull request Sep 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datamel datamel datamel approved these changes

@dwsutherland dwsutherland dwsutherland approved these changes

Assignees

@hjoliver hjoliver

Labels
bug Something is wrong :(
Projects
None yet
Milestone
cylc-8.0.2
Development

Successfully merging this pull request may close these issues.

Restart fails after removing a task from flow.cylc
4 participants
@hjoliver @oliver-sanders @dwsutherland @datamel