-
Notifications
You must be signed in to change notification settings - Fork 27
restart: resolve entries under --restartdir #486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| #!/usr/bin/env python3 | ||
| import os | ||
| import shutil | ||
| import subprocess | ||
| import tempfile | ||
| import unittest | ||
| from pathlib import Path | ||
|
|
||
| ROOT = Path(__file__).resolve().parents[1] | ||
| WRAPPER = ROOT / "bin" / "mana_restart" | ||
|
|
||
| class ManaRestartTest(unittest.TestCase): | ||
| def setUp(self): | ||
| self.temp = tempfile.TemporaryDirectory(prefix="mana-restart-test-") | ||
| self.root = Path(self.temp.name) | ||
| self.fake_bin = self.root / "fake-mana" / "bin" | ||
| self.fake_bin.mkdir(parents=True) | ||
| self.home = self.root / "home" | ||
| self.home.mkdir() | ||
| self.cwd = self.root / "unrelated" | ||
| self.cwd.mkdir() | ||
| self.restart = self.root / "restart" | ||
| (self.restart / "ckpt_rank_0").mkdir(parents=True) | ||
| (self.restart / "ckpt_rank_1").mkdir(parents=True) | ||
| (self.restart / "ckpt_rank_0" / "ckpt_0.dmtcp").touch() | ||
| (self.restart / "ckpt_rank_1" / "ckpt_1.dmtcp").touch() | ||
| self.capture = self.root / "args.txt" | ||
| shutil.copy2(WRAPPER, self.fake_bin / "mana_restart") | ||
| os.chmod(self.fake_bin / "mana_restart", 0o755) | ||
| self._exe(self.fake_bin / "dmtcp_command", "#!/bin/sh\nexit 0\n") | ||
| self._exe(self.fake_bin / "lower-half", '#!/bin/sh\nprintf "%s\\n" "$@" > "$MANA_TEST_CAPTURE"\nexit 0\n') | ||
| (self.home / ".mana.rc").write_text("Host: localhost\nPort: 7780\n", encoding="utf-8") | ||
|
|
||
| def tearDown(self): | ||
| self.temp.cleanup() | ||
|
|
||
| @staticmethod | ||
| def _exe(path, content): | ||
| path.write_text(content, encoding="utf-8") | ||
| os.chmod(path, 0o755) | ||
|
|
||
| def run_wrapper(self, *args): | ||
| env = os.environ.copy() | ||
| env["HOME"] = str(self.home) | ||
| env["MANA_TEST_CAPTURE"] = str(self.capture) | ||
| env.pop("SLURM_JOB_ID", None) | ||
| return subprocess.run([str(self.fake_bin / "mana_restart"), *args], cwd=self.cwd, env=env, text=True, capture_output=True, check=False, timeout=30) | ||
|
|
||
| def captured(self): | ||
| return self.capture.read_text(encoding="utf-8").splitlines() | ||
|
|
||
| def test_outside_current_directory(self): | ||
| result = self.run_wrapper("--verbose", "--restartdir", str(self.restart)) | ||
| self.assertEqual(result.returncode, 0, result.stdout + result.stderr) | ||
| args = self.captured() | ||
| i = args.index("--restartdir") | ||
| self.assertEqual(args[i + 1], str(self.restart.resolve())) | ||
|
|
||
| def test_tmp_detection(self): | ||
| (self.restart / "ckpt_rank_1" / "incomplete.tmp").touch() | ||
| result = self.run_wrapper("--restartdir", str(self.restart)) | ||
| self.assertNotEqual(result.returncode, 0) | ||
| self.assertIn("Restart directory has .tmp files", result.stdout + result.stderr) | ||
|
|
||
| def test_missing_value(self): | ||
| result = self.run_wrapper("--restartdir") | ||
| self.assertNotEqual(result.returncode, 0) | ||
| self.assertIn("requires a directory", result.stdout + result.stderr) | ||
|
|
||
| def test_restartdir_after_other_options(self): | ||
| result = self.run_wrapper("--restartdir", str(self.restart), "--ckptdir", str(self.restart)) | ||
| self.assertEqual(result.returncode, 0, result.stdout + result.stderr) | ||
| args = self.captured() | ||
| self.assertGreater(args.index("--restartdir"), args.index("--ckptdir")) | ||
|
|
||
| def test_default_current_directory(self): | ||
| (self.cwd / "ckpt_rank_0").mkdir() | ||
| result = self.run_wrapper() | ||
| self.assertEqual(result.returncode, 0, result.stdout + result.stderr) | ||
| args = self.captured() | ||
| i = args.index("--restartdir") | ||
| self.assertEqual(args[i + 1], str(self.cwd.resolve())) | ||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,3 +21,6 @@ cd $SCRIPT_DIR/../mpi-proxy-split/unit-test | |||||||||
| make || exit 1 | ||||||||||
| make clean | ||||||||||
| make check || exit 1 | ||||||||||
|
|
||||||||||
| # Additional wrapper regression test. | ||||||||||
| python3 ci/test-mana-restart.py -v || exit 1 | ||||||||||
|
Comment on lines
+25
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win Fix the path to the test script. At this point in the script, the working directory is Use the 🐛 Proposed fix # Additional wrapper regression test.
-python3 ci/test-mana-restart.py -v || exit 1
+python3 "$SCRIPT_DIR/test-mana-restart.py" -v || exit 1📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Conditionally append
--restartdirto avoid duplicating the argument.Unconditionally extending
dmtcp_flagsintroduces a bug when the user explicitly provides--restartdir. If the user passes--restartdir /path, this logic adds a second--restartdir ./to the end of the argument list, which will be erroneously forwarded to the lower half.Furthermore, if the user provides
--restartdirwithout a value, the added flag (--restartdir) is treated as the missing value, bypassing the missing-value check entirely and failing with an incorrect "Restart directory doesn't exist" error instead.🐛 Proposed fix to apply the default conditionally
📝 Committable suggestion
🤖 Prompt for AI Agents