New syntax to mount Docker volumes with --mount

[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

• Mounting directories using docker operator on airflow is not working #12537: Confusing on the bind syntax.
• DockerSwarmOperator can not mount a volume #9047: Implement mounting in DockerSwarmOperator (it’s a subclass of DockerOperator, but the volumes option is currently unused).

[potiuk]

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.

[uranusjr]

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

[potiuk]

Works for me!

[uranusjr]

PR created 🙂

[eladkal]

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

[akki]

Hi @eladkal

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