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 2022/10/18 19:41:19 UTC
[GitHub] [airflow] dstandish opened a new pull request, #27116: Use current namespace if none provided in KPO
dstandish opened a new pull request, #27116:
URL: https://github.com/apache/airflow/pull/27116
In KPO namespace is currently required, either through pod template file, airflow connection, or KPO arg. If we're in the in_cluster scenario though, we should be able to derive the current namespace from /var/run/secrets/kubernetes.io/serviceaccount/namespace. This seems like a reasonable thing to do as a default.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r1001010516
##########
airflow/providers/cncf/kubernetes/hooks/kubernetes.py:
##########
@@ -329,11 +329,18 @@ def get_custom_object(
def get_namespace(self) -> str | None:
"""Returns the namespace that defined in the connection"""
if self.conn_id:
- connection = self.get_connection(self.conn_id)
- extras = connection.extra_dejson
- namespace = extras.get("extra__kubernetes__namespace", "default")
- return namespace
- return None
+ namespace = self.conn_extras.get("extra__kubernetes__namespace")
Review Comment:
yes, i'm currently getting rid of all of that........ but that's in diff PR .... so not taking that on here....
too many irons in the fire! and no want wants to review those prs!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r999998675
##########
tests/providers/cncf/kubernetes/operators/test_kubernetes_pod.py:
##########
@@ -102,15 +106,14 @@ def run_pod(self, operator: KubernetesPodOperator, map_index: int = -1) -> k8s.V
remote_pod_mock = MagicMock()
remote_pod_mock.status.phase = 'Succeeded'
self.await_pod_mock.return_value = remote_pod_mock
- if not isinstance(self.hook_mock.return_value.is_in_cluster, bool):
- self.hook_mock.return_value.is_in_cluster = True
operator.execute(context=context)
return self.await_start_mock.call_args[1]['pod']
def sanitize_for_serialization(self, obj):
return ApiClient().sanitize_for_serialization(obj)
- def test_config_path(self):
+ @patch(HOOK_CLASS)
Review Comment:
you see this added in many places because we're not patching it globally
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r1001008280
##########
tests/providers/cncf/kubernetes/operators/test_kubernetes_pod.py:
##########
@@ -123,20 +123,18 @@ def test_config_path(self, hook_mock):
labels={"foo": "bar"},
name="test",
task_id="task",
Review Comment:
now if only i can get the tests to pass...
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r999998911
##########
tests/providers/cncf/kubernetes/operators/test_kubernetes_pod.py:
##########
@@ -169,7 +172,7 @@ def test_security_context(self):
in_cluster=False,
do_xcom_push=False,
)
- pod = self.run_pod(k)
+ pod = k.build_pod_request_obj(create_context(k))
Review Comment:
when we're just building the pod object we don't need the full "run" 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r1001009198
##########
airflow/providers/cncf/kubernetes/hooks/kubernetes.py:
##########
@@ -329,11 +329,18 @@ def get_custom_object(
def get_namespace(self) -> str | None:
"""Returns the namespace that defined in the connection"""
if self.conn_id:
- connection = self.get_connection(self.conn_id)
- extras = connection.extra_dejson
- namespace = extras.get("extra__kubernetes__namespace", "default")
- return namespace
- return None
+ namespace = self.conn_extras.get("extra__kubernetes__namespace")
Review Comment:
Non-blocking: I can't seem to find it now, but I thought there was recent discussion about removing the "extra__" part of these index names. I could be completely mistaken though and I can't find it now so maybe I'm just making this up.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r999999987
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -574,6 +581,11 @@ def build_pod_request_obj(self, context: Context | None = None) -> k8s.V1Pod:
if self.random_name_suffix:
pod.metadata.name = PodGenerator.make_unique_pod_id(pod.metadata.name)
+ if not pod.metadata.namespace:
+ hook_namespace = self.hook.conn_extras.get('extra__kubernetes__namespace')
Review Comment:
there is a `get_namespace` method on the hook but it's problematic. i may resolve that in future PR and then can update this. i'll resolve that and update this in a followup PR
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a diff in pull request #27116: Make namespace optional for KPO
Posted by GitBox <gi...@apache.org>.
kaxil commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r1002047251
##########
airflow/providers/cncf/kubernetes/CHANGELOG.rst:
##########
@@ -35,7 +35,9 @@ Previously KubernetesPodOperator considered some settings from the Airflow confi
Features
~~~~~~~~
-Previously, ``name`` was a required argument for KubernetesPodOperator (when also not supplying pod template or full pod spec). Now, if ``name`` is not supplied, ``task_id`` will be used.
+* Previously, ``name`` was a required argument for KubernetesPodOperator (when also not supplying pod template or full pod spec). Now, if ``name`` is not supplied, ``task_id`` will be used.
+* KubernetsPodOperator argument ``namespace`` is now optional. If not supplied, we'll check the airflow conn,
Review Comment:
Should we document this priority?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r1001010516
##########
airflow/providers/cncf/kubernetes/hooks/kubernetes.py:
##########
@@ -329,11 +329,18 @@ def get_custom_object(
def get_namespace(self) -> str | None:
"""Returns the namespace that defined in the connection"""
if self.conn_id:
- connection = self.get_connection(self.conn_id)
- extras = connection.extra_dejson
- namespace = extras.get("extra__kubernetes__namespace", "default")
- return namespace
- return None
+ namespace = self.conn_extras.get("extra__kubernetes__namespace")
Review Comment:
yes, i'm currently getting rid of all of that........ but that's in diff PR .... so not taking that on here....
too many irons in the fire! and no one wants to review those prs!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r999998470
##########
tests/providers/cncf/kubernetes/operators/test_kubernetes_pod.py:
##########
@@ -102,15 +106,14 @@ def run_pod(self, operator: KubernetesPodOperator, map_index: int = -1) -> k8s.V
remote_pod_mock = MagicMock()
remote_pod_mock.status.phase = 'Succeeded'
self.await_pod_mock.return_value = remote_pod_mock
- if not isinstance(self.hook_mock.return_value.is_in_cluster, bool):
- self.hook_mock.return_value.is_in_cluster = True
Review Comment:
instead of doing this, i just read the value from the actual hook (since we're no longer patching it globally)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a diff in pull request #27116: Make namespace optional for KPO
Posted by GitBox <gi...@apache.org>.
kaxil commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r1002047760
##########
airflow/providers/cncf/kubernetes/CHANGELOG.rst:
##########
@@ -35,7 +35,9 @@ Previously KubernetesPodOperator considered some settings from the Airflow confi
Features
~~~~~~~~
-Previously, ``name`` was a required argument for KubernetesPodOperator (when also not supplying pod template or full pod spec). Now, if ``name`` is not supplied, ``task_id`` will be used.
+* Previously, ``name`` was a required argument for KubernetesPodOperator (when also not supplying pod template or full pod spec). Now, if ``name`` is not supplied, ``task_id`` will be used.
+* KubernetsPodOperator argument ``namespace`` is now optional. If not supplied, we'll check the airflow conn,
Review Comment:
https://github.com/apache/airflow/blob/main/docs/apache-airflow-providers-cncf-kubernetes/operators.rst
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish merged pull request #27116: Make namespace optional for KPO
Posted by GitBox <gi...@apache.org>.
dstandish merged PR #27116:
URL: https://github.com/apache/airflow/pull/27116
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r1001006380
##########
tests/providers/cncf/kubernetes/operators/test_kubernetes_pod.py:
##########
@@ -123,20 +123,18 @@ def test_config_path(self, hook_mock):
labels={"foo": "bar"},
name="test",
task_id="task",
Review Comment:
Removed so much boilerplate. 😍
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r1001013416
##########
airflow/providers/cncf/kubernetes/hooks/kubernetes.py:
##########
@@ -329,11 +329,18 @@ def get_custom_object(
def get_namespace(self) -> str | None:
"""Returns the namespace that defined in the connection"""
if self.conn_id:
- connection = self.get_connection(self.conn_id)
- extras = connection.extra_dejson
- namespace = extras.get("extra__kubernetes__namespace", "default")
- return namespace
- return None
+ namespace = self.conn_extras.get("extra__kubernetes__namespace")
Review Comment:
Totally cool, sorry about that. Just making sure that one didn't already get done and we were regressing here.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #27116: Use current namespace if none provided in KPO
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27116:
URL: https://github.com/apache/airflow/pull/27116#discussion_r999998169
##########
tests/providers/cncf/kubernetes/operators/test_kubernetes_pod.py:
##########
@@ -74,19 +80,17 @@ def create_context(task, persist_to_db=False):
class TestKubernetesPodOperator:
@pytest.fixture(autouse=True)
def setup(self, dag_maker):
- self.create_pod_patch = mock.patch(f"{POD_MANAGER_CLASS}.create_pod")
- self.await_pod_patch = mock.patch(f"{POD_MANAGER_CLASS}.await_pod_start")
- self.await_pod_completion_patch = mock.patch(f"{POD_MANAGER_CLASS}.await_pod_completion")
- self.hook_patch = mock.patch(HOOK_CLASS)
Review Comment:
here we remove the global hook patch because we rely on it for namespace test
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org