-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add retrieve output docker swarm operator #41531
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
Add retrieve output docker swarm operator #41531
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
906897c to
bde1589
Compare
bde1589 to
651d9d5
Compare
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
|
@potiuk Thank you for validating the PR. Do I need to make a specific one for |
|
This is provider-only change - it will be released in the next docker provider Note! Current state of airflow development is that technically there will be no new 2.* releases any more (just 2.11 as a bridge release for Airflow 3) - but users will be able to upgrade providers for 2 for quite some time and we will keep on releasing them. |
|
unless I'm missing something, the docker sdk function to i also noticed that you're calling the The docker "tasks" APIs are what we should be using to access Docker Services tasks not the "container" APIs. What's confusing me is that this was tested but I'm not sure how. Unless I'm confused about the Docker Swarm APIs or there is some form of a Docker API gateway / proxy that is routing the docker APIs to the correct workers I'm not sure how this is working. |
|
Any comments for the above @rgriffier ? |
|
I'll have to check, I did test the code in Swarm mode but I'm not sure whether I was testing with a container deployed on a node other than the manager node... I'll have a look at it this weekend, I'll tell you again (but I'm fairly convinced by @spoutin's arguments, I'm afraid I tested in a configuration far from the reality of use)... |
|
I've just checked and it doesn't work if the container being deployed is not on the same host as the manager. |
Closes: #41445
Updates the
DockerSwarmOperatorto include the sameretrieve_outputfunctionality as theDockerOperator: get access of the content of a file as XCom at the end of a task, before the container is destroyed. To take account of possible container replication at deployment time, aTaskandContainerretrieval step has been added, in addition to the specificretrieve_outputfunctionality.The modifications were tested with the Dag provided below:
Here the results:
DockerOperator:DockerSwarmOperatorwithout replication:DockerSwarmOperatorwithreplicas=2:As
replicas=2in the Service configuration inside the DAG, 2 files were loaded inside thereturn_valueXCOM.If needed, I can create a specific PR for the
v2-10-testbranch !^ 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.rstor{issue_number}.significant.rst, in newsfragments.