Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting a ExecutionClusterLabel when triggering a Launchplan/Workflow/Task #4998

Merged
merged 12 commits into from
Mar 25, 2024

Conversation

RRap0so
Copy link
Contributor

@RRap0so RRap0so commented Mar 4, 2024

Why are the changes needed?

This is an alternative implementation to the #4956. Instead of trying to implement the clusterPool functionality we reuse the ClusterAssignment struct and Execution Label.

This was changes are kept to a minimal and would cover the needs of the linked RFC

Closes #5081

What changes were proposed in this pull request?

How was this patch tested?

This was tested with two additional unit tests.

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

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Mar 4, 2024
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 89.83051% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 59.00%. Comparing base (feb4777) to head (72d52ca).

Files Patch % Lines
...g/executioncluster/impl/random_cluster_selector.go 85.00% 2 Missing and 1 partial ⚠️
flyteadmin/pkg/manager/impl/execution_manager.go 90.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4998      +/-   ##
==========================================
+ Coverage   58.98%   59.00%   +0.02%     
==========================================
  Files         645      645              
  Lines       55648    55670      +22     
==========================================
+ Hits        32824    32850      +26     
+ Misses      20229    20224       -5     
- Partials     2595     2596       +1     
Flag Coverage Δ
unittests 59.00% <89.83%> (+0.02%) ⬆️

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.

@davidmirror-ops
Copy link
Contributor

cc @gdabisias

@RRap0so RRap0so changed the title [WIP] Execution Label in cluster assignment Execution Label in cluster assignment Mar 7, 2024
@RRap0so RRap0so changed the title Execution Label in cluster assignment Allow setting a ExecutionClusterLabel when triggering a Launchplan/Workflow/Task Mar 20, 2024
@wild-endeavor
Copy link
Contributor

Won't we also need to add the message to the incoming request? It'll need to start from there no?

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 21, 2024
@wild-endeavor
Copy link
Contributor

hey @RRap0so do you have a real multi-cluster setup available to help test this? It's a fair bit of work for us to set one up to test. We can fake it, but it'd be nice to have a real setup to test against. I pulled this pr, built a new sandbox image for it, and ran a local/normal test, and it works, but i'm obviously not testing any of the new functionality.

@RRap0so
Copy link
Contributor Author

RRap0so commented Mar 21, 2024

@wild-endeavor I think we might have something that we can use. I'll report back.

Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
@RRap0so
Copy link
Contributor Author

RRap0so commented Mar 24, 2024

Hi @wild-endeavor.

All done and tested. I've created executions via grpc and manage to reproduce everything perfectly:

  • if a label is found, goes through the proper cluster
  • if not found then default behaviours applies (in our case the execution would randomly hit one of the two clusters we have)

I've also added a Debug line to help out and done another comparison fix.

Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for testing this out end to end!

do you mind also updating https://github.com/flyteorg/flyte/blob/master/flyteadmin/pkg/executioncluster/impl/in_cluster.go#L25 to fail fast when a ExecutionTargetSpec.ExecutionClusterLabel is specified (like we do for TargetID)?

flyteadmin/pkg/executioncluster/execution_target.go Outdated Show resolved Hide resolved
RRap0so and others added 3 commits March 25, 2024 18:09
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
@RRap0so RRap0so requested a review from katrogan March 25, 2024 17:16
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 25, 2024
@wild-endeavor wild-endeavor merged commit 2260ca2 into flyteorg:master Mar 25, 2024
48 checks passed
yubofredwang pushed a commit to yubofredwang/flyte that referenced this pull request Mar 26, 2024
…rkflow/Task (flyteorg#4998)

Add ExecutionClusterLabel to the ExecutionSpec so users can override the label at kick off time.

Closes flyteorg#5081

Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
@RRap0so RRap0so deleted the execution_label_in_cluster branch March 26, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core feature] Allow setting a ExecutionClusterLabel when triggering a Launchplan/Workflow/Task
4 participants