Skip to content

New syntax to mount Docker volumes with --mount #15815

Description

@uranusjr

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. 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 to describe the mounts. They will be passed directly to the mounts API. Some shorthands could be useful as well, for example:

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

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

Activity

  1. potiuk commented on May 14, 2021

    @potiuk
    Member

    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.

  2. uranusjr commented on May 14, 2021

    @uranusjr
    MemberAuthor

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

  3. potiuk commented on May 14, 2021

    @potiuk
    Member

    Works for me!

  4. uranusjr commented on May 14, 2021

    @uranusjr
    MemberAuthor

    PR created 🙂

  5. eladkal commented on May 14, 2021

    @eladkal
    Contributor

    @akki followup on #9047 maybe you can share your thoughts :)

  6. akki commented on May 16, 2021

    @akki
    Contributor

    Hi @eladkal

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions