Replace DockerOperator's 'volumes' arg for 'mounts' #15843
Conversation
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
There was a problem hiding this comment.
Thanks for the patch & great work.
I have left some comments regarding some concerns that I see.
One more thing that I am concerned about is that will users now explicitly need to do pip install docker to use this feature (as they will now need to import docker.types.Mount into their DAGs) or is that taken care of by your changes to docs/conf.py?
| @@ -77,6 +77,7 @@ def _client_service_logs_effect(): | |||
| image='ubuntu:latest', | |||
| command='env', | |||
| user='unittest', | |||
| mounts=[], | |||
There was a problem hiding this comment.
IIUC this test would pass even if there were no changes made to DockerSwarmOperator in this PR (since we're checking the default value of DockerOperator right now).
I think it is worth passing some value to the operator & checking it gets passed here as expected.
There was a problem hiding this comment.
I’ve modified the test case to ensure the variable is passed.
|
Ohh yeah and BTW, since it's not too late, you might also be able to fix a typo in "Swarm" in your commit message 😉 |
|
Much nicer rendering:
|
All existing 'volumes' usages are also migrated to use 'mounts' instead. This also includes a fix to DockerSwarmOperator's inability to mount things into the container; the new argument is also passed to Swam, so users can mount by setting 'mounts' on DockerSwarmOprerator as well.
uranusjr
force-pushed
the
docker-volumes-to-mount
branch
from
a7258bd to
f01c89f
Compare
uranusjr
force-pushed
the
docker-volumes-to-mount
branch
from
f01c89f to
785c141
Compare
github-actions
bot
added
the
okay to merge
|
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease. |
The DockerOperator by default mounts temporary folder to inside the container in order to allow to store files bigger than default size of disk for the container, however this did not work when remote Docker engine or Docker-In-Docker solution was used. This worked before the apache#15843 change, because the /tmp has been ignored, however when we change to "Mounts", the "/tmp" mount fails when using remote docker engine. This PR adds parameter that allows to disable this temporary directory mounting (and adds a note that it can be replaced with mounting existing volumes). Also it prints a warning if the directory cannot be mounted and attempts to re-run such failed attempt without mounting the temporary directory which brings back backwards-compatible behaviour for remote engines and docker-in-docker. Fixes: apache#16803 Fixes: apache#16806
…16932) * Adds option to disable mounting temporary folder in DockerOperator The DockerOperator by default mounts temporary folder to inside the container in order to allow to store files bigger than default size of disk for the container, however this did not work when remote Docker engine or Docker-In-Docker solution was used. This worked before the #15843 change, because the /tmp has been ignored, however when we change to "Mounts", the "/tmp" mount fails when using remote docker engine. This PR adds parameter that allows to disable this temporary directory mounting (and adds a note that it can be replaced with mounting existing volumes). Also it prints a warning if the directory cannot be mounted and attempts to re-run such failed attempt without mounting the temporary directory which brings back backwards-compatible behaviour for remote engines and docker-in-docker. Fixes: #16803 Fixes: #16806
Fix #9047, close #15815. I *think this can also resolve #12537 (less sure about that).
All existing
volumesusages are also migrated to usemountsinstead.This also includes a fix to
DockerSwamOperator’s inability to mount things into the container; the new argument is also passed to Swam, so users can mount by settingmountsonDockerSwamOpreratoras well.Should I add entries to the Docker provider’s changelog now, or is it done when the provider is released?
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.