You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/01 02:27:34 UTC
[GitHub] [airflow] kaxil opened a new pull request #10084: Fix more PodMutationHook issues for backwards compatibility
kaxil opened a new pull request #10084:
URL: https://github.com/apache/airflow/pull/10084
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
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#discussion_r466548177
##########
File path: airflow/kubernetes/pod_generator.py
##########
@@ -637,6 +640,37 @@ def extend_object_field(base_obj, client_obj, field_name):
setattr(client_obj_cp, field_name, base_obj_field)
return client_obj_cp
- appended_fields = base_obj_field + client_obj_field
+ base_obj_set = get_dict_from_list(base_obj_field)
+ client_obj_set = get_dict_from_list(client_obj_field)
+
+ appended_fields = _merge_list_of_objects(base_obj_set, client_obj_set)
+
setattr(client_obj_cp, field_name, appended_fields)
return client_obj_cp
+
+
+def _merge_list_of_objects(base_obj_set, client_obj_set):
+ for k, v in base_obj_set.items():
+ if k not in client_obj_set:
+ client_obj_set[k] = v
+ else:
+ client_obj_set[k] = merge_objects(v, client_obj_set[k])
+ appended_field_keys = sorted(client_obj_set.keys())
+ appended_fields = [client_obj_set[k] for k in appended_field_keys]
+ return appended_fields
+
+
+def get_dict_from_list(base_list):
+ """
+ :param base_list:
+ :type base_list: list(Optional[dict,
Review comment:
We need to fix this or remove the type and make it an internal Method
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman edited a comment on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
dimberman edited a comment on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-668860781
```
│ │
│ 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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#discussion_r466834542
##########
File path: airflow/kubernetes/pod_generator.py
##########
@@ -36,6 +36,9 @@
import kubernetes.client.models as k8s
import yaml
from kubernetes.client.api_client import ApiClient
+from ..contrib.kubernetes.pod import (
Review comment:
Why not use full path?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#discussion_r466837581
##########
File path: airflow/kubernetes/pod_generator.py
##########
@@ -36,6 +36,9 @@
import kubernetes.client.models as k8s
import yaml
from kubernetes.client.api_client import ApiClient
+from ..contrib.kubernetes.pod import (
Review comment:
@turbaszek it seems you need to use relative paths if you want to use private functions
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#discussion_r466958929
##########
File path: airflow/kubernetes/pod_generator.py
##########
@@ -36,6 +36,9 @@
import kubernetes.client.models as k8s
import yaml
from kubernetes.client.api_client import ApiClient
+from ..contrib.kubernetes.pod import (
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#discussion_r466839460
##########
File path: airflow/kubernetes/pod_generator.py
##########
@@ -36,6 +36,9 @@
import kubernetes.client.models as k8s
import yaml
from kubernetes.client.api_client import ApiClient
+from ..contrib.kubernetes.pod import (
Review comment:
Hm, I checked out this code in both cases `..` and `airflow.contrib` I see warning `Access to a protected member _extract_volume_mounts of a module`. So I don't think this is a problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-669554963
@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 :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman edited a comment on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
dimberman edited a comment on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-668860781
```
│ │
│ 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-668776716
addresses https://github.com/apache/airflow/issues/9827 as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-670457858
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. 🚀
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] aneesh-joseph commented on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
aneesh-joseph commented on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-667512321
> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-668863204
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-672331118
Please also refer to https://github.com/apache/airflow/commit/c230156739178762d5cef482ace3d7a05e683cc1 for all the changes in 1.10.12
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#discussion_r466839460
##########
File path: airflow/kubernetes/pod_generator.py
##########
@@ -36,6 +36,9 @@
import kubernetes.client.models as k8s
import yaml
from kubernetes.client.api_client import ApiClient
+from ..contrib.kubernetes.pod import (
Review comment:
Hm, I checked out this code in both cases `..` and `airflow.contrib` I see warning `Access to a protected member _extract_volume_mounts of a module`. So I don't think this is a problem. If there's any particular reason to have such import I would be happy to have a comment on why we do this way
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-669926567
> @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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-668979398
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #10084:
URL: https://github.com/apache/airflow/pull/10084#issuecomment-668860781
`│ │
│ 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #10084: Fix more PodMutationHook issues for backwards compatibility
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #10084:
URL: https://github.com/apache/airflow/pull/10084
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org