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

7.8.x Correctly store stop_point for restart #3147

Merged
merged 7 commits into from
Jun 6, 2019
Merged

7.8.x Correctly store stop_point for restart #3147

merged 7 commits into from
Jun 6, 2019

Conversation

matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented May 1, 2019 •
edited
Loading

Address mainly #2799 and #3019 (with consideration of #3161) for 7.8.x.

Changes:

  1. Store suite start up options for initial, final, start, stop cycle points, stop task, stop clock time, auto stop option in suite run SQLite file. (Rationale: on restart, these options are normally calculated in combination of the settings in the suite configuration when it loads the suite.rc, so it is safer to store the initial options rather than the values of the latest state.)
  2. Store hold point in suite run SQLite file.
  3. Simplify the constructor of the cylc.config.SuiteConfig class, by making use of the start up options (normally from CLI option parser) directly.
  4. Re-introduce --icp=CYCLE-POINT for cylc run so it has a more uniform interface with other commands, and with the final/stop cycle point options. Now cylc run SUITE POINT is equivalent to cylc run --icp=POINT SUITE, and cylc run -w SUITE POINT is equivalent to cylc run -w --start-point=POINT SUITE.
  5. New final-point=POINT and --stop-point=POINT option for cylc run and cylc restart. The --until=POINT option is now an alias for --final-point=POINT option.
  6. Auto shutdown can now be enabled on cylc run and cylc restart command line (to override suite configuration.) Previously, it is only possible to disable the setting.

To do:

  • Forward port to master - Correctly store stop_point, etc for restart #3184
  • Test restart/reload with:
    • stop point - before/after it is consumed
    • stop task - before/after it is consumed
    • stop clock time - before/after wall clock time
    • no auto stop
    • hold point (test already exists)
    • the --ignore-final-cycle-point and the --ignore-stop-cycle-point options.
  • Test CLI override to enable auto shutdown.
  • For no-detach suite on condemned host, shutdown any way. Let the caller deal with it.

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.

@matthewrmshin matthewrmshin added the bug Something is wrong :( label May 1, 2019
@matthewrmshin matthewrmshin added this to the cylc-7.8.3 milestone May 1, 2019
@matthewrmshin matthewrmshin self-assigned this May 1, 2019
@matthewrmshin matthewrmshin changed the title Correctly store stop_point for restart 7.8.x Correctly store stop_point for restart May 1, 2019
@matthewrmshin
Copy link
Contributor Author

Note: We seem to be mixing initial cycle point with start point and final cycle point with stop point in many parts of the logic.

As mentioned in #2799, we have an inconsistency between cylc run|restart --until=POINT SUITE (override final cycle point) and cylc stop SUITE POINT (set stop point).

And the water gets even muddier with the cylc restart --ignore-final-cycle-point option...

@hjoliver
Copy link
Member

hjoliver commented May 5, 2019

Some of that confusion might pre-date our ISO8601 support, when those distinctions didn't really exist.

@matthewrmshin matthewrmshin force-pushed the 7.8.x-stop-point branch 9 times, most recently from 03765f5 to 7dbda01 Compare May 17, 2019 07:59
@cylc cylc deleted a comment May 17, 2019
@cylc cylc deleted a comment May 17, 2019
@cylc cylc deleted a comment May 17, 2019
@cylc cylc deleted a comment May 17, 2019
@cylc cylc deleted a comment May 28, 2019
@matthewrmshin
Correctly store stop_point for restart
e39c013 
Store stop point in suite run time SQLite file.

Slightly improved interface for telling restart to ignore previous initial,
start, final and/or stop points.
@matthewrmshin
Store remaining suite parameters for restart
bf185c1 
Rationalise these settings between suite start up options and suite
configuration, and prevent repetitive propagation of attributes. Store the user
specified suite start up options as opposed to the calculated values - because
those values are calculated by combining the original user specified value with
the loaded configuration on restart.
@matthewrmshin
Test restart with stop point
633ce60 
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
Copy link
Contributor Author

Will update non-local tests. (I have deleted restart/36 and can get restart/37 to work, but restart/34,41,42 are still failing for some reason. I'll look further on Monday.)

@cylc cylc deleted a comment May 31, 2019
dwsutherland
dwsutherland reviewed Jun 1, 2019
View reviewed changes
lib/cylc/scheduler.py Outdated Show resolved Hide resolved
@cylc cylc deleted a comment Jun 1, 2019
@matthewrmshin
Copy link
Contributor Author

I now have a slightly better handle of why some of the non-local tests are failing. (Guess what?) It is to do with:

  • Recent SSH host key change related to OS upgrade.
  • Discrepancy between the values returned by hostname -f and hostname -A.

(I'll check that the tests fail in the same way under the vanilla 7.8.x branch or not on Monday.)

@oliver-sanders
Copy link
Member

Recent SSH host key change related to OS upgrade.
Discrepancy between the values returned by hostname -f and hostname -A.

This all sounds oddly familiar...

@matthewrmshin
Copy link
Contributor Author

On the failing tests:

  • 36 is removed.
  • 34, 37, 41 and 42 should all pass (provided that you use a good remote host for testing).

@cylc cylc deleted a comment Jun 3, 2019
@matthewrmshin
Copy link
Contributor Author

Added change log.

@oliver-sanders
Copy link
Member

test/restart now all pass for me.

@oliver-sanders
Copy link
Member

Have tested restart compatibility with Cylc 7.8.0, can confirm that CLI stop point is preserved with restart with this branch. +1

oliver-sanders
oliver-sanders approved these changes Jun 4, 2019
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

I'm happy with this, approved pending the doc change.

I'm not too fond of variables like holdcp and stopcp (perhaps stop_cp) but once you know what they are it's OK.

@matthewrmshin
Doc: auto stop-restart: remove limitation paragraph
244e176 
Now that stop point, etc are stored correctly, the only thing that limits auto
stop-restart is no detach mode.
@matthewrmshin
Copy link
Contributor Author

Doc updated.

@cylc cylc deleted a comment Jun 4, 2019
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jun 5, 2019
@matthewrmshin
cylc#3147 address Oliver's feedback 1
c95da46
@matthewrmshin matthewrmshin mentioned this pull request Jun 5, 2019
Merged
6 tasks
hjoliver
hjoliver approved these changes Jun 6, 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.

Nice work. I have not found any problems in a quick review+test ("time is of the essence", at the moment!); thanks to @oliver-sanders for the more thorough review.

@hjoliver hjoliver merged commit adbae58 into cylc:7.8.x Jun 6, 2019
@matthewrmshin matthewrmshin deleted the 7.8.x-stop-point branch June 6, 2019 08:27
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jun 6, 2019
@matthewrmshin
cylc#3147 address Oliver's feedback 1
889e8ea
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jun 13, 2019
@matthewrmshin
cylc#3147 address Oliver's feedback 1
69f897a
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jun 27, 2019
@matthewrmshin
cylc#3147 address Oliver's feedback 1
dcc405c
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jun 28, 2019
@matthewrmshin
cylc#3147 address Oliver's feedback 1
cb76f6d
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jun 28, 2019
@matthewrmshin
cylc#3147 address Oliver's feedback 1
34f9b6d
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Jul 2, 2019
@matthewrmshin
cylc#3147 address Oliver's feedback 1
4099876
@hjoliver hjoliver mentioned this pull request Nov 8, 2019
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwsutherland dwsutherland dwsutherland left review comments

@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-7.8.3
Development

Successfully merging this pull request may close these issues.
4 participants
@matthewrmshin @hjoliver @oliver-sanders @dwsutherland