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

allow env support for airflow loggroomer sidecar #1517

Open
wants to merge 2 commits into
base: helm-chart/v1.13.1
Choose a base branch
from
Open

allow env support for airflow loggroomer sidecar #1517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d93f0e
allow env support for airflow loggroomer sidecar
pgvishnuram Jan 23, 2025
3c13f46
add custom env vars test case
pgvishnuram Jan 23, 2025
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
5 changes: 4 additions & 1 deletion chart/templates/dag-processor/dag-processor-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,12 +206,15 @@ spec:
{{- if .Values.dagProcessor.logGroomerSidecar.args }}
args: {{- tpl (toYaml .Values.dagProcessor.logGroomerSidecar.args) . | nindent 12 }}
{{- end }}
{{- if .Values.dagProcessor.logGroomerSidecar.retentionDays }}
env:
{{- if .Values.dagProcessor.logGroomerSidecar.retentionDays }}
- name: AIRFLOW__LOG_RETENTION_DAYS
value: "{{ .Values.dagProcessor.logGroomerSidecar.retentionDays }}"
{{- end }}
- name: AIRFLOW_HOME
value: "{{ .Values.airflowHome }}"
Comment on lines 209 to 215
Copy link
Member

Choose a reason for hiding this comment

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

This used to be that if retentionDays was not set, AIRFLOW_HOME would not be set. The new behavior always sets AIRFLOW_HOME. is that intentional? It seems to me like the new behavior would be correct, but it is a change from the old behavior. I think we should get an opinion from somebody on the airflow team.

Copy link

Choose a reason for hiding this comment

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

seems like it was a bug

Copy link
Member

Choose a reason for hiding this comment

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

@dstandish When we submit this as a PR to apache/airflow, will they ask for any changes, or do you think this will be fine as it is?

Copy link
Member

Choose a reason for hiding this comment

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

PR is here btw: apache#46003

Copy link
Member

Choose a reason for hiding this comment

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

There is also this pr to add env to these containers: apache#45483

Different approaches, but I think this one is better.

Not sure why this and the OSS one are different though. I agree - not setting home seems like a bug.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I just want to make sure we're not diverging from upstream airflow here.

{{- if .Values.dagProcessor.logGroomerSidecar.env }}
{{- tpl (toYaml .Values.dagProcessor.logGroomerSidecar.env) $ | nindent 12 }}
{{- end }}
volumeMounts:
- name: logs
Expand Down
5 changes: 4 additions & 1 deletion chart/templates/scheduler/scheduler-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,12 +261,15 @@ spec:
{{- if .Values.scheduler.logGroomerSidecar.args }}
args: {{- tpl (toYaml .Values.scheduler.logGroomerSidecar.args) . | nindent 12 }}
{{- end }}
{{- if .Values.scheduler.logGroomerSidecar.retentionDays }}
env:
{{- if .Values.scheduler.logGroomerSidecar.retentionDays }}
- name: AIRFLOW__LOG_RETENTION_DAYS
value: "{{ .Values.scheduler.logGroomerSidecar.retentionDays }}"
{{- end }}
- name: AIRFLOW_HOME
pgvishnuram marked this conversation as resolved.
Show resolved Hide resolved
value: "{{ .Values.airflowHome }}"
{{- if .Values.scheduler.logGroomerSidecar.env }}
{{- tpl (toYaml .Values.scheduler.logGroomerSidecar.env) $ | nindent 12 }}
{{- end }}
volumeMounts:
- name: logs
Expand Down
5 changes: 4 additions & 1 deletion chart/templates/triggerer/triggerer-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,12 +234,15 @@ spec:
{{- if .Values.triggerer.logGroomerSidecar.args }}
args: {{- tpl (toYaml .Values.triggerer.logGroomerSidecar.args) . | nindent 12 }}
{{- end }}
{{- if .Values.triggerer.logGroomerSidecar.retentionDays }}
env:
{{- if .Values.triggerer.logGroomerSidecar.retentionDays }}
- name: AIRFLOW__LOG_RETENTION_DAYS
value: "{{ .Values.triggerer.logGroomerSidecar.retentionDays }}"
{{- end }}
pgvishnuram marked this conversation as resolved.
Show resolved Hide resolved
- name: AIRFLOW_HOME
value: "{{ .Values.airflowHome }}"
{{- if .Values.triggerer.logGroomerSidecar.env }}
{{- tpl (toYaml .Values.triggerer.logGroomerSidecar.env) $ | nindent 12 }}
{{- end }}
volumeMounts:
- name: logs
Expand Down
5 changes: 4 additions & 1 deletion chart/templates/workers/worker-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,12 +316,15 @@ spec:
{{- if .Values.workers.logGroomerSidecar.args }}
args: {{ tpl (toYaml .Values.workers.logGroomerSidecar.args) . | nindent 12 }}
{{- end }}
{{- if .Values.workers.logGroomerSidecar.retentionDays }}
env:
{{- if .Values.workers.logGroomerSidecar.retentionDays }}
- name: AIRFLOW__LOG_RETENTION_DAYS
value: "{{ .Values.workers.logGroomerSidecar.retentionDays }}"
{{- end }}
pgvishnuram marked this conversation as resolved.
Show resolved Hide resolved
- name: AIRFLOW_HOME
value: "{{ .Values.airflowHome }}"
{{- if .Values.workers.logGroomerSidecar.env }}
{{- tpl (toYaml .Values.workers.logGroomerSidecar.env) $ | nindent 12 }}
{{- end }}
resources: {{- toYaml .Values.workers.logGroomerSidecar.resources | nindent 12 }}
volumeMounts:
Expand Down
10 changes: 10 additions & 0 deletions chart/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10486,6 +10486,16 @@
"/clean-logs"
]
},
"env": {
"description": "Add additional env vars to log groomer sidecar container.",
"items": {
"$ref": "#/definitions/io.k8s.api.core.v1.EnvVar"
},
"type": "array",
"default": [],
"x-kubernetes-patch-merge-key": "name",
"x-kubernetes-patch-strategy": "merge"
},
"retentionDays": {
"description": "Number of days to retain the logs when running the Airflow log groomer sidecar.",
"type": "integer",
Expand Down
4 changes: 4 additions & 0 deletions chart/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,7 @@ workers:
# Detailed default security context for logGroomerSidecar for container level
securityContexts:
container: {}
env: []

waitForMigrations:
# Whether to create init container to wait for db migrations
Expand Down Expand Up @@ -951,6 +952,7 @@ scheduler:
container: {}
# container level lifecycle hooks
containerLifecycleHooks: {}
env: []

waitForMigrations:
# Whether to create init container to wait for db migrations
Expand Down Expand Up @@ -1520,6 +1522,7 @@ triggerer:

# container level lifecycle hooks
containerLifecycleHooks: {}
env: []

waitForMigrations:
# Whether to create init container to wait for db migrations
Expand Down Expand Up @@ -1696,6 +1699,7 @@ dagProcessor:
# memory: 128Mi
securityContexts:
container: {}
env: []

waitForMigrations:
# Whether to create init container to wait for db migrations
Expand Down
13 changes: 13 additions & 0 deletions tests/charts/log_groomer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,19 @@ def test_log_groomer_collector_default_retention_days(self):
)
assert "15" == jmespath.search("spec.template.spec.containers[1].env[0].value", docs[0])

def test_log_groomer_collector_custom_env(self):
env = {"name": "ENABLE_KUBE_MUTATION_TYPE", "value": "upsert"}
if self.obj_name == "dag-processor":
values = {"dagProcessor": {"enabled": True, "env": env}}
else:
values = None

docs = render_chart(
values=values, show_only=[f"templates/{self.folder}/{self.obj_name}-deployment.yaml"]
)

assert env in jmespath.search("spec.template.spec.containers[1].env", docs[0])

@pytest.mark.parametrize("command", [None, ["custom", "command"]])
@pytest.mark.parametrize("args", [None, ["custom", "args"]])
def test_log_groomer_command_and_args_overrides(self, command, args):
Expand Down