Add flyteconsole url to FlyteWorkflow CRD #5449

eapolinario · 2024-06-04T18:50:57Z

Tracking issue

Related to #4773

Why are the changes needed?

~~This is a stacked PR on top of #5432. It adds a new field to the FlyteWorkflow CRD to help in the construction of flyteconsole urls pointing to node executions.~~
edit: This PR started its life as a stacked PR, but can be completely separate from #5432. Let's separate those out in order to de-risk this change.

We need to provide a way for tasks to point to a flyteconsole address that corresponds to the execution of those particular tasks. This is useful, for example, if we want to provide third-party services with links to task executions. The first user of this feature is going to be the wandb flytekit plugin.

What changes were proposed in this pull request?

In this PR we introduce a new field to the FlyteWorkflow CRD to represent a flyteconsole url. Most of the changes in the PR are about plumbing the necessary interfaces and implementations to use this new field.

The end result is that the pod will contain a new environment variable called FLYTE_EXECUTION_URL in case it requested it and the flyteconsole url is provided in the flyteadmin configuration. The way a task requests to have this new env var injected is to define link_type in its task template. This is the de factor protocol we've been using to enable dynamic log links, so this PR leans on it.

How was this patch tested?

functional tests and local testing using the demo cluster.

Setup process

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

codecov · 2024-06-04T18:55:30Z

Codecov Report

Attention: Patch coverage is 76.66667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 60.97%. Comparing base (bd2f9a8) to head (501d3ee).
Report is 139 commits behind head on master.

Files with missing lines	Patch %	Lines
...ns/go/tasks/pluginmachinery/flytek8s/pod_helper.go	69.23%	3 Missing and 1 partial ⚠️
flyteadmin/pkg/workflowengine/impl/k8s_executor.go	0.00%	1 Missing and 1 partial ⚠️
...peller/pkg/apis/flyteworkflow/v1alpha1/workflow.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5449      +/-   ##
==========================================
- Coverage   61.93%   60.97%   -0.96%     
==========================================
  Files         611      793     +182     
  Lines       36463    51352   +14889     
==========================================
+ Hits        22582    31313    +8731     
- Misses      11907    17152    +5245     
- Partials     1974     2887     +913

Flag	Coverage Δ
unittests-datacatalog	`69.31% <ø> (ø)`
unittests-flyteadmin	`58.65% <0.00%> (?)`
unittests-flytecopilot	`17.79% <ø> (ø)`
unittests-flytectl	`67.97% <ø> (ø)`
unittests-flyteidl	`79.04% <ø> (ø)`
unittests-flyteplugins	`61.81% <85.18%> (-0.04%)`	⬇️
unittests-flytepropeller	`57.32% <0.00%> (-0.01%)`	⬇️
unittests-flytestdlib	`65.82% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eapolinario · 2024-06-06T21:04:37Z

flytepropeller/pkg/controller/executors/execution_context.go

@@ -4,7 +4,7 @@ import (
 	"github.com/flyteorg/flyte/flytepropeller/pkg/apis/flyteworkflow/v1alpha1"
 )

-// go:generate mockery -case=underscore
+//go:generate mockery-v2 --case=underscore --with-expecter --name ExecutionContext --output=mocks


Notice that we were not generating the mocks before. flyteorg/flytepropeller#600 introduced the go:generate directive but unfortunately there was a typo.

Also notice that I'm moving to upstream mockery (as described in #4937) because our mockery fork couldn't generate the code for GetConsoleURL due to the fact that it was coming to ExecutionContext via other interface.

Signed-off-by: Eduardo Apolinario <[email protected]>

…late Signed-off-by: Eduardo Apolinario <[email protected]>

Signed-off-by: Eduardo Apolinario <[email protected]>

EngHabu

A couple of nits

EngHabu · 2024-06-13T13:40:46Z

flyteadmin/pkg/runtime/interfaces/application_configuration.go

@@ -103,6 +103,9 @@ type ApplicationConfig struct {
 	Envs map[string]string `json:"envs,omitempty"`

 	FeatureGates FeatureGates `json:"featureGates" pflag:",Enable experimental features."`
+
+	// A URL pointing to the flyteconsole instance used to hit this flyteadmin instance.
+	ConsoleURL string `json:"consoleUrl,omitempty"`


Suggested change

ConsoleURL string `json:"consoleUrl,omitempty"`

ConsoleURL string `json:"consoleUrl,omitempty" pflag:",A URL pointing to the flyteconsole instance used to hit this flyteadmin instance."`

EngHabu · 2024-06-13T13:41:46Z

flyteadmin/pkg/workflowengine/impl/k8s_executor.go

+	if e.config.ApplicationConfiguration().GetTopLevelConfig().ConsoleURL != "" {
+		flyteWf.ConsoleURL = e.config.ApplicationConfiguration().GetTopLevelConfig().ConsoleURL
+	}


Suggested change

if e.config.ApplicationConfiguration().GetTopLevelConfig().ConsoleURL != "" {

flyteWf.ConsoleURL = e.config.ApplicationConfiguration().GetTopLevelConfig().ConsoleURL

}

if consoleURL := e.config.ApplicationConfiguration().GetTopLevelConfig().ConsoleURL; len(consoleURL) > 0 {

flyteWf.ConsoleURL = consoleURL

}

EngHabu · 2024-06-13T13:42:48Z

flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go

@@ -69,6 +70,14 @@ func GetExecutionEnvVars(id pluginsCore.TaskExecutionID) []v1.EnvVar {
 		// },
 	}

+	if consoleURL != "" {


Suggested change

if consoleURL != "" {

if len(consoleURL) > 0 {

EngHabu · 2024-06-13T13:42:59Z

flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go

@@ -69,6 +70,14 @@ func GetExecutionEnvVars(id pluginsCore.TaskExecutionID) []v1.EnvVar {
 		// },
 	}

+	if consoleURL != "" {
+		envVars = append(envVars, v1.EnvVar{
+			Name: "FLYTE_EXECUTION_URL",


Move to const?

flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go

EngHabu · 2024-06-13T13:45:19Z

flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper.go

@@ -316,6 +316,19 @@ func BuildRawPod(ctx context.Context, tCtx pluginsCore.TaskExecutionContext) (*v
 	return podSpec, &objectMeta, primaryContainerName, nil
 }

+func shouldIncludeConsoleURL(taskTemplate *core.TaskTemplate) bool {


Suggested change

func shouldIncludeConsoleURL(taskTemplate *core.TaskTemplate) bool {

func hasExternalLinkType(taskTemplate *core.TaskTemplate) bool {

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add `ConsoleURL` to `FlyteWorkflow` CRD Signed-off-by: Eduardo Apolinario <[email protected]> * Add `ConsoleURL` to flyteadmin config and write it to CRD Signed-off-by: Eduardo Apolinario <[email protected]> * Add ConsoleURL to application_config_provider and DeepCopyInto Signed-off-by: Eduardo Apolinario <[email protected]> * wip Signed-off-by: Eduardo Apolinario <[email protected]> * more wip Signed-off-by: Eduardo Apolinario <[email protected]> * Fix flyteplugins unit tests Signed-off-by: Eduardo Apolinario <[email protected]> * Fix existing propeller unit tests Signed-off-by: Eduardo Apolinario <[email protected]> * Add a few unit tests Signed-off-by: Eduardo Apolinario <[email protected]> * Include FLYTE_EXECUTION_URL iff "link_type" is set in the task template Signed-off-by: Eduardo Apolinario <[email protected]> * Remove TODOs Signed-off-by: Eduardo Apolinario <[email protected]> * Only include consoleURL if task set the relevant bit in its task template Signed-off-by: Eduardo Apolinario <[email protected]> * Fix flyteplugins tests Signed-off-by: Eduardo Apolinario <[email protected]> * Remove attempt number from the url Signed-off-by: Eduardo Apolinario <[email protected]> * Include review feedback Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>

* Add flyteconsole url to FlyteWorkflow CRD (#5449) * Add `ConsoleURL` to `FlyteWorkflow` CRD Signed-off-by: Eduardo Apolinario <[email protected]> * Add `ConsoleURL` to flyteadmin config and write it to CRD Signed-off-by: Eduardo Apolinario <[email protected]> * Add ConsoleURL to application_config_provider and DeepCopyInto Signed-off-by: Eduardo Apolinario <[email protected]> * wip Signed-off-by: Eduardo Apolinario <[email protected]> * more wip Signed-off-by: Eduardo Apolinario <[email protected]> * Fix flyteplugins unit tests Signed-off-by: Eduardo Apolinario <[email protected]> * Fix existing propeller unit tests Signed-off-by: Eduardo Apolinario <[email protected]> * Add a few unit tests Signed-off-by: Eduardo Apolinario <[email protected]> * Include FLYTE_EXECUTION_URL iff "link_type" is set in the task template Signed-off-by: Eduardo Apolinario <[email protected]> * Remove TODOs Signed-off-by: Eduardo Apolinario <[email protected]> * Only include consoleURL if task set the relevant bit in its task template Signed-off-by: Eduardo Apolinario <[email protected]> * Fix flyteplugins tests Signed-off-by: Eduardo Apolinario <[email protected]> * Remove attempt number from the url Signed-off-by: Eduardo Apolinario <[email protected]> * Include review feedback Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> * Add mockery/v2 to boilerplate Signed-off-by: Eduardo Apolinario <[email protected]> * Remove TODO Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>

eapolinario force-pushed the add-console-to-CRD branch from 75a3394 to 18011ab Compare June 6, 2024 20:57

eapolinario commented Jun 6, 2024

View reviewed changes

eapolinario changed the title ~~Add console to crd~~ [wip] Add console to crd Jun 6, 2024

eapolinario changed the title ~~[wip] Add console to crd~~ Add flyteconsole url to crd Jun 11, 2024

eapolinario changed the base branch from regenerate-CRD to master June 11, 2024 23:34

eapolinario changed the base branch from master to regenerate-CRD June 11, 2024 23:36

eapolinario force-pushed the add-console-to-CRD branch from 9d93791 to d4e4e9f Compare June 11, 2024 23:55

eapolinario requested review from neverett and ppiegaze as code owners June 11, 2024 23:55

eapolinario changed the base branch from regenerate-CRD to master June 11, 2024 23:55

eapolinario force-pushed the add-console-to-CRD branch from d4e4e9f to 43b5e0d Compare June 11, 2024 23:56

eapolinario added 12 commits June 11, 2024 16:57

Add ConsoleURL to FlyteWorkflow CRD

389a51c

Signed-off-by: Eduardo Apolinario <[email protected]>

Add ConsoleURL to flyteadmin config and write it to CRD

54f43eb

Signed-off-by: Eduardo Apolinario <[email protected]>

Add ConsoleURL to application_config_provider and DeepCopyInto

944be08

Signed-off-by: Eduardo Apolinario <[email protected]>

wip

9b08b5b

Signed-off-by: Eduardo Apolinario <[email protected]>

more wip

d62bd96

Signed-off-by: Eduardo Apolinario <[email protected]>

Fix flyteplugins unit tests

5d25965

Signed-off-by: Eduardo Apolinario <[email protected]>

Fix existing propeller unit tests

554da58

Signed-off-by: Eduardo Apolinario <[email protected]>

Add a few unit tests

80286f2

Signed-off-by: Eduardo Apolinario <[email protected]>

Include FLYTE_EXECUTION_URL iff "link_type" is set in the task template

13fe7ad

Signed-off-by: Eduardo Apolinario <[email protected]>

Remove TODOs

38f57ad

Signed-off-by: Eduardo Apolinario <[email protected]>

Only include consoleURL if task set the relevant bit in its task temp…

6816823

…late Signed-off-by: Eduardo Apolinario <[email protected]>

Fix flyteplugins tests

6593fdf

Signed-off-by: Eduardo Apolinario <[email protected]>

eapolinario force-pushed the add-console-to-CRD branch from 43b5e0d to 6593fdf Compare June 11, 2024 23:57

eapolinario changed the title ~~Add flyteconsole url to crd~~ Add flyteconsole url to FlyteWorkflow CRD Jun 12, 2024

eapolinario requested review from EngHabu and removed request for neverett and ppiegaze June 12, 2024 00:11

Remove attempt number from the url

04facbd

Signed-off-by: Eduardo Apolinario <[email protected]>

EngHabu reviewed Jun 13, 2024

View reviewed changes

Include review feedback

501d3ee

Signed-off-by: Eduardo Apolinario <[email protected]>

EngHabu approved these changes Jun 14, 2024

View reviewed changes

eapolinario merged commit 7d788cb into master Jun 14, 2024
49 of 50 checks passed

eapolinario deleted the add-console-to-CRD branch June 14, 2024 17:42

hamersaw mentioned this pull request Jul 16, 2024

reverted mockery-v2 on ExecutionContext #5562

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flyteconsole url to FlyteWorkflow CRD #5449

Add flyteconsole url to FlyteWorkflow CRD #5449

eapolinario commented Jun 4, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

eapolinario Jun 6, 2024 •

edited

Loading

EngHabu left a comment

EngHabu Jun 13, 2024

EngHabu Jun 13, 2024

EngHabu Jun 13, 2024

EngHabu Jun 13, 2024

EngHabu Jun 13, 2024

	ConsoleURL string `json:"consoleUrl,omitempty"`
	ConsoleURL string `json:"consoleUrl,omitempty" pflag:",A URL pointing to the flyteconsole instance used to hit this flyteadmin instance."`

	func shouldIncludeConsoleURL(taskTemplate *core.TaskTemplate) bool {
	func hasExternalLinkType(taskTemplate *core.TaskTemplate) bool {

Add flyteconsole url to FlyteWorkflow CRD #5449

Add flyteconsole url to FlyteWorkflow CRD #5449

Conversation

eapolinario commented Jun 4, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

codecov bot commented Jun 4, 2024 • edited Loading

Codecov Report

eapolinario Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

EngHabu left a comment

Choose a reason for hiding this comment

EngHabu Jun 13, 2024

Choose a reason for hiding this comment

EngHabu Jun 13, 2024

Choose a reason for hiding this comment

EngHabu Jun 13, 2024

Choose a reason for hiding this comment

EngHabu Jun 13, 2024

Choose a reason for hiding this comment

EngHabu Jun 13, 2024

Choose a reason for hiding this comment

eapolinario commented Jun 4, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

eapolinario Jun 6, 2024 •

edited

Loading