Add git sync option and unit tests for the Helm chart #9371

aneesh-joseph · 2020-06-18T08:43:08Z

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Target Github ISSUE in description if exists
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

Improve the helm chart with:

Add capability to sync dags using a git-sync sidecar
- git sync over ssh with ssh key sourced from kubernetes secrets
- provide a way to specify known_hosts when using git sync over ssh with an ssh key
- git sync without ssh
- credentials support for git sync without ssh(credentials sourced from a kubernetes secret)
- Persistence support using PVC. if enabled the scheduler will sync dags to the PVC and others will mount the synced dags
Add helm-unit tests. The CI tests use an image published using the Dockerfile from a forked repo https://github.com/aneesh-joseph/helm-unittest.
- Add CI flow for helm unit tests
- Add tests for git sync

aneesh-joseph · 2020-06-18T15:46:00Z

chart/templates/webserver/webserver-deployment.yaml

@@ -82,6 +82,9 @@ spec:
          {{- include "custom_airflow_environment" . | indent 10 }}
          {{- include "standard_airflow_environment" . | indent 10 }}
      containers:
+{{- if .Values.gitSync.enabled }}


dag sync won't be needed on the webserver if dag serialization is enabled

dimberman · 2020-06-18T16:06:51Z

chart/values.yaml

@@ -434,3 +434,23 @@ postgresql:
  enabled: true
  postgresqlPassword: postgres
  postgresqlUsername: postgres
+# Git sync
+gitSync:


We should put this under a kubernetesExecutor section in case we need to expand to further k8sexecutor-specific features

I initially thought the same, but the git sync sidecars might also be used without the KubernetesExecutor ?

Ok yeah I can see that. would just need to set a relatively fast cycle for pinging/re-pulling the repo s.t. the worker stays in sync with the scheduler/webserver.

updated the sync interval to a default of 1 minute, have added in a PVC as well

dimberman · 2020-06-18T16:16:48Z

chart/values.yaml

+  # ssh example: [email protected]:aneesh-joseph/dags-repo.git
+  # https example: https://github.com/aneesh-joseph/dags-repo.git
+  #TODO Change the URLs used for testing
+  repo: https://github.com/aneesh-joseph/dags-repo.git


Please change this to the apache project.

done, updated it to use dags from https://github.com/apache/airflow/tree/v1-10-stable/tests/dags

aneesh-joseph · 2020-06-19T06:36:57Z

scripts/ci/ci_run_helm_testing.sh

+
+docker run -w /airflow-chart -v "$CHART_DIR":/airflow-chart \
+  --entrypoint /bin/sh \
+  divya12/helm-unittest \


need to change this image

I created an issue: #9401 - we have a few more "private" images that we depend on, we should replace them with our own copy of those images. For now it can stay like it is .

potiuk · 2020-06-19T09:17:57Z

chart/values.yaml

+  # [email protected]:apache/airflow.git
+  # https example: https://github.com/apache/airflow.git
+  repo: https://github.com/apache/airflow.git
+  branch: v1-10-stable


I will change it later to a variable. But for now iI think it's good. I plan to use the Helm chart not only for unit tests (which are great BTW) but also to run our Kubernetes tests (we are currently using some custom, simple templates for the kubernetes resources - replacing it with the helm chart from master and using images built locally will be a great way to do doggfooding.

cool, that would be awesome 👍

potiuk

LGTM

Co-authored-by: Ash Berlin-Taylor <[email protected]>

aneesh-joseph · 2020-07-04T10:14:07Z

@dimberman @ashb @potiuk - the tests are now passing, can you help in getting this merged please

potiuk

Great Change! I was thinking - shoudl not we also update the tests in test_kubernetes? we already have git_sync mode for those tests but it is disable - I am happy to help in switching the git_sync tests on. We can do it in a separate change but it actually belongs here.
@dimberman WDYT?

potiuk · 2020-07-04T14:05:43Z

chart/README.md

@@ -66,7 +66,7 @@ The command removes all the Kubernetes components associated with the chart and

 ## Updating DAGs

-The recommended way to update your DAGs with this chart is to build a new docker image with the latest code (`docker build -t my-company/airflow:8a0da78 .`), push it to an accessible registry (`docker push my-company/airflow:8a0da78`), then update the Airflow pods with that image:
+The recommended way to update your DAGs with this chart is to build a new docker image with the latest DAG code (`docker build -t my-company/airflow:8a0da78 .`), push it to an accessible registry (`docker push my-company/airflow:8a0da78`), then update the Airflow pods with that image:


I think we will want to change that description as it is not valid any more and we have "EMBED_DAGS" directive while building the dockerfiles + we will add on-build most likely to add the DAGs on building deppendent image but I will do it separately.

potiuk · 2020-07-04T14:07:06Z

chart/README.md

+  # Refer values.yaml for details
+```
+
+## Mounting DAGS using Git-Sync side car without Persistence


Nice to have this option.!

chart/README.md

potiuk · 2020-07-05T07:04:23Z

chart/requirements.lock

@@ -1,6 +0,0 @@
-dependencies:


Why do wer delete this ?

potiuk · 2020-07-05T07:05:47Z

chart/tests/dags-persistent-volume-claim_test.yaml

@@ -0,0 +1,64 @@
+# Licensed to the Apache Software Foundation (ASF) under one


I love the unit tests of helm chart! ❤️

Co-authored-by: Jarek Potiuk <[email protected]>

friedhardware · 2020-07-05T22:33:29Z

Where is this chart hosted? Is it related to https://hub.helm.sh/charts/stable/airflow?

marclamberti · 2020-07-05T22:38:07Z

@friedhardware, this chart is only available from this repo for now and it is the official one, not related to the current one on Helm.sh

Swalloow · 2020-07-06T06:28:06Z

chart/templates/webserver/webserver-deployment.yaml

+        {{- if .Values.dags.persistence.enabled }}
+        - name: dags
+          persistentVolumeClaim:
+            claimName: {{ .Release.Name }}-dags


If uses dags.persistence.existingClaim, it should be modified to
claimName: {{ .Values.dags.persistence.existingClaim }}.
Currently, the webserver pod cannot find pvc, so it is put in a pending state.

have raised a PR to fix this - #9688

marclamberti · 2020-07-16T13:09:40Z

I got an issue yesterday with git-sync and the K8s worker pods.
If we specify:
AIRFLOW__KUBERNETES__RUN_AS_USER="50000" in values, git-sync ends up with the following error in K8s worker pods:
E0715 21:13:28.459419 9 main.go:347] "msg"="failed to sync repo, aborting" "error"="error running command: exit status 128: { stdout: \"\", stderr: \"Cloning into '/git'...\\nNo user exists for uid 50000\\r\\nfatal: Could not read from remote repository.\\n\\nPlease make sure you have the correct access rights\\nand the repository exists.\\n\" }"

Nonetheless, git-sync along with the webserver and scheduler of Airflow works since the env var AIRFLOW__KUBERNETES__RUN_AS_USER="50000" isn't set which is not the case in the K8S worker pods.

To fix this, we have to define
AIRFLOW__KUBERNETES__GIT_SYNC_RUN_AS_USER="65553" corresponding to the default uid of git-sync.

Also, we have to set the UID to 50000 otherwise Airflow fails with the following error:

Traceback (most recent call last):
  File "/home/airflow/.local/bin/airflow", line 23, in <module>
    import argcomplete
ModuleNotFoundError: No module named 'argcomplete'

aneesh-joseph · 2020-07-16T13:28:47Z

I got an issue yesterday with git-sync and the K8s worker pods.
If we specify:
AIRFLOW__KUBERNETES__RUN_AS_USER="50000" in values, git-sync ends up with the following error in K8s worker pods:
E0715 21:13:28.459419 9 main.go:347] "msg"="failed to sync repo, aborting" "error"="error running command: exit status 128: { stdout: \"\", stderr: \"Cloning into '/git'...\\nNo user exists for uid 50000\\r\\nfatal: Could not read from remote repository.\\n\\nPlease make sure you have the correct access rights\\nand the repository exists.\\n\" }"

Nonetheless, git-sync along with the webserver and scheduler of Airflow works since the env var AIRFLOW__KUBERNETES__RUN_AS_USER="50000" isn't set which is not the case in the K8S worker pods.

To fix this, we have to define
AIRFLOW__KUBERNETES__GIT_SYNC_RUN_AS_USER="65553" corresponding to the default uid of git-sync.

Also, we have to set the UID to 50000 otherwise Airflow fails with the following error:
Traceback (most recent call last):
  File "/home/airflow/.local/bin/airflow", line 23, in <module>
    import argcomplete
ModuleNotFoundError: No module named 'argcomplete'

yes it might be failing as the user 50000 doesn't exist in /etc/passwd and ssh needs that. setting AIRFLOW__KUBERNETES__GIT_SYNC_RUN_AS_USER to 65553(may need to set fs_group as well - not very sure) will fix it or setting GIT_SYNC_ADD_USER env var on the init container should fix it as well(we do this on scheduler & webserver). I had added that change to the worker pods but that's not part of 1.10.11 https://github.com/apache/airflow/pull/9371/files#r442960495. think run_as_user is already defaulted to 50000 ({{ .Values.uid }}) in the config map rendered by the chart

* add git sync sidecars * add a helm test * add more tests * allow users to provide git username and pass via a k8s secrets * set default values for airflow worker repository & tag * change ci timeout * fix link * add credentials_secret to airflow.cfg configmap * set GIT_SYNC_ADD_USER on kubernetes worker pods, set uid * add fsGroup to webserver and kubernete workers * move gitSync to dags.gitSync * rename valueFields * turn off git sync and dag persistence by default * provide option to specify known_hosts * add git-sync details into the chart documentation * Update .gitignore Co-authored-by: Ash Berlin-Taylor <[email protected]> * make git sync max failures configurable * Apply suggestions from code review Co-authored-by: Jarek Potiuk <[email protected]> * add back requirements.lock Co-authored-by: Ash Berlin-Taylor <[email protected]> Co-authored-by: Jarek Potiuk <[email protected]> (cherry picked from commit d93555b)

boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Jun 18, 2020

aneesh-joseph force-pushed the git-sync branch 4 times, most recently from d2162f6 to 51fb039 Compare June 18, 2020 15:33

aneesh-joseph commented Jun 18, 2020

View reviewed changes

dimberman requested changes Jun 18, 2020

View reviewed changes

aneesh-joseph force-pushed the git-sync branch from ffd5481 to fd95e47 Compare June 18, 2020 18:46

aneesh-joseph requested a review from dimberman June 18, 2020 19:40

aneesh-joseph force-pushed the git-sync branch 2 times, most recently from 8b13e51 to 5d60c26 Compare June 19, 2020 02:58

aneesh-joseph changed the title ~~Add git sync option to the Helm chart~~ Add git sync option & helm unit tests Jun 19, 2020

aneesh-joseph force-pushed the git-sync branch 10 times, most recently from c317639 to d2abd4f Compare June 19, 2020 06:34

aneesh-joseph commented Jun 19, 2020

View reviewed changes

aneesh-joseph force-pushed the git-sync branch from d2abd4f to a891f9e Compare June 19, 2020 07:24

This was referenced Jun 19, 2020

Add Production Helm chart support #8777

Merged

All "private" images and binaries used by Airflow should be manage by community #9401

Closed

potiuk reviewed Jun 19, 2020

View reviewed changes

potiuk approved these changes Jun 19, 2020

View reviewed changes

aneesh-joseph force-pushed the git-sync branch from 19ffd81 to 4682d7c Compare June 19, 2020 11:36

aneesh-joseph and others added 9 commits July 4, 2020 07:37

add credentials_secret to airflow.cfg configmap

60f768f

set GIT_SYNC_ADD_USER on kubernetes worker pods, set uid

0e69d66

add fsGroup to webserver and kubernete workers

22b1b15

move gitSync to dags.gitSync

8fdc1bd

rename valueFields

c4370ea

turn off git sync and dag persistence by default

362e163

provide option to specify known_hosts

4f2496c

add git-sync details into the chart documentation

a4a0cf5

Update .gitignore

465a018

Co-authored-by: Ash Berlin-Taylor <[email protected]>

aneesh-joseph force-pushed the git-sync branch 2 times, most recently from 1f54704 to 5b36045 Compare July 4, 2020 02:09

make git sync max failures configurable

0dc3471

aneesh-joseph force-pushed the git-sync branch from 5b36045 to 0dc3471 Compare July 4, 2020 07:19

potiuk reviewed Jul 5, 2020

View reviewed changes

aneesh-joseph and others added 2 commits July 5, 2020 14:06

Apply suggestions from code review

b7a40e8

Co-authored-by: Jarek Potiuk <[email protected]>

add back requirements.lock

eabf6c0

potiuk approved these changes Jul 5, 2020

View reviewed changes

dimberman approved these changes Jul 5, 2020

View reviewed changes

dimberman merged commit d93555b into apache:master Jul 5, 2020

Swalloow reviewed Jul 6, 2020

View reviewed changes

aneesh-joseph deleted the git-sync branch July 6, 2020 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add git sync option and unit tests for the Helm chart #9371

Add git sync option and unit tests for the Helm chart #9371

aneesh-joseph commented Jun 18, 2020 •

edited

Loading

aneesh-joseph Jun 18, 2020 •

edited

Loading

dimberman Jun 18, 2020

aneesh-joseph Jun 18, 2020 •

edited

Loading

dimberman Jun 18, 2020

aneesh-joseph Jun 18, 2020 •

edited

Loading

dimberman Jun 18, 2020

aneesh-joseph Jun 18, 2020 •

edited

Loading

aneesh-joseph Jun 19, 2020

potiuk Jun 19, 2020

potiuk Jun 19, 2020

aneesh-joseph Jun 19, 2020

potiuk left a comment

aneesh-joseph commented Jul 4, 2020

potiuk left a comment

potiuk Jul 4, 2020

potiuk Jul 4, 2020

potiuk Jul 5, 2020

potiuk Jul 5, 2020

friedhardware commented Jul 5, 2020

marclamberti commented Jul 5, 2020

Swalloow Jul 6, 2020

aneesh-joseph Jul 6, 2020

marclamberti commented Jul 16, 2020

aneesh-joseph commented Jul 16, 2020

		@@ -0,0 +1,64 @@
		# Licensed to the Apache Software Foundation (ASF) under one

Add git sync option and unit tests for the Helm chart #9371

Add git sync option and unit tests for the Helm chart #9371

Conversation

aneesh-joseph commented Jun 18, 2020 • edited Loading

aneesh-joseph Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aneesh-joseph Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aneesh-joseph Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aneesh-joseph Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

aneesh-joseph commented Jul 4, 2020

potiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedhardware commented Jul 5, 2020

marclamberti commented Jul 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marclamberti commented Jul 16, 2020

aneesh-joseph commented Jul 16, 2020

aneesh-joseph commented Jun 18, 2020 •

edited

Loading

aneesh-joseph Jun 18, 2020 •

edited

Loading

aneesh-joseph Jun 18, 2020 •

edited

Loading

aneesh-joseph Jun 18, 2020 •

edited

Loading

aneesh-joseph Jun 18, 2020 •

edited

Loading