Improve test coverage of 'eden start', 'eden stop', and 'eden restart'

Summary: While creating a systemd service unit for EdenFS, I noticed a few features of the current implementation of 'eden start', 'eden stop', and 'eden restart' which are not covered by tests. Write tests for these features to ensure they don't regress with systemd integration.

Reviewed By: wez

Differential Revision: D10434379

fbshipit-source-id: 6c828a85d59179bbc4beda87e1bf2534543b60b2

eden/integration/helpers/fake_edenfs.cpp

@@ -16,6 +16,7 @@
 #include <thrift/lib/cpp2/server/ThriftServer.h>
 #include <array>
 #include <chrono>
+#include <optional>
 #include <thread>
 
 #include "eden/fs/eden-config.h"
@@ -33,6 +34,10 @@ using std::make_shared;
 using std::string;
 
 DEFINE_bool(allowRoot, false, "Allow running eden directly as root");
+DEFINE_string(
+    cleanShutdownFile,
+    "",
+    "If set, create the given file when shutting down cleanly");
 DEFINE_bool(
     exitWithoutCleanupOnStop,
     false,
@@ -43,6 +48,10 @@ DEFINE_bool(
     false,
     "EXPERIMENTAL: Run edenfs as if systemd controls its lifecycle");
 #endif
+DEFINE_bool(
+    failDuringStartup,
+    false,
+    "Instead of reporting success after starting up, report failure and exit");
 DEFINE_bool(ignoreStop, false, "Ignore attempts to stop edenfs");
 DEFINE_double(
     sleepBeforeGetPid,
@@ -128,13 +137,24 @@ class FakeEdenServer {
     stopSleepDuration_ = stopSleepDuration;
   }
 
+  void setCleanShutdownFile(AbsolutePath path) {
+    cleanShutdownFile_ = path;
+  }
+
  private:
+  void reportCleanShutdown() {
+    if (cleanShutdownFile_) {
+      folly::File{cleanShutdownFile_->c_str(), O_WRONLY | O_CREAT};
+    }
+  }
+
   EventBase* eventBase_{nullptr};
   ThriftServer server_;
   std::shared_ptr<FakeEdenServiceHandler> handler_;
   StopBehavior stopBehavior_{StopBehavior::TerminateEventLoop};
   std::chrono::milliseconds getPidSleepDuration_{0ms};
   std::chrono::milliseconds stopSleepDuration_{0ms};
+  std::optional<AbsolutePath> cleanShutdownFile_{};
 };
 
 class FakeEdenServiceHandler : virtual public StreamingEdenServiceSvIf {
@@ -240,8 +260,15 @@ void FakeEdenServer::run(
 
   // Run the thrift server
   server_.setup();
+  if (FLAGS_failDuringStartup) {
+    startupLogger.exitUnsuccessfully(
+        1,
+        "Started successfully, but reporting failure because --failDuringStartup was specified");
+  }
   startupLogger.success();
   eventBase_->loopForever();
+
+  reportCleanShutdown();
 }
 
 bool acquireLock(AbsolutePathPiece edenDir) {
@@ -310,6 +337,9 @@ int main(int argc, char** argv) {
   }
 
   FakeEdenServer server;
+  if (!FLAGS_cleanShutdownFile.empty()) {
+    server.setCleanShutdownFile(AbsolutePath{FLAGS_cleanShutdownFile});
+  }
   if (FLAGS_ignoreStop) {
     server.setStopBehavior(StopBehavior::DoNothing);
   }

eden/integration/restart_test.py

@@ -11,18 +11,22 @@ import os
 import pathlib
 import subprocess
 import sys
+import typing
 
 import eden.thrift
 import eden.thrift.client
 import pexpect
+from eden.cli.config import EdenInstance
+from eden.cli.util import HealthStatus
 
 from .lib.find_executables import FindExe
+from .lib.pexpect import PexpectAssertionMixin
 from .lib.service_test_case import ServiceTestCaseBase, service_test
 from .lib.temporary_directory import TemporaryDirectoryMixin
 
 
 @service_test
-class RestartTest(ServiceTestCaseBase, TemporaryDirectoryMixin):
+class RestartTest(ServiceTestCaseBase, PexpectAssertionMixin, TemporaryDirectoryMixin):
     def setUp(self) -> None:
         self.tmp_dir = self.make_temporary_directory()
 
@@ -52,6 +56,10 @@ class RestartTest(ServiceTestCaseBase, TemporaryDirectoryMixin):
         daemon = self.spawn_fake_edenfs(eden_dir=pathlib.Path(self.tmp_dir))
         return daemon.process_id
 
+    def _check_edenfs_health(self) -> HealthStatus:
+        instance = EdenInstance(self.tmp_dir, etc_eden_dir=None, home_dir=None)
+        return instance.check_health()
+
     def test_restart_starts_edenfs_if_not_running(self) -> None:
         """
         Run "eden restart".  It should start it without prompting since edenfs
@@ -91,6 +99,27 @@ class RestartTest(ServiceTestCaseBase, TemporaryDirectoryMixin):
         p.wait()
         self.assertEqual(p.exitstatus, 0)
 
+    def test_eden_restart_creates_new_edenfs_process(self) -> None:
+        old_pid = self._start_fake_edenfs()
+
+        p = self._spawn_restart("--force")
+        p.expect(r"Started edenfs \(pid (?P<pid>\d+)\)")
+        new_pid_from_restart: int = int(p.match.group("pid"))
+        new_pid_from_health_check: typing.Optional[
+            int
+        ] = self._check_edenfs_health().pid
+
+        self.assertIsNotNone(new_pid_from_health_check, "EdenFS should be alive")
+        self.assertNotEqual(
+            old_pid, new_pid_from_health_check, "EdenFS process ID should have changed"
+        )
+        self.assertEqual(
+            new_pid_from_restart,
+            new_pid_from_health_check,
+            "'eden restart' should have shown the process ID for the new "
+            "EdenFS process",
+        )
+
     def test_restart_sigkill(self) -> None:
         self._start_fake_edenfs()
 
@@ -209,3 +238,11 @@ class RestartTest(ServiceTestCaseBase, TemporaryDirectoryMixin):
         )
         p.wait()
         self.assertEqual(p.exitstatus, 0)
+
+    def test_eden_restart_fails_if_edenfs_crashes_on_start(self) -> None:
+        self._start_fake_edenfs()
+        restart_process = self._spawn_restart(
+            "--force", "--daemon-binary", "/bin/false"
+        )
+        restart_process.expect_exact("Failed to start edenfs")
+        self.assert_process_fails(restart_process, 1)

eden/integration/start_test.py

@@ -8,8 +8,20 @@
 # of patent rights can be found in the PATENTS file in the same directory.
 
 import os
+import pathlib
+import sys
+import typing
+
+import pexpect
+from eden.cli.config import EdenInstance
+from eden.cli.util import HealthStatus
+from fb303.ttypes import fb_status
 
 from .lib import testcase
+from .lib.find_executables import FindExe
+from .lib.pexpect import PexpectAssertionMixin
+from .lib.service_test_case import ServiceTestCaseBase, service_test
+from .lib.temporary_directory import TemporaryDirectoryMixin
 
 
 class StartTest(testcase.EdenTestCase):
@@ -58,3 +70,83 @@ class StartTest(testcase.EdenTestCase):
         # so the self.eden class doesn't really know it is running and that
         # it needs to be shut down.
         self.eden.run_cmd("stop")
+
+
+@service_test
+class StartFakeEdenFSTest(
+    ServiceTestCaseBase, PexpectAssertionMixin, TemporaryDirectoryMixin
+):
+    def setUp(self):
+        super().setUp()
+        self.eden_dir = pathlib.Path(self.make_temporary_directory())
+
+    def test_eden_start_launches_separate_processes_for_separate_eden_dirs(
+        self
+    ) -> None:
+        eden_dir_1 = self.eden_dir
+        eden_dir_2 = pathlib.Path(self.make_temporary_directory())
+
+        start_1_process = self.spawn_start(eden_dir=eden_dir_1)
+        self.assert_process_succeeds(start_1_process)
+        start_2_process = self.spawn_start(eden_dir=eden_dir_2)
+        self.assert_process_succeeds(start_2_process)
+
+        instance_1_health: HealthStatus = EdenInstance(
+            str(eden_dir_1), etc_eden_dir=None, home_dir=None
+        ).check_health()
+        self.assertEqual(
+            instance_1_health.status,
+            fb_status.ALIVE,
+            f"First edenfs process should be healthy, but it isn't: "
+            f"{instance_1_health}",
+        )
+
+        instance_2_health: HealthStatus = EdenInstance(
+            str(eden_dir_2), etc_eden_dir=None, home_dir=None
+        ).check_health()
+        self.assertEqual(
+            instance_2_health.status,
+            fb_status.ALIVE,
+            f"Second edenfs process should be healthy, but it isn't: "
+            f"{instance_2_health}",
+        )
+
+        self.assertNotEqual(
+            instance_1_health.pid,
+            instance_2_health.pid,
+            f"The edenfs process should have separate process IDs",
+        )
+
+    def test_eden_start_fails_if_edenfs_is_already_running(self) -> None:
+        with self.spawn_fake_edenfs(self.eden_dir) as daemon_pid:
+            start_process = self.spawn_start()
+            start_process.expect_exact(f"edenfs is already running (pid {daemon_pid})")
+            self.assert_process_fails(start_process, 1)
+
+    def test_eden_start_fails_if_edenfs_fails_during_startup(self) -> None:
+        start_process = self.spawn_start(daemon_args=["--failDuringStartup"])
+        start_process.expect_exact(
+            "Started successfully, but reporting failure because "
+            "--failDuringStartup was specified"
+        )
+        self.assert_process_fails(start_process, 1)
+
+    def spawn_start(
+        self,
+        daemon_args: typing.Sequence[str] = (),
+        eden_dir: typing.Optional[pathlib.Path] = None,
+    ) -> "pexpect.spawn[str]":
+        if eden_dir is None:
+            eden_dir = self.eden_dir
+        args = [
+            "--config-dir",
+            str(eden_dir),
+            "start",
+            "--daemon-binary",
+            FindExe.FAKE_EDENFS,
+            "--",
+        ]
+        args.extend(daemon_args)
+        return pexpect.spawn(
+            FindExe.EDEN_CLI, args, encoding="utf-8", logfile=sys.stderr
+        )

eden/integration/stop_test.py

@@ -20,7 +20,7 @@ from eden.cli.daemon import did_process_exit
 from eden.cli.util import poll_until
 
 from .lib.find_executables import FindExe
-from .lib.pexpect import PexpectAssertionMixin
+from .lib.pexpect import PexpectAssertionMixin, wait_for_pexpect_process
 from .lib.service_test_case import ServiceTestCaseBase, service_test
 from .lib.temporary_directory import TemporaryDirectoryMixin
 
@@ -46,6 +46,26 @@ class StopTest(ServiceTestCaseBase, PexpectAssertionMixin, TemporaryDirectoryMix
                 did_process_exit(daemon_pid), f"Process {daemon_pid} should have died"
             )
 
+    def test_eden_stop_shuts_down_edenfs_cleanly(self) -> None:
+        clean_shutdown_file = pathlib.Path(self.tmp_dir) / "clean_shutdown"
+        assert not clean_shutdown_file.exists()
+
+        with self.spawn_fake_edenfs(
+            pathlib.Path(self.tmp_dir),
+            ["--cleanShutdownFile", str(clean_shutdown_file)],
+        ):
+            self.assertFalse(
+                clean_shutdown_file.exists(),
+                f"{clean_shutdown_file} should not exist after starting EdenFS",
+            )
+
+            stop_process = self.spawn_stop([])
+            wait_for_pexpect_process(stop_process)
+            self.assertTrue(
+                clean_shutdown_file.exists(),
+                f"{clean_shutdown_file} should exist after EdenFS cleanly shuts down",
+            )
+
     def test_stop_sigkill(self):
         with self.spawn_fake_edenfs(pathlib.Path(self.tmp_dir), ["--ignoreStop"]):
             # Ask the CLI to stop edenfs, with a 1 second timeout.