Skip to content

Commit 855a0e7

Browse files
committed
code review changes
1 parent f7c40bc commit 855a0e7

8 files changed

Lines changed: 155 additions & 37 deletions

File tree

src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ public async Task Start(
8484
var pollingPeriod = TimeSpan.FromSeconds(1);
8585

8686
var timeToRetry = (int)containerTimeout.TotalSeconds;
87+
var completed = false;
8788
while (timeToRetry-- > 0)
8889
{
8990
try
@@ -92,22 +93,43 @@ public async Task Start(
9293

9394
if (IsContainerCompleted(response.State))
9495
{
95-
await UploadOutputArtifacts(intermediateVolumeMount, outputVolumeMounts, cancellationToken).ConfigureAwait(false);
96-
await SendCallbackMessage(taskDispatchEvent, containerId).ConfigureAwait(false);
97-
return;
96+
completed = true;
97+
break;
9898
}
9999
}
100100
catch (Exception ex)
101101
{
102102
_logger.ErrorMonitoringContainerStatus(containerId, ex);
103103
}
104-
finally
104+
105+
await Task.Delay(pollingPeriod, cancellationToken).ConfigureAwait(false);
106+
}
107+
108+
if (completed)
109+
{
110+
try
105111
{
106-
await Task.Delay(pollingPeriod, cancellationToken).ConfigureAwait(false);
112+
await UploadOutputArtifacts(intermediateVolumeMount, outputVolumeMounts, cancellationToken).ConfigureAwait(false);
113+
}
114+
catch (Exception ex)
115+
{
116+
_logger.ErrorMonitoringContainerStatus(containerId, ex);
117+
throw;
107118
}
108-
}
109119

110-
_logger.TimedOutMonitoringContainerStatus(containerId);
120+
try
121+
{
122+
await SendCallbackMessage(taskDispatchEvent, containerId).ConfigureAwait(false);
123+
}
124+
catch (Exception ex)
125+
{
126+
_logger.ErrorMonitoringContainerStatus(containerId, ex);
127+
}
128+
}
129+
else
130+
{
131+
_logger.TimedOutMonitoringContainerStatus(containerId);
132+
}
111133
}
112134

113135
internal static bool IsContainerCompleted(ContainerState state)
@@ -161,8 +183,13 @@ private async Task UploadOutputArtifacts(IStorageService storageService, IConten
161183
{
162184
try
163185
{
164-
var objectName = file.Replace(artifactsPath, string.Empty).TrimStart('/');
165-
objectName = _fileSystem.Path.Combine(destination.RelativeRootPath, objectName);
186+
var relativePart = file.StartsWith(artifactsPath, StringComparison.Ordinal)
187+
? file.Substring(artifactsPath.Length)
188+
: file;
189+
relativePart = relativePart.TrimStart('/');
190+
var objectName = string.IsNullOrEmpty(destination.RelativeRootPath)
191+
? relativePart
192+
: destination.RelativeRootPath.TrimEnd('/') + "/" + relativePart;
166193
_logger.UploadingFile(file, destination.Bucket, objectName);
167194
if (!contentTypeProvider.TryGetContentType(file, out var contentType))
168195
{
@@ -175,6 +202,7 @@ private async Task UploadOutputArtifacts(IStorageService storageService, IConten
175202
catch (Exception ex)
176203
{
177204
_logger.ErrorUploadingFile(file, ex);
205+
throw;
178206
}
179207
}
180208
}

src/TaskManager/Plug-ins/Podman/ContainerVolumeMount.cs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,6 @@
1414
* limitations under the License.
1515
*/
1616

17-
using Ardalis.GuardClauses;
18-
1917
namespace Monai.Deploy.WorkflowManager.TaskManager.Podman
2018
{
2119
public class ContainerVolumeMount

src/TaskManager/Plug-ins/Podman/IPodmanContainerCreator.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ public async Task<PodmanCreateContainerResponse> CreateContainerAsync(Uri podman
6565
var json = JsonSerializer.Serialize(request, s_jsonOptions);
6666
using var content = new StringContent(json, System.Text.Encoding.UTF8, "application/json");
6767

68+
// The path targets the libpod API at v4.0.0 — the minimum supported Podman version for CDI device support.
6869
using var response = await httpClient.PostAsync("/v4.0.0/libpod/containers/create", content, cancellationToken).ConfigureAwait(false);
6970
var responseBody = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
7071

src/TaskManager/Plug-ins/Podman/Keys.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ namespace Monai.Deploy.WorkflowManager.TaskManager.Podman
1919
internal static class Keys
2020
{
2121
/// <summary>
22-
/// Key for the endpoint where the Docker server is running.
22+
/// Key for the endpoint where the Podman server is running.
2323
/// </summary>
2424
public static readonly string BaseUrl = "server_url";
2525

@@ -34,7 +34,7 @@ internal static class Keys
3434
public static readonly string EntryPoint = "entrypoint";
3535

3636
/// <summary>
37-
/// Key for specifying the user to the container. Same as -u argument for docker run.
37+
/// Key for specifying the user to the container. Same as -u argument for podman run.
3838
/// </summary>
3939
public static readonly string User = "user";
4040

@@ -59,7 +59,7 @@ internal static class Keys
5959
public static readonly string TemporaryStorageContainerPath = "temp_storage_container_path";
6060

6161
/// <summary>
62-
/// Prefix for envrionment variables.
62+
/// Prefix for environment variables.
6363
/// </summary>
6464
public static readonly string EnvironmentVariableKeyPrefix = "env_";
6565

@@ -79,7 +79,7 @@ internal static class Keys
7979
public static readonly string DefaultGpuDevice = "nvidia.com/gpu=all";
8080

8181
/// <summary>
82-
/// Required arguments to run the Docker workflow.
82+
/// Required arguments to run the Podman workflow.
8383
/// </summary>
8484
public static readonly IReadOnlyList<string> RequiredParameters =
8585
new List<string> {

src/TaskManager/Plug-ins/Podman/Logging/Log.cs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -38,16 +38,16 @@ public static partial class Log
3838
[LoggerMessage(EventId = 1005, Level = LogLevel.Information, Message = "Podman container terminated: container Id={containerId}.")]
3939
public static partial void TerminatedContainer(this ILogger logger, string containerId);
4040

41-
[LoggerMessage(EventId = 1006, Level = LogLevel.Information, Message = "Input volume mapping host=={hostPath}, container={containerPath}.")]
42-
public static partial void DockerInputMapped(this ILogger logger, string hostPath, string containerPath);
41+
[LoggerMessage(EventId = 1006, Level = LogLevel.Information, Message = "Input volume mapping host={hostPath}, container={containerPath}.")]
42+
public static partial void PodmanInputMapped(this ILogger logger, string hostPath, string containerPath);
4343

44-
[LoggerMessage(EventId = 1007, Level = LogLevel.Information, Message = "Output volume mapping host=={hostPath}, container={containerPath}.")]
45-
public static partial void DockerOutputMapped(this ILogger logger, string hostPath, string containerPath);
44+
[LoggerMessage(EventId = 1007, Level = LogLevel.Information, Message = "Output volume mapping host={hostPath}, container={containerPath}.")]
45+
public static partial void PodmanOutputMapped(this ILogger logger, string hostPath, string containerPath);
4646

47-
[LoggerMessage(EventId = 1008, Level = LogLevel.Information, Message = "Environment variabled added {key}={value}.")]
48-
public static partial void DockerEnvironmentVariableAdded(this ILogger logger, string key, string value);
47+
[LoggerMessage(EventId = 1008, Level = LogLevel.Information, Message = "Environment variable added {key}.")]
48+
public static partial void PodmanEnvironmentVariableAdded(this ILogger logger, string key);
4949

50-
[LoggerMessage(EventId = 1009, Level = LogLevel.Error, Message = "Error retreiving status from container {identity}.")]
50+
[LoggerMessage(EventId = 1009, Level = LogLevel.Error, Message = "Error retrieving status from container {identity}.")]
5151
public static partial void ErrorGettingStatusFromDocker(this ILogger logger, string identity, Exception ex);
5252

5353
[LoggerMessage(EventId = 1010, Level = LogLevel.Debug, Message = "Downloading artifact {source} to {target}.")]
@@ -62,8 +62,8 @@ public static partial class Log
6262
[LoggerMessage(EventId = 1013, Level = LogLevel.Warning, Message = "No output volumes configured for the task.")]
6363
public static partial void NoOutputVolumesConfigured(this ILogger logger);
6464

65-
[LoggerMessage(EventId = 10014, Level = LogLevel.Information, Message = "Intermediate volume mapping host=={hostPath}, container={containerPath}.")]
66-
public static partial void DockerIntermediateVolumeMapped(this ILogger logger, string hostPath, string containerPath);
65+
[LoggerMessage(EventId = 1014, Level = LogLevel.Information, Message = "Intermediate volume mapping host={hostPath}, container={containerPath}.")]
66+
public static partial void PodmanIntermediateVolumeMapped(this ILogger logger, string hostPath, string containerPath);
6767

6868
[LoggerMessage(EventId = 1015, Level = LogLevel.Error, Message = "Error generating volume mounts.")]
6969
public static partial void ErrorGeneratingVolumeMounts(this ILogger logger, Exception exception);
@@ -114,6 +114,6 @@ public static partial class Log
114114
public static partial void IntermediateVolumeMountAdded(this ILogger logger, string hostPath, string containerPath);
115115

116116
[LoggerMessage(EventId = 1031, Level = LogLevel.Error, Message = "Error setting directory {path} with permission {user}.")]
117-
public static partial void ErrorSettingDirectoryPermission(this ILogger logger, string path, string user);
117+
public static partial void ErrorSettingDirectoryPermission(this ILogger logger, Exception exception, string path, string user);
118118
}
119119
}

src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -191,10 +191,13 @@ public override async Task<ExecutionStatus> ExecuteTask(CancellationToken cancel
191191
try
192192
{
193193
var monitor = _scope.ServiceProvider.GetService<IContainerStatusMonitor>() ?? throw new ServiceNotFoundException(nameof(IContainerStatusMonitor));
194-
_ = Task.Run(async () =>
194+
var monitorTask = Task.Run(async () =>
195195
{
196-
await monitor.Start(Event, _containerTimeout, containerId, intermediateVolumeMount, outputVolumeMounts, cancellationToken);
196+
await monitor.Start(Event, _containerTimeout, containerId, intermediateVolumeMount, outputVolumeMounts, CancellationToken.None);
197197
});
198+
_ = monitorTask.ContinueWith(
199+
t => _logger.ErrorLaunchingContainerMonitor(containerId, t.Exception!.GetBaseException()),
200+
TaskContinuationOptions.OnlyOnFaulted);
198201
}
199202
catch (Exception exception)
200203
{
@@ -244,10 +247,32 @@ public override async Task<ExecutionStatus> GetStatus(string identity, TaskCallb
244247
var stats = GetExecutuionStats(response);
245248
if (ContainerStatusMonitor.IsContainerCompleted(response.State))
246249
{
250+
if (response.State.OOMKilled || response.State.Dead)
251+
{
252+
return new ExecutionStatus
253+
{
254+
Status = TaskExecutionStatus.Failed,
255+
FailureReason = FailureReason.ExternalServiceError,
256+
Errors = $"Exit code={response.State.ExitCode}",
257+
Stats = stats
258+
};
259+
}
260+
261+
if (response.State.ExitCode == 0)
262+
{
263+
return new ExecutionStatus
264+
{
265+
Status = TaskExecutionStatus.Succeeded,
266+
FailureReason = FailureReason.None,
267+
Stats = stats
268+
};
269+
}
270+
247271
return new ExecutionStatus
248272
{
249-
Status = TaskExecutionStatus.Succeeded,
250-
FailureReason = FailureReason.None,
273+
Status = TaskExecutionStatus.Failed,
274+
FailureReason = FailureReason.Unknown,
275+
Errors = $"Exit code={response.State.ExitCode}. Status={response.State.Status}.",
251276
Stats = stats
252277
};
253278
}
@@ -334,7 +359,7 @@ private PodmanCreateContainerRequest BuildContainerSpecification(IList<Container
334359
Source = input.HostPath,
335360
Options = new List<string> { "rbind", "ro" }
336361
});
337-
_logger.DockerInputMapped(input.HostPath, input.ContainerPath);
362+
_logger.PodmanInputMapped(input.HostPath, input.ContainerPath);
338363
}
339364

340365
foreach (var output in outputs)
@@ -346,7 +371,7 @@ private PodmanCreateContainerRequest BuildContainerSpecification(IList<Container
346371
Source = output.HostPath,
347372
Options = new List<string> { "rbind", "rw" }
348373
});
349-
_logger.DockerOutputMapped(output.HostPath, output.ContainerPath);
374+
_logger.PodmanOutputMapped(output.HostPath, output.ContainerPath);
350375
}
351376

352377
if (intermediateVolumeMount is not null)
@@ -358,7 +383,7 @@ private PodmanCreateContainerRequest BuildContainerSpecification(IList<Container
358383
Source = intermediateVolumeMount.HostPath,
359384
Options = new List<string> { "rbind", "rw" }
360385
});
361-
_logger.DockerIntermediateVolumeMapped(intermediateVolumeMount.HostPath, intermediateVolumeMount.ContainerPath);
386+
_logger.PodmanIntermediateVolumeMapped(intermediateVolumeMount.HostPath, intermediateVolumeMount.ContainerPath);
362387
}
363388

364389
var envvars = new Dictionary<string, string>();
@@ -369,7 +394,7 @@ private PodmanCreateContainerRequest BuildContainerSpecification(IList<Container
369394
{
370395
var envVarKey = key.Replace(Keys.EnvironmentVariableKeyPrefix, string.Empty);
371396
envvars[envVarKey] = Event.TaskPluginArguments[key];
372-
_logger.DockerEnvironmentVariableAdded(envVarKey, Event.TaskPluginArguments[key]);
397+
_logger.PodmanEnvironmentVariableAdded(envVarKey);
373398
}
374399
}
375400

@@ -449,9 +474,10 @@ private async Task<List<ContainerVolumeMount>> SetupInputs(CancellationToken can
449474
Directory.CreateDirectory(fileDirectory!);
450475

451476
_logger.DownloadingArtifactFromStorageService(obj.Filename, filePath);
452-
using var stream = await storageService.GetObjectAsync(input.Bucket, obj.FilePath, cancellationToken).ConfigureAwait(false) as MemoryStream;
477+
using var stream = await storageService.GetObjectAsync(input.Bucket, obj.FilePath, cancellationToken).ConfigureAwait(false)
478+
?? throw new InvalidOperationException($"Unable to download '{obj.FilePath}' from bucket '{input.Bucket}'.");
453479
using var fileStream = new FileStream(filePath, FileMode.CreateNew, FileAccess.Write);
454-
stream!.WriteTo(fileStream);
480+
await stream.CopyToAsync(fileStream, cancellationToken).ConfigureAwait(false);
455481
}
456482
}
457483

@@ -520,8 +546,9 @@ private void SetPermission(string path)
520546

521547
if (process.ExitCode != 0)
522548
{
523-
_logger.ErrorSettingDirectoryPermission(path, Event.TaskPluginArguments[Keys.User]);
524-
throw new SetPermissionException($"chown command exited with code {process.ExitCode}");
549+
var permissionException = new SetPermissionException($"chown command exited with code {process.ExitCode}");
550+
_logger.ErrorSettingDirectoryPermission(permissionException, path, Event.TaskPluginArguments[Keys.User]);
551+
throw permissionException;
525552
}
526553
}
527554
}

