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

Helm chart with dependencies #784

Closed
wants to merge 21 commits into from

Conversation

sbrunk
Copy link
Member

@sbrunk sbrunk commented Mar 1, 2021

This is based on the work of @rstanevich in #550 with the following changes:

  • Use external chart dependencies (for contour and spark-operator for now)
  • Bump image versions
  • Rebased against master

Draft PR so people can try it and as a basis for discussion about the chart.

  • I.e. should we also use dependencies for minio and postgres
  • Should we use subcharts for components
  • What things need to be more customizable
  • etc.

@kumare3
Copy link
Contributor

kumare3 commented Mar 3, 2021

Is this ready for review?

@sbrunk
Copy link
Member Author

sbrunk commented Mar 10, 2021

Not yet. There are still some issues with the spark operator I need to investigate. I'm starting to look into this again now.

@sbrunk sbrunk force-pushed the helm-chart-with-dependencies branch 2 times, most recently from ac6db54 to 5071f7e Compare March 21, 2021 12:28
@sbrunk sbrunk force-pushed the helm-chart-with-dependencies branch 3 times, most recently from 8047bb0 to 69e32fe Compare April 1, 2021 19:36
@sbrunk sbrunk marked this pull request as ready for review April 1, 2021 19:43

## Values

| Key | Type | Default | Description |
Copy link
Contributor

Choose a reason for hiding this comment

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

ohh man this is crazy, maintaining this will be a nightmare right? We need to auto-generate this

Copy link
Member Author

Choose a reason for hiding this comment

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

Completely agree. Looking at 6517fbb it seems it's already autogenerated

@rstanevich did you use https://github.com/norwoodj/helm-docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kumare3
Copy link
Contributor

kumare3 commented Apr 9, 2021

@sbrunk I had a question, so in Helm, is it possible to create 3 different configuration options - Flyte Low, Medium, High (performance) and we create precanned configuration. this would make configuring Flyte even easier.
so as a user one needs to provide

  1. A postgres database
  2. A Blob store
  3. A K8s cluster
  4. Optional: If you want eventing then choose the pubsub module
  5. Optional: Choose the authentication module
  6. Optional: choose Ingress
  7. And then choose the performance mode: Low, medium, High

I am also working on a sharded/distributed FlytePropeller mode, the problem is config. This can help simplify

@EngHabu @jeevb what do you guys think?

@@ -0,0 +1,50 @@
{{- if .Values.minio.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just use the minio helm chart as a dependency?
https://github.com/minio/charts (probably this is the old one)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that should work. I think I actually looked it up and didn't add it because the repo was archived. But minio is only for sandbox anyway right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup it's only sandbox- I am ok keeping it too

@@ -0,0 +1,45 @@
{{- if .Values.postgres.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

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

same for postgres? https://bitnami.com/stack/postgresql/helm
I guess this is an overkill?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah if deploying a postgres is only for sandbox it might be overkill.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

rstanevich and others added 17 commits April 12, 2021 13:09
Signed-off-by: Sören Brunk <[email protected]>
Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize changes:
- cd4a3d5 Trim sandbox (flyteorg#727)
- 9b59ac6 Update propeller version and config (flyteorg#580)

Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize change:
- 0e2dd72 Updating Spark config to force default credentials chain for AWS (flyteorg#674)

Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize change:
- 46864a5 Access GRPC services using projectcontour (flyteorg#686)

Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize change:
- 658aecd Enabling Event version 1 by default (flyteorg#689)

Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize change:
- fbe7e68 Improve getting started experience (flyteorg#694)

Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize change:
- 0844a44 Exponential error back-off in case of system errors (flyteorg#693)

Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize change:
- cd4a3d5 Trim sandbox (flyteorg#727)

Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize change:
- 72c5705 Update default RAM Request for tasks in sandbox (flyteorg#750)

Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize change:
- b13de21 Update Admin task resources (flyteorg#753)

Signed-off-by: Sören Brunk <[email protected]>
Based on the following kustomize changes:
- a14fd47 Update deployments with latest k8s library deps (flyteorg#826)
- f149af8 Add kubernetes dashboard to sandbox overlay (flyteorg#789)

Signed-off-by: Sören Brunk <[email protected]>
Use default Docker images in EKS Helm values.

Based on the following kustomize changes:
- 4e0eed6 Update EKS Ingress and Instructions (flyteorg#825)

Signed-off-by: Sören Brunk <[email protected]>
host: postgres
dbname: flyte_development

storage:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to change this
Have a parameter like
storage-kind: s3 | gcs | minio | custom

If custom then you provide the entire configuration, else the config is automatic?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is now done in 151446d (in the new internal branch).

namespace: {{ namespace }}
data:
config.yaml: |
storage:
Copy link
Contributor

Choose a reason for hiding this comment

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

samething, is it possible to derive this from the other config, even though this is actually a string?

kubernetes-enabled: true
kubernetes-template-uri: "http://localhost:30082/#/log/{{ .namespace }}/{{ .podName }}/pod?namespace={{ .namespace }}"

logger:
Copy link
Contributor

Choose a reason for hiding this comment

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

What about having a simple mode called debug?

Signed-off-by: Sören Brunk <[email protected]>
@sbrunk sbrunk force-pushed the helm-chart-with-dependencies branch from 125710b to ff67bdb Compare April 13, 2021 10:30
@sbrunk
Copy link
Member Author

sbrunk commented Apr 22, 2021

Superseded by #916

@sbrunk sbrunk closed this Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants