Cylc registration fix.

[hjoliver]

Close #2758
Close #2749

[UPDATED description 15 Oct]:

Abort suite registration if:

• suite.rcnot found in target directory (this allows us to distinguish correct arg order)
• name is an absolute path (can only be relative).
• name is already registered to another suite
  • but allow forced re-use of existing name and run directory with --redirect

Also support:

• auto-registration of suite in $PWD
  • cylc reg NAME (auto-reg)
  • cylc reg (auto reg and auto-name - with name of parent dir)
• on-the-fly auto-registration by "cylc run" (of suite in $PWD, with name of parent dir).
  • replacing (via exec) process cylc run with cylc run NAME for easy identification with ps etc.

[hjoliver]

Travis CI is maddening at the moment, but I see one genuine test fail due to relying on the old reg behaviour...

[oliver-sanders]

$ ls
suite.rc
$ cylc reg $PWD
ERROR: no suite.rc found in /var/tmp/tmp.tHegPaMTo9

[hjoliver]

@oliver-sanders - technically that's user error. cylc register NAME [PATH] - name is not optional, but path is. But fair enough, I should handle that error better. I suppose we can say a hierarchical suite name can look like a relative file path, but not an absolute path, for one check...

[hjoliver]

I'll abort with an error if there is only one argument and it is either an absolute path or a valid relative path from cwd that is not also an existing registered suite name. Does that sounds reasonable?

[oliver-sanders]

That sounds a bit complicated, could we just reject any NAME which is an absolute path?

From the help page I would have expected cylc reg $PWD to be equivalent to cylc reg $PWD .:

$ cylc reg $PWD
ERROR: no suite.rc found in /var/tmp/tmp.0Rbo39DoUR
$ cylc reg $PWD .
REGISTER /var/tmp/tmp.0Rbo39DoUR: /var/tmp/tmp.0Rbo39DoUR

The leading slash messes things up:

# register suite in ~/cylc/run/var/tmp/tmp.0Rbo39DoUR
cylc reg var/tmp/tmp.0Rbo39DoUR /var/tmp/tmp.0Rbo39DoUR

# register suite in /var/tmp/tmp.0Rbo39DoUR
cylc reg /var/tmp/tmp.0Rbo39DoUR /var/tmp/tmp.0Rbo39DoUR

Do we currently make use of this behaviour?

[hjoliver]

From the help page I would have expected cylc reg $PWD to be equivalent to cylc reg $PWD .

Que? The help page (on this branch) only says you can omit PATH (2nd arg), not NAME, so neither of those examples should be allowed. We definitely shouldn't be able to use a leading / in a NAME.

The complication is just that a relative path can't be distinguished from a name, which makes it difficult to know if the user got the args reversed, or if he/she omitted one, was it the right one...

[hjoliver]

(Sorry, just re-read above, I guess you intended $PWD as a NAME, in those examples?)

[oliver-sanders]

Indeed.

We have to assume that whatever the user provides as the first argument is a NAME, if that's not what they intended, user error, replace user. If it is an absolute path abort with error?

We definitely shouldn't be able to use a leading / in a NAME.

It's interesting that this seems to work fine. It could be a good way to run the test battery?

[hjoliver]

OK, let's make it that a NAME can be the same as the relative source path, because that actually seems like a sensible thing to do, but it can't be the same as the absolute source path, because the name gets used as a relative path under ~/cylc-run.

[hjoliver]

It might be nice to be able to cylc reg PATH as a shortcut for cylc reg $(basename PATH) PATH, or even cylc reg (no args) as a shortcut for the same, with $PWD as PATH. Or is this getting more than the legal amount of complicated... (how to distinguish cylc reg PATH from cylc reg NAME ...)

[oliver-sanders]

The cleanest way I can think to do that would be:

cylc reg [NAME] [PATH]

With the following being equivalent:

cylc reg
cylc reg $(basename $PWD)
cylc reg $(basename $PWD) $PWD

This way we don't have to do any path checking, just so long as NAME matches the criterion for a reg (no funny chars, isn't an absolute path) and PATH exists and contains a suite.rc file we are good to go?

[hjoliver]

Have not address the "no funny chars", due to this comment: #2281 (comment)

[hjoliver]

@oliver-sanders - implemented as you suggested (which turned out to be mildly fiendish due to subtly different command line semantics for cylc register vs on-the-fly registration by cylc run).

[hjoliver]

Updated issue description.

[oliver-sanders]

Sorry it's taken a while to come back to this. All looks good and tests as expected, I'm pretty happy with this new functionality.

My only comment is that with the new cylc run "run and register" system we are unable to see what suite is being run just be inspecting the command being run:

$ ps -fu user | grep cylc
user 1234     1  2 12:34 ?    00:00:00 python2 /path/to/cylc/cylc-run foo  # cylc run foo
user 1234     1  2 12:34 ?    00:00:00 python2 /path/to/cylc/cylc-run      # cylc run

This will mess up our suite monitoring unless we can come up with a better way of doing things. I guess we could exec the cylc run with the "full" arguments but is is worth it?

[hjoliver]

@oliver-sanders - that is unfortunate (what do you mean by "our suite monitoring" exactly?). Do we need to disable auto-registration for that reason?

[hjoliver]

I suppose your exec idea could work if I moved auto-registration out of scheduler.py and into the main cylc run command.

[matthewrmshin]

I think it should only regenerate the SSL certificate if the current host name differs. However the behaviour is also dependent on the version of the SSL library. Older version of the library does not have the functionality to extract the information from the existing certificate.

[matthewrmshin]

(So maybe it ends up generating a new certificate every run because it is not able to do the host match?)

[matthewrmshin]

(Sorry for making things complicated!)

[hjoliver]

(No worries, I think I know the way forward ... and will do it late tonight if I get home in time...).

[hjoliver]

Last fix separates run directory and auth files creation, and renews the ssl.cert on the suite host at suite startup. So this preserves current behaviour (with respect to ssl.cert) on master. Test should pass now.

[matthewrmshin]

All good in my environment.

[matthewrmshin]

@oliver-sanders please do a final sanity check.

[hjoliver]

@oliver-sanders - if you are able to do the requested sanity check fairly quickly, I have another PR ready to go that is based on top of this one (thanks in advance ... I hope).

[oliver-sanders]

Sorry!

$ cylc run elephant
Traceback (most recent call last):
  File "cylc/bin/cylc-run", line 25, in <module>
    main(is_restart=False)
  File "cylc/lib/cylc/scheduler_cli.py", line 116, in main
    scheduler = Scheduler(is_restart, options, args)
  File "cylc/lib/cylc/scheduler.py", line 126, in __init__
    self.suite)
  File "cylc/lib/cylc/suite_srv_files_mgr.py", line 327, in get_suite_source_dir
    raise SuiteServiceFileError("ERROR: Suite not found %s" % reg)
cylc.suite_srv_files_mgr.SuiteServiceFileError: ERROR: Suite not found elephant

[hjoliver]

Ah ... do you have a suite named 'elephant'? I guess your point is, for consistency, it shouldn't drop a traceback without --debug?

[oliver-sanders]

Looks good!