From 3c8a896db6545416edefd8709e4a34713d12c502 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 2 Sep 2025 16:48:46 +0000 Subject: [PATCH 1/2] Initial plan From 6645340ee7b2f72cac00c76d11ac577562edea8d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 2 Sep 2025 16:54:39 +0000 Subject: [PATCH 2/2] fix: Replace rsync -az with -rlptDz to avoid permission issues - Change rsync flags from -az to -rlptDz to exclude owner (-o) and group (-g) preservation - Add test coverage for rsync flag validation - Update comments to explain the permission issue fix - Resolves "Operation not permitted" chown errors when downloading files Co-authored-by: njzjz <9496702+njzjz@users.noreply.github.com> --- dpdispatcher/utils/utils.py | 6 +-- tests/test_rsync_flags.py | 87 +++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 3 deletions(-) create mode 100644 tests/test_rsync_flags.py diff --git a/dpdispatcher/utils/utils.py b/dpdispatcher/utils/utils.py index 4a75fe5d..b6c5e392 100644 --- a/dpdispatcher/utils/utils.py +++ b/dpdispatcher/utils/utils.py @@ -138,9 +138,9 @@ def rsync( cmd = [ "rsync", - # -a: archieve - # -z: compress - "-az", + # -r: recursive, -l: links, -p: perms, -t: times, -D: devices/specials + # -z: compress (exclude -o: owner, -g: group to avoid permission issues) + "-rlptDz", "-e", ssh_cmd_str, "-q", diff --git a/tests/test_rsync_flags.py b/tests/test_rsync_flags.py new file mode 100644 index 00000000..13ebe6e0 --- /dev/null +++ b/tests/test_rsync_flags.py @@ -0,0 +1,87 @@ +import os +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) +__package__ = "tests" + +from dpdispatcher.utils.utils import rsync + + +class TestRsyncFlags(unittest.TestCase): + """Test rsync function flags to ensure correct options are used.""" + + @patch("dpdispatcher.utils.utils.run_cmd_with_all_output") + def test_rsync_flags_exclude_owner_group(self, mock_run_cmd): + """Test that rsync uses flags that exclude owner and group preservation.""" + # Mock successful command execution + mock_run_cmd.return_value = (0, "", "") + + # Call rsync function + rsync("source_file", "dest_file", key_filename="test_key") + + # Verify the command was called + mock_run_cmd.assert_called_once() + + # Get the command that was executed + called_cmd = mock_run_cmd.call_args[0][0] + + # Verify the command contains the correct flags + self.assertIn("-rlptDz", called_cmd) + self.assertNotIn("-az", called_cmd) + + # Verify rsync command structure + self.assertIn("rsync", called_cmd) + self.assertIn("source_file", called_cmd) + self.assertIn("dest_file", called_cmd) + self.assertIn("-e", called_cmd) + self.assertIn("-q", called_cmd) + + @patch("dpdispatcher.utils.utils.run_cmd_with_all_output") + def test_rsync_with_proxy_command_flags(self, mock_run_cmd): + """Test that rsync uses correct flags even with proxy command.""" + # Mock successful command execution + mock_run_cmd.return_value = (0, "", "") + + # Call rsync function with proxy command + rsync( + "source_file", + "dest_file", + key_filename="test_key", + proxy_command="ssh -W target:22 jump_host", + ) + + # Verify the command was called + mock_run_cmd.assert_called_once() + + # Get the command that was executed + called_cmd = mock_run_cmd.call_args[0][0] + + # Verify the command contains the correct flags + self.assertIn("-rlptDz", called_cmd) + self.assertNotIn("-az", called_cmd) + + @patch("dpdispatcher.utils.utils.run_cmd_with_all_output") + def test_rsync_error_handling(self, mock_run_cmd): + """Test that rsync properly handles errors.""" + # Mock failed command execution + mock_run_cmd.return_value = ( + 23, + "", + "rsync: chown failed: Operation not permitted", + ) + + # Call rsync function and expect RuntimeError + with self.assertRaises(RuntimeError) as context: + rsync("source_file", "dest_file") + + # Verify error message contains the command and error + self.assertIn("Failed to run", str(context.exception)) + self.assertIn( + "rsync: chown failed: Operation not permitted", str(context.exception) + ) + + +if __name__ == "__main__": + unittest.main()