Re: [PR] Remove init container from k8 [airflow]
davidsharp7 commented on code in PR #50448:
URL: https://github.com/apache/airflow/pull/50448#discussion_r2087323638
##
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/pod_generator.py:
##
@@ -355,38 +355,12 @@ def construct_pod(
containers=[main_container],
)
-if content_json_for_volume:
+if content_json:
import shlex
-input_file_path = "/tmp/execute/input.json"
-execute_volume = V1Volume(
-name="execute-volume",
-empty_dir=V1EmptyDirVolumeSource(),
-)
-
-execute_volume_mount = V1VolumeMount(
-name="execute-volume",
-mount_path="/tmp/execute",
-read_only=False,
-)
-
-escaped_json = shlex.quote(content_json_for_volume)
-init_container = k8s.V1Container(
-name="init-container",
-image="busybox",
-command=["/bin/sh", "-c", f"echo {escaped_json} >
{input_file_path}"],
-volume_mounts=[execute_volume_mount],
-)
-
-main_container.volume_mounts = [execute_volume_mount]
+escaped_json = shlex.quote(content_json)
Review Comment:
I will test this out today.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
davidsharp7 commented on code in PR #50448:
URL: https://github.com/apache/airflow/pull/50448#discussion_r2087322828
##
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/pod_generator.py:
##
@@ -355,38 +355,12 @@ def construct_pod(
containers=[main_container],
)
-if content_json_for_volume:
+if content_json:
import shlex
-input_file_path = "/tmp/execute/input.json"
-execute_volume = V1Volume(
-name="execute-volume",
-empty_dir=V1EmptyDirVolumeSource(),
-)
-
-execute_volume_mount = V1VolumeMount(
-name="execute-volume",
-mount_path="/tmp/execute",
-read_only=False,
-)
-
-escaped_json = shlex.quote(content_json_for_volume)
-init_container = k8s.V1Container(
-name="init-container",
-image="busybox",
-command=["/bin/sh", "-c", f"echo {escaped_json} >
{input_file_path}"],
-volume_mounts=[execute_volume_mount],
-)
-
-main_container.volume_mounts = [execute_volume_mount]
+escaped_json = shlex.quote(content_json)
main_container.command = args[:-1]
-main_container.args = args[-1:]
-
-podspec = k8s.V1PodSpec(
-containers=[main_container],
-volumes=[execute_volume],
-init_containers=[init_container],
-)
+main_container.args = escaped_json
Review Comment:
You are indeed correct.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
jpmenil commented on code in PR #50448:
URL: https://github.com/apache/airflow/pull/50448#discussion_r2086712744
##
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/pod_generator.py:
##
@@ -355,38 +355,12 @@ def construct_pod(
containers=[main_container],
)
-if content_json_for_volume:
+if content_json:
import shlex
-input_file_path = "/tmp/execute/input.json"
-execute_volume = V1Volume(
-name="execute-volume",
-empty_dir=V1EmptyDirVolumeSource(),
-)
-
-execute_volume_mount = V1VolumeMount(
-name="execute-volume",
-mount_path="/tmp/execute",
-read_only=False,
-)
-
-escaped_json = shlex.quote(content_json_for_volume)
-init_container = k8s.V1Container(
-name="init-container",
-image="busybox",
-command=["/bin/sh", "-c", f"echo {escaped_json} >
{input_file_path}"],
-volume_mounts=[execute_volume_mount],
-)
-
-main_container.volume_mounts = [execute_volume_mount]
+escaped_json = shlex.quote(content_json)
main_container.command = args[:-1]
-main_container.args = args[-1:]
-
-podspec = k8s.V1PodSpec(
-containers=[main_container],
-volumes=[execute_volume],
-init_containers=[init_container],
-)
+main_container.args = escaped_json
Review Comment:
```suggestion
main_container.args = [escaped_json]
```
should be a list if i'm not mistaken
https://github.com/kubernetes-client/python/blob/master/kubernetes/docs/V1Container.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.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
jpmenil commented on code in PR #50448:
URL: https://github.com/apache/airflow/pull/50448#discussion_r2086720268
##
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/pod_generator.py:
##
@@ -355,38 +355,12 @@ def construct_pod(
containers=[main_container],
)
-if content_json_for_volume:
+if content_json:
import shlex
-input_file_path = "/tmp/execute/input.json"
-execute_volume = V1Volume(
-name="execute-volume",
-empty_dir=V1EmptyDirVolumeSource(),
-)
-
-execute_volume_mount = V1VolumeMount(
-name="execute-volume",
-mount_path="/tmp/execute",
-read_only=False,
-)
-
-escaped_json = shlex.quote(content_json_for_volume)
-init_container = k8s.V1Container(
-name="init-container",
-image="busybox",
-command=["/bin/sh", "-c", f"echo {escaped_json} >
{input_file_path}"],
-volume_mounts=[execute_volume_mount],
-)
-
-main_container.volume_mounts = [execute_volume_mount]
+escaped_json = shlex.quote(content_json)
Review Comment:
doesn't seem to be needed as pydantic `model_dump_json()` should already
return a json encoded string?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
jpmenil commented on code in PR #50448:
URL: https://github.com/apache/airflow/pull/50448#discussion_r2086720268
##
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/pod_generator.py:
##
@@ -355,38 +355,12 @@ def construct_pod(
containers=[main_container],
)
-if content_json_for_volume:
+if content_json:
import shlex
-input_file_path = "/tmp/execute/input.json"
-execute_volume = V1Volume(
-name="execute-volume",
-empty_dir=V1EmptyDirVolumeSource(),
-)
-
-execute_volume_mount = V1VolumeMount(
-name="execute-volume",
-mount_path="/tmp/execute",
-read_only=False,
-)
-
-escaped_json = shlex.quote(content_json_for_volume)
-init_container = k8s.V1Container(
-name="init-container",
-image="busybox",
-command=["/bin/sh", "-c", f"echo {escaped_json} >
{input_file_path}"],
-volume_mounts=[execute_volume_mount],
-)
-
-main_container.volume_mounts = [execute_volume_mount]
+escaped_json = shlex.quote(content_json)
Review Comment:
doesn't seem to be needed as pydantic `model_dump_json()` should already a
json encoded string?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
eladkal commented on PR #50448: URL: https://github.com/apache/airflow/pull/50448#issuecomment-2875867056 Tests fail ``` === short test summary info FAILED tests/kubernetes_tests/test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag - AssertionError: assert equals failed 'failed' 'success' FAILED tests/kubernetes_tests/test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_task_mapping - AssertionError: assert equals failed 'failed' 'success' FAILED tests/kubernetes_tests/test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure - AssertionError: assert equals failed 'failed' 'success' === 3 failed, 47 passed, 3 skipped, 6 warnings in 287.36s (0:04:47) ``` -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
amoghrajesh commented on code in PR #50448: URL: https://github.com/apache/airflow/pull/50448#discussion_r2083855989 ## providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/pod_generator.py: ## @@ -358,35 +358,9 @@ def construct_pod( if content_json_for_volume: import shlex -input_file_path = "/tmp/execute/input.json" -execute_volume = V1Volume( -name="execute-volume", -empty_dir=V1EmptyDirVolumeSource(), -) - -execute_volume_mount = V1VolumeMount( -name="execute-volume", -mount_path="/tmp/execute", -read_only=False, -) - escaped_json = shlex.quote(content_json_for_volume) Review Comment: Lets just rename the `content_json_for_volume` -- its not that anymore. https://github.com/apache/airflow/pull/50448/files#diff-a82432d2e0fd9cf3be813faa6d4361eb102df82c56583caab8ebe30f10a7eb1dR290 ## providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py: ## @@ -398,8 +398,8 @@ def run_next(self, next_job: KubernetesJobType) -> None: "python", "-m", "airflow.sdk.execution_time.execute_workload", -"--json-path", -"/tmp/execute/input.json", +"--json-string", +"temp", Review Comment: Should this not be `ser_input`? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
davidsharp7 commented on PR #50448: URL: https://github.com/apache/airflow/pull/50448#issuecomment-2870694730 > > I will look at the integration tests. Breeze is being problematic. > > > > What problems do you have with it? Would love to hear. Corporate proxy 😀 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
potiuk commented on PR #50448: URL: https://github.com/apache/airflow/pull/50448#issuecomment-2870258256 > I will look at the integration tests. Breeze is being problematic. What problems do you have with it? Would love to hear. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
davidsharp7 commented on PR #50448: URL: https://github.com/apache/airflow/pull/50448#issuecomment-2870204265 I will look at the integration tests. Breeze is being problematic. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove init container from k8 [airflow]
amoghrajesh commented on PR #50448: URL: https://github.com/apache/airflow/pull/50448#issuecomment-2869843542 Thanks! I will take a look later today or tomorrow -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
