Disallow nested run directories #3852

MetRonnie · 2020-10-05T16:52:58Z

These changes close #3835

This PR prevents registering a suite in a subdirectory of another run directory, or registering a suite in a directory whose children contain a run directory. It does this by checking upwards for the existence of a flow config file or a .service dir in successive parent dirs up to the cylc run dir, as well as checking downwards in subdirectories to a depth of 4 (the default cylc scan depth).

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).

Appropriate change log entry included.

No documentation update required.

No dependency changes.

MetRonnie · 2020-10-05T16:55:05Z

cylc/flow/suite_files.py

+    if (os.path.isfile(os.path.join(path, SuiteFiles.FLOW_FILE)) or
+            os.path.isdir(os.path.join(path, SuiteFiles.Service.DIRNAME)) or
+            os.path.isfile(os.path.join(path, SuiteFiles.SUITE_RC))):
+        return True


Instead of checking for the existence of the .service dir, should we check that .sevice/source is an existing symlink?

Existing or valid?

Are you suggesting we disallow flows inside existing valid run directories, but we allow them inside existing invalid ones?

That seems untidy at best, to me (is there a use case?).

I was wondering whether testing for .service/source was a more appropriate gauge of whether the directory was valid, but I guess just .service is good enough

Actually checking for flow.cylc or suite.rc or .service/ as above might be overkill. Detecting .service/ is probably sufficient isn't it? I would say the presence of a service directory is really what defines a Cylc run directory, not the presence of a flow config file, which can appear in other places too.

I see... Though checking for flow.cylc will catch if you tried to register a suite in a sub dir of an unregistered suite. But come to think of it, I suppose that isn't desirable, actually? It's only the registered ones we care about here?

I've pushed a new commit removes the check for flow.cylc / suite.rc

MetRonnie · 2020-10-05T16:59:55Z

Still 2 unresolved points Oliver commented on the issue:

In-place installations (e.g. run a suite in a temp dir)

Should we permit these, e.g. in the work dir (for subsuites).

Might be a good time to remove the cylc register --run-dir=RUNDIR option.

Obsolete once we have symlink support built-in.

hjoliver · 2020-10-06T02:04:38Z

We can already run a suite in a temp dir using --run-dir which (as per your second point) creates a symlink under cylc-run so that everything behaves normally even though the run-dir is actually located somewhere else.

In-place installations (e.g. run a suite in a temp dir)

Presumably by in-place installations you mean run a flow in-place without installing it to the normal cylc-run location? The trouble is, that's not discoverable by FS scan and other tools. But maybe that's OK if we have real built-in support for sub-suites, so that you're actually not supposed to be able to discover them as independent entities as they are now.

Should we permit these, e.g. in the work dir (for subsuites).

I suppose cylc-run/foo/work/sub1,sub2,... would be a sensible place for in-place sub-suites. It wouldn't mess with cylc scan too much (only need to look in one defined location below foo; or don't make them independently discoverable).

Might be a good time to remove the cylc register --run-dir=RUNDIR option.
Obsolete once we have symlink support built-in.

I'd rather that got removed by the PR that makes it obsolete.

Including converting unittest.TestCase class to standard pytest functions in test_suite_files.py

oliver-sanders · 2020-10-07T10:02:12Z

