[GitHub] [airflow] jedcunningham commented on a change in pull request #16425: Allow configuration of pod resources in helm chart

[GitBox <gi...@apache.org>]

jedcunningham commented on a change in pull request #16425:
URL: https://github.com/apache/airflow/pull/16425#discussion_r651331183



##########
File path: chart/files/pod-template-file.kubernetes-helm-yaml
##########
@@ -54,6 +54,8 @@ spec:
       imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
       name: base
       ports: []
+      resources:
+{{ toYaml .Values.podTemplateResources | indent 8 }}

Review comment:
       ```suggestion
   {{ toYaml .Values.workers.resources | indent 8 }}
   ```
   
   For other things we've used the `workers` parameters in the pod_template_file instead of duplicating them. Is there a reason not to in this specific case?

##########
File path: chart/values.schema.json
##########
@@ -2798,6 +2875,24 @@
                     }
                 }
             }
+        },
+        "initContainerResources": {

Review comment:
       I think we should be more specific with this naming. Maybe `waitForMigrationsInitContainerResources`? Open to other options.
   
   That said, init containers may not need separate resources anyways? Can we just use the resources for the given component, e.g. `scheduler.resources` for the scheduler? Based on how [k8s handles pod resources](https://kubernetes.io/docs/concepts/workloads/pods/init-containers/#resources), once the init containers finish you'll just need those same resources for the main component anyways (meaning, if setting lower init resources 'fixes' a resourcing issue you're flying too close to sun already)?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org