Validate buffer names (#182)

Author: johnstairs

cli/integrationtest/controlplane_test.go

@@ -688,6 +688,7 @@ maxReplicas: 1
 	require.Equal(t, "ubuntu", receivedSpec.Image)
 }
 func TestInvalidCodespecNames(t *testing.T) {
+	t.Parallel()
 	testCases := []struct {
 		name  string
 		valid bool
@@ -718,8 +719,42 @@ func TestInvalidCodespecNames(t *testing.T) {
 	}
 }
 
-func TestCodespecNameRequirements(t *testing.T) {
-	runTyger("codespec", "create", "Foo", "--image", BasicImage)
+func TestInvalidBufferNames(t *testing.T) {
+	t.Parallel()
+	testCases := []string{
+		"FOO",
+		"foo_bar",
+		"-foo",
+		"bar-",
+	}
+	for _, tC := range testCases {
+		t.Run(tC, func(t *testing.T) {
+			_, stdErr, err := runTyger("codespec", "create", "testinvalidbuffernames", "--image", BasicImage, "--input", tC)
+			require.NotNil(t, err)
+			require.Contains(t, stdErr, "Buffer names must consist")
+		})
+	}
+}
+
+func TestInvalidBufferNamesInInlineCodespec(t *testing.T) {
+	t.Parallel()
+	require := require.New(t)
+
+	runSpec := fmt.Sprintf(`
+job:
+  codespec:
+    image: %s
+    buffers:
+      inputs: ["INVALID_NAME"]
+timeoutSeconds: 600`, BasicImage)
+
+	tempDir := t.TempDir()
+	runSpecPath := filepath.Join(tempDir, "runspec.yaml")
+	require.NoError(os.WriteFile(runSpecPath, []byte(runSpec), 0644))
+
+	_, stdErr, err := runTyger("run", "exec", "--file", runSpecPath)
+	require.NotNil(err)
+	require.Contains(stdErr, "Buffer names must consist")
 }
 
 // Verify that a run using a codespec that requires a GPU

server/ControlPlane/Compute/Docker/DockerRunCreator.cs

@@ -107,6 +107,8 @@ public async Task<Run> CreateRun(Run run, string? idempotencyKey, CancellationTo
             throw new ArgumentException($"The codespec for the job is required to be a job codespec");
         }
 
+        Validator.ValidateObject(jobCodespec, new(jobCodespec));
+
         try
         {
             await _client.Images.InspectImageAsync(jobCodespec.Image, cancellationToken: cancellationToken);

server/ControlPlane/Compute/Kubernetes/KubernetesRunCreator.cs

@@ -109,9 +109,18 @@ private async Task ProcessPageOfNewRuns(IReadOnlyList<Run> runs, CancellationTok
             catch (OperationCanceledException) when (ct.IsCancellationRequested)
             {
             }
+            catch (HttpOperationException e) when (e.Response.StatusCode is HttpStatusCode.TooManyRequests or HttpStatusCode.InternalServerError or HttpStatusCode.ServiceUnavailable or HttpStatusCode.GatewayTimeout)
+            {
+                _logger.RetryableErrorCreatingRunResources(run.Id!.Value, e);
+            }
+            catch (IOException e)
+            {
+                _logger.RetryableErrorCreatingRunResources(run.Id!.Value, e);
+            }
             catch (Exception ex)
             {
                 _logger.ErrorCreatingRunResources(run.Id!.Value, ex);
+                await Repository.ForceUpdateRun(run with { Status = RunStatus.Failed, StatusReason = "Failed to create Kubernetes objects. See server logs." }, cancellationToken);
             }
         });
     }
@@ -127,6 +136,8 @@ private async Task<Run> CreateRunCore(Run run, string? idempotencyKey, Cancellat
             throw new ArgumentException($"The codespec for the job is required to be a job codespec");
         }
 
+        Validator.ValidateObject(jobCodespec, new(jobCodespec));
+
         run = run with
         {
             Cluster = targetCluster.Name,

server/ControlPlane/Compute/Kubernetes/KubernetesRunLogReader.cs

@@ -17,7 +17,6 @@ namespace Tyger.ControlPlane.Compute.Kubernetes;
 
 public class KubernetesRunLogReader : ILogSource
 {
-    private static readonly Pipeline s_emptyPipeline = new([]);
     private readonly k8s.Kubernetes _client;
     private readonly Repository _repository;
     private readonly ILogArchive _logArchive;
@@ -63,7 +62,7 @@ public KubernetesRunLogReader(
             return await FollowLogs(run, options, cancellationToken);
         }
 
-        return (await InnerGetLogs()) ?? s_emptyPipeline;
+        return (await InnerGetLogs()) ?? new([]);
     }
 
     private Task<Pipeline> FollowLogs(Run run, GetLogsOptions options, CancellationToken cancellationToken)

server/ControlPlane/Compute/Kubernetes/LoggerExtensions.cs

@@ -20,6 +20,9 @@ public static partial class LoggerExtensions
     [LoggerMessage(LogLevel.Error, "Error creating run {runId} resources.")]
     public static partial void ErrorCreatingRunResources(this ILogger logger, long runId, Exception exception);
 
+    [LoggerMessage(LogLevel.Warning, "Retryable error creating run {runId} resources.")]
+    public static partial void RetryableErrorCreatingRunResources(this ILogger logger, long runId, Exception exception);
+
     [LoggerMessage(LogLevel.Error, "Error while watching resources.")]
     public static partial void ErrorWatchingResources(this ILogger logger, Exception exception);
 

server/ControlPlane/Database/Repository.cs

@@ -723,9 +723,9 @@ RETURNING modified_at
 
             await tx.CommitAsync(cancellationToken);
 
-            if (updatedRun.Status is RunStatus.Succeeded or RunStatus.Failed)
+            if (updatedRun.Status is RunStatus.Succeeded or RunStatus.Failed && updatedRun.FinishedAt.HasValue)
             {
-                var timeToDetect = DateTimeOffset.UtcNow - updatedRun.FinishedAt!.Value;
+                var timeToDetect = DateTimeOffset.UtcNow - updatedRun.FinishedAt.Value;
                 _logger.TerminalStateRecorded(state.Id, timeToDetect);
             }
         }, cancellationToken);

server/ControlPlane/Model/Model.cs

@@ -243,9 +243,9 @@ public IEnumerable<ValidationResult> Validate(ValidationContext validationContex
                     yield return new ValidationResult(string.Format(CultureInfo.InvariantCulture, "All buffer names must be unique across inputs and outputs. Buffer names are case-insensitive. '{0}' is duplicated", group.Key));
                 }
 
-                if (group.Key.Contains('/'))
+                if (!BufferNameRegex().IsMatch(group.Key))
                 {
-                    yield return new ValidationResult(string.Format(CultureInfo.InvariantCulture, "The buffer '{0}' cannot contain '/' in its name.", group.Key));
+                    yield return new ValidationResult(string.Format(CultureInfo.InvariantCulture, "The name of buffer '{0}' is invalid. Buffer names must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character.", group.Key));
                 }
             }
         }
@@ -345,6 +345,9 @@ private static IEnumerable<ValidationResult> VerifyNoBufferReferencesUsedBySocke
 
     [GeneratedRegex(@"\$\(([^)]+)\)|\$\$([^)]+)")]
     internal static partial Regex EnvironmentVariableExpansionRegex();
+
+    [GeneratedRegex(@"^[a-z0-9]([-a-z0-9]*[a-z0-9])?$")]
+    internal static partial Regex BufferNameRegex();
 }
 
 [Equatable]