Skip to content

[AIRFLOW-5704] Improve Kind Kubernetes scripts for local testing#6496

Merged
potiuk merged 2 commits into
apache:masterfrom
PolideaInternal:improve-kind-kubernetes
Nov 6, 2019
Merged

[AIRFLOW-5704] Improve Kind Kubernetes scripts for local testing#6496
potiuk merged 2 commits into
apache:masterfrom
PolideaInternal:improve-kind-kubernetes

Conversation

@potiuk
Copy link
Copy Markdown
Member

@potiuk potiuk commented Nov 4, 2019

  • Fixed problem that Kubernetes tests were testing latest master
    rather than what came from the local sources.
  • Moved Kubernetes scripts to 'in_container' dir where they belong now
  • Kubernetes tests are now better suited for running locally
  • Kubernetes cluster is not deleted until environment is stopped
  • Kubernetes image is built outside of the container and passed as .tar
  • Kubectl version name is corrected in the Dockerfile
  • Kubernetes Version can be used to select Kubernetes versio
  • Running kubernetes scripts is now easy in Breeze
  • Instructions on how to run Kubernetes tests are updated
  • Better flags in Breeze are used to run Kubernetes environment/tests
  • The old "bare" environment is replaced by --no-deps switch

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@potiuk potiuk requested review from ashb, dimberman and mik-laj November 4, 2019 09:17
@potiuk potiuk self-assigned this Nov 4, 2019
@potiuk potiuk force-pushed the improve-kind-kubernetes branch from 466b5af to 315e6a0 Compare November 4, 2019 09:50
@potiuk potiuk changed the title [AIRFLOW-5704] Improve Kind Kubernetes scripts for local testing. Depends on [AIRFLOW-5826] [AIRFLOW-5827] [AIRFLOW-5830] [AIRFLOW-5829] [AIRFLOW-5704] Improve Kind Kubernetes scripts for local testing. Depends on [AIRFLOW-5827] [AIRFLOW-5830] [AIRFLOW-5829] Nov 4, 2019
@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented Nov 4, 2019

Notes especially to @dimberman and @gerardo (but also @ashb and @mik-laj you might be interested).

As preparation work for the production images I improved the way how the kind cluster is run.

First of all I realised that the current cluster does not test really the latest sources - but instead whatever could be pulled from the latest master CI image on Dockerhub (which was usually at least one commit old). I think @gerardo mentioned it last time we spoke. Luckily we did not have breaking changes :)

This change fixes it - I am now building the Kubernetes image from locally built image in the host and run docker save it to a tar archive which is then mounted to airflow-testing container so that it can be loaded to DinD docker and further loaded to the KinD cluster.

The .tar image contains latest sources:

  • In CI it is done right after the image is built (so has latest sources by definition).
  • In Breeze I ask the user to "force build" the image always when the user enters Breeze with --start-kubernetes-cluster flag.

This way the image with latest sources will be build/saved and made available to airflow-testing container via mounted volume.

