From 28aaa051fc1ba01749eab5a4a280c8d0e1a2e981 Mon Sep 17 00:00:00 2001 From: Eduardo Apolinario Date: Mon, 29 Jan 2024 12:07:33 -0800 Subject: [PATCH] Use a map of TemplateLogPlugin in log config Signed-off-by: Eduardo Apolinario --- flyteplugins/go/tasks/logs/config.go | 2 +- flyteplugins/go/tasks/logs/logconfig_flags.go | 1 - .../go/tasks/logs/logconfig_flags_test.go | 14 ----- flyteplugins/go/tasks/logs/logging_utils.go | 10 ++-- .../go/tasks/logs/logging_utils_test.go | 51 ++++++++++++----- .../tasks/pluginmachinery/tasklog/plugin.go | 1 - .../tasks/plugins/array/k8s/config_flags.go | 1 - .../plugins/array/k8s/config_flags_test.go | 14 ----- .../tasks/plugins/k8s/spark/config_flags.go | 4 -- .../plugins/k8s/spark/config_flags_test.go | 56 ------------------- 10 files changed, 43 insertions(+), 111 deletions(-) diff --git a/flyteplugins/go/tasks/logs/config.go b/flyteplugins/go/tasks/logs/config.go index b644dd12c4..c8eb293035 100644 --- a/flyteplugins/go/tasks/logs/config.go +++ b/flyteplugins/go/tasks/logs/config.go @@ -28,7 +28,7 @@ type LogConfig struct { StackdriverLogResourceName string `json:"stackdriver-logresourcename" pflag:",Name of the logresource in stackdriver"` StackDriverTemplateURI tasklog.TemplateURI `json:"stackdriver-template-uri" pflag:",Template Uri to use when building stackdriver log links"` - DynamicLogLinks map[string]tasklog.TemplateURI `json:"dynamic-log-links" pflag:",Map of dynamic log links"` + DynamicLogLinks map[string]tasklog.TemplateLogPlugin `json:"dynamic-log-links" pflag:"-,Map of dynamic log links"` Templates []tasklog.TemplateLogPlugin `json:"templates" pflag:"-,"` } diff --git a/flyteplugins/go/tasks/logs/logconfig_flags.go b/flyteplugins/go/tasks/logs/logconfig_flags.go index 5cf5b8f88c..00c08a8a58 100755 --- a/flyteplugins/go/tasks/logs/logconfig_flags.go +++ b/flyteplugins/go/tasks/logs/logconfig_flags.go @@ -61,6 +61,5 @@ func (cfg LogConfig) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags.String(fmt.Sprintf("%v%v", prefix, "gcp-project"), DefaultConfig.GCPProjectName, "Name of the project in GCP") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "stackdriver-logresourcename"), DefaultConfig.StackdriverLogResourceName, "Name of the logresource in stackdriver") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "stackdriver-template-uri"), DefaultConfig.StackDriverTemplateURI, "Template Uri to use when building stackdriver log links") - cmdFlags.StringToString(fmt.Sprintf("%v%v", prefix, "dynamic-log-links"), DefaultConfig.DynamicLogLinks, "Map of dynamic log links") return cmdFlags } diff --git a/flyteplugins/go/tasks/logs/logconfig_flags_test.go b/flyteplugins/go/tasks/logs/logconfig_flags_test.go index 6f42d36d38..8bb775df1f 100755 --- a/flyteplugins/go/tasks/logs/logconfig_flags_test.go +++ b/flyteplugins/go/tasks/logs/logconfig_flags_test.go @@ -253,18 +253,4 @@ func TestLogConfig_SetFlags(t *testing.T) { } }) }) - t.Run("Test_dynamic-log-links", func(t *testing.T) { - - t.Run("Override", func(t *testing.T) { - testValue := "a=1,b=2" - - cmdFlags.Set("dynamic-log-links", testValue) - if vStringToString, err := cmdFlags.GetStringToString("dynamic-log-links"); err == nil { - testDecodeRaw_LogConfig(t, vStringToString, &actual.DynamicLogLinks) - - } else { - assert.FailNow(t, err.Error()) - } - }) - }) } diff --git a/flyteplugins/go/tasks/logs/logging_utils.go b/flyteplugins/go/tasks/logs/logging_utils.go index bde0fe33a9..04d23f4ec8 100644 --- a/flyteplugins/go/tasks/logs/logging_utils.go +++ b/flyteplugins/go/tasks/logs/logging_utils.go @@ -115,12 +115,10 @@ func InitializeLogPlugins(cfg *LogConfig) (tasklog.Plugin, error) { dynamicPlugins = append( dynamicPlugins, tasklog.TemplateLogPlugin{ - Name: logLinkType, - DisplayName: logLinkType, - DynamicTemplateURIs: []tasklog.TemplateURI{ - dynamicLogLink, - }, - MessageFormat: core.TaskLog_JSON, + Name: logLinkType, + DisplayName: dynamicLogLink.DisplayName, + DynamicTemplateURIs: dynamicLogLink.TemplateURIs, + MessageFormat: core.TaskLog_JSON, }) } diff --git a/flyteplugins/go/tasks/logs/logging_utils_test.go b/flyteplugins/go/tasks/logs/logging_utils_test.go index ce171bfc39..946d069d50 100644 --- a/flyteplugins/go/tasks/logs/logging_utils_test.go +++ b/flyteplugins/go/tasks/logs/logging_utils_test.go @@ -360,8 +360,13 @@ func TestGetLogsForContainerInPod_Flyteinteractive(t *testing.T) { { "Flyteinteractive enabled but no task template", &LogConfig{ - DynamicLogLinks: map[string]tasklog.TemplateURI{ - "vscode": "https://flyteinteractive.mydomain.com:{{ .taskConfig.port }}/{{ .namespace }}/{{ .podName }}/{{ .containerName }}/{{ .containerId }}", + DynamicLogLinks: map[string]tasklog.TemplateLogPlugin{ + "vscode": tasklog.TemplateLogPlugin{ + DisplayName: "vscode link", + TemplateURIs: []tasklog.TemplateURI{ + "https://flyteinteractive.mydomain.com:{{ .taskConfig.port }}/{{ .namespace }}/{{ .podName }}/{{ .containerName }}/{{ .containerId }}", + }, + }, }, }, nil, @@ -370,8 +375,13 @@ func TestGetLogsForContainerInPod_Flyteinteractive(t *testing.T) { { "Flyteinteractive enabled but config not found in task template", &LogConfig{ - DynamicLogLinks: map[string]tasklog.TemplateURI{ - "vscode": "https://flyteinteractive.mydomain.com:{{ .taskConfig.port }}/{{ .namespace }}/{{ .podName }}/{{ .containerName }}/{{ .containerId }}", + DynamicLogLinks: map[string]tasklog.TemplateLogPlugin{ + "vscode": tasklog.TemplateLogPlugin{ + DisplayName: "vscode link", + TemplateURIs: []tasklog.TemplateURI{ + "https://flyteinteractive.mydomain.com:{{ .taskConfig.port }}/{{ .namespace }}/{{ .podName }}/{{ .containerName }}/{{ .containerId }}", + }, + }, }, }, &core.TaskTemplate{}, @@ -391,8 +401,13 @@ func TestGetLogsForContainerInPod_Flyteinteractive(t *testing.T) { { "Flyteinteractive - multiple dynamic options", &LogConfig{ - DynamicLogLinks: map[string]tasklog.TemplateURI{ - "vscode": "https://abc.com:{{ .taskConfig.port }}/{{ .taskConfig.route }}", + DynamicLogLinks: map[string]tasklog.TemplateLogPlugin{ + "vscode": tasklog.TemplateLogPlugin{ + DisplayName: "vscode link", + TemplateURIs: []tasklog.TemplateURI{ + "https://abc.com:{{ .taskConfig.port }}/{{ .taskConfig.route }}", + }, + }, }, }, &core.TaskTemplate{ @@ -406,15 +421,20 @@ func TestGetLogsForContainerInPod_Flyteinteractive(t *testing.T) { { Uri: "https://abc.com:65535/a-route", MessageFormat: core.TaskLog_JSON, - Name: "vscode my-Suffix", + Name: "vscode link my-Suffix", }, }, }, { "Flyteinteractive - multiple uses of the template (invalid use of ports in a URI)", &LogConfig{ - DynamicLogLinks: map[string]tasklog.TemplateURI{ - "vscode": "https://abc.com:{{ .taskConfig.port }}:{{ .taskConfig.port}}", + DynamicLogLinks: map[string]tasklog.TemplateLogPlugin{ + "vscode": tasklog.TemplateLogPlugin{ + DisplayName: "vscode link", + TemplateURIs: []tasklog.TemplateURI{ + "https://abc.com:{{ .taskConfig.port }}:{{ .taskConfig.port}}", + }, + }, }, }, &core.TaskTemplate{ @@ -427,7 +447,7 @@ func TestGetLogsForContainerInPod_Flyteinteractive(t *testing.T) { { Uri: "https://abc.com:65535:65535", MessageFormat: core.TaskLog_JSON, - Name: "vscode my-Suffix", + Name: "vscode link my-Suffix", }, }, }, @@ -456,8 +476,13 @@ func TestGetLogsForContainerInPod_Flyteinteractive(t *testing.T) { &LogConfig{ IsKubernetesEnabled: true, KubernetesTemplateURI: "https://k8s.com/{{ .namespace }}/{{ .podName }}/{{ .containerName }}/{{ .containerId }}", - DynamicLogLinks: map[string]tasklog.TemplateURI{ - "vscode": "https://flyteinteractive.mydomain.com:{{ .taskConfig.port }}/{{ .namespace }}/{{ .podName }}/{{ .containerName }}/{{ .containerId }}", + DynamicLogLinks: map[string]tasklog.TemplateLogPlugin{ + "vscode": tasklog.TemplateLogPlugin{ + DisplayName: "vscode link", + TemplateURIs: []tasklog.TemplateURI{ + "https://flyteinteractive.mydomain.com:{{ .taskConfig.port }}/{{ .namespace }}/{{ .podName }}/{{ .containerName }}/{{ .containerId }}", + }, + }, }, }, &core.TaskTemplate{ @@ -475,7 +500,7 @@ func TestGetLogsForContainerInPod_Flyteinteractive(t *testing.T) { { Uri: "https://flyteinteractive.mydomain.com:65535/my-namespace/my-pod/ContainerName/ContainerID", MessageFormat: core.TaskLog_JSON, - Name: "vscode my-Suffix", + Name: "vscode link my-Suffix", }, }, }, diff --git a/flyteplugins/go/tasks/pluginmachinery/tasklog/plugin.go b/flyteplugins/go/tasks/pluginmachinery/tasklog/plugin.go index c995892d97..1caf8a2bac 100644 --- a/flyteplugins/go/tasks/pluginmachinery/tasklog/plugin.go +++ b/flyteplugins/go/tasks/pluginmachinery/tasklog/plugin.go @@ -46,7 +46,6 @@ type Plugin interface { } type TemplateLogPlugin struct { - // TODO: these don't need pflags anymore? Name string `json:"name" pflag:",Name of the plugin."` DisplayName string `json:"displayName" pflag:",Display name for the generated log when displayed in the console."` TemplateURIs []TemplateURI `json:"templateUris" pflag:",URI Templates for generating task log links."` diff --git a/flyteplugins/go/tasks/plugins/array/k8s/config_flags.go b/flyteplugins/go/tasks/plugins/array/k8s/config_flags.go index a593c5879c..1497f64bdc 100755 --- a/flyteplugins/go/tasks/plugins/array/k8s/config_flags.go +++ b/flyteplugins/go/tasks/plugins/array/k8s/config_flags.go @@ -70,6 +70,5 @@ func (cfg Config) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.config.gcp-project"), defaultConfig.LogConfig.Config.GCPProjectName, "Name of the project in GCP") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.config.stackdriver-logresourcename"), defaultConfig.LogConfig.Config.StackdriverLogResourceName, "Name of the logresource in stackdriver") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.config.stackdriver-template-uri"), defaultConfig.LogConfig.Config.StackDriverTemplateURI, "Template Uri to use when building stackdriver log links") - cmdFlags.StringToString(fmt.Sprintf("%v%v", prefix, "logs.config.dynamic-log-links"), defaultConfig.LogConfig.Config.DynamicLogLinks, "Map of dynamic log links") return cmdFlags } diff --git a/flyteplugins/go/tasks/plugins/array/k8s/config_flags_test.go b/flyteplugins/go/tasks/plugins/array/k8s/config_flags_test.go index 8dbce02379..3737c45c3b 100755 --- a/flyteplugins/go/tasks/plugins/array/k8s/config_flags_test.go +++ b/flyteplugins/go/tasks/plugins/array/k8s/config_flags_test.go @@ -379,18 +379,4 @@ func TestConfig_SetFlags(t *testing.T) { } }) }) - t.Run("Test_logs.config.dynamic-log-links", func(t *testing.T) { - - t.Run("Override", func(t *testing.T) { - testValue := "a=1,b=2" - - cmdFlags.Set("logs.config.dynamic-log-links", testValue) - if vStringToString, err := cmdFlags.GetStringToString("logs.config.dynamic-log-links"); err == nil { - testDecodeRaw_Config(t, vStringToString, &actual.LogConfig.Config.DynamicLogLinks) - - } else { - assert.FailNow(t, err.Error()) - } - }) - }) } diff --git a/flyteplugins/go/tasks/plugins/k8s/spark/config_flags.go b/flyteplugins/go/tasks/plugins/k8s/spark/config_flags.go index b8807386e3..d5d6945f71 100755 --- a/flyteplugins/go/tasks/plugins/k8s/spark/config_flags.go +++ b/flyteplugins/go/tasks/plugins/k8s/spark/config_flags.go @@ -62,7 +62,6 @@ func (cfg Config) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.mixed.gcp-project"), defaultConfig.LogConfig.Mixed.GCPProjectName, "Name of the project in GCP") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.mixed.stackdriver-logresourcename"), defaultConfig.LogConfig.Mixed.StackdriverLogResourceName, "Name of the logresource in stackdriver") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.mixed.stackdriver-template-uri"), defaultConfig.LogConfig.Mixed.StackDriverTemplateURI, "Template Uri to use when building stackdriver log links") - cmdFlags.StringToString(fmt.Sprintf("%v%v", prefix, "logs.mixed.dynamic-log-links"), defaultConfig.LogConfig.Mixed.DynamicLogLinks, "Map of dynamic log links") cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "logs.user.cloudwatch-enabled"), defaultConfig.LogConfig.User.IsCloudwatchEnabled, "Enable Cloudwatch Logging") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.user.cloudwatch-region"), defaultConfig.LogConfig.User.CloudwatchRegion, "AWS region in which Cloudwatch logs are stored.") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.user.cloudwatch-log-group"), defaultConfig.LogConfig.User.CloudwatchLogGroup, "Log group to which streams are associated.") @@ -74,7 +73,6 @@ func (cfg Config) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.user.gcp-project"), defaultConfig.LogConfig.User.GCPProjectName, "Name of the project in GCP") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.user.stackdriver-logresourcename"), defaultConfig.LogConfig.User.StackdriverLogResourceName, "Name of the logresource in stackdriver") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.user.stackdriver-template-uri"), defaultConfig.LogConfig.User.StackDriverTemplateURI, "Template Uri to use when building stackdriver log links") - cmdFlags.StringToString(fmt.Sprintf("%v%v", prefix, "logs.user.dynamic-log-links"), defaultConfig.LogConfig.User.DynamicLogLinks, "Map of dynamic log links") cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "logs.system.cloudwatch-enabled"), defaultConfig.LogConfig.System.IsCloudwatchEnabled, "Enable Cloudwatch Logging") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.system.cloudwatch-region"), defaultConfig.LogConfig.System.CloudwatchRegion, "AWS region in which Cloudwatch logs are stored.") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.system.cloudwatch-log-group"), defaultConfig.LogConfig.System.CloudwatchLogGroup, "Log group to which streams are associated.") @@ -86,7 +84,6 @@ func (cfg Config) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.system.gcp-project"), defaultConfig.LogConfig.System.GCPProjectName, "Name of the project in GCP") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.system.stackdriver-logresourcename"), defaultConfig.LogConfig.System.StackdriverLogResourceName, "Name of the logresource in stackdriver") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.system.stackdriver-template-uri"), defaultConfig.LogConfig.System.StackDriverTemplateURI, "Template Uri to use when building stackdriver log links") - cmdFlags.StringToString(fmt.Sprintf("%v%v", prefix, "logs.system.dynamic-log-links"), defaultConfig.LogConfig.System.DynamicLogLinks, "Map of dynamic log links") cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "logs.all-user.cloudwatch-enabled"), defaultConfig.LogConfig.AllUser.IsCloudwatchEnabled, "Enable Cloudwatch Logging") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.all-user.cloudwatch-region"), defaultConfig.LogConfig.AllUser.CloudwatchRegion, "AWS region in which Cloudwatch logs are stored.") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.all-user.cloudwatch-log-group"), defaultConfig.LogConfig.AllUser.CloudwatchLogGroup, "Log group to which streams are associated.") @@ -98,6 +95,5 @@ func (cfg Config) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.all-user.gcp-project"), defaultConfig.LogConfig.AllUser.GCPProjectName, "Name of the project in GCP") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.all-user.stackdriver-logresourcename"), defaultConfig.LogConfig.AllUser.StackdriverLogResourceName, "Name of the logresource in stackdriver") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "logs.all-user.stackdriver-template-uri"), defaultConfig.LogConfig.AllUser.StackDriverTemplateURI, "Template Uri to use when building stackdriver log links") - cmdFlags.StringToString(fmt.Sprintf("%v%v", prefix, "logs.all-user.dynamic-log-links"), defaultConfig.LogConfig.AllUser.DynamicLogLinks, "Map of dynamic log links") return cmdFlags } diff --git a/flyteplugins/go/tasks/plugins/k8s/spark/config_flags_test.go b/flyteplugins/go/tasks/plugins/k8s/spark/config_flags_test.go index c8e16e6d7d..ea8659a48a 100755 --- a/flyteplugins/go/tasks/plugins/k8s/spark/config_flags_test.go +++ b/flyteplugins/go/tasks/plugins/k8s/spark/config_flags_test.go @@ -267,20 +267,6 @@ func TestConfig_SetFlags(t *testing.T) { } }) }) - t.Run("Test_logs.mixed.dynamic-log-links", func(t *testing.T) { - - t.Run("Override", func(t *testing.T) { - testValue := "a=1,b=2" - - cmdFlags.Set("logs.mixed.dynamic-log-links", testValue) - if vStringToString, err := cmdFlags.GetStringToString("logs.mixed.dynamic-log-links"); err == nil { - testDecodeRaw_Config(t, vStringToString, &actual.LogConfig.Mixed.DynamicLogLinks) - - } else { - assert.FailNow(t, err.Error()) - } - }) - }) t.Run("Test_logs.user.cloudwatch-enabled", func(t *testing.T) { t.Run("Override", func(t *testing.T) { @@ -435,20 +421,6 @@ func TestConfig_SetFlags(t *testing.T) { } }) }) - t.Run("Test_logs.user.dynamic-log-links", func(t *testing.T) { - - t.Run("Override", func(t *testing.T) { - testValue := "a=1,b=2" - - cmdFlags.Set("logs.user.dynamic-log-links", testValue) - if vStringToString, err := cmdFlags.GetStringToString("logs.user.dynamic-log-links"); err == nil { - testDecodeRaw_Config(t, vStringToString, &actual.LogConfig.User.DynamicLogLinks) - - } else { - assert.FailNow(t, err.Error()) - } - }) - }) t.Run("Test_logs.system.cloudwatch-enabled", func(t *testing.T) { t.Run("Override", func(t *testing.T) { @@ -603,20 +575,6 @@ func TestConfig_SetFlags(t *testing.T) { } }) }) - t.Run("Test_logs.system.dynamic-log-links", func(t *testing.T) { - - t.Run("Override", func(t *testing.T) { - testValue := "a=1,b=2" - - cmdFlags.Set("logs.system.dynamic-log-links", testValue) - if vStringToString, err := cmdFlags.GetStringToString("logs.system.dynamic-log-links"); err == nil { - testDecodeRaw_Config(t, vStringToString, &actual.LogConfig.System.DynamicLogLinks) - - } else { - assert.FailNow(t, err.Error()) - } - }) - }) t.Run("Test_logs.all-user.cloudwatch-enabled", func(t *testing.T) { t.Run("Override", func(t *testing.T) { @@ -771,18 +729,4 @@ func TestConfig_SetFlags(t *testing.T) { } }) }) - t.Run("Test_logs.all-user.dynamic-log-links", func(t *testing.T) { - - t.Run("Override", func(t *testing.T) { - testValue := "a=1,b=2" - - cmdFlags.Set("logs.all-user.dynamic-log-links", testValue) - if vStringToString, err := cmdFlags.GetStringToString("logs.all-user.dynamic-log-links"); err == nil { - testDecodeRaw_Config(t, vStringToString, &actual.LogConfig.AllUser.DynamicLogLinks) - - } else { - assert.FailNow(t, err.Error()) - } - }) - }) }