-
Notifications
You must be signed in to change notification settings - Fork 25
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
[151] Runner executor #404
Conversation
9f1c96c
to
feef1e4
Compare
…4gh-tes into ja-nodetask-builder
6ffdbcf
to
760348a
Compare
…4gh-tes into ja-nodetask-builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nitpicks and cleanup. Please open issues for any you don't do now.
@@ -42,7 +42,7 @@ public class RuntimeOptions | |||
{ | |||
public TerraRuntimeOptions? Terra { get; set; } | |||
|
|||
public DockerCleanUpOptions DockerCleanUp { get; set; } | |||
public string? NodeManagedIdentityResourceId { get; set; } | |||
} | |||
|
|||
public class DockerCleanUpOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is no longer used, please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have been line 48
{ | ||
return retryPolicy.Execute(() => GetTokenCredentialImpl(runtimeOptions)); | ||
} | ||
private TokenCredential GetTokenCredentialImpl(RuntimeOptions runtimeOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a blank line here
@@ -169,8 +154,7 @@ public partial class BatchScheduler : IBatchScheduler | |||
_batchPoolFactory = poolFactory; | |||
batchPrefix = batchSchedulingOptions.Value.Prefix; | |||
logger.LogInformation("BatchPrefix: {BatchPrefix}", batchPrefix); | |||
taskRunScriptContent = File.ReadAllLines(Path.Combine(AppContext.BaseDirectory, "scripts/task-run.sh")); | |||
taskCleanupScriptContent = File.ReadAllLines(Path.Combine(AppContext.BaseDirectory, "scripts/clean-executor.sh")); | |||
File.ReadAllLines(Path.Combine(AppContext.BaseDirectory, "scripts/task-run.sh")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this isn't storing the contents of the file anywhere, this line can go away. If that is the case, please also delete the two files in the scripts
directory.
return await UploadContentAndCreateTesInputAsync(tesTask, input.Path, input.Content, cancellationToken); | ||
} | ||
|
||
private static string EscapeBashArgument(string arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you dumping the commands from the TesTask into a script in a single string or are you passing them to the Docker API? If the latter, we should NOT be shell escaping them. The Docker API does not turn them into a single string command-line, it passes them to the process's main() function directly.
Closes: #151
In this PR:
TaskExecutionScriptingManager
Manages the creation and uploading of the Batch script and all its dependencies, this is the entry-point class the BatchScheduler uses.BatchNodeScriptBuilder
. Fluid builder of the script to be executed by Batch. Used byTaskExecutionScriptingManager
.TaskToNodeTaskConverter
Handles the creation of a NodeTask from a TesTask. Used byTaskExecutionScriptingManager
. Creation of the NodeTask's inputs and outputs has the following key functionality:NodeTaskBuilder
Fluid builder of NodeTask instances. Contains the logic that sets the URL transformation strategy for inputs/outputs using the configuration settings. Used byTaskToNodeTaskConverter
.id
andworkflowId
, uniquely identifies the task and its associated workflow id.SasTransformationStrategy
property inFileOutput
toTransformationStrategy
RuntimeOptions.NodeManagedIdentityResourceId
(workflow ID or global ID as fallback) to the node task and changed the runner to use itManagedIdentityCredential(new ResourceIdentifier(runtimeOptions.NodeManagedIdentityResourceId));
If NodeManagedIdentiyResourceId is not set, the runner will use
DefaultAzureCredentials()
as a fallback, which works when identity is assigned and available.The TES server now uploads the TES task to storage account.