implement logging timestamps and record container ID in a file #649
Conversation
|
Can one of the admins verify this patch? |
|
Jenkins, okay to test |
|
Not sure why failed Jenkins... Console Output didn't give much information.. |
|
Jenkins, test this please |
cwltool/main.py
Outdated
|
|
||
| parser.add_argument("--cidfile-dir", type=Text, | ||
| help="Directory for storing cidfiles. Default at /tmp/", | ||
| default="/tmp/", |
There was a problem hiding this comment.
I think the current directory is a better default and is more portable.
cwltool/main.py
Outdated
| @@ -84,6 +84,20 @@ def arg_parser(): # type: () -> argparse.ArgumentParser | |||
| default=True, help="Do not delete Docker container used by jobs after they exit", | |||
| dest="rm_container") | |||
|
|
|||
| parser.add_argument("--record-container-id", action="store_true", default=False, | |||
| help="If enabled, a file with suffix \".cid\" will be created storing the container ID under CIDFILE_DIR", | |||
There was a problem hiding this comment.
Can you linewrap at 80 for easier reviews?
There was a problem hiding this comment.
"storing the container ID under the directory specified by --cidfile-dir"
There was a problem hiding this comment.
These new options would be best to put in their own https://docs.python.org/2.7/library/argparse.html#argument-groups. You can use that help to explain what a "cidfile" is.
cwltool/main.py
Outdated
| @@ -161,6 +175,7 @@ def arg_parser(): # type: () -> argparse.ArgumentParser | |||
| exgroup.add_argument("--verbose", action="store_true", help="Default logging") | |||
| exgroup.add_argument("--quiet", action="store_true", help="Only print warnings and errors.") | |||
| exgroup.add_argument("--debug", action="store_true", help="Print even more logging") | |||
| parser.add_argument("--logtstp", action="store_true", help="Print timestamps with logging") | |||
There was a problem hiding this comment.
I think --timestamps would be a better name and "Add timestamps to the errors, warnings, and notifications."
cwltool/job.py
Outdated
| if not os.path.isdir(cidfile_dir): | ||
| _logger.error("--cidfile-dir %s error:\n%s", cidfile_dir, | ||
| cidfile_dir + " is not a directory or directory doesn't exist, please check it first") | ||
| exit(-1) |
There was a problem hiding this comment.
Should be exit(2): https://docs.python.org/2/library/sys.html#sys.exit
cwltool/job.py
Outdated
| if not os.path.exists(cidfile_dir): | ||
| _logger.error("--cidfile-dir %s error:\n%s", cidfile_dir, | ||
| "directory doesn't exist, please create it first") | ||
| exit(-1) |
There was a problem hiding this comment.
Should be exit(2): https://docs.python.org/2/library/sys.html#sys.exit
cwltool/job.py
Outdated
| if cidfile_prefix != "": | ||
| cidfile_name = str(cidfile_prefix + "-" + cidfile_name) | ||
| cidfile_path = cidfile_dir + cidfile_name | ||
| runtime.append(u"--cidfile=%s" % cidfile_path) |
There was a problem hiding this comment.
Is there an equivalent for Singularity?
There was a problem hiding this comment.
Sorry but I don't think I understand this. Can you explain it a little bit.
I have finished other suggested changes
There was a problem hiding this comment.
If this option only works for the Docker runtime then we should say so. If we can add the equivalent option for Singularity then that would be nice.
There was a problem hiding this comment.
As far as I know, it comes with docker. I didn't know singularity before, but by reading its documents, I believe it's possible to add the equivalent option.
|
Jenkins, test this please |
… into feature/time-logging
Added options: