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 more PodMutationHook issues for backwards compatibility #10084

Merged
merged 33 commits into from
Aug 7, 2020

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Aug 1, 2020

This PR/commit

  • Adds missing affinity from old POD
  • Adds comprehensive tests to check pod_mutation_hook works well with both new and old PODs with various configs like volume, volumeMounts, Ports, affinity, tolerations etc
  • Refactors various parts of k8s code

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@kaxil kaxil requested a review from dimberman August 1, 2020 02:27
@boring-cyborg boring-cyborg bot added the k8s label Aug 1, 2020
Copy link
Member Author

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

cc @aneesh-joseph Thanks for testing the last fix, can you please help me by testing this PR too -- Can you run your pod_mutations hooks with this PR, please (both pre 1.10.11 and 1.10.11 behaviors) ?

@kaxil kaxil added this to the Airflow 1.10.12 milestone Aug 1, 2020
@aneesh-joseph
Copy link
Contributor

cc @aneesh-joseph Thanks for testing the last fix, can you please help me by testing this PR too -- Can you run your pod_mutations hooks with this PR, please (both pre 1.10.11 and 1.10.11 behaviors) ?

sure, will test it out and slack you

@dimberman dimberman force-pushed the fix-more-pod-mutations-tests branch from 9c6b8fc to 371dcb4 Compare August 4, 2020 18:23
@dimberman
Copy link
Contributor

addresses #9827 as well

@dimberman dimberman force-pushed the fix-more-pod-mutations-tests branch from 494ac69 to ddb28dc Compare August 4, 2020 21:16
@dimberman
Copy link
Contributor

dimberman commented Aug 4, 2020

│                                                                                      │
│ Traceback (most recent call last):                                                   │
│   File "/home/airflow/.local/bin/airflow", line 25, in <module>                      │
│     from airflow.configuration import conf                                           │
│   File "/home/airflow/.local/lib/python3.6/site-packages/airflow/__init__.py", line  │
│     from airflow.utils.log.logging_mixin import LoggingMixin                         │
│   File "/home/airflow/.local/lib/python3.6/site-packages/airflow/utils/__init__.py", │
│     from .decorators import apply_defaults as _apply_defaults                        │
│   File "/home/airflow/.local/lib/python3.6/site-packages/airflow/utils/decorators.py │
│     from airflow import settings                                                     │
│   File "/home/airflow/.local/lib/python3.6/site-packages/airflow/settings.py", line  │
│     from airflow.configuration import conf, AIRFLOW_HOME, WEBSERVER_CONFIG  # NOQA F │
│   File "/home/airflow/.local/lib/python3.6/site-packages/airflow/configuration.py",  │
│     with open(AIRFLOW_CONFIG, 'w') as f:                                             │
│ IsADirectoryError: [Errno 21] Is a directory: '/opt/airflow/airflow.cfg'             │
│ stream closed                                                                        │
│

that is causing k8s tests to fail (this only happens on the workers, not the scheduler)

@dimberman
Copy link
Contributor

scheduler

│   config:                                                                            │
│     Type:      ConfigMap (a volume populated by a ConfigMap)                         │
│     Name:      airflow-airflow-config                                                │
│     Optional:  false

worker

   airflow-config:                                                                    │
│     Type:       EmptyDir (a temporary directory that shares a pod's lifetime)        │
│     Medium:                                                                          │
│     SizeLimit:  <unset>

for some reason the workers are mounting the cfg as an emptydir

@dimberman
Copy link
Contributor

@kaxil so the tasks are completing correctly but then the pod goes into a crash loop and I'm not sure why. Will confer with you in the morning.

@kaxil kaxil force-pushed the fix-more-pod-mutations-tests branch from 084b28d to 66e986e Compare August 5, 2020 14:07
@dimberman
Copy link
Contributor

@kaxil everything is working except those random mongo_hook tests. Can you take a look? After that we just need to refactor.

At the very least k8s stuff is WAY better tested now :)

@kaxil
Copy link
Member Author

kaxil commented Aug 6, 2020

@kaxil everything is working except those random mongo_hook tests. Can you take a look? After that we just need to refactor.

At the very least k8s stuff is WAY better tested now :)

Aneesh has reported that pod_mutation_hook isn't working for pre-1.10.11

@kaxil kaxil force-pushed the fix-more-pod-mutations-tests branch 2 times, most recently from cf68859 to 4991c58 Compare August 6, 2020 19:31
@kaxil kaxil force-pushed the fix-more-pod-mutations-tests branch from 7fcbb38 to ebcae45 Compare August 6, 2020 21:05
@kaxil kaxil merged commit 21aade4 into apache:v1-10-test Aug 7, 2020
@kaxil kaxil deleted the fix-more-pod-mutations-tests branch August 7, 2020 10:50
@kaxil
Copy link
Member Author

kaxil commented Aug 7, 2020

A big thanks and a shoutout to @aneesh-joseph for helping us with testing that both pre-1.10.11 and 1.10.11 behaviours are maintained for 1.10.12. 🚀

@kaxil
Copy link
Member Author

kaxil commented Aug 11, 2020

Please also refer to c230156 for all the changes in 1.10.12

kaxil added a commit that referenced this pull request Aug 15, 2020
@ashb ashb added the provider:cncf-kubernetes Kubernetes provider related issues label Feb 10, 2021
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants