Use chmod -R 777 via Docker

Author: mollyheamazon

sagemaker-core/src/sagemaker/core/modules/local_core/local_container.py

@@ -63,7 +63,7 @@ def _rmtree(path, image=None, is_studio=False):
         shutil.rmtree(path)
     except PermissionError:
         # Files created by Docker containers are owned by root.
-        # Use a Docker container to remove them since os.chmod will also fail.
+        # Use docker to chmod as root, then retry shutil.rmtree.
         if image is None:
             logger.warning(
                 "Failed to clean up root-owned files in %s. "
@@ -75,11 +75,9 @@ def _rmtree(path, image=None, is_studio=False):
             cmd = ["docker", "run", "--rm"]
             if is_studio:
                 cmd += ["--network", "sagemaker"]
-            cmd += ["-v", f"{path}:/delete", image, "sh", "-c", "find /delete -mindepth 1 -delete"]
+            cmd += ["-v", f"{path}:/delete", image, "chmod", "-R", "777", "/delete"]
             subprocess.run(cmd, check=True, capture_output=True)
-            # The mount point directory itself may remain — clean it up
-            if os.path.exists(path):
-                shutil.rmtree(path, ignore_errors=True)
+            shutil.rmtree(path)
         except Exception:
             logger.warning(
                 "Failed to clean up root-owned files in %s. "

sagemaker-train/src/sagemaker/train/local/local_container.py

@@ -71,7 +71,7 @@ def _rmtree(path, image=None, is_studio=False):
         shutil.rmtree(path)
     except PermissionError:
         # Files created by Docker containers are owned by root.
-        # Use a Docker container to remove them since os.chmod will also fail.
+        # Use docker to chmod as root, then retry shutil.rmtree.
         if image is None:
             logger.warning(
                 "Failed to clean up root-owned files in %s. "
@@ -83,11 +83,9 @@ def _rmtree(path, image=None, is_studio=False):
             cmd = ["docker", "run", "--rm"]
             if is_studio:
                 cmd += ["--network", "sagemaker"]
-            cmd += ["-v", f"{path}:/delete", image, "sh", "-c", "find /delete -mindepth 1 -delete"]
+            cmd += ["-v", f"{path}:/delete", image, "chmod", "-R", "777", "/delete"]
             subprocess.run(cmd, check=True, capture_output=True)
-            # The mount point directory itself may remain — clean it up
-            if os.path.exists(path):
-                shutil.rmtree(path, ignore_errors=True)
+            shutil.rmtree(path)
         except Exception:
             logger.warning(
                 "Failed to clean up root-owned files in %s. "

sagemaker-train/tests/unit/train/local/test_local_container.py

@@ -29,29 +29,24 @@ def test_rmtree_success(self, mock_rmtree):
 
     @patch("sagemaker.train.local.local_container.shutil.rmtree")
     @patch("sagemaker.train.local.local_container.subprocess.run")
-    @patch("sagemaker.train.local.local_container.os.path.exists", return_value=False)
-    def test_rmtree_permission_error_docker_fallback(self, mock_exists, mock_run, mock_rmtree):
-        """PermissionError triggers docker fallback using the training image."""
-        mock_rmtree.side_effect = PermissionError("Permission denied")
+    def test_rmtree_permission_error_docker_chmod_fallback(self, mock_run, mock_rmtree):
+        """PermissionError triggers docker chmod then retry."""
+        mock_rmtree.side_effect = [PermissionError("Permission denied"), None]
 
         _rmtree("/tmp/test", IMAGE)
 
         mock_run.assert_called_once_with(
-            [
-                "docker", "run", "--rm",
-                "-v", "/tmp/test:/delete", IMAGE,
-                "sh", "-c", "find /delete -mindepth 1 -delete",
-            ],
+            ["docker", "run", "--rm", "-v", "/tmp/test:/delete", IMAGE, "chmod", "-R", "777", "/delete"],
             check=True,
             capture_output=True,
         )
+        assert mock_rmtree.call_count == 2
 
     @patch("sagemaker.train.local.local_container.shutil.rmtree")
     @patch("sagemaker.train.local.local_container.subprocess.run")
-    @patch("sagemaker.train.local.local_container.os.path.exists", return_value=False)
-    def test_rmtree_studio_adds_network(self, mock_exists, mock_run, mock_rmtree):
+    def test_rmtree_studio_adds_network(self, mock_run, mock_rmtree):
         """In Studio, docker run includes --network sagemaker."""
-        mock_rmtree.side_effect = PermissionError("Permission denied")
+        mock_rmtree.side_effect = [PermissionError("Permission denied"), None]
 
         _rmtree("/tmp/test", IMAGE, is_studio=True)
 
@@ -60,27 +55,12 @@ def test_rmtree_studio_adds_network(self, mock_exists, mock_run, mock_rmtree):
                 "docker", "run", "--rm",
                 "--network", "sagemaker",
                 "-v", "/tmp/test:/delete", IMAGE,
-                "sh", "-c", "find /delete -mindepth 1 -delete",
+                "chmod", "-R", "777", "/delete",
             ],
             check=True,
             capture_output=True,
         )
 
-    @patch("sagemaker.train.local.local_container.shutil.rmtree")
-    @patch("sagemaker.train.local.local_container.subprocess.run")
-    @patch("sagemaker.train.local.local_container.os.path.exists", return_value=True)
-    def test_rmtree_cleans_up_mount_point(self, mock_exists, mock_run, mock_rmtree):
-        """After docker cleanup, remaining mount point directory is removed."""
-        mock_rmtree.side_effect = [PermissionError("Permission denied"), None]
-
-        _rmtree("/tmp/test", IMAGE)
-
-        assert mock_rmtree.call_count == 2
-        mock_rmtree.assert_has_calls([
-            call("/tmp/test"),
-            call("/tmp/test", ignore_errors=True),
-        ])
-
     @patch("sagemaker.train.local.local_container.shutil.rmtree")
     @patch("sagemaker.train.local.local_container.subprocess.run")
     def test_rmtree_docker_fallback_fails_raises(self, mock_run, mock_rmtree):