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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Jan 22, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a433b33
Add FlyteAgent Default Service Config Using Round Robin Mechanism
Future-Outlier Jan 17, 2025
0d2539f
update dns endpoint to k8s endpoint
Future-Outlier Jan 18, 2025
498f6f3
update
Future-Outlier Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/flyte-binary/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Chart for basic single Flyte executable deployment
| commonAnnotations | object | `{}` | |
| commonLabels | object | `{}` | |
| configuration.agentService.defaultAgent.defaultTimeout | string | `"10s"` | |
| configuration.agentService.defaultAgent.endpoint | string | `"dns:///flyteagent.flyte.svc.cluster.local:8000"` | |
| configuration.agentService.defaultAgent.endpoint | string | `"k8s://flyteagent.flyte:8000"` | |
| configuration.agentService.defaultAgent.insecure | bool | `true` | |
| configuration.agentService.defaultAgent.timeouts.GetTask | string | `"10s"` | |
| configuration.annotations | object | `{}` | |
Expand Down
2 changes: 1 addition & 1 deletion charts/flyte-binary/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ configuration:
# agentService Flyte Agent configuration
agentService:
defaultAgent:
endpoint: "dns:///flyteagent.flyte.svc.cluster.local:8000"
endpoint: "k8s://flyteagent.flyte:8000"
insecure: true
timeouts:
GetTask: 10s
Expand Down
6 changes: 3 additions & 3 deletions charts/flyte-core/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,9 @@ helm install gateway bitnami/contour -n flyte
| flyteadmin.serviceMonitor.scrapeTimeout | string | `"30s"` | Sets the timeout after which request to scrape metrics will time out |
| flyteadmin.tolerations | list | `[]` | tolerations for Flyteadmin deployment |
| flyteagent.enabled | bool | `false` | |
| flyteagent.plugin_config.plugins.agent-service | object | `{"defaultAgent":{"endpoint":"dns:///flyteagent.flyte.svc.cluster.local:8000","insecure":true},"supportedTaskTypes":[]}` | Agent service configuration for propeller. |
| flyteagent.plugin_config.plugins.agent-service.defaultAgent | object | `{"endpoint":"dns:///flyteagent.flyte.svc.cluster.local:8000","insecure":true}` | The default agent service to use for plugin tasks. |
| flyteagent.plugin_config.plugins.agent-service.defaultAgent.endpoint | string | `"dns:///flyteagent.flyte.svc.cluster.local:8000"` | The agent service endpoint propeller should connect to. |
| flyteagent.plugin_config.plugins.agent-service | object | `{"defaultAgent":{"endpoint":"k8s://flyteagent.flyte:8000","insecure":true},"supportedTaskTypes":[]}` | Agent service configuration for propeller. |
| flyteagent.plugin_config.plugins.agent-service.defaultAgent | object | `{"endpoint":"k8s://flyteagent.flyte:8000","insecure":true}` | The default agent service to use for plugin tasks. |
| flyteagent.plugin_config.plugins.agent-service.defaultAgent.endpoint | string | `"k8s://flyteagent.flyte:8000"` | The agent service endpoint propeller should connect to. |
| flyteagent.plugin_config.plugins.agent-service.defaultAgent.insecure | bool | `true` | Whether the connection from propeller to the agent service should use TLS. |
| flyteagent.plugin_config.plugins.agent-service.supportedTaskTypes | list | `[]` | The task types supported by the default agent. As of #5460 these are discovered automatically and don't need to be configured. |
| flyteagent.podLabels | object | `{}` | Labels for flyteagent pods |
Expand Down
2 changes: 1 addition & 1 deletion charts/flyte-core/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

