[GitHub] [airflow] akki commented on a change in pull request #16932: Adds option to disable mounting temporary folder in DockerOperator

[GitBox <gi...@apache.org>]

akki commented on a change in pull request #16932:
URL: https://github.com/apache/airflow/pull/16932#discussion_r668269023



##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -227,66 +240,66 @@ def get_hook(self) -> DockerHook:
     def _run_image(self) -> Optional[str]:
         """Run a Docker container with the provided image"""
         self.log.info('Starting docker container from image %s', self.image)
+        if not self.cli:
+            raise Exception("The 'cli' should be initialized before!")
+        if self.mount_tmp_dir:
+            with TemporaryDirectory(prefix='airflowtmp', dir=self.host_tmp_dir) as host_tmp_dir:
+                tmp_mount = Mount(self.tmp_dir, host_tmp_dir, "bind")
+                target_mounts = self.mounts + [tmp_mount]
+                return self._run_image_with_mounts(target_mounts)
+        else:
+            return self._run_image_with_mounts(self.mounts)
 
-        with TemporaryDirectory(prefix='airflowtmp', dir=self.host_tmp_dir) as host_tmp_dir:
-            if not self.cli:
-                raise Exception("The 'cli' should be initialized before!")
-            tmp_mount = Mount(self.tmp_dir, host_tmp_dir, "bind")

Review comment:
       We were using `bind` instead of `mount` before [PR#15843](https://github.com/apache/airflow/pull/15843/files#diff-5ec82e1fcaaa38e04471f56b64ec319f67b146d3a18d8bdb69c2f973ea3dd585R243).
   From the Docker docs API specs, I found that they are calling `bind` as "volume bindings" (search for "Binds" under `HostConfig` under the [`/container/create` API](https://docs.docker.com/engine/api/v1.30/#operation/ContainerCreate)).
   
   
   In the [Python Docker API `Mount` specs](https://docker-py.readthedocs.io/en/stable/api.html#docker.types.Mount), I see the option of `volume`.
   If you already have the setup to reproduce this bug, may I request you to try with just this one-liner change in `2.0.0` release code -> `Mount(self.tmp_dir, host_tmp_dir, "bind")` to `Mount(self.tmp_dir, host_tmp_dir, "volume")` and see if that does the trick?
   
   
   Unfortunately, the Docker API spec doesn't seem very clear about how `bind` is different from `mount` BUT the Python spec describes `mounts` as "a More powerful alternative to binds" so I believe we should be able to achieve the older "binds" behaviour with the new "mounts" parameter too.




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

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