Improve flyte-core helm chart #5362
Conversation
Fixed flyte-core helm chart where ServiceAccounts were missing and some naming inconsistencies Signed-off-by: mvaal <[email protected]>
Signed-off-by: mvaal <[email protected]>
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
| @@ -55,7 +55,7 @@ spec: | |||
| - mountPath: /var/run/credentials | |||
| name: cluster-secrets | |||
| {{- end }} | |||
| serviceAccountName: {{ .Values.cluster_resource_manager.service_account_name }} | |||
| serviceAccountName: {{ .Values.cluster_resource_manager.serviceAccount.create | ternary (include "flyteclusterresourcesync.name" .) .Values.cluster_resource_manager.service_account_name }} | |||
There was a problem hiding this comment.
Attempted to keep backwards compatibility here.
| @@ -2,7 +2,7 @@ | |||
| apiVersion: apps/v1 | |||
| kind: Deployment | |||
| metadata: | |||
| name: syncresources | |||
| name: {{ template "flyteclusterresourcesync.name" . }} | |||
There was a problem hiding this comment.
This will technically recreate the deployment when it renames, not sure if that is an issue.
There was a problem hiding this comment.
does this re-create the deployment? or does this create a new deployment?
There was a problem hiding this comment.
new, it will tear down the old deployment and make a new one. it renames this to flyteclusterresourcesync
https://github.com/flyteorg/flyte/blob/master/charts/flyte-core/templates/_helpers.tpl#L62
| @@ -171,7 +171,7 @@ flytescheduler: | |||
| # -- Annotations for Flytescheduler pods | |||
| podAnnotations: {} | |||
| # -- Additional Flytescheduler container environment variables | |||
| podEnv: {} | |||
| podEnv: [] | |||
There was a problem hiding this comment.
These are mapped to arrays, so it gives a warning when you set them in the values.yaml.
There was a problem hiding this comment.
Ah thanks! 👍
That's my mistake and I probably missed this when I added it because we're setting podEnv values.
Signed-off-by: mvaal <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5362 +/- ##
==========================================
+ Coverage 60.97% 60.99% +0.02%
==========================================
Files 793 793
Lines 51331 51346 +15
==========================================
+ Hits 31299 31319 +20
+ Misses 17147 17141 -6
- Partials 2885 2886 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: mvaal <[email protected]>
|
sorry it's taken a long time to get to this @mvaalexp - would you mind taking one more look and regenerating the generated files to fix the conflicts please? Only flyte console and the cluster resource controller are getting new service accounts correct? The others already have them? Also the cluster controller deployment will get recreated? Or will there be a duplicate one? If you think it would be helpful to the community, could you also write a tiny blurb please that we can include on the release notes when this goes out? Just so there are no surprises for anyone using this helm chart. Would be most helpful. |
Signed-off-by: Marcus Vaal <[email protected]>
|
No problem. I will take a look tomorrow and see what I am missing and write the blurb. As I mentioned in the comment it will be new as a rename is basically a delete and create (as k8s can't rename a resource as far as I know). it renames this to flyteclusterresourcesync which is what was defined in the helpers file. If we can't afford to risk a rename here, we can probably work around it with a ternary or a new helper function, but it seems reasonable to keep it consistent if its not going to be a problem. https://github.com/flyteorg/flyte/blob/master/charts/flyte-core/templates/_helpers.tpl#L62 |
Since clustersync is running flyteadmin, we want to give it the same permissions as flyteadmin SA (since that is the one we are replacing) Signed-off-by: mvaal <[email protected]>
📝 Description
📝 User Notes
|
Fixed flyte-core helm chart where ServiceAccounts were missing, incorrect typing, and some naming inconsistencies
Tracking issue
Closes #5361
Why are the changes needed?
Removes error messages in when running the chart
Adds missing ServiceAccounts to services.
In our production clusters, we have governance that requires all Deployments must have an associated SA, this is currently preventing us from using the chart.
Adds the ability to override the service names.
Changed FlyteClusterResourceSync Deployment name
What changes were proposed in this pull request?
Helper methods refactored to allow
nameOverride.Service Accounts added and applied to Deployments.
values.yaml default values updated to array from map as that is what is expected
How was this patch tested?
Hand tested, I didn't see unit tests for the helm chart.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link