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

Correctly store stop_point, etc for restart #3184

Merged
merged 13 commits into from
Jul 5, 2019
Merged

Correctly store stop_point, etc for restart #3184

merged 13 commits into from
Jul 5, 2019

Conversation

matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented Jun 5, 2019 •
edited
Loading

Fix #2799. Fix #3019. (With consideration of #3161). Forward port of #3147.

Requirements 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).
  • Appropriate tests are included (unit and/or functional).
  • Includes an appropriate entry in the release change log CHANGES.md.
  • I have opened a corresponding documentation PR at Update auto restart restriction cylc-doc#21.

@matthewrmshin matthewrmshin added the bug Something is wrong :( label Jun 5, 2019
@matthewrmshin matthewrmshin added this to the cylc-8.0a1 milestone Jun 5, 2019
@matthewrmshin matthewrmshin self-assigned this Jun 5, 2019
@matthewrmshin matthewrmshin mentioned this pull request Jun 5, 2019
Merged
15 tasks
@cylc cylc deleted a comment Jun 5, 2019
@cylc cylc deleted a comment Jun 5, 2019
@cylc cylc deleted a comment Jun 5, 2019
@cylc cylc deleted a comment Jun 5, 2019
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jun 6, 2019
@matthewrmshin
Update CHANGES.md
8ae76a2 
For cylc#3184.
@matthewrmshin matthewrmshin marked this pull request as ready for review June 6, 2019 08:54
@cylc cylc deleted a comment Jun 6, 2019
@matthewrmshin matthewrmshin changed the title Correctly store stop_point for restart Correctly store stop_point, etc for restart Jun 6, 2019
@matthewrmshin matthewrmshin mentioned this pull request Jun 13, 2019
Merged
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jun 13, 2019
@matthewrmshin
Update CHANGES.md
57cd958 
For cylc#3184.
@matthewrmshin
Copy link
Contributor Author

Rebased. Doc change moved to cylc/cylc-doc#21.

@cylc cylc deleted a comment Jun 13, 2019
kinow
kinow reviewed Jun 13, 2019
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
kinow
kinow reviewed Jun 13, 2019
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
@kinow
Copy link
Member

kinow commented Jun 13, 2019

Didn't review it, only quickly looked at what was changing, and spotted one or two minor things 👍

@matthewrmshin
Copy link
Contributor Author

@kinow Thanks for spotting the issues. I have fixed them in the latest commit.

@cylc cylc deleted a comment Jun 13, 2019
kinow
kinow approved these changes Jun 21, 2019
View reviewed changes
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Two comments, but no blockers. +1 from me 😬

cylc/flow/option_parsers.py Show resolved Hide resolved
cylc/flow/scheduler_cli.py Show resolved Hide resolved
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jun 27, 2019
@matthewrmshin
Update CHANGES.md
2104bef 
For cylc#3184.
@matthewrmshin
Test restart with stop point
943720b 
Test setting stop point on suite start up and via cylc stop command.
Test restart with stop point.
Test stop with stop point consumed and restart.
Test reload with stop point after restart.
@matthewrmshin
Test restart with stop task, clock time, auto, etc
94d57f6 
Test restart with stop task and stop clock time, before and after the settings
are consumed.

Test restart with auto shutdown disabled via command line.
Test restart with auto shutdown enabled via command line (overriding suite.rc).
Test restart with auto shutdown enabled via command line (overriding DB).

Test ignore stop cycle point on restart.
Test ignore final cycle point on restart.
@matthewrmshin
cylc#3147 address Oliver's feedback 1
4099876
@matthewrmshin
Use constants for repeated suite params DB keys
c222dd2
@matthewrmshin
Update CHANGES.md
690652a 
For cylc#3184.
@matthewrmshin
Fix CHANGES.md
b41bc54
@matthewrmshin
Fix merge bug
a428fb9
@matthewrmshin
Review feedback 1 + test happiness
8bfda6e
@cylc cylc deleted a comment Jul 2, 2019
@matthewrmshin
Copy link
Contributor Author

Added tests to prove that both cylc run SUITE now and cylc run --icp=now SUITE work.

Restarting suites the other way (run under 8, restart under 7.8.x)

We'll definitely recommend users against downgrade on restart - like we do now.

@oliver-sanders
Copy link
Member

oliver-sanders commented Jul 2, 2019 •
edited
Loading

Added tests to prove that both cylc run SUITE now and cylc run --icp=now SUITE work.

It was restart which was causing the problem i.e:

$ cylc run <suite> now
$ sleep 10
$ cylc stop <suite>
$ cylc restart <suite>  # me-thinks that `now` gets re-evaluated on restart?

[edit] Attempting to re-produce, haven't managed yet, have just cleaned my environment
[edit] Issue on this branch and 7.8.x
[edit] Fixed by latest commit

@cylc cylc deleted a comment Jul 2, 2019
@cylc cylc deleted a comment Jul 2, 2019
@matthewrmshin
Fix restart on cylc run --icp=now SUITE
f1a6c00 
(And `--icp=previous(...)` and `--icp=next(...)`.)
@cylc cylc deleted a comment Jul 2, 2019
@oliver-sanders
Copy link
Member

I've run through most of the restart scenarios now, looks OK.

The run mode isn't preserved in a restart on this branch (though that is also the case in 7.8.x), I think we had a discussion about this once, can't remember what we decided.

@matthewrmshin
Travis CI happiness
6da4a28 
A bit more job time for `tests/execution-time-limit/04`.
A bit more event driven for `tests/hold-release/15`.
@cylc cylc deleted a comment Jul 3, 2019
@cylc cylc deleted a comment Jul 3, 2019
@matthewrmshin
Copy link
Contributor Author

matthewrmshin commented Jul 4, 2019 •
edited
Loading

I've now included run mode command line option in the suite_params table. However, if the option is changed in the configuration file at restart, it will not know.

hjoliver
hjoliver approved these changes Jul 5, 2019
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

I haven't really done a proper in-depth review of this, but approving it on the basis that I did look closely the 7.8.x version before approving that, and this is obviously very similar. @matthewrmshin you can hit merge if you're comfortable with that, else request review again and it'll have to wait till I get time (note I have only two days at work next week - school holidays).

@matthewrmshin
Copy link
Contributor Author

3 approvals!

@matthewrmshin matthewrmshin merged commit d774fe0 into cylc:master Jul 5, 2019
@matthewrmshin matthewrmshin deleted the stop-point branch July 5, 2019 14:53
@wxtim wxtim mentioned this pull request Aug 22, 2024
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinow kinow kinow approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

@hjoliver hjoliver hjoliver approved these changes

Assignees

@matthewrmshin matthewrmshin

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

Successfully merging this pull request may close these issues.

reload: final cycle point cannot be changed after restart Formalise Suite Parameters
5 participants
@matthewrmshin @kinow @dwsutherland @oliver-sanders @hjoliver