tests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,32 @@ public async Task Start_WhenCalledWithoutAnyArtifacts_ExpectToSendCallbackEvent(
124124
_messageBrokerPublisherService.Verify(p => p.Publish(It.IsAny<string>(), It.IsAny<Monai.Deploy.Messaging.Messages.Message>()), Times.Once());
125125
}
126126

127+
[Fact(DisplayName = "Start - when upload fails expect callback not to be published")]
128+
public async Task Start_WhenUploadFails_ExpectCallbackNotPublished()
129+
{
130+
var files = new List<string>() { "/taskmanagerpath/output/b.dcm" };
131+
CreateFiles(files);
132+
var outputVolumeMounts = new List<ContainerVolumeMount>() { new ContainerVolumeMount(new Storage { Bucket = "bucket", RelativeRootPath = "/svc" }, "/containerpath", "/hostpath", "/taskmanagerpath/output") };
133+
var monitor = new ContainerStatusMonitor(_serviceScopeFactory.Object, _logger.Object, _fileSystem, _options);
134+
var taskDispatchEvent = GenerateTaskDispatchEventWithValidArguments();
135+
136+
_storageService.Setup(p => p.PutObjectAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Stream>(), It.IsAny<long>(), It.IsAny<string>(), It.IsAny<Dictionary<string, string>>(), It.IsAny<CancellationToken>()))
137+
.ThrowsAsync(new Exception("upload error"));
138+
_podmanClient.Setup(p => p.Containers.InspectContainerAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
139+
.ReturnsAsync(new ContainerInspectResponse
140+
{
141+
State = new ContainerState
142+
{
143+
Status = Strings.DockerStatusExited,
144+
FinishedAt = DateTime.MinValue.ToString("s")
145+
}
146+
});
147+
148+
await Assert.ThrowsAsync<Exception>(() => monitor.Start(taskDispatchEvent, TimeSpan.FromSeconds(3), "container", null!, outputVolumeMounts, CancellationToken.None));
149+
150+
_messageBrokerPublisherService.Verify(p => p.Publish(It.IsAny<string>(), It.IsAny<Monai.Deploy.Messaging.Messages.Message>()), Times.Never());
151+
}
152+
127153
[Fact(DisplayName = "Start - when called expect to upload artifacts and send callback event")]
128154
public async Task Start_WhenCalled_ExpectToUploadArtifactsAndSendCallbackEvent()
129155
{

0 commit comments

Comments
 (0)