Skip to content

Fix passing dags.gitSync.sshKeySecret with dags.persistence.enabled=true#36628

Closed
dolfinus wants to merge 1 commit into
apache:mainfrom
dolfinus:main
Closed

Fix passing dags.gitSync.sshKeySecret with dags.persistence.enabled=true#36628
dolfinus wants to merge 1 commit into
apache:mainfrom
dolfinus:main

Conversation

@dolfinus

@dolfinus dolfinus commented Jan 6, 2024

Copy link
Copy Markdown
Contributor

  • Deploy Airflow using Heml chart 1.10.0 with dags.gitSync.enabled=True
  • After testing git sync is working as expected, set dags.persistence.enabled=true to store dags to persistent volume instead of empty dir.
  • Got exception that git-sync-ssh-key volume is missing in scheduler pod description:
  volumes:
    - name: config
      configMap:
        name: airflow-airflow-config
    - name: dags
-     emptyDir: {}
-
-   - name: git-sync-ssh-key
-     secret:
-       secretName: airflow-ssh-secret
-       defaultMode: 288
+     persistentVolumeClaim:
+       claimName: airflow-dags
    - name: logs
      persistentVolumeClaim:
cannot patch "airflow-scheduler" with kind Deployment: Deployment.apps "airflow-scheduler" is invalid: [spec.template.spec.containers[1].volumeMounts[1].name: Not found: "git-sync-ssh-key", spec.template.spec.initContainers[1].volumeMounts[1].name: Not found: "git-sync-ssh-key"]
helm.go:84: [debug] cannot patch "airflow-scheduler" with kind Deployment: Deployment.apps "airflow-scheduler" is invalid: [spec.template.spec.containers[1].volumeMounts[1].name: Not found: "git-sync-ssh-key", spec.template.spec.initContainers[1].volumeMounts[1].name: Not found: "git-sync-ssh-key"]

helm.log

This is because of wrong condition in helm chart - git-sync-ssh-key volumeMount is always present in git-sync container inside scheduler pod, but corresponding volume is added to pod only if dags.persistence.enabled=false. Changed this condition to always add git-sync-ssh-key to scheduler volumes if git sync is enabled and ssh-key secret is passed to chart.

This issue was introduced in #22913 - git sync should be disabled on all pods if dag persistence is enabled, except scheduler (because scheduler pod is the sync point of dags).

This is alternative implementation of #30896, and second try of #34331.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this was right before. If the dag processor is on, I think it makes sense for the syncing to happen here, thus we need the key.

- name: dags
emptyDir: {{- toYaml (default (dict) .Values.dags.gitSync.emptyDirConfig) | nindent 12 }}
{{- end }}
{{- if .Values.dags.gitSync.sshKeySecret }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
{{- if .Values.dags.gitSync.sshKeySecret }}
{{- if and $localOrDagProcessorDisabled .Values.dags.gitSync.enabled .Values.dags.gitSync.sshKeySecret }}

It think this, in addition to your other changes in this file, is the right fix.

emptyDir: {{- toYaml (default (dict) .Values.dags.gitSync.emptyDirConfig) | nindent 12 }}
{{- end }}
{{- if .Values.dags.gitSync.sshKeySecret }}
# unlike other pods, git-sync in scheduler is enabled even if dag.persistence is enabled

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These comments aren't correct if the dag processor is enabled.

@github-actions

github-actions Bot commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 5, 2024
@github-actions github-actions Bot closed this Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:helm-chart Airflow Helm Chart stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants