-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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 @task.kubernetes to receive input and send output #28942
Conversation
60e2d56
to
0a9a738
Compare
Hi @eladkal, do you know who might be a good person to review this? |
airflow/providers/cncf/kubernetes/python_kubernetes_script.jinja2
Outdated
Show resolved
Hide resolved
a55a51a
to
7251917
Compare
fd56410
to
9ba4d5d
Compare
Oops - sorry for pinging everyone. Forgot that I had done a rebase on the UI and didn't pull first. Feel free to unassign here! |
f"{_generate_decoded_command(quote(_PYTHON_SCRIPT_ENV), quote(script_filename))}" | ||
) | ||
write_local_input_file_cmd = ( | ||
f"{_generate_decoded_command(quote(_PYTHON_INPUT_ENV), quote(input_filename))}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shlex.quote here isn't really necessary anymore since they don't capture user input but I left it in anyways. Thought that made it a bit more readable but I can remove if you prefer.
b136a38
to
ed11a95
Compare
Hey @uranusjr , anything else I can do here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
5d20cbb
to
3ae0602
Compare
3ae0602
to
36b0d1d
Compare
Can someone help merge this? |
36b0d1d
to
2c6cbce
Compare
@uranusjr , can you help merge this please? |
Hey @vchiapaikeo, I was just looking in to this very problem and saw this fix. Nice work! Thank you :) |
@fletchjeff Do you know when this gonna be released? |
@okulbida , this went out with cncf providers release 5.2.1 https://airflow.apache.org/docs/apache-airflow-providers-cncf-kubernetes/stable/index.html#id4 |
closes: #28933
@task.kubernetes
currently does not handle receiving input nor does it properly return output. This PR addresses these issues by passing input to the K8's pod (via an env var) and output where the K8's xcom sidecar expects it (as /airflow/xcom/result.json).It also seemed like the initial implementation did not properly base64 encode / decode the values. Since there might be unusual characters / line breaks in both the function and inputs, I added base64 encoding to the env var setting (similar to how it's handled in docker).
Testing
To test, I started Airflow w/ breeze (
breeze --python 3.9 --backend postgres start-airflow
), added a google_cloud_default connection_id to my sandbox GCP project, and copied my kube_config file to /files/.kube/config. Because I am using GCP, I needed to install gcloud. I do so with the following commands in the breeze shell:I then ran this test DAG:
Results
With Input:
With Input and Output:
No input and Output:
With multiple_outputs = True:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.