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

Jump to bottom

Flyte Agent Helm Chart #3935

Merged
merged 22 commits into from
Sep 1, 2023
Merged

Flyte Agent Helm Chart #3935

merged 22 commits into from
Sep 1, 2023

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Aug 9, 2023 •
edited
Loading

Describe your changes

Add a flyte agent helm chart, so that people can easily to install flyte agent independently.

git clone git@github.com:flyteorg/flyte
git checkout agent-chart
helm install flyte-agent
helm install flyte-agent-dev . --set nameOverride=flyte-agent-dev  # Install another agent
  • Add a new helm chart for flyte agent
  • flyte-core / flyte / flyte-binary import flyte-agent

Check all the applicable boxes

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

Screenshots

image

Note to reviewers

@pingsutw
Add flyte agent helm chart
6cc60b6 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Add generated file
93e86df 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Merged master
fbb0760 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Update github action
5557cab 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
lint
71cd378 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
lint
9278896 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
lint
77913bd 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw pingsutw mentioned this pull request Aug 9, 2023
Closed
11 tasks
@pingsutw
update secret
e9ff03b 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
update value.yaml
f231b81 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
update value.yaml
6602957 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
update value.yaml
4919221 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
make helm
7296af7 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
disable agent in flyte-binary
b8eb4b8 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
helm dep update
56d80c9 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
helm dep update
7b6b387 
Signed-off-by: Kevin Su <pingsutw@apache.org>
jeevb
jeevb reviewed Aug 9, 2023
View reviewed changes
charts/flyte-agent/templates/agent/deployment.yaml Outdated
@@ -1,53 +1,55 @@
{{- if .Values.flyteagent.enabled }}
{{- if .Values.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still necessary? If the chart is being installed, the intent is clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would using a condition in Chart.yaml solve this problem?

Copy link
Member Author

@pingsutw pingsutw Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just realized we can use condition in helm, will update it, thanks!

jeevb
jeevb reviewed Aug 9, 2023
View reviewed changes
charts/flyte-agent/README.md Outdated
| additionalVolumes | list | `[]` | Appends additional volumes to the deployment spec. May include template values. |
| affinity | object | `{}` | affinity for flyteagent deployment |
| agentSecret.name | string | `""` | Specify name of K8s Secret. Leave it empty if you don't need this Secret |
| agentSecret.secretData | object | `{"data":{"username":"User"}}` | Specify your Secret (with sensitive data) or pseudo-manifest (without sensitive data). See https://github.com/godaddy/kubernetes-external-secrets |
Copy link
Contributor

@jeevb jeevb Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is the updated link: https://github.com/external-secrets/external-secrets. For my own understanding, how can external-secrets be used with this block?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, forgot to remove it

jeevb
jeevb reviewed Aug 9, 2023
View reviewed changes
charts/flyte-core/Chart.yaml
@@ -3,3 +3,8 @@ name: flyte-core
description: A Helm chart for Flyte core
type: application
version: v0.1.10 # VERSION
dependencies:
Copy link
Contributor

Choose a reason for hiding this comment

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

We can specify the conditional here as opposed to everywhere in the child chart:
https://helm.sh/docs/chart_best_practices/dependencies/#conditions-and-tags

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note how flyte-binary uses the flyteagent service:

flyte/charts/flyte-binary/templates/configmap.yaml

Line 69 in 809c657

defaultGrpcEndpoint: {{ include "flyte-binary.agent.name" . }}:{{ include "flyte-binary.agent.servicePort" . }}

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, updated it, thanks

jeevb
jeevb reviewed Aug 9, 2023
View reviewed changes
charts/flyte-agent/values.yaml
@@ -0,0 +1,77 @@
# ---------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Will there be value in consolidating the format of our values.yaml files so that users can start getting familiar. Right now, all charts are very different from each other.

@pingsutw
update chart
dae513c 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Merged master
2fc4ace 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Updated configmap
5f67a62 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Add agent dir
72d7e09 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@eapolinario eapolinario mentioned this pull request Aug 11, 2023
Merged
3 tasks
@pingsutw
merged master
feefa16 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
remove name from agent secret
371dd2f 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
make generate
b0a581d 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw pingsutw requested a review from jeevb August 28, 2023 18:24
@pingsutw pingsutw merged commit 44f5f42 into master Sep 1, 2023
12 checks passed
@pingsutw pingsutw deleted the agent-chart branch September 1, 2023 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeevb jeevb jeevb approved these changes

@flyte-bot flyte-bot Awaiting requested review from flyte-bot

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pingsutw @jeevb