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.

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

Prevent Cylc 7 from running suite.rc workflow that was previously run with Cylc 8 #4838

Merged
merged 5 commits into from
Jun 17, 2022

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Apr 27, 2022

These changes close #4835.

Fix bug where Cylc 7 would happily run a suite.rc workflow that was previously run with Cylc 8. (This bug came after we stopped doing the flow.cylc -> suite.rc symlink)

Do not run if:

  • flow.cylc detected in run dir
  • _cylc-install dir detected in run dir or 1 above
  • Cylc 8 workflow DB detected

This PR does not do anything about:

  • cylc register or the auto-register that happens on start. All this does is create a .service/source symlink, which is relatively harmless
  • Creation of auth files on start. This is also relatively harmless.

Testing

Pytest

Functional tests I have run (cylc test-battery):

  • tests/cylc8
  • tests/database
  • tests/registration
  • tests/restart
  • tests/startup

Manual tests to run (from #4320):

First create a suite.rc workflow that is compatible with both Cylc 7 and 8

Run in Cylc 8:

# cylc8 conda env
$ cylc play myflow -n --pause

Then open a new terminal for Cylc 7:

  1. cylc hold myflow
  2. cylc scan --debug
  3. cylc run myflow
  4. cylc restart myflow
  5. cylc stop myflow
  6. cylc get-suite-contact myflow
  7. cylc get-suite-version

Now stop the workflow:

# cylc8 conda env
$ cylc stop myflow

Then run those same Cylc 7 commands again.

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).
  • Applied any dependency changes to both setup.cfg and conda-environment.yml.
  • Appropriate tests are included
  • Appropriate change log entry included.
  • No docs update needed

@MetRonnie MetRonnie added the bug Something is wrong :( label Apr 27, 2022
@MetRonnie MetRonnie added this to the cylc-7.8.x milestone Apr 27, 2022
@MetRonnie MetRonnie self-assigned this Apr 27, 2022
@MetRonnie MetRonnie linked an issue Apr 28, 2022 that may be closed by this pull request
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • read.
  • Manually tested.

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.

LGTM.

One thing I noticed is that Cylc 7 let me run a workflow which had been "installed" by Cylc 8.

Perhaps this should also be blocked, could/should we add a check for the existence of _cylc-install, ../_cylc-install?

@MetRonnie
Copy link
Member Author

MetRonnie commented Jun 14, 2022

What if a user is trying to make use of the back-compat interoperability even though they have installed from a newfangled source dir?

@oliver-sanders
Copy link
Member

The back compat is with the workflow definition not with the install process.

The cylc register and rose suite-run commands both worked in very different ways to cylc install. Running a workflow that was installed by Cylc 8 under Cylc 7 could be dangerous, especially for Rose workflows as the Rose config will be completely ignored by Rose 2019 (it was only loaded during installation) and remote tasks will fail or pick up an older installation (Rose performed remote installation during workflow installation).

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.

Looks like this is good to go now 🌮

@hjoliver hjoliver merged commit 882f184 into cylc:7.8.x Jun 17, 2022
@MetRonnie MetRonnie deleted the c7-c8-conflict branch June 17, 2022 08:33
@hjoliver hjoliver modified the milestones: cylc-7.8.x, 7.8.12 Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cylc 7: check cylc 8 safety check still works
4 participants