Add agent to helm chart and single binary #3454
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
pingsutw
changed the title
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
8 tasks
Signed-off-by: Kevin Su <[email protected]>
pingsutw
changed the title
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
pingsutw
changed the title
Signed-off-by: Kevin Su <[email protected]>
pmahindrakar-oss
previously approved these changes
Jun 9, 2023
charts/flyte-core/values.yaml
Outdated
| service: | ||
| annotations: | ||
| projectcontour.io/upstream-protocol.h2c: grpc | ||
| type: NodePort |
There was a problem hiding this comment.
Any reason we want this to be Nodeport and not ClusterIP. Since the flyteAgent would be running primarily within the same cluster as propeller
Comment on lines
37
to
87
| # Allow Event recording access | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - events | ||
| verbs: | ||
| - create | ||
| - update | ||
| - delete | ||
| - patch | ||
| # Allow Access All plugin objects | ||
| - apiGroups: | ||
| - '*' | ||
| resources: | ||
| - '*' | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - delete | ||
| - patch | ||
| # Allow Access to CRD | ||
| - apiGroups: | ||
| - apiextensions.k8s.io | ||
| resources: | ||
| - customresourcedefinitions | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - delete | ||
| - update | ||
| # Allow Access to all resources under flyte.lyft.com | ||
| - apiGroups: | ||
| - flyte.lyft.com | ||
| resources: | ||
| - flyteworkflows | ||
| - flyteworkflows/finalizers | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - delete | ||
| - patch | ||
| - post | ||
| - deletecollection |
There was a problem hiding this comment.
Why do we need all these permissions
Comment on lines
31
to
36
| resources: | ||
| - pods | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
And similarly these. The agent would primarily be calling into external api's so not sure if we need these unless the agent is calling these for some kube api lookup and update
| {{- end }} | ||
| ports: | ||
| - name: agent-grpc | ||
| port: 8000 |
There was a problem hiding this comment.
Can we make this configurable in values chart and also use in the deployment yaml
Comment on lines
90
to
103
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: {{ template "flyte.namespace" . -}}-{{- template "flyteagent.name" . }} | ||
| labels: {{ include "flyteagent.labels" . | nindent 4 }} | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: {{ template "flyte.namespace" . -}}-{{- template "flyteagent.name" . }} | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ template "flyteagent.name" . }} | ||
| namespace: {{ template "flyte.namespace" . }} | ||
| {{- end }} |
There was a problem hiding this comment.
Dont see a need for this entirely , Please let me know where do we want to use clusterwide role for agent
Comment on lines
73
to
76
| agent-service: | ||
| defaultGrpcEndpoint: flyteagent.flyte.svc.cluster.local:8000 | ||
| supportedTaskTypes: | ||
| - bigquery_query_job_task |
There was a problem hiding this comment.
Making this configurable through values files can help testing new plugins easily and similarly L62
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
|
@pmahindrakar-oss @eapolinario I've update the PR, and tested agent in the sandbox |
eapolinario
reviewed
Jun 9, 2023
Comment on lines
415
to
423
| 001-plugins.yaml: "plugins:\n logs:\n kubernetes-enabled: true\n kubernetes-template-uri: | ||
| http://localhost:30080/kubernetes-dashboard/#/log/{{.namespace }}/{{ .podName | ||
| }}/pod?namespace={{ .namespace }}\n cloudwatch-enabled: false\n stackdriver-enabled: | ||
| false\n k8s:\n co-pilot:\n image: \"cr.flyte.org/flyteorg/flytecopilot:v0.0.30\"\n | ||
| \ k8s-array:\n logs:\n config:\n kubernetes-enabled: true\n kubernetes-template-uri: | ||
| http://localhost:30080/kubernetes-dashboard/#/log/{{.namespace }}/{{ .podName | ||
| }}/pod?namespace={{ .namespace }}\n cloudwatch-enabled: false\n stackdriver-enabled: | ||
| false\n agent-service: \n defaultGrpcEndpoint: flyte-sandbox-http.flyte.svc.cluster.local:8000\n | ||
| \ supportedTaskTypes:\n - bigquery_query_job_task\n" |
There was a problem hiding this comment.
why do we need this change?
There was a problem hiding this comment.
It's auto-generated. I just fixed indentation errors.
| @@ -81,16 +81,20 @@ data: | |||
| propeller: | |||
| disableWebhook: false | |||
| disabled: false | |||
| 001-plugins.yaml: | | |||
| enabled_plugins.yaml: | | |||
There was a problem hiding this comment.
the numbering scheme for files is a convention used by k8s to enforce execution order. Does order matter in this case?
There was a problem hiding this comment.
moved enabled_plugins to 001-plugins.yaml
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
pmahindrakar-oss
previously approved these changes
Jun 10, 2023
Signed-off-by: Kevin Su <[email protected]>
3 tasks
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracking issue
#3282
Describe your changes
Check all the applicable boxes
Screenshots
Note to reviewers