You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/05/13 06:29:13 UTC

[GitHub] [airflow] uranusjr opened a new issue #15815: New syntax to mount Docker volumes with --mount

uranusjr opened a new issue #15815:
URL: https://github.com/apache/airflow/issues/15815


   I had this after reading #12537 and #9047. Currently `DockerOperator`’s `volumes` argument is passed directly to docker-py’s `bind` (aka `docker -v`). But `-v`’s behaviour has long been problematic, and [Docker has been pushing users to the new `--mount` syntax instead](https://docs.docker.com/storage/bind-mounts/#choose-the--v-or---mount-flag). With #12537, it seems like `-v`’s behaviour is also confusing to some Airflow users, so I want to migrate Airflow’s internals to `--mount`.
   
   However, `--mount` has a different syntax to `-v`, and the behaviour is also slightly different, so for compatibility reasons we can’t just do it under the hood. I can think of two possible solutions to this:
   
   A. Deprecate `volumes` altogether and introduce `DockerOperator(mounts=...)`
   
   This will emit a deprecation warning when the user passes `DockerOperator(volumes=...)` to tell them to convert to `DockerOperator(mounts=...)` instead. `volumes` will stay unchanged otherwise, and continue to be passed to bind mounts.
   
   `mounts` will take a list of [`docker.types.Mount`](https://docker-py.readthedocs.io/en/stable/api.html#docker.types.Mount) to describe the mounts. They will be passed directly to the mounts API. Some shorthands could be useful as well, for example:
   
   ```python
   DockerOperator(
       ...
       mounts=[
           ('/root/data1', './data1'),  # Source and target, default to volume mount.
           ('/root/data2', './data2', 'bind'),  # Bind mount.
       ],
   )
   ```
   
   B. Reuse `volumes` and do introspection to choose between binds and mounts
   
   The `volumes` argument can be augmented to also accept `docker.types.Mount` instances, and internally we’ll do something like
   
   ```python
   binds = []
   mounts = []
   for vol in volumes:
       if isinstance(vol, str):
           binds.append(vol)
       elif isintance(vol, docker.types.Mount):
           mounts.append(vol)
       else:
           raise ValueError('...')
   if binds:
       warnings.warn('...', DeprecationWarning)
   ```
   
   and pass the collected lists to binds and mounts respectively.
   
   I’m very interested in hearing thoughts on this.
   
   
   **Are you willing to submit a PR?**
   Yes
   
   **Related Issues**
   
   * #12537: Confusing on the bind syntax.
   * #9047: Implement mounting in `DockerSwarmOperator` (it’s a subclass of `DockerOperator`, but the `volumes` option is currently unused).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #15815:
URL: https://github.com/apache/airflow/issues/15815#issuecomment-841027480


   DockerOperator is part of Docker Provider. We are not too concerned about the compatibiliy of providers to be honest, because those can be rather easily updated on provider-by-provider case (and in most cases you can downgrade the provider).
   
   So I think this is perfectly ok to make breaking change in Docker Provider and bump major version for the provider.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk closed issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

potiuk closed issue #15815:
URL: https://github.com/apache/airflow/issues/15815


   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] eladkal commented on issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

eladkal commented on issue #15815:
URL: https://github.com/apache/airflow/issues/15815#issuecomment-841104606


   @akki followup on https://github.com/apache/airflow/issues/9047 maybe you can share your thoughts :) 


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] akki commented on issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

akki commented on issue #15815:
URL: https://github.com/apache/airflow/issues/15815#issuecomment-841785008


   Hi @eladkal 
   
   Thanks for pinging me. I had a look at the PR & left some comments. Hope that helps. :) 


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] akki commented on issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

akki commented on issue #15815:
URL: https://github.com/apache/airflow/issues/15815#issuecomment-841785008


   Hi @eladkal 
   
   Thanks for pinging me. I had a look at the PR & left some comments. Hope that helps. :) 


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #15815:
URL: https://github.com/apache/airflow/issues/15815#issuecomment-841089574


   Works for me!


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk closed issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

potiuk closed issue #15815:
URL: https://github.com/apache/airflow/issues/15815


   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] uranusjr commented on issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

uranusjr commented on issue #15815:
URL: https://github.com/apache/airflow/issues/15815#issuecomment-841066451


   In that case I think I’ll go with Option A, but removing `volumes` instead of deprecating.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] uranusjr commented on issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

uranusjr commented on issue #15815:
URL: https://github.com/apache/airflow/issues/15815#issuecomment-841099892


   PR created 🙂 


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #15815: New syntax to mount Docker volumes with --mount

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #15815:
URL: https://github.com/apache/airflow/issues/15815#issuecomment-841027480


   DockerOperator is part of Docker Provider. We are not too concerned about the compatibiliy of providers to be honest, because those can be rather easily updated on provider-by-provider case (and in most cases you can downgrade the provider.
   
   So I think this is perfectly ok to make breaking change in Docker Provider and bump major version for the provider.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org