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

Fix flyte-core helm charts for multi cluster configuration #3993

Merged
merged 5 commits into from
Sep 27, 2023
Merged

Fix flyte-core helm charts for multi cluster configuration #3993

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0cc1ef0
Fix cluster secrets access for flyte-core helm charts when used in mu…
gdabisias Aug 25, 2023
9a7c705
Merge branch 'flyteorg:master' into master
gdabisias Aug 28, 2023
a071bad
Fix init container secret
gdabisias Aug 29, 2023
0e5752e
Fix secret name
gdabisias Sep 18, 2023
58ad1d8
Generalise volume mounting points
gdabisias Sep 22, 2023
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
4 changes: 4 additions & 0 deletions charts/flyte-core/templates/admin/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ spec:
name: clusters-config-volume
- mountPath: /etc/secrets/
name: admin-secrets
{{- if gt (len .Values.configmap.clusters.labelClusterMap) 0 }}
- mountPath: /var/run/credentials
name: cluster-credentials
Copy link
Contributor

Choose a reason for hiding this comment

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

but this is a volumeMount right? doesn't that mean there needs to be a volume declared with the same name in the Pod?

Copy link
Contributor Author

@gdabisias gdabisias Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

no, because this is done in the guide as part of the multicluster setup (not ideal, but following what is currently there)
https://docs.flyte.org/en/latest/deployment/deployment/multicluster.html#id2

Copy link
Contributor

Choose a reason for hiding this comment

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

oh i see it's in additionalVolumes

Copy link
Contributor

Choose a reason for hiding this comment

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

but wait shouldn't that come through in the additional mounts?

  additionalVolumeMounts:
  - name: cluster-credentials
    mountPath: /var/run/credentials

which gets injected here: https://github.com/flyteorg/flyte/blob/a071bade39bdad80fff042c235a1c3c046a82a09/charts/flyte-core/templates/admin/deployment.yaml#L137C37-L137C59

Copy link
Contributor

Choose a reason for hiding this comment

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

@gdabisias I just found that if we change the process to, instead of creating a new secret, editing the existing flyte-admin-secrets and adding the data plane cluster token and cert there, the syncresources Pod works just fine.
Right, no surprises there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidmirror-ops as you mentioned in the other comment, we should have a separate secret

Copy link
Contributor Author

@gdabisias gdabisias Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@wild-endeavor no because here we are updating the sync-cluster-resources init container and not the admin one. For the admin one, we add the secret mounting point to the values-eks.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

should we reference Values.flyteadmin.additionalVolumeMounts also instead of directly adding the mountpath?

The issue is that this only works if the user specifies that string in the values file for the additional volumes. if they use a different string than "cluster-credentials" then this won't work right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it won't work. My idea was to fix things according to the guide, but we can also just add that to the values-eks.yaml.
Up to you, I don't mind. Either way, we should remove this secret creation and mounting completely and it should be part of the general chart, with the user only adding the secret value itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change but notice that we are mounting anything specified in the additional volumes section, so there might be also some other stuff (Still better than what we had before and I don't see why something mounted by the admin container should not be mounted by it's init container)

{{- end }}
{{- end }}
- name: generate-secrets
image: "{{ .Values.flyteadmin.image.repository }}:{{ .Values.flyteadmin.image.tag }}"
Expand Down
6 changes: 3 additions & 3 deletions charts/flyte-core/templates/clusterresourcesync/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ spec:
name: config-volume
{{- if gt (len .Values.configmap.clusters.labelClusterMap) 0 }}
- mountPath: /var/run/credentials
name: flyte-admin-secrets
name: cluster-secrets
{{- end }}
serviceAccountName: {{ .Values.cluster_resource_manager.service_account_name }}
{{- if .Values.cluster_resource_manager.resources }}
Expand All @@ -55,9 +55,9 @@ spec:
name: flyte-clusterresourcesync-config
name: config-volume
{{- if gt (len .Values.configmap.clusters.labelClusterMap) 0 }}
- name: flyte-admin-secrets
- name: cluster-secrets
secret:
secretName: flyte-admin-secrets
secretName: cluster-secrets
gdabisias marked this conversation as resolved.
Show resolved Hide resolved
{{- end }}
{{- if .Values.cluster_resource_manager.config.cluster_resources.standaloneDeployment }}
- name: auth
Expand Down
Loading