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

Fix log persistence with KubernetesExecutor #15659

Merged
merged 1 commit into from
May 4, 2021

Conversation

jedcunningham
Copy link
Member

KubernetesExecutor workers also need the log volume mounted.

KubernetesExecutor workers also need the log volume mounted.
@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label May 4, 2021
@github-actions
Copy link

github-actions bot commented May 4, 2021

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label May 4, 2021
@kaxil
Copy link
Member

kaxil commented May 4, 2021

Looks like K8s test is failing

@jedcunningham
Copy link
Member Author

Wasn't able to reproduce the failure locally so letting CI run it again.

@kaxil kaxil merged commit d723ba5 into apache:master May 4, 2021
@kaxil kaxil deleted the log_persistence_kubeexecutor branch May 4, 2021 22:29
@cdibble
Copy link

cdibble commented May 4, 2021

I tried to apply this with helm upgrade to my k8s deployment on EKS (AWS) and it failed to upgrade with the following output, which looks to be mostly just a dump of the pod-template-file.kubernetes-helm-yaml file. Let me know if there is some other info I can provide.

Error: UPGRADE FAILED: template: airflow/templates/webserver/webserver-deployment.yaml:60:36: executing "airflow/templates/webserver/webserver-deployment.yaml" at <include (print $.Template.BasePath "/configmaps/configmap.yaml") .>: error calling include: template: airflow/templates/configmaps/configmap.yaml:58:3: executing "airflow/templates/configmaps/configmap.yaml" at <tpl (.Files.Get "files/pod-template-file.kubernetes-helm-yaml") .>: error calling tpl: error during tpl function execution for "# Licensed to the Apache Software Foundation (ASF) under one\n# or more contributor license agreements.  See the NOTICE file\n# distributed with this work for additional information\n# regarding copyright ownGIT-STASH(1)                                                               Git Manual                                                               GIT-STASH(1)
ership.  The ASF licenses this file\n# to you under the Apache License, Version 2.0 (the\n# \"License\"); you may not use this file except in compliance\n# with the License.  You may obtain a copy of the License at\n#\n#   http://www.apache.org/licenses/LICENSE-2.0\n#\n# Unless required by applicable law or agreed to in writing,\n# software distributed under the License is distributed on an\n# \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n# KIND, either express or implied.  See the License for the\n# specific language governing permissions and limitations\n# under the License.\n---\napiVersion: v1\nkind: Pod\nmetadata:\n  name: dummy-name\nspec:\n{{- if and .Values.dags.gitSync.enabled (not .Values.dags.persistence.enabled) }}\n  initContainers:\n{{- include \"git_sync_container\" (dict \"Values\" .Values \"is_init\" \"true\") | indent 4 }}\n{{- end }}\n  containers:\n    - args: []\n      command: []\n      envFrom:\n      {{- include \"custom_airflow_environment_from\" . | default \"\\n  []\" | indent 6 }}\n      env:\n        - name: AIRFLOW__CORE__EXECUTOR\n          value: LocalExecutor\n{{- include \"standard_airflow_environment\" . | indent 6}}\n{{- include \"custom_airflow_environment\" . | indent 6 }}\n      image: {{ template \"pod_template_image\" . }}\n      imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}\n      name: base\n      ports: []\n      volumeMounts:\n        - mountPath: {{ template \"airflow_logs\" . }}\n          name: logs\n        - name: config\n          mountPath: {{ template \"airflow_config_path\" . }}\n          subPath: airflow.cfg\n          readOnly: true\n{{- if .Values.scheduler.airflowLocalSettings }}\n        - name: config\n          mountPath: {{ template \"airflow_local_setting_path\" . }}\n          subPath: airflow_local_settings.py\n          readOnly: true\n{{- end }}\n{{- if .Values.dags.gitSync.knownHosts }}\n        - mountPath: /etc/git-secret/known_hosts\n          name: git-sync-known-hosts\n          subPath: known_hosts\n{{- end }}\n{{- if .Values.dags.gitSync.sshKeySecret }}\n        - mountPath: /etc/git-secret/ssh\n          name: git-sync-ssh-key\n          subPath: ssh\n{{- end }}\n{{- if or .Values.dags.gitSync.enabled .Values.dags.persistence.enabled }}\n        - mountPath: {{ include \"airflow_dags_mount_path\" . }}\n          name: dags\n          readOnly: true\n{{- end }}\n{{- if .Values.workers.extraVolumeMounts }}\n{{ toYaml .Values.workers.extraVolumeMounts | indent 8 }}\n{{- end }}\n  hostNetwork: false\n  {{- if or .Values.registry.secretName .Values.registry.connection }}\n  imagePullSecrets:\n    - name: {{ template \"registry_secret\" . }}\n  {{- end }}\n  restartPolicy: Never\n  securityContext:\n    runAsUser: {{ .Values.uid }}\n    fsGroup: {{ .Values.gid }}\n  nodeSelector: {{ toYaml .Values.nodeSelector | nindent 4 }}\n  affinity: {{ toYaml .Values.affinity | nindent 4 }}\n  tolerations: {{ toYaml .Values.tolerations | nindent 4 }}\n  serviceAccountName: {{ include \"worker.serviceAccountName\" . }}\n  volumes:\n  {{- if .Values.dags.persistence.enabled }}\n  - name: dags\n    persistentVolumeClaim:\n      claimName: {{ template \"airflow_dags_volume_claim\" . }}\n  {{- else if .Values.dags.gitSync.enabled }}\n  - name: dags\n    emptyDir: {}\n  {{- end }}\n  {{- if and  .Values.dags.gitSync.enabled  .Values.dags.gitSync.sshKeySecret }}\n{{- include \"git_sync_ssh_key_volume\" . | indent 2 }}\n  {{- end }}\n{{- if .Values.logs.persistence.enabled }}\n  - name: logs\n    persistentVolumeClaim:\n      claimName: {{ template \"airflow_logs_volume_claim\" . }}\n{{- else }}\n  - emptyDir: {}\n    name: logs\n{{- end }}\n{{- if .Values.dags.gitSync.knownHosts }}\n  - configMap:\n      defaultMode: 288\n      name: {{ include \"airflow_config\" . }}\n    name: git-sync-known-hosts\n{{- end }}\n  - configMap:\n      name: {{ include \"airflow_config\" . }}\n    name: config\n{{- if .Values.workers.extraVolumes }}\n{{ toYaml .Values.workers.extraVolumes | indent 2 }}\n{{- end }}\n": template: airflow/templates/webserver/webserver-deployment.yaml:84:25: executing "airflow/templates/webserver/webserver-deployment.yaml" at <include "worker.serviceAccountName" .>: error calling include: template: no template "worker.serviceAccountName" associated with template "gotpl"

@jedcunningham
Copy link
Member Author

@cdibble, looks related to #14152. Can you confirm you are running the whole chart from latest master (e.g. not a patch applied to an older version)? If that doesn't do it, can you open a new issue?

@cdibble
Copy link

cdibble commented May 5, 2021

Thanks @jedcunningham - good catch. After copying over all of the recent commits to my version, I can run the upgrade successfully. I can't actually get the PVC to work, but that isn't relevant to this issue. I'm going to see if I can go with a remote-logging solution, though, which I prefer anyway and which avoids the now apparent problem that I cannot use a PVC with a ReadWriteMany AccessMode with AWS Elastic Block Storage as a backing- only ReadWriteOnce is supported. So it's either remote logging or EFS, it seems.

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 okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants