From b0d37db1fdf601fdd7d7554e7911fad910833efa Mon Sep 17 00:00:00 2001 From: Kevin Su Date: Wed, 11 Sep 2024 13:12:12 -0700 Subject: [PATCH] Revert "Improve execution name readability (#5637)" This reverts commit 2ed24086f3d5eac5b779c1755642d4ac1b1d0f76. --- flyteadmin/go.mod | 1 - flyteadmin/go.sum | 2 - .../async/schedule/aws/workflow_executor.go | 5 +- flyteadmin/pkg/common/executions.go | 13 ++++ flyteadmin/pkg/common/executions_test.go | 23 +++++++ .../pkg/common/naming/execution_name.go | 30 --------- .../pkg/common/naming/execution_name_test.go | 64 ------------------- flyteadmin/pkg/manager/impl/util/shared.go | 3 +- .../pkg/manager/impl/util/shared_test.go | 4 +- .../interfaces/application_configuration.go | 3 +- .../scheduler/executor/executor_impl.go | 21 ++++-- go.mod | 1 - go.sum | 2 - 13 files changed, 60 insertions(+), 112 deletions(-) create mode 100644 flyteadmin/pkg/common/executions_test.go delete mode 100644 flyteadmin/pkg/common/naming/execution_name.go delete mode 100644 flyteadmin/pkg/common/naming/execution_name_test.go diff --git a/flyteadmin/go.mod b/flyteadmin/go.mod index b9eba5b83a..ac74384250 100644 --- a/flyteadmin/go.mod +++ b/flyteadmin/go.mod @@ -48,7 +48,6 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.9.0 github.com/wI2L/jsondiff v0.5.0 - github.com/wolfeidau/humanhash v1.1.0 go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.47.0 go.opentelemetry.io/otel v1.24.0 golang.org/x/oauth2 v0.16.0 diff --git a/flyteadmin/go.sum b/flyteadmin/go.sum index 049add4bbc..dba9da2e86 100644 --- a/flyteadmin/go.sum +++ b/flyteadmin/go.sum @@ -1297,8 +1297,6 @@ github.com/valyala/bytebufferpool v1.0.0 h1:GqA5TC/0021Y/b9FG4Oi9Mr3q7XYx6Kllzaw github.com/valyala/bytebufferpool v1.0.0/go.mod h1:6bBcMArwyJ5K/AmCkWv1jt77kVWyCJ6HpOuEn7z0Csc= github.com/wI2L/jsondiff v0.5.0 h1:RRMTi/mH+R2aXcPe1VYyvGINJqQfC3R+KSEakuU1Ikw= github.com/wI2L/jsondiff v0.5.0/go.mod h1:qqG6hnK0Lsrz2BpIVCxWiK9ItsBCpIZQiv0izJjOZ9s= -github.com/wolfeidau/humanhash v1.1.0 h1:06KgtyyABJGBbrfMONrW7S+b5TTYVyrNB/jss5n7F3E= -github.com/wolfeidau/humanhash v1.1.0/go.mod h1:jkpynR1bfyfkmKEQudIC0osWKynFAoayRjzH9OJdVIg= github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c/go.mod h1:lB8K/P019DLNhemzwFU4jHLhdvlE6uDZjXFejJXr49I= github.com/xdg/stringprep v0.0.0-20180714160509-73f8eece6fdc/go.mod h1:Jhud4/sHMO4oL310DaZAKk9ZaJ08SJfe+sJh0HrGL1Y= github.com/xdg/stringprep v1.0.0/go.mod h1:Jhud4/sHMO4oL310DaZAKk9ZaJ08SJfe+sJh0HrGL1Y= diff --git a/flyteadmin/pkg/async/schedule/aws/workflow_executor.go b/flyteadmin/pkg/async/schedule/aws/workflow_executor.go index 523fdd077e..c4a5d75d14 100644 --- a/flyteadmin/pkg/async/schedule/aws/workflow_executor.go +++ b/flyteadmin/pkg/async/schedule/aws/workflow_executor.go @@ -15,7 +15,7 @@ import ( "github.com/flyteorg/flyte/flyteadmin/pkg/async" scheduleInterfaces "github.com/flyteorg/flyte/flyteadmin/pkg/async/schedule/interfaces" - "github.com/flyteorg/flyte/flyteadmin/pkg/common/naming" + "github.com/flyteorg/flyte/flyteadmin/pkg/common" "github.com/flyteorg/flyte/flyteadmin/pkg/errors" "github.com/flyteorg/flyte/flyteadmin/pkg/manager/interfaces" runtimeInterfaces "github.com/flyteorg/flyte/flyteadmin/pkg/runtime/interfaces" @@ -129,7 +129,7 @@ func generateExecutionName(launchPlan *admin.LaunchPlan, kickoffTime time.Time) Name: launchPlan.Id.Name, }) randomSeed := kickoffTime.UnixNano() + int64(hashedIdentifier) - return naming.GetExecutionName(randomSeed) + return common.GetExecutionName(randomSeed) } func (e *workflowExecutor) formulateExecutionCreateRequest( @@ -207,6 +207,7 @@ func (e *workflowExecutor) run() error { continue } executionRequest := e.formulateExecutionCreateRequest(launchPlan, scheduledWorkflowExecutionRequest.KickoffTime) + ctx = contextutils.WithWorkflowID(ctx, fmt.Sprintf(workflowIdentifierFmt, executionRequest.Project, executionRequest.Domain, executionRequest.Name)) err = e.resolveKickoffTimeArg(scheduledWorkflowExecutionRequest, launchPlan, executionRequest) diff --git a/flyteadmin/pkg/common/executions.go b/flyteadmin/pkg/common/executions.go index 4ac1ec7300..fbb5bdd6bd 100644 --- a/flyteadmin/pkg/common/executions.go +++ b/flyteadmin/pkg/common/executions.go @@ -1,9 +1,22 @@ package common import ( + "fmt" + + "k8s.io/apimachinery/pkg/util/rand" + "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core" ) +const ExecutionIDLength = 20 +const ExecutionStringFormat = "a%s" + +/* #nosec */ +func GetExecutionName(seed int64) string { + rand.Seed(seed) + return fmt.Sprintf(ExecutionStringFormat, rand.String(ExecutionIDLength-1)) +} + var terminalExecutionPhases = map[core.WorkflowExecution_Phase]bool{ core.WorkflowExecution_SUCCEEDED: true, core.WorkflowExecution_FAILED: true, diff --git a/flyteadmin/pkg/common/executions_test.go b/flyteadmin/pkg/common/executions_test.go new file mode 100644 index 0000000000..628abd6e9d --- /dev/null +++ b/flyteadmin/pkg/common/executions_test.go @@ -0,0 +1,23 @@ +package common + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +const AllowedExecutionIDStartCharStr = "abcdefghijklmnopqrstuvwxyz" +const AllowedExecutionIDStr = "abcdefghijklmnopqrstuvwxyz1234567890" + +var AllowedExecutionIDStartChars = []rune(AllowedExecutionIDStartCharStr) +var AllowedExecutionIDChars = []rune(AllowedExecutionIDStr) + +func TestGetExecutionName(t *testing.T) { + randString := GetExecutionName(time.Now().UnixNano()) + assert.Len(t, randString, ExecutionIDLength) + assert.Contains(t, AllowedExecutionIDStartChars, rune(randString[0])) + for i := 1; i < len(randString); i++ { + assert.Contains(t, AllowedExecutionIDChars, rune(randString[i])) + } +} diff --git a/flyteadmin/pkg/common/naming/execution_name.go b/flyteadmin/pkg/common/naming/execution_name.go deleted file mode 100644 index 01aa3fe8b6..0000000000 --- a/flyteadmin/pkg/common/naming/execution_name.go +++ /dev/null @@ -1,30 +0,0 @@ -package naming - -import ( - "fmt" - - "github.com/wolfeidau/humanhash" - "k8s.io/apimachinery/pkg/util/rand" - - "github.com/flyteorg/flyte/flyteadmin/pkg/runtime" - runtimeInterfaces "github.com/flyteorg/flyte/flyteadmin/pkg/runtime/interfaces" -) - -const ExecutionIDLength = 20 -const ExecutionIDLengthLimit = 63 -const ExecutionStringFormat = "a%s" - -var configProvider runtimeInterfaces.ApplicationConfiguration = runtime.NewApplicationConfigurationProvider() - -/* #nosec */ -func GetExecutionName(seed int64) string { - rand.Seed(seed) - config := configProvider.GetTopLevelConfig() - if config.FeatureGates.EnableFriendlyNames { - hashKey := []byte(rand.String(ExecutionIDLength)) - // Ignoring the error as it's guaranteed hash key longer than result in this context. - result, _ := humanhash.Humanize(hashKey, 4) - return result - } - return fmt.Sprintf(ExecutionStringFormat, rand.String(ExecutionIDLength-1)) -} diff --git a/flyteadmin/pkg/common/naming/execution_name_test.go b/flyteadmin/pkg/common/naming/execution_name_test.go deleted file mode 100644 index 22729dbb9b..0000000000 --- a/flyteadmin/pkg/common/naming/execution_name_test.go +++ /dev/null @@ -1,64 +0,0 @@ -package naming - -import ( - "strings" - "testing" - "time" - - "github.com/stretchr/testify/assert" - - runtimeInterfaces "github.com/flyteorg/flyte/flyteadmin/pkg/runtime/interfaces" - runtimeMocks "github.com/flyteorg/flyte/flyteadmin/pkg/runtime/mocks" -) - -const AllowedExecutionIDAlphabetStr = "abcdefghijklmnopqrstuvwxyz" -const AllowedExecutionIDAlphanumericStr = "abcdefghijklmnopqrstuvwxyz1234567890" -const AllowedExecutionIDFriendlyNameStr = "abcdefghijklmnopqrstuvwxyz-" - -var AllowedExecutionIDAlphabets = []rune(AllowedExecutionIDAlphabetStr) -var AllowedExecutionIDAlphanumerics = []rune(AllowedExecutionIDAlphanumericStr) -var AllowedExecutionIDFriendlyNameChars = []rune(AllowedExecutionIDFriendlyNameStr) - -func TestGetExecutionName(t *testing.T) { - originalConfigProvider := configProvider - defer func() { configProvider = originalConfigProvider }() - - mockConfigProvider := &runtimeMocks.MockApplicationProvider{} - configProvider = mockConfigProvider - - t.Run("general name", func(t *testing.T) { - appConfig := runtimeInterfaces.ApplicationConfig{ - FeatureGates: runtimeInterfaces.FeatureGates{ - EnableFriendlyNames: false, - }, - } - mockConfigProvider.SetTopLevelConfig(appConfig) - - randString := GetExecutionName(time.Now().UnixNano()) - assert.Len(t, randString, ExecutionIDLength) - assert.Contains(t, AllowedExecutionIDAlphabets, rune(randString[0])) - for i := 1; i < len(randString); i++ { - assert.Contains(t, AllowedExecutionIDAlphanumerics, rune(randString[i])) - } - }) - - t.Run("friendly name", func(t *testing.T) { - appConfig := runtimeInterfaces.ApplicationConfig{ - FeatureGates: runtimeInterfaces.FeatureGates{ - EnableFriendlyNames: true, - }, - } - mockConfigProvider.SetTopLevelConfig(appConfig) - - randString := GetExecutionName(time.Now().UnixNano()) - assert.LessOrEqual(t, len(randString), ExecutionIDLengthLimit) - for i := 0; i < len(randString); i++ { - assert.Contains(t, AllowedExecutionIDFriendlyNameChars, rune(randString[i])) - } - hyphenCount := strings.Count(randString, "-") - assert.Equal(t, 3, hyphenCount, "FriendlyName should contain exactly three hyphens") - words := strings.Split(randString, "-") - assert.Equal(t, 4, len(words), "FriendlyName should be split into exactly four words") - }) - -} diff --git a/flyteadmin/pkg/manager/impl/util/shared.go b/flyteadmin/pkg/manager/impl/util/shared.go index ba8fc41760..8402451200 100644 --- a/flyteadmin/pkg/manager/impl/util/shared.go +++ b/flyteadmin/pkg/manager/impl/util/shared.go @@ -8,7 +8,6 @@ import ( "google.golang.org/grpc/codes" "github.com/flyteorg/flyte/flyteadmin/pkg/common" - "github.com/flyteorg/flyte/flyteadmin/pkg/common/naming" "github.com/flyteorg/flyte/flyteadmin/pkg/errors" "github.com/flyteorg/flyte/flyteadmin/pkg/manager/impl/shared" "github.com/flyteorg/flyte/flyteadmin/pkg/manager/impl/validation" @@ -26,7 +25,7 @@ func GetExecutionName(request *admin.ExecutionCreateRequest) string { if request.Name != "" { return request.Name } - return naming.GetExecutionName(time.Now().UnixNano()) + return common.GetExecutionName(time.Now().UnixNano()) } func GetTask(ctx context.Context, repo repoInterfaces.Repository, identifier *core.Identifier) ( diff --git a/flyteadmin/pkg/manager/impl/util/shared_test.go b/flyteadmin/pkg/manager/impl/util/shared_test.go index 114dbebdfb..b9b296971e 100644 --- a/flyteadmin/pkg/manager/impl/util/shared_test.go +++ b/flyteadmin/pkg/manager/impl/util/shared_test.go @@ -12,8 +12,8 @@ import ( "github.com/stretchr/testify/assert" "google.golang.org/grpc/codes" + "github.com/flyteorg/flyte/flyteadmin/pkg/common" commonMocks "github.com/flyteorg/flyte/flyteadmin/pkg/common/mocks" - "github.com/flyteorg/flyte/flyteadmin/pkg/common/naming" flyteAdminErrors "github.com/flyteorg/flyte/flyteadmin/pkg/errors" "github.com/flyteorg/flyte/flyteadmin/pkg/manager/impl/testutils" managerInterfaces "github.com/flyteorg/flyte/flyteadmin/pkg/manager/interfaces" @@ -42,7 +42,7 @@ func TestPopulateExecutionID(t *testing.T) { Domain: "domain", }) assert.NotEmpty(t, name) - assert.Len(t, name, naming.ExecutionIDLength) + assert.Len(t, name, common.ExecutionIDLength) } func TestPopulateExecutionID_ExistingName(t *testing.T) { diff --git a/flyteadmin/pkg/runtime/interfaces/application_configuration.go b/flyteadmin/pkg/runtime/interfaces/application_configuration.go index 3505150919..94c9ab174b 100644 --- a/flyteadmin/pkg/runtime/interfaces/application_configuration.go +++ b/flyteadmin/pkg/runtime/interfaces/application_configuration.go @@ -49,8 +49,7 @@ type PostgresConfig struct { } type FeatureGates struct { - EnableArtifacts bool `json:"enableArtifacts" pflag:",Enable artifacts feature."` - EnableFriendlyNames bool `json:"enableFriendlyNames" pflag:",Enable generation of friendly execution names feature."` + EnableArtifacts bool `json:"enableArtifacts" pflag:",Enable artifacts feature."` } // ApplicationConfig is the base configuration to start admin diff --git a/flyteadmin/scheduler/executor/executor_impl.go b/flyteadmin/scheduler/executor/executor_impl.go index f3fd86c6cf..dffb98e1b6 100644 --- a/flyteadmin/scheduler/executor/executor_impl.go +++ b/flyteadmin/scheduler/executor/executor_impl.go @@ -2,6 +2,7 @@ package executor import ( "context" + "strings" "time" "github.com/prometheus/client_golang/prometheus" @@ -11,7 +12,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/util/retry" - "github.com/flyteorg/flyte/flyteadmin/pkg/common/naming" + "github.com/flyteorg/flyte/flyteadmin/scheduler/identifier" "github.com/flyteorg/flyte/flyteadmin/scheduler/repositories/models" "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/admin" "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core" @@ -52,11 +53,23 @@ func (w *executor) Execute(ctx context.Context, scheduledTime time.Time, s model } } - executionName := naming.GetExecutionName(time.Now().UnixNano()) + // Making the identifier deterministic using the hash of the identifier and scheduled time + executionIdentifier, err := identifier.GetExecutionIdentifier(ctx, core.Identifier{ + Project: s.Project, + Domain: s.Domain, + Name: s.Name, + Version: s.Version, + }, scheduledTime) + + if err != nil { + logger.Errorf(ctx, "failed to generate execution identifier for schedule %+v due to %v", s, err) + return err + } + executionRequest := &admin.ExecutionCreateRequest{ Project: s.Project, Domain: s.Domain, - Name: executionName, + Name: "f" + strings.ReplaceAll(executionIdentifier.String(), "-", "")[:19], Spec: &admin.ExecutionSpec{ LaunchPlan: &core.Identifier{ ResourceType: core.ResourceType_LAUNCH_PLAN, @@ -84,7 +97,7 @@ func (w *executor) Execute(ctx context.Context, scheduledTime time.Time, s model // Do maximum of 30 retries on failures with constant backoff factor opts := wait.Backoff{Duration: 3000, Factor: 2.0, Steps: 30} - err := retry.OnError(opts, + err = retry.OnError(opts, func(err error) bool { // For idempotent behavior ignore the AlreadyExists error which happens if we try to schedule a launchplan // for execution at the same time which is already available in admin. diff --git a/go.mod b/go.mod index 8fd55ed61a..3a7098d3c0 100644 --- a/go.mod +++ b/go.mod @@ -178,7 +178,6 @@ require ( github.com/tidwall/pretty v1.2.0 // indirect github.com/tidwall/sjson v1.2.5 // indirect github.com/wI2L/jsondiff v0.5.0 // indirect - github.com/wolfeidau/humanhash v1.1.0 // indirect go.opencensus.io v0.24.0 // indirect go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.47.0 // indirect go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.46.1 // indirect diff --git a/go.sum b/go.sum index ae60f26800..05db1b9c1c 100644 --- a/go.sum +++ b/go.sum @@ -1333,8 +1333,6 @@ github.com/valyala/bytebufferpool v1.0.0 h1:GqA5TC/0021Y/b9FG4Oi9Mr3q7XYx6Kllzaw github.com/valyala/bytebufferpool v1.0.0/go.mod h1:6bBcMArwyJ5K/AmCkWv1jt77kVWyCJ6HpOuEn7z0Csc= github.com/wI2L/jsondiff v0.5.0 h1:RRMTi/mH+R2aXcPe1VYyvGINJqQfC3R+KSEakuU1Ikw= github.com/wI2L/jsondiff v0.5.0/go.mod h1:qqG6hnK0Lsrz2BpIVCxWiK9ItsBCpIZQiv0izJjOZ9s= -github.com/wolfeidau/humanhash v1.1.0 h1:06KgtyyABJGBbrfMONrW7S+b5TTYVyrNB/jss5n7F3E= -github.com/wolfeidau/humanhash v1.1.0/go.mod h1:jkpynR1bfyfkmKEQudIC0osWKynFAoayRjzH9OJdVIg= github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c/go.mod h1:lB8K/P019DLNhemzwFU4jHLhdvlE6uDZjXFejJXr49I= github.com/xdg/stringprep v0.0.0-20180714160509-73f8eece6fdc/go.mod h1:Jhud4/sHMO4oL310DaZAKk9ZaJ08SJfe+sJh0HrGL1Y= github.com/xdg/stringprep v1.0.0/go.mod h1:Jhud4/sHMO4oL310DaZAKk9ZaJ08SJfe+sJh0HrGL1Y=