@hjoliver I think this is the PR (#2817) which (re)instated what I called the "run in place" model, you've called it "local run mode".

I think the desire is that functionality should not be effected by any change here.

hjoliver · 2020-10-08T03:01:18Z

@hjoliver I think this is the PR (#2817) which (re)instated what I called the "run in place" model, you've called it "local run mode".

I called it "local run-dir mode", which makes a bit more sense than "local run mode".

That PR was actually aborted and superseded by #2935 ("support symlinked alternate run-directories") which is where the cylc reg --run-dir option appeared.

hjoliver · 2020-10-08T03:03:12Z

I think the desire is that functionality should not be effected by any change here.

I agree, in which case (in light of my previous comment) I'm a bit confused about your statement Might be a good time to remove the cylc register --run-dir=RUNDIR option. 🤔

hjoliver · 2020-10-08T03:23:16Z

By "run in place" model, do you mean "run a suite that has not been registered, by cylc run /path/to/flow.cylc"? We can't do that anymore; but the cylc register --run-dir option provides the important part of that old functionality via a symlink in the standard location (e.g. cylc reg --run-dir=$HOME/flows foo $HOME/flows/foo (the run-dir is now the src-dir).

oliver-sanders · 2020-10-08T08:07:56Z

cylc register --run-dir option provides the important part of that old functionality via a symlink in the standard location

This should become obsolete with the upcoming changes to symlink run dirs (#3688) and the cylc install command.

Can remove later.

run a suite that has not been registered, by cylc run /path/to/flow.cylc

This is what I was thinking of.

We used to have the ability to write and run a workflow in an arbitrary directory without symlinking anything into cylc-run. I think it "worked by accident" but proved to have some valid use cases for fire-and-forget suites, --no-detach and make-like use cases.

Ok, don't have to worry about that then, can come back to it later if there is demand.

hjoliver · 2020-10-08T21:41:53Z

This should become obsolete with the upcoming changes to symlink run dirs (#3688) and the cylc install command.

👍 I've added a note to that issue. #3688 (comment)

hjoliver · 2020-10-08T21:45:09Z

run a suite that has not been registered, by cylc run /path/to/flow.cylc

This is what I was thinking of.

We used to have the ability to write and run a workflow in an arbitrary directory without symlinking anything into cylc-run. I think it "worked by accident" but proved to have some valid use cases for fire-and-forget suites, --no-detach and make-like use cases.

That's right. But when I went to restore that capability (and make it official) we ended up with the --run-dir solution for reasons detailed on the PR. You can still use that for the same make-like use cases, it just requires the one extra registration step first.

Ok, don't have to worry about that then, can come back to it later if there is demand.

Yep.

Because flow.cylc can be in a non-run dir

oliver-sanders · 2020-10-13T16:51:37Z

Had a go at testing this, for the expected use case cylc register blocked the registration for the nested suite due to the presence of the .service directory.

$ tree -a
.
└── a
    ├── b
    │   └── suite.rc
    └── suite.rc

2 directories, 2 files
$ cylc reg a a
2020-10-13T17:39:28+01:00 WARNING - The filename "suite.rc" is deprecated in
	favor of "flow.cylc". Symlink created.
REGISTERED a -> /Users/oliver/cylc-run/a
$ cylc reg a/b a/b
SuiteServiceFileError: Nested run directories not allowed - cannot register suite name "a/b" as a is already a valid run directory.

However, you can still register nested suites by doing it the other way around:

$ tree -a
.
└── a
    ├── b
    │   └── suite.rc
    └── suite.rc

2 directories, 2 files
$ cylc reg a/b a/b
2020-10-13T17:38:41+01:00 WARNING - The filename "suite.rc" is deprecated in
	favor of "flow.cylc". Symlink created.
REGISTERED a/b -> /Users/oliver/cylc-run/a/b
$ cylc reg a a
2020-10-13T17:38:44+01:00 WARNING - The filename "suite.rc" is deprecated in
	favor of "flow.cylc". Symlink created.
REGISTERED a -> /Users/oliver/cylc-run/a
$ tree -a
.
└── a
    ├── .service
    │   └── source -> ..
    ├── b
    │   ├── .service
    │   │   └── source -> ..
    │   ├── flow.cylc -> /Users/oliver/cylc-run/a/b/suite.rc
    │   └── suite.rc
    ├── flow.cylc -> /Users/oliver/cylc-run/a/suite.rc
    └── suite.rc

6 directories, 4 files

This isn't a facetious example as with hierarchically registered suites it's quite likely that someone might try to absentmindedly register a suite somewhere in the hierarchy where it's not meant to be.

Blocking registration in this case would involve searching down the file tree in search of .service directories which is a bit of a pain, however, we can depth-limit this search to make it a little less painful.

If only we had a nice convenient database of registrations 😈 (Cylc 6 joke).

hjoliver · 2020-10-13T20:29:20Z

If only we had a nice convenient database of registrations 😈 (Cylc 6 joke).

A classic 🤣

oliver-sanders · 2020-10-14T13:18:44Z

The default depth that cylc scan uses is defined in the scan method. Could move this to a constant somewhere and use it for both purposes for consistency.

cylc-flow/cylc/flow/network/scan.py

Line 106 in fe83115

async def scan(run_dir=None, scan_dir=None, max_depth=4):

oliver-sanders · 2020-10-15T12:44:07Z

Nice.

hjoliver

🎉 nice

MetRonnie added the bug? Not sure if this is a bug or not label Oct 5, 2020

MetRonnie added this to the cylc-8.0a3 milestone Oct 5, 2020

MetRonnie self-assigned this Oct 5, 2020

MetRonnie commented Oct 5, 2020

View reviewed changes

MetRonnie added 3 commits October 6, 2020 17:32

Disallow registering of a suite nested inside another suite dir

0bb6f66

Tidy

d77966b

Including converting unittest.TestCase class to standard pytest functions in test_suite_files.py

Update changelog

0a9b21e

MetRonnie force-pushed the disallow-nested-run-dir branch from f236fc5 to 0a9b21e Compare October 6, 2020 17:43

MetRonnie requested a review from oliver-sanders October 6, 2020 17:43

MetRonnie marked this pull request as ready for review October 6, 2020 17:43

Run dir is only valid if contains .service dir, not flow.cylc

6308a4d

Because flow.cylc can be in a non-run dir

MetRonnie marked this pull request as draft October 14, 2020 15:19

Check child dirs for valid run dir when registering suite

9be0e55

MetRonnie marked this pull request as ready for review October 15, 2020 10:29

MetRonnie requested a review from hjoliver October 15, 2020 10:32

oliver-sanders approved these changes Oct 15, 2020

View reviewed changes

hjoliver approved these changes Oct 19, 2020

View reviewed changes

hjoliver merged commit 9ab2115 into cylc:master Oct 19, 2020

MetRonnie deleted the disallow-nested-run-dir branch October 20, 2020 10:05

hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021

MetRonnie mentioned this pull request Oct 4, 2021

Remove obsolete checking for child nested run dirs during cylc install #4448

Merged

7 tasks

MetRonnie mentioned this pull request Jan 19, 2022

install: can install over the top of workflows #4594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow nested run directories #3852

Disallow nested run directories #3852

MetRonnie commented Oct 5, 2020 •

edited

Loading

MetRonnie Oct 5, 2020

hjoliver Oct 6, 2020 •

edited

Loading

MetRonnie Oct 6, 2020

hjoliver Oct 8, 2020

MetRonnie Oct 8, 2020 •

edited

Loading

MetRonnie Oct 9, 2020

MetRonnie commented Oct 5, 2020

hjoliver commented Oct 6, 2020 •

edited

Loading

oliver-sanders commented Oct 7, 2020

hjoliver commented Oct 8, 2020 •

edited

Loading

hjoliver commented Oct 8, 2020

hjoliver commented Oct 8, 2020 •

edited

Loading

oliver-sanders commented Oct 8, 2020 •

edited

Loading

hjoliver commented Oct 8, 2020

hjoliver commented Oct 8, 2020

oliver-sanders commented Oct 13, 2020

hjoliver commented Oct 13, 2020

oliver-sanders commented Oct 14, 2020

oliver-sanders commented Oct 15, 2020

hjoliver left a comment

Disallow nested run directories #3852

Disallow nested run directories #3852

Conversation

MetRonnie commented Oct 5, 2020 • edited Loading

MetRonnie Oct 5, 2020

Choose a reason for hiding this comment

hjoliver Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

MetRonnie Oct 6, 2020

Choose a reason for hiding this comment

hjoliver Oct 8, 2020

Choose a reason for hiding this comment

MetRonnie Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

MetRonnie Oct 9, 2020

Choose a reason for hiding this comment

MetRonnie commented Oct 5, 2020

hjoliver commented Oct 6, 2020 • edited Loading

oliver-sanders commented Oct 7, 2020

hjoliver commented Oct 8, 2020 • edited Loading

hjoliver commented Oct 8, 2020

hjoliver commented Oct 8, 2020 • edited Loading

oliver-sanders commented Oct 8, 2020 • edited Loading

hjoliver commented Oct 8, 2020

hjoliver commented Oct 8, 2020

oliver-sanders commented Oct 13, 2020

hjoliver commented Oct 13, 2020

oliver-sanders commented Oct 14, 2020

oliver-sanders commented Oct 15, 2020

hjoliver left a comment

Choose a reason for hiding this comment

MetRonnie commented Oct 5, 2020 •

edited

Loading

hjoliver Oct 6, 2020 •

edited

Loading

MetRonnie Oct 8, 2020 •

edited

Loading

hjoliver commented Oct 6, 2020 •

edited

Loading

hjoliver commented Oct 8, 2020 •

edited

Loading

hjoliver commented Oct 8, 2020 •

edited

Loading

oliver-sanders commented Oct 8, 2020 •

edited

Loading