fix: local mode deletion of temp files

Author: jmahlik

src/sagemaker/local/utils.py

@@ -14,14 +14,18 @@
 from __future__ import absolute_import
 
 import os
+import logging
 import shutil
 import subprocess
+import errno
 
 from distutils.dir_util import copy_tree
 from six.moves.urllib.parse import urlparse
 
 from sagemaker import s3
 
+logger = logging.getLogger(__name__)
+
 
 def copy_directory_structure(destination_directory, relative_path):
     """Creates intermediate directory structure for relative_path.
@@ -74,7 +78,19 @@ def move_to_destination(source, destination, job_name, sagemaker_session):
     else:
         raise ValueError("Invalid destination URI, must be s3:// or file://, got: %s" % destination)
 
-    shutil.rmtree(source)
+    try:
+        shutil.rmtree(source)
+    except OSError as exc:
+        # on Linux, when docker writes to any mounted volume, it uses the container's user. In most
+        # cases this is root. When the container exits and we try to delete them we can't because
+        # root owns those files. We expect this to happen, so we handle EACCESS. Any other error
+        # we will raise the exception up.
+        if exc.errno == errno.EACCES:
+            logger.warning("Failed to delete: %s Please remove it manually.", source)
+        else:
+            logger.error("Failed to delete: %s", source)
+            raise
+
     return final_uri
 
 

tests/unit/test_local_utils.py

@@ -12,6 +12,7 @@
 # language governing permissions and limitations under the License.
 from __future__ import absolute_import
 
+import errno
 import pytest
 from mock import patch, Mock
 
@@ -52,3 +53,17 @@ def test_move_to_destination_s3(recursive_copy):
 def test_move_to_destination_illegal_destination():
     with pytest.raises(ValueError):
         sagemaker.local.utils.move_to_destination("/tmp/data", "ftp://ftp/in/2018", "job", None)
+
+
+def raise_os_error(args):
+    err = OSError()
+    err.errno = errno.EACCES
+    raise err
+
+
+@patch("shutil.rmtree", side_effect=raise_os_error)
+@patch("sagemaker.local.utils.recursive_copy")
+def test_move_to_destination_local_root_failure(recursive_copy, mock_rmtree):
+    # This should not raise, in case root owns files, make sure it doesn't
+    sagemaker.local.utils.move_to_destination("/tmp/data", "file:///target/dir/", "job", None)
+    recursive_copy.assert_called_with("/tmp/data", "/target/dir/")