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 2023/01/09 17:22:53 UTC
[GitHub] [airflow] csm10495 opened a new pull request, #28808: Allow setting the name for the base container within K8s Pod Operator
csm10495 opened a new pull request, #28808:
URL: https://github.com/apache/airflow/pull/28808
Some downstream machinery may require a specific name for the 'base' container.
This change should be backwards compatible since the default base container name is still base.. it can just now be changed to something besides base.
This is the second part of splitting up: #28752
--
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] csm10495 commented on pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by "csm10495 (via GitHub)" <gi...@apache.org>.
csm10495 commented on PR #28808:
URL: https://github.com/apache/airflow/pull/28808#issuecomment-1399136793
Also does that change it just for the instance or for the class if done that way? .. I'd have to check to know for sure.
--
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] jedcunningham commented on a diff in pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #28808:
URL: https://github.com/apache/airflow/pull/28808#discussion_r1067376758
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -272,6 +273,7 @@ def __init__(
pod_runtime_info_envs: list[k8s.V1EnvVar] | None = None,
termination_grace_period: int | None = None,
configmaps: list[str] | None = None,
+ base_container_name: str = "base",
Review Comment:
That's basically what I'm saying. The "how" (default of none vs the constant) doesn't really matter at the end of the day, they are functionally equivalent:
```
BASE_CONTAINER_NAME = "base"
...
base_container_name: str = BASE_CONTAINER_NAME
...
self.base_container_name = base_container_name
```
and
```
BASE_CONTAINER_NAME = "base"
...
base_container_name: str | None = None
...
self.base_container_name = base_container_name or BASE_CONTAINER_NAME
```
--
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] jedcunningham commented on a diff in pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #28808:
URL: https://github.com/apache/airflow/pull/28808#discussion_r1067281656
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -272,6 +273,7 @@ def __init__(
pod_runtime_info_envs: list[k8s.V1EnvVar] | None = None,
termination_grace_period: int | None = None,
configmaps: list[str] | None = None,
+ base_container_name: str = "base",
Review Comment:
I think we should switch this back so the default is still coming from `BASE_CONTAINER_NAME`. This makes it possible to change the default instance wide, and it also means this is a feature that can ship in a minor release vs being a breaking change that requires a major version.
--
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] csm10495 commented on pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
csm10495 commented on PR #28808:
URL: https://github.com/apache/airflow/pull/28808#issuecomment-1384319670
@eladkal can you check again?
--
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] csm10495 commented on pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
csm10495 commented on PR #28808:
URL: https://github.com/apache/airflow/pull/28808#issuecomment-1381019209
@jedcunningham check again when you can. Thanks!
--
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] csm10495 commented on a diff in pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
csm10495 commented on code in PR #28808:
URL: https://github.com/apache/airflow/pull/28808#discussion_r1067534621
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -272,6 +273,7 @@ def __init__(
pod_runtime_info_envs: list[k8s.V1EnvVar] | None = None,
termination_grace_period: int | None = None,
configmaps: list[str] | None = None,
+ base_container_name: str = "base",
Review Comment:
Got it. Updated. Please check again.
--
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] eladkal commented on a diff in pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #28808:
URL: https://github.com/apache/airflow/pull/28808#discussion_r1065003786
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -206,9 +206,10 @@ class KubernetesPodOperator(BaseOperator):
to populate the environment variables with. The contents of the target
ConfigMap's Data field will represent the key-value pairs as environment variables.
Extends env_from.
+ :param base_container_name: The name of the base container in the pod. This container's logs
Review Comment:
Should we have some doc explnations about this? Mainly around what this capability brings
--
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] potiuk commented on pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28808:
URL: https://github.com/apache/airflow/pull/28808#issuecomment-1398633574
@jedcunningham @eladkal - any comments or shall we merge it ?
--
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] jedcunningham commented on pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #28808:
URL: https://github.com/apache/airflow/pull/28808#issuecomment-1398700496
Looking at this with fresh eyes, this works?
```
kpo_task = KubernetesPodOperator(...)
kpo_task.BASE_CONTAINER_NAME = "whatever"
```
Is it worth adding this to init for an edge case?
--
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] jedcunningham commented on a diff in pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #28808:
URL: https://github.com/apache/airflow/pull/28808#discussion_r1084275635
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
@@ -1153,3 +1153,134 @@ def test_using_resources(self):
do_xcom_push=False,
resources=resources,
)
+
+ def test_changing_base_container_name_with_get_logs(self):
+ k = KubernetesPodOperator(
+ namespace="default",
+ image="ubuntu:16.04",
+ cmds=["bash", "-cx"],
+ arguments=["echo 10"],
+ labels=self.labels,
+ task_id=str(uuid4()),
+ in_cluster=False,
+ do_xcom_push=False,
+ get_logs=True,
+ base_container_name="apple-sauce",
+ )
+ assert k.base_container_name == "apple-sauce"
+ context = create_context(k)
+ with mock.patch.object(
+ k.pod_manager, "fetch_container_logs", wraps=k.pod_manager.fetch_container_logs
+ ) as mock_fetch_container_logs:
+ k.execute(context)
+
+ assert mock_fetch_container_logs.call_args[1]["container_name"] == "apple-sauce"
+ actual_pod = self.api_client.sanitize_for_serialization(k.pod)
+ self.expected_pod["spec"]["containers"][0]["name"] = "apple-sauce"
+ assert self.expected_pod["spec"] == actual_pod["spec"]
+
+ def test_changing_base_container_name_no_logs(self):
+ """
+ This test checks BOTH a modified base container name AND the get_logs=False flow..
+ and as a result, also checks that the flow works with fast containers
+ See #26796
Review Comment:
```suggestion
This test checks BOTH a modified base container name AND the get_logs=False flow,
and as a result, also checks that the flow works with fast containers
See https://github.com/apache/airflow/issues/26796
```
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
@@ -1153,3 +1153,134 @@ def test_using_resources(self):
do_xcom_push=False,
resources=resources,
)
+
+ def test_changing_base_container_name_with_get_logs(self):
+ k = KubernetesPodOperator(
+ namespace="default",
+ image="ubuntu:16.04",
+ cmds=["bash", "-cx"],
+ arguments=["echo 10"],
+ labels=self.labels,
+ task_id=str(uuid4()),
+ in_cluster=False,
+ do_xcom_push=False,
+ get_logs=True,
+ base_container_name="apple-sauce",
+ )
+ assert k.base_container_name == "apple-sauce"
+ context = create_context(k)
+ with mock.patch.object(
+ k.pod_manager, "fetch_container_logs", wraps=k.pod_manager.fetch_container_logs
+ ) as mock_fetch_container_logs:
+ k.execute(context)
+
+ assert mock_fetch_container_logs.call_args[1]["container_name"] == "apple-sauce"
+ actual_pod = self.api_client.sanitize_for_serialization(k.pod)
+ self.expected_pod["spec"]["containers"][0]["name"] = "apple-sauce"
+ assert self.expected_pod["spec"] == actual_pod["spec"]
+
+ def test_changing_base_container_name_no_logs(self):
+ """
+ This test checks BOTH a modified base container name AND the get_logs=False flow..
+ and as a result, also checks that the flow works with fast containers
+ See #26796
+ """
+ k = KubernetesPodOperator(
+ namespace="default",
+ image="ubuntu:16.04",
+ cmds=["bash", "-cx"],
+ arguments=["echo 10"],
+ labels=self.labels,
+ task_id=str(uuid4()),
+ in_cluster=False,
+ do_xcom_push=False,
+ get_logs=False,
+ base_container_name="apple-sauce",
+ )
+ assert k.base_container_name == "apple-sauce"
+ context = create_context(k)
+ with mock.patch.object(
+ k.pod_manager, "await_container_completion", wraps=k.pod_manager.await_container_completion
+ ) as mock_await_container_completion:
+ k.execute(context)
+
+ assert mock_await_container_completion.call_args[1]["container_name"] == "apple-sauce"
+ actual_pod = self.api_client.sanitize_for_serialization(k.pod)
+ self.expected_pod["spec"]["containers"][0]["name"] = "apple-sauce"
+ assert self.expected_pod["spec"] == actual_pod["spec"]
+
+ def test_changing_base_container_name_no_logs_long(self):
+ """
+ Similar to test_changing_base_container_name_no_logs, but ensures that
+ pods running longer than 1 second work too. See #26796
Review Comment:
```suggestion
pods running longer than 1 second work too.
See https://github.com/apache/airflow/issues/26796
```
--
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] csm10495 commented on pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by "csm10495 (via GitHub)" <gi...@apache.org>.
csm10495 commented on PR #28808:
URL: https://github.com/apache/airflow/pull/28808#issuecomment-1399135597
It doesn't really match how params are normally passed through.
Also it would look kind of weird in a taskflow dag. I think the PR as is makes a bit more sense.
--
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] jedcunningham merged pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham merged PR #28808:
URL: https://github.com/apache/airflow/pull/28808
--
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] csm10495 commented on a diff in pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
csm10495 commented on code in PR #28808:
URL: https://github.com/apache/airflow/pull/28808#discussion_r1065007142
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -206,9 +206,10 @@ class KubernetesPodOperator(BaseOperator):
to populate the environment variables with. The contents of the target
ConfigMap's Data field will represent the key-value pairs as environment variables.
Extends env_from.
+ :param base_container_name: The name of the base container in the pod. This container's logs
Review Comment:
I don't think the concept of 'base container' is mentioned anywhere already. It seems to just be the name given to the first container.. and is also the one that can be logged via get_logs=True. That info is now in this docstr. Anywhere else where we should mention this?
--
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] csm10495 commented on a diff in pull request #28808: Allow setting the name for the base container within K8s Pod Operator
Posted by GitBox <gi...@apache.org>.
csm10495 commented on code in PR #28808:
URL: https://github.com/apache/airflow/pull/28808#discussion_r1067308485
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -272,6 +273,7 @@ def __init__(
pod_runtime_info_envs: list[k8s.V1EnvVar] | None = None,
termination_grace_period: int | None = None,
configmaps: list[str] | None = None,
+ base_container_name: str = "base",
Review Comment:
But what about for folks who don't want to change this instance wide and instead want to set it on a single basis? To fully maintain support for BASE_CONTAINER_NAME why not also have that and default this new param to None, then if this is set it takes precedence, if not falls back to BASE_CONTAINER_NAME?
--
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