[flyteagent] Default Service Config Using Round Robin Mechanism #6179

Future-Outlier · 2025-01-17T08:52:47Z

Tracking issue

Why are the changes needed?

This PR adds a default gRPC service configuration to the agent plugin that enables round-robin load balancing.
This ensures consistent and fair distribution of requests across multiple agent instances.

What changes were proposed in this pull request?

Added DefaultServiceConfig in the default agent configuration with round-robin load balancing policy
Default value set to: {"loadBalancingConfig": [{"round_robin":{}}]}
change agent endpoint from dns:///flyteagent.flyte.svc.cluster.local:8000 to k8s://flyteagent.flyte:8000
("Resolver"."Service Name"."Service Namespace"."Port")

https://github.com/flyteorg/flyte/pull/6179/files#diff-98835a3d321cfb860916a188ca66ea98efe6c9f3a8314ea89ec2cfdf8ad50066R42-R47

How was this patch tested?

unit test and sandbox.

from flytekit import task, workflow
from flytekit.sensor.file_sensor import FileSensor

sensor = FileSensor(name="test_file_sensor")


@workflow()
def wf():
    sensor(path="s3://my-s3-bucket/a")

if __name__ == "__main__":
    import os
    from flytekit.clis.sdk_in_container import pyflyte
    from click.testing import CliRunner

    runner = CliRunner()
    path = os.path.realpath(__file__)
    
    for _ in range(100):  # Loop to trigger the workflow 100 times
        result = runner.invoke(pyflyte.main, ["run", "--remote", path, "wf"])
        print(result.output)

Labels

Please add one or more of the following labels to categorize your PR:

added: For new features.
changed: For changes in existing functionality.
deprecated: For soon-to-be-removed features.
removed: For features being removed.
fixed: For any bug fixed.
security: In case of vulnerabilities

This is important to improve the readability of release notes.

Setup process

agent-service:
defaultAgent:
  defaultTimeout: 10s
  endpoint: k8s://flyteagent.flyte:8000
  insecure: true
  timeouts:
    GetTask: 10s

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

Summary by Bito

Implementation of round-robin load balancing for Flyte agent service, transitioning from DNS-based ('dns:///') to Kubernetes native ('k8s://') service discovery. Updates include DefaultServiceConfig field implementation, endpoint configuration modifications, and security-related changes to shared secrets and checksums in sandbox and development manifests. Comprehensive test coverage included to validate configuration values.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 2

Signed-off-by: Future-Outlier <[email protected]>

flyte-bot · 2025-01-17T08:53:10Z

Code Review Agent Run #141009

Actionable Suggestions - 1

flyteplugins/go/tasks/plugins/webapi/agent/config.go - 1
- Consider making service config configurable · Line 46-46

Review Details

Files reviewed - 2 · Commit Range: a433b33..a433b33
- flyteplugins/go/tasks/plugins/webapi/agent/config.go
- flyteplugins/go/tasks/plugins/webapi/agent/config_test.go
Files skipped - 0
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-17T08:55:13Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Agent Load Balancing Enhancement	- `config.go` - Added DefaultServiceConfig for round-robin load balancing - `config_test.go` - Added unit tests for default agent configuration - `values.yaml` - Updated agent endpoint to use k8s service discovery - `values.yaml` - Updated agent endpoint to use k8s service discovery
Other Improvements - Security and Configuration Updates	- `complete-agent.yaml` - Updated shared secrets and configuration checksums - `complete.yaml` - Updated shared secrets and configuration checksums - `dev.yaml` - Updated shared secrets and configuration checksums

flyte-bot · 2025-01-17T08:55:15Z

flyteplugins/go/tasks/plugins/webapi/agent/config.go

+			Endpoint:             "",
+			Insecure:             true,
+			DefaultTimeout:       config.Duration{Duration: 10 * time.Second},
+			DefaultServiceConfig: `{"loadBalancingConfig": [{"round_robin":{}}]}`,


Consider making service config configurable

Consider making the default service config more configurable by moving it to a separate constant or configuration option rather than hardcoding the round robin load balancing config directly in the default config struct.

Code suggestion

Check the AI-generated fix before applying

const ( + DefaultServiceConfig = `{"loadBalancingConfig": [{"round_robin":{}}]}` ) Endpoint: "", Insecure: true, DefaultTimeout: config.Duration{Duration: 10 * time.Second}, - DefaultServiceConfig: `{"loadBalancingConfig": [{"round_robin":{}}]}`, + DefaultServiceConfig: DefaultServiceConfig,

Code Review Run #141009

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

codecov · 2025-01-17T08:58:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.06%. Comparing base (ab463c3) to head (498f6f3).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6179   +/-   ##
=======================================
  Coverage   37.06%   37.06%           
=======================================
  Files        1318     1318           
  Lines      132644   132644           
=======================================
+ Hits        49164    49166    +2     
+ Misses      79230    79229    -1     
+ Partials     4250     4249    -1

Flag	Coverage Δ
unittests-datacatalog	`51.58% <ø> (ø)`
unittests-flyteadmin	`54.33% <ø> (ø)`
unittests-flytecopilot	`30.99% <ø> (ø)`
unittests-flytectl	`62.29% <ø> (ø)`
unittests-flyteidl	`7.23% <ø> (ø)`
unittests-flyteplugins	`53.87% <ø> (+0.01%)`	⬆️
unittests-flytepropeller	`42.70% <ø> (ø)`
unittests-flytestdlib	`55.27% <ø> (-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.

Signed-off-by: Future-Outlier <[email protected]>

flyte-bot · 2025-01-18T04:35:49Z

Code Review Agent Run #f18921

Actionable Suggestions - 0

Review Details

Files reviewed - 5 · Commit Range: a433b33..0d2539f
- charts/flyte-binary/values.yaml
- charts/flyte-core/values.yaml
- docker/sandbox-bundled/manifests/complete-agent.yaml
- docker/sandbox-bundled/manifests/complete.yaml
- docker/sandbox-bundled/manifests/dev.yaml
Files skipped - 3
- charts/flyte-binary/README.md - Reason: Filter setting
- charts/flyte-core/README.md - Reason: Filter setting
- docs/user_guide/flyte_agents/developing_agents.md - Reason: Filter setting
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

Signed-off-by: Future-Outlier <[email protected]>

flyte-bot · 2025-01-20T13:59:52Z

Code Review Agent Run #22c6ef

Actionable Suggestions - 1

charts/flyte-core/values.yaml - 1
- Consider making endpoint URL configurable · Line 291-291

Review Details

Files reviewed - 5 · Commit Range: 0d2539f..498f6f3
- charts/flyte-binary/values.yaml
- charts/flyte-core/values.yaml
- docker/sandbox-bundled/manifests/complete-agent.yaml
- docker/sandbox-bundled/manifests/complete.yaml
- docker/sandbox-bundled/manifests/dev.yaml
Files skipped - 3
- charts/flyte-binary/README.md - Reason: Filter setting
- charts/flyte-core/README.md - Reason: Filter setting
- docs/user_guide/flyte_agents/developing_agents.md - Reason: Filter setting
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-20T14:03:06Z

charts/flyte-core/values.yaml

@@ -288,7 +288,7 @@ flyteagent:
        # -- The default agent service to use for plugin tasks.
        defaultAgent:
          # -- The agent service endpoint propeller should connect to.
-          endpoint: "dns:///flyteagent.flyte.svc.cluster.local:8000"
+          endpoint: "k8s://flyteagent.flyte:8000"


Consider making endpoint URL configurable

Consider if the endpoint URL k8s://flyteagent.flyte:8000 should be configurable through a Helm value to support different namespace deployments.

Code suggestion

Check the AI-generated fix before applying

Suggested change

endpoint: "k8s://flyteagent.flyte:8000"

endpoint: "k8s://{{ .Values.flyteagent.serviceName }}.{{ .Values.flyteagent.namespace | default .Release.Namespace }}:{{ .Values.flyteagent.port | default "8000" }}"

Code Review Run #22c6ef

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

Add FlyteAgent Default Service Config Using Round Robin Mechanism

a433b33

Signed-off-by: Future-Outlier <[email protected]>

flyte-bot reviewed Jan 17, 2025

View reviewed changes

update dns endpoint to k8s endpoint

0d2539f

Signed-off-by: Future-Outlier <[email protected]>

Future-Outlier requested a review from ppiegaze as a code owner January 18, 2025 03:31

update

498f6f3

Signed-off-by: Future-Outlier <[email protected]>

flyte-bot reviewed Jan 20, 2025

View reviewed changes

pingsutw approved these changes Jan 21, 2025

View reviewed changes

Future-Outlier merged commit 29ad645 into master Jan 22, 2025
58 checks passed

Future-Outlier deleted the agent-default-load-balancer-config branch January 22, 2025 00:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flyteagent] Default Service Config Using Round Robin Mechanism #6179

[flyteagent] Default Service Config Using Round Robin Mechanism #6179

Future-Outlier commented Jan 17, 2025 •

edited

Loading

flyte-bot commented Jan 17, 2025 •

edited

Loading

Code Review Agent Run #141009

flyte-bot commented Jan 17, 2025 •

edited

Loading

Changelist by Bito

flyte-bot Jan 17, 2025

codecov bot commented Jan 17, 2025 •

edited

Loading

flyte-bot commented Jan 18, 2025 •

edited

Loading

Code Review Agent Run #f18921

flyte-bot commented Jan 20, 2025 •

edited

Loading

Code Review Agent Run #22c6ef

flyte-bot Jan 20, 2025

	endpoint: "k8s://flyteagent.flyte:8000"
	endpoint: "k8s://{{ .Values.flyteagent.serviceName }}.{{ .Values.flyteagent.namespace \| default .Release.Namespace }}:{{ .Values.flyteagent.port \| default "8000" }}"

[flyteagent] Default Service Config Using Round Robin Mechanism #6179

[flyteagent] Default Service Config Using Round Robin Mechanism #6179

Conversation

Future-Outlier commented Jan 17, 2025 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Labels

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

Summary by Bito

flyte-bot commented Jan 17, 2025 • edited Loading

Code Review Agent Run #141009

flyte-bot commented Jan 17, 2025 • edited Loading

Changelist by Bito

flyte-bot Jan 17, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 17, 2025 • edited Loading

Codecov Report

flyte-bot commented Jan 18, 2025 • edited Loading

Code Review Agent Run #f18921

flyte-bot commented Jan 20, 2025 • edited Loading

Code Review Agent Run #22c6ef

flyte-bot Jan 20, 2025

Choose a reason for hiding this comment

Future-Outlier commented Jan 17, 2025 •

edited

Loading

flyte-bot commented Jan 17, 2025 •

edited

Loading

flyte-bot commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading

flyte-bot commented Jan 18, 2025 •

edited

Loading

flyte-bot commented Jan 20, 2025 •

edited

Loading