fix: local mode deletion of temp files on job end (#3017)

Author: jmahlik

src/sagemaker/local/utils.py

@@ -14,16 +14,20 @@
 from __future__ import absolute_import
 
 import os
+import logging
 import shutil
 import subprocess
 import json
 import re
+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.
@@ -77,7 +81,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/sagemaker/local/test_local_utils.py

@@ -13,6 +13,7 @@
 from __future__ import absolute_import
 
 import os
+import errno
 import pytest
 from mock import patch, Mock
 
@@ -168,3 +169,20 @@ def test_get_using_dot_notation_key_error():
 def test_get_using_dot_notation_index_error():
     with pytest.raises(ValueError):
         sagemaker.local.utils.get_using_dot_notation({"foo": ["bar"]}, "foo[1]")
+
+
+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)
+    mock_rmtree.assert_called_once()
+    recursive_copy.assert_called_with(
+        "/tmp/data", os.path.abspath(os.path.join(os.sep, "target", "dir"))
+    )