Implement download and upload operations in TES scheduler via the TES Task runner #232
Conversation
downloads and uploads
There was a problem hiding this comment.
Thanks for the draft PR! Most of my comments circle around two high level issues:
- Not fully understanding why we need to skip missing files and if that is required, make the implementation upstream in the stack wherever the node task definition is created instead of down the weeds of the runner library.
- Refactoring and functionality that seems out of scope for the issue.
| sb.AppendLinuxLine($"write_ts DownloadEnd && \\"); | ||
| sb.AppendLinuxLine($"chmod -R o+rwx $AZ_BATCH_TASK_WORKING_DIR/wd && \\"); | ||
| sb.AppendLinuxLine($"export TES_TASK_WD=$AZ_BATCH_TASK_WORKING_DIR/wd && \\"); | ||
| sb.AppendLinuxLine($"write_ts ExecutorStart && \\"); | ||
| sb.AppendLinuxLine($"docker run --rm {volumeMountsOption} --entrypoint= --workdir / {executor.Image} {executor.Command[0]} \"{string.Join(" && ", executor.Command.Skip(1))}\" && \\"); | ||
| sb.AppendLinuxLine($"write_ts ExecutorEnd && \\"); | ||
| sb.AppendLinuxLine($"write_ts UploadStart && \\"); | ||
| sb.AppendLinuxLine($"docker run --rm {volumeMountsOption} {MountBlobxferScriptAndMetrics(UploadFilesScriptFileName)} --entrypoint=/bin/sh {blobxferImageName} /{UploadFilesScriptFileName} && \\"); | ||
| sb.AppendLinuxLine($"./{NodeTaskRunnerFilename} upload --file {UploadFilesScriptFileName} && \\"); |
There was a problem hiding this comment.
A possible simpler implementation would be to create a single node task definition file with the inputs and outputs defined. The download/upload commands will only consider the relevant parts of the definition. Also, if you name the file TesTask.json you don't even need to set the --file option.
There was a problem hiding this comment.
Since we are calling this three times instead of two, and because we are calling this as a replacement for blobxfer/wget, not for its ultimate purpose, it makes sense to have the files replace the upload/download scripts for now, to continue to preserve as much of the current structure as possible. When the node task runner agent takes over the primary responsibility for everything this method currently does, that will be the moment to create one file and name it TesTask.json
|
[pre-PR reading note] |
Each of those have either already been handled or are #238 (#239). The only other item (that I don't believe is captured above) is a better way to implement directory uploading which we should do when we start adding support for replacing the bash script we create to run in the node with code in this new node task runner and that is captured in #245 |
| @@ -47,7 +48,9 @@ private static async Task ExecuteNodeContainerTaskAsync(FileInfo file, Uri docke | |||
| { | |||
| try | |||
| { | |||
| var executor = new Executor(file.FullName, options); | |||
| var nodeTask = DeserializeNodeTask(file.FullName); | |||
There was a problem hiding this comment.
Do we need to wrap this with 'Environment.ExpandEnvironmentVariables'? I am not sure if the CLI will pass an expanded path here?
There was a problem hiding this comment.
When the command line option creates the FileInfo object, it is parsed based on the current directory if it was not already and absolute path. That is automatically expanded by the FileInfo object when accessing the FullName property. Since we call the executor in the same directory as the file was dropped by Azure Batch, it expands correctly, There's no environment variable added to this argument in BatchScheduler since everything at this stage is all in that one directory.
fixes #144
I ran both the blobuploadertests and blobdownloadertests and they both passed.
The new process argument
skipMissingSourcesis temporary: it will be retired when we only call the node runner once per task, instead of 3 times.The three new properties of
NodeTaskI expect are also temporary: they should be folded into the metadata requirements as the content of the currentmetrics.txtfile is part of that larger effort.To minimize the effects of this PR, and for simplicity's sake, we are downloading the node task runner for each task. In future work, that will be moved to a permanent start-task operation.
To obtain the self-contained/single-file published build of the node task runner, we are accessing it via a project reference in
TesApi.Web. We should consider moving that out of the project and into the build pipeline.