-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
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
DockerSwarmOperator
to include the sameretrieve_output
functionality 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, aTask
andContainer
retrieval step has been added, in addition to the specificretrieve_output
functionality.The modifications were tested with the Dag provided below:
Here the results:
DockerOperator
:DockerSwarmOperator
without replication:DockerSwarmOperator
withreplicas=2
:As
replicas=2
in the Service configuration inside the DAG, 2 files were loaded inside thereturn_value
XCOM.If needed, I can create a specific PR for the
v2-10-test
branch !^ 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.