Adds option to disable mounting temporary folder in DockerOperator

[potiuk]

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

---

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

[potiuk]

Hey @uranusjr @mik-laj @akki -> this turned out to be a bit of aftermath after #15843 - seems that remote docker engine or docker-in-docker case are broken in the new provider 2.0.0. I am not 100% sure if the fix I provided is good enough - it's essentially a breaking change for those cases. Not sure if we can do it more transparently ?

[akki]

Added a suggestion.

[akki]

Oops, selected "requested changes" by mistake.
I don't have a strong opinion about the solution proposed in this PR yet.

[potiuk]

Hey @uranusjr @akki - I think this version is a bit better.

In this version you can still disable the temporary mount, but also I brought in the fallback - in case we run the docker command and it fails (and from the message we suspect it is about this problem) we try again without mounting (and without setting the _TMP variable) - we also log warning in this case. I think that should brig the old behaviour back (with slight overhead/performance hit), while also adding flexibility to disable the /tmp mount and a warning when it should be disabled.

I'd love to merge this one rather quickly as this is the last 'serious' provider change I think is "urgent-ish" to merge before releasing the next wave of providers.

[raphaelauv]

Since it's from 2.0.0 let's consider the mount of the tmp folder the default pattern and simply add an option to disable it in case of docker in docker

I prefer to keep the operator KISS, so it should not retry in case of bad configuration.

Thanks @potiuk for writing a fix 👍

[uranusjr]

The cause to the failure is that old binds thing automatically create an empty directory if the host path does not exist; the new mounts thing does not do that, but fails loudly (a delibrate decision made by Docker to prevent subtle bugs just like this). So the previous implementation isn’t really working either, but broken in a different way, the apparently-mounted temporary directory was actually not able to be used when used in the Docker-in-Docker setup. So in a way this is catching a subtle bug that went unnoticed previously.

So IMO your fix makes sense. The temporary directory thing won’t work for the Docker-in-Docker setup anyway, so an additional argument makes sense. This should also be recorded in the 2.0 changelog; not in the way proposed in #17008, but something like “previously the temporary directory mount was silently failing when [not sure what best to put here], now it will fail loudly and you must explicitly pass mount_tmp_dir=False when you don’t need the temporary directory”. And maybe we can also deprecate the implicit mount_tmp_dir=True behaviour and switch to not mount the temporary directory by default in 3.0.

[uranusjr]

Change is good as-is, the changelog addition can be done either here or in another PR.

[github-actions]

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 main or amend the last commit of the PR, and push it with --force-with-lease.

[potiuk]

Change is good as-is, the changelog addition can be done either here or in another PR.

Yep. I will add the changelog when preparing provider's release.

[potiuk]

The cause to the failure is that old binds thing automatically create an empty directory if the host path does not exist; the new mounts thing does not do that, but fails loudly (a delibrate decision made by Docker to prevent subtle bugs just like this). So the previous implementation isn’t really working either, but broken in a different way, the apparently-mounted temporary directory was actually not able to be used when used in the Docker-in-Docker setup. So in a way this is catching a subtle bug that went unnoticed previously.

Make perfect sense :). And also I think in this case, it is much better to give the user an explicit decision on the mounting. With this approach, you can make your own decisions how to prepare the kind of "temporary" folder you want to use - you can use tmpfs volume if you have plenty of memory, you can use existing volume, or let the engine create the volume for you.

[potiuk]

I prefer to keep the operator KISS, so it should not retry in case of bad configuration.

I thought about it, and I think I prefer to have this retry mechanism for at least some time (until the next major version bump). The reason is that we really try hard to keep to the spirit of the SemVer, and we have a clear regression that was not the reason for the 2.0.0 bump and user should expect not to have to modify their dags. So 2.1.0 will come with a fix and then provider 2.1.0 will work 'same' way for vast majority of cases. The "temporary folder" approach was not needed by everyone (it seems to address a very specific case). And it was not really working as expected anyway in case of remote/dind. Here we have at least clear warning and indication that the parameters should be updated. Also the fact that the variable is not set in this case might make the container to fail, but in this case it is good, because it will reveal the subtle bug which was hidden before.

[potiuk]

Release notes added: #17015