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

add values to specify resources on init-containers #684

Conversation

fogelman
Copy link

@fogelman fogelman commented Jan 3, 2023

What issues does your PR fix?

What does your PR do?

Allows users to specify resources for initContainers.

Checklist

For all Pull Requests

For releasing ONLY

@fogelman fogelman force-pushed the feature/add-init-container-resources branch from c9d99fc to 3a057b4 Compare January 3, 2023 13:18
@fogelman fogelman marked this pull request as ready for review January 3, 2023 13:26
Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

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

@fogelman This PR does not work as it currently stands (see comments from the review).

But either way, I think we should use a different approach, and have global resource values for each init-container type. This is because it does not make sense to give the worker/scheduler resources to the install_pip_packages, wait_for_db_migrations, or check_db init-containers, as they very likely do not need 2 CPU / 4GB RAM, for example.

These values might look like:

airflow:
  
  ## << put these configs JUST BEFORE the `airflow.localSettings` in the default values.yaml >>

  ## configs for the airflow init-containers
  ##
  initContainers:
    
    ## configs for the `check-db` init-container
    checkDb:
      
      ## resource requests/limits for the `check-db` init-container
      ## - spec for ResourceRequirements:
      ##   https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#resourcerequirements-v1-core
      ##
      resources: {}

    ## configs for the `wait-for-db-migrations` init-container
    waitForDbMigrations:
      
      ## resource requests/limits for the `wait-for-db-migrations` init-container
      ## - spec for ResourceRequirements:
      ##   https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#resourcerequirements-v1-core
      ##
      resources: {}

    ## configs for the `install-pip-packages` init-container
    installPipPackages:

      ## resource requests/limits for the `install-pip-packages` init-container
      ## - spec for ResourceRequirements:
      ##   https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#resourcerequirements-v1-core
      ##
      resources: {}

This also lets us add values to disable these init-containers in future, if people like.

@@ -60,6 +60,8 @@ EXAMPLE USAGE: {{ include "airflow.init_container.check_db" (dict "Release" .Rel
{{- define "airflow.init_container.check_db" }}
- name: check-db
{{- include "airflow.image" . | indent 2 }}
resources:
{{- toYaml .resources | nindent 4 }}
Copy link
Member

Choose a reason for hiding this comment

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

The value .resources will not be defined, you must also change every time the "airflow.init_container.check_db" template is called to include the appropriate resources.

For example, in the scheduler this might look like:

# top of file
{{- $resources := .Values.scheduler.resources }}

# in the init container section
{{- include "airflow.init_container.check_db" (dict "Release" .Release "Values" .Values "volumeMounts" $volumeMounts "resources" $resources) | indent 8 }}

@thesuperzapper thesuperzapper changed the title Add resources on init_container section add values to specify resources on init-containers Feb 7, 2023
@thesuperzapper thesuperzapper added this to the airflow-8.7.0 milestone Feb 7, 2023
@thesuperzapper
Copy link
Member

@fogelman thanks for the PR, but I am closing in favor of #855

@thesuperzapper thesuperzapper removed this from the airflow-8.9.0 milestone Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

add values to specify resources on init-containers
2 participants