# -- Whether the connection from propeller to the agent service should use TLS.
insecure: true
# -- The task types supported by the default agent. As of #5460 these are discovered automatically and don't
Expand Down
8 changes: 4 additions & 4 deletions docker/sandbox-bundled/manifests/complete-agent.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ data:
agent-service:
defaultAgent:
defaultTimeout: 10s
endpoint: dns:///flyteagent.flyte.svc.cluster.local:8000
endpoint: k8s://flyteagent.flyte:8000
insecure: true
timeouts:
GetTask: 10s
Expand Down Expand Up @@ -823,7 +823,7 @@ type: Opaque
---
apiVersion: v1
data:
haSharedSecret: UnZJZHEzUExzbkJsOW1wYw==
haSharedSecret: bjlDbkZsVU1UVmpLdGU4TQ==
proxyPassword: ""
proxyUsername: ""
kind: Secret
Expand Down Expand Up @@ -1254,7 +1254,7 @@ spec:
metadata:
annotations:
checksum/cluster-resource-templates: 6fd9b172465e3089fcc59f738b92b8dc4d8939360c19de8ee65f68b0e7422035
checksum/configuration: 5a537c05dbd27a7f2884eb78f4e762205c3bcc3248ab9e509ab7074c7e5f953d
checksum/configuration: 7841a55b7d0bd6a6d44f37ccf05297fb7c3338c1ebd9c2608d499e4f8c817383
checksum/configuration-secret: 09216ffaa3d29e14f88b1f30af580d02a2a5e014de4d750b7f275cc07ed4e914
labels:
app.kubernetes.io/component: flyte-binary
Expand Down Expand Up @@ -1420,7 +1420,7 @@ spec:
metadata:
annotations:
checksum/config: 8f50e768255a87f078ba8b9879a0c174c3e045ffb46ac8723d2eedbe293c8d81
checksum/secret: ce172103045f4215e361b4c109776a78fe06660a4ade01c7351ea07212e7cfb9
checksum/secret: fd8767b33da59722977568a2a3f4cee7ef850b02f579d8d5e381c038d5a20d42
labels:
app: docker-registry
release: flyte-sandbox
Expand Down
4 changes: 2 additions & 2 deletions docker/sandbox-bundled/manifests/complete.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ type: Opaque
---
apiVersion: v1
data:
haSharedSecret: dDFiem04NjFzb29ZWHFtNA==
haSharedSecret: ZzdHam5sWUQ2RGlIVHBOSw==
proxyPassword: ""
proxyUsername: ""
kind: Secret
Expand Down Expand Up @@ -1369,7 +1369,7 @@ spec:
metadata:
annotations:
checksum/config: 8f50e768255a87f078ba8b9879a0c174c3e045ffb46ac8723d2eedbe293c8d81
checksum/secret: 529d34a9c4d3c82b9eec5028fcc30f26e923fa77a57eb29c4705d28c85355963
checksum/secret: 3ce2fdef0e59a7eba350423dd49173ed960af107bc4c0873e80ea1cf5895e160
labels:
app: docker-registry
release: flyte-sandbox
Expand Down
4 changes: 2 additions & 2 deletions docker/sandbox-bundled/manifests/dev.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ metadata:
---
apiVersion: v1
data:
haSharedSecret: Y1V1RU03eGVhUDFFc1pSdQ==
haSharedSecret: ckNscjBXTmJMdmF6QXJ5aw==
proxyPassword: ""
proxyUsername: ""
kind: Secret
Expand Down Expand Up @@ -934,7 +934,7 @@ spec:
metadata:
annotations:
checksum/config: 8f50e768255a87f078ba8b9879a0c174c3e045ffb46ac8723d2eedbe293c8d81
checksum/secret: 66507f448be8010226a1ad2c741fb2866ef4372b68e61287c7500b47fae05572
checksum/secret: ee0ad692a622e9665348e44e2168eb06e24ac7683fe357f9ca92ea77283fa4d7
labels:
app: docker-registry
release: flyte-sandbox
Expand Down
2 changes: 1 addition & 1 deletion docs/user_guide/flyte_agents/developing_agents.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ plugins:
agent-service:
# By default, all requests will be sent to the default agent.
defaultAgent:
endpoint: "dns:///flyteagent.flyte.svc.cluster.local:8000"
endpoint: "k8s://flyteagent.flyte:8000"
insecure: true
timeouts:
# CreateTask, GetTask and DeleteTask are for async agents.
Expand Down
7 changes: 4 additions & 3 deletions flyteplugins/go/tasks/plugins/webapi/agent/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,10 @@ var (
},
},
DefaultAgent: Deployment{
Endpoint: "",
Insecure: true,
DefaultTimeout: config.Duration{Duration: 10 * time.Second},
Endpoint: "",
Insecure: true,
DefaultTimeout: config.Duration{Duration: 10 * time.Second},
DefaultServiceConfig: `{"loadBalancingConfig": [{"round_robin":{}}]}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

},
// AsyncPlugin should be registered to at least one task type.
// Reference: https://github.com/flyteorg/flyte/blob/master/flyteplugins/go/tasks/pluginmachinery/registry.go#L27
Expand Down
20 changes: 20 additions & 0 deletions flyteplugins/go/tasks/plugins/webapi/agent/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,23 @@ func TestGetAndSetConfig(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, &cfg, GetConfig())
}

func TestDefaultAgentConfig(t *testing.T) {
cfg := defaultConfig

assert.Equal(t, "", cfg.DefaultAgent.Endpoint)
assert.True(t, cfg.DefaultAgent.Insecure)
assert.Equal(t, 10*time.Second, cfg.DefaultAgent.DefaultTimeout.Duration)
assert.Equal(t, `{"loadBalancingConfig": [{"round_robin":{}}]}`, cfg.DefaultAgent.DefaultServiceConfig)

assert.Empty(t, cfg.DefaultAgent.Timeouts)

expectedTaskTypes := []string{"task_type_1", "task_type_2"}
assert.Equal(t, expectedTaskTypes, cfg.SupportedTaskTypes)

assert.Empty(t, cfg.AgentDeployments)

assert.Empty(t, cfg.AgentForTaskTypes)

assert.Equal(t, 10*time.Second, cfg.PollInterval.Duration)
}
Loading