@dimberman - you migth also be interested that I made the development workflow for Kubernetes tests with Breeze as nice as possible (we will later improve it - maybe with the production image). The original "Kind" change was working fine for CI (except not taking latest sources), but running it for local development was not really feasible. You really needed to rebuild everything from the scratch when you entered the environment (which took like 10-20 minutes on my PC) and there was no easy way to test the latest sources - short of forwarding, building and pushing your own images to dind container :(.

I optimised it now so that when you have not closed the Breeze environment it will take far less time:

  • kind cluster is not deleted every time - it is reused when you exit/enter Breeze (and do not stop environment)
  • if you enter the cluster with --start-kubernetes-cluster, the CI image will be rebuilt taking latest sources, then kubernetes image is build out of it and saved/mounted to airflow-testing container
  • If you do not stop the environment, the DiND container is running so it keeps both Kind cluster running as well as the kubernetes images pushed.
  • this means that if we rebuild the image with latest sources, only few last layers change and it is much faster to load the image - both to DiND and to Cluster.
  • I've done it in the way that if you have enough power/memory, you could run multiple clusters (each for different kubernetes version) and multiple airflows - one for each python version, all in one DiND. That might be useful when we migrate out of Travis.
  • still if you stop the environment everything will be deleted and whole setup will be setup from the scratch (which is good for cleanup etc.)
  • It should be easy to switch to an external cluster with the current setup in the future.
  • I added instructions how to run the Kubernetes tests in Breeze

It's not ideal and rather slow, but at least it is automated now and you can run Kubernetes tests with your latest sources rather easily/automatically.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 4, 2019

Codecov Report

Merging #6496 into master will decrease coverage by 0.3%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6496      +/-   ##
=========================================
- Coverage      84%   83.7%   -0.31%     
=========================================
  Files         635     635              
  Lines       36715   36715              
=========================================
- Hits        30844   30733     -111     
- Misses       5871    5982     +111
Impacted Files Coverage Δ
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/kube_client.py 33.33% <0%> (-41.67%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 70.14% <0%> (-28.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4ff529...9d921a1. Read the comment docs.

Comment thread BREEZE.rst Outdated
Comment thread BREEZE.rst Outdated
Comment thread breeze
Comment thread breeze
Comment thread scripts/ci/in_container/kubernetes/app/deploy_app.sh Outdated
Comment thread scripts/ci/in_container/kubernetes/app/deploy_app.sh
@potiuk potiuk force-pushed the improve-kind-kubernetes branch 2 times, most recently from 1944e80 to b1de806 Compare November 4, 2019 20:44
Comment thread .travis.yml Outdated
@potiuk potiuk changed the title [AIRFLOW-5704] Improve Kind Kubernetes scripts for local testing. Depends on [AIRFLOW-5827] [AIRFLOW-5830] [AIRFLOW-5829] [AIRFLOW-5704] Improve Kind Kubernetes scripts for local testing. Depends on [AIRFLOW-5830] [AIRFLOW-5829] Nov 4, 2019
@potiuk potiuk force-pushed the improve-kind-kubernetes branch from b1de806 to 256385d Compare November 4, 2019 20:57
@gerardo
Copy link
Copy Markdown
Contributor

gerardo commented Nov 4, 2019

Good catch and improvements @potiuk. I particularly like how the K8S image is built outside of the container.

I have one observation. Regarding Kubernetes cluster is not deleted until the environment is stopped. we just need to be careful that the scripts that prepare the cluster to run the tests also can reset the cluster to an empty state so that the tests keep being idempotent.

@potiuk potiuk force-pushed the improve-kind-kubernetes branch 2 times, most recently from a270f54 to 66297db Compare November 4, 2019 21:47
@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented Nov 4, 2019

I actually will improve it even further @gerardo -> when I get the prod image up and running i will actually use this production image for those tests. Which will basically mean that we will test both the Kubernetes Tests and Production image at the same time (and it will be way smaller so it's likely we will have much faster tests overall :). Vast majority of the test now is just preparation of the environment and particularly exporting/loading the Airflow images :(.

What do you mean by resetting the cluster ? From what I understand the "deploy_app.sh" script will actually delete the pods - postgres/airflow/webserver. This will effectively destroy any state - i.e. the postgres volume, and next time when we redeploy the app we anyhow run "initContainer" in the airflow.yaml (airflow-test-env-init.sh) which will perform airflow initdb. So I think just making sure that we redeploy the app before rerunning test is enough?

If we want to re-run tests, presumably we want to do it because we modified code, so we have to rebuild and re-deploy the application (after building the image) so we should be fine anyway ?

I just now will have to make an easy way to trigger it for iterative development.

I think it should be possible to rebuild the image and re-deploy app with a single trigger from the host without even restarting breeze. I will likely have to mount the whole folder rather than single file to let the exported .tar file to be updated in container.

@potiuk potiuk force-pushed the improve-kind-kubernetes branch 2 times, most recently from e3db1ee to 1216627 Compare November 5, 2019 23:45
Comment thread scripts/ci/in_container/kubernetes/setup_kubernetes_and_deploy_app.sh Outdated
Copy link
Copy Markdown
Member

@mik-laj mik-laj left a comment

Choose a reason for hiding this comment

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

LGTM. Good job!

@potiuk potiuk changed the title [AIRFLOW-5704] Improve Kind Kubernetes scripts for local testing. Depends on [AIRFLOW-5830] [AIRFLOW-5829] [AIRFLOW-5704] Improve Kind Kubernetes scripts for local testing Nov 6, 2019
@potiuk potiuk force-pushed the improve-kind-kubernetes branch from 1216627 to 55cb1b9 Compare November 6, 2019 11:00
* Fixed problem that Kubernetes tests were testing latest master
  rather than what came from the local sources.
* Moved Kubernetes scripts to 'in_container' dir where they belong now
* Kubernetes tests are now better suited for running locally
* Kubernetes cluster is not deleted until environment is stopped
* Kubernetes image is built outside of the container and passed as .tar
* Kubectl version name is corrected in the Dockerfile
* Kubernetes Version can be used to select Kubernetes versio
* Running kubernetes scripts is now easy in Breeze
* Instructions on how to run Kubernetes tests are updated
* Better flags in Breeze are used to run Kubernetes environment/tests
* The old "bare" environment is replaced by --no-deps switch
@potiuk potiuk force-pushed the improve-kind-kubernetes branch from 55cb1b9 to 9d921a1 Compare November 6, 2019 11:04
Copy link
Copy Markdown
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

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

This is awesome @potiuk ! 🚀

I am not that great at bash but when I am reading your scripts I nearly understand everything directly :D It looks really clean 👍
I only have one suggestion related to the upcoming prod image.

Comment thread BREEZE.rst Outdated
@potiuk potiuk merged commit 8e789a3 into apache:master Nov 6, 2019
@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented Nov 6, 2019

@feluelle - wait for #6500 :)... That's even better.

potiuk added a commit that referenced this pull request Nov 7, 2019
@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented Nov 12, 2019

This one has been reverted - we have #6516 instead.

@potiuk potiuk added the area:production-image Production image improvements and fixes label Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:production-image Production image improvements and fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants