Refactor instance manager stop

base/cvd/cuttlefish/host/commands/cvd/cli/commands/remove.cpp

@@ -16,7 +16,6 @@
 
 #include "cuttlefish/host/commands/cvd/cli/commands/remove.h"
 
-#include <chrono>
 #include <string>
 #include <vector>
 
@@ -94,7 +93,7 @@ Result<void> RemoveCvdCommandHandler::StopGroup(
     return {};
   }
   CF_EXPECT(instance_manager_.StopInstanceGroup(
-      group, std::chrono::seconds(5), InstanceDirActionOnStop::Clear));
+      group, InstanceDirActionOnStop::Clear));
   return {};
 }
 

base/cvd/cuttlefish/host/commands/cvd/cli/commands/start.cpp

@@ -24,7 +24,6 @@
 #include <unistd.h>
 
 #include <algorithm>
-#include <chrono>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
@@ -504,7 +503,7 @@ Result<void> CvdStartCommandHandler::Handle(const CommandRequest& request) {
 
     LOG(INFO) << "Stopping device...";
     CF_EXPECT(instance_manager_.StopInstanceGroup(
-        group, std::chrono::seconds(5), InstanceDirActionOnStop::Keep, {}));
+        group, InstanceDirActionOnStop::Keep));
     LOG(INFO) << "Device stopped.";
     return monitor_res;
   }

base/cvd/cuttlefish/host/commands/cvd/cli/commands/stop.cpp

@@ -41,10 +41,6 @@ namespace {
 
 constexpr char kSummaryHelpText[] = "Stop Cuttlefish instances";
 
-struct StopFlags {
-  size_t wait_for_launcher_secs = 5;
-  bool clear_instance_dirs = false;
-};
 }  // namespace
 
 CvdStopCommandHandler::CvdStopCommandHandler(InstanceManager& instance_manager)
@@ -80,10 +76,10 @@ Result<void> CvdStopCommandHandler::Handle(const CommandRequest& request) {
   }
 
   Result<void> stop_outcome = instance_manager_.StopInstanceGroup(
-      group, launcher_timeout,
+      group,
       flags_.clear_instance_dirs ? InstanceDirActionOnStop::Clear
                                  : InstanceDirActionOnStop::Keep,
-      instance_nums);
+      launcher_timeout.value_or(std::chrono::seconds(0)), instance_nums);
 
   GatherVmStopMetrics(group);
 

base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.cpp

@@ -212,9 +212,8 @@ Result<void> InstanceManager::UpdateInstanceGroup(
 }
 
 Result<void> InstanceManager::StopInstanceGroup(
-    LocalInstanceGroup& group,
-    std::optional<std::chrono::seconds> launcher_timeout,
-    InstanceDirActionOnStop instance_dir_action,
+    LocalInstanceGroup& group, InstanceDirActionOnStop instance_dir_action,
+    std::chrono::seconds launcher_timeout,
     const std::vector<unsigned>& instance_nums) {
   // Validate that the requested instances actually belong to this group
   std::set<unsigned> valid_ids;
@@ -231,14 +230,10 @@ Result<void> InstanceManager::StopInstanceGroup(
 
   const auto stop_bin = CF_EXPECT(StopBin(group.HostArtifactsPath()));
   const auto stop_bin_path = group.HostArtifactsPath() + "/bin/" + stop_bin;
-  int wait_for_launcher_secs = 0;
-  if (launcher_timeout.has_value()) {
-    wait_for_launcher_secs = launcher_timeout->count();
-  }
   Result<void> cmd_result = RunStopCvd(StopCvdParams{
       .bin_path = stop_bin_path,
       .home_dir = group.HomeDir(),
-      .wait_for_launcher_secs = wait_for_launcher_secs,
+      .wait_for_launcher_secs = static_cast<int>(launcher_timeout.count()),
       .clear_runtime_dirs =
           instance_dir_action == InstanceDirActionOnStop::Clear,
       .instance_nums = instance_nums,
@@ -271,8 +266,8 @@ Result<void> InstanceManager::Clear() {
   for (auto& group : instance_groups) {
     // Only stop running instances.
     if (group.HasActiveInstances()) {
-      auto stop_result = StopInstanceGroup(group, std::chrono::seconds(5),
-                                           InstanceDirActionOnStop::Clear);
+      Result<void> stop_result =
+          StopInstanceGroup(group, InstanceDirActionOnStop::Clear);
       if (!stop_result.ok()) {
         LOG(ERROR) << stop_result.error();
       }

base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.h

@@ -83,9 +83,8 @@ class InstanceManager {
   // Stops the device by asking it over the control socket. If launcher_timeout
   // has a value, it will wait for at most that time before returning an error.
   Result<void> StopInstanceGroup(
-      LocalInstanceGroup& group,
-      std::optional<std::chrono::seconds> launcher_timeout,
-      InstanceDirActionOnStop instance_dir_action,
+      LocalInstanceGroup& group, InstanceDirActionOnStop instance_dir_action,
+      std::chrono::seconds launcher_timeout = std::chrono::seconds(5),
       const std::vector<unsigned>& instance_nums = {});
 
  private:

base/cvd/cuttlefish/host/commands/cvd/instances/stop.cpp

@@ -27,7 +27,6 @@
 #include <vector>
 
 #include "absl/log/log.h"
-#include "absl/strings/str_join.h"
 #include "android-base/file.h"
 #include "fmt/core.h"
 #include "fmt/ranges.h"
@@ -63,22 +62,6 @@ static Command CreateStopCvdCommand(
   return command;
 }
 
-Result<void> RunStopCvdCmd(
-    const std::string& stopper_path,
-    const std::unordered_map<std::string, std::string>& env,
-    const std::vector<std::string>& args) {
-  Command stop_cmd = CreateStopCvdCommand(stopper_path, env, args);
-
-  LOG(INFO) << "Running " << stop_cmd.ToString();
-  Result<std::string> cmd_res = RunAndCaptureStdout(std::move(stop_cmd));
-  if (!cmd_res.ok()) {
-    LOG(ERROR) << "Failed to run " << stopper_path;
-    CF_EXPECT(std::move(cmd_res));
-  }
-  VLOG(1) << "\"" << stopper_path << " successfully ";
-  return {};
-}
-
 Result<void> RunStopCvdAll(bool clear_runtime_dirs) {
   std::vector<GroupProcInfo> group_infos = CF_EXPECT(CollectRunCvdGroups());
   LOG(INFO) << "Found " << group_infos.size()
@@ -250,45 +233,46 @@ Result<void> ForcefullyStopGroup(const uid_t any_id_in_group) {
 }
 
 Result<void> RunStopCvd(StopCvdParams params) {
-  const auto& stopper_path = params.bin_path;
-  std::unordered_map<std::string, std::string> stop_cvd_envs;
-  stop_cvd_envs["HOME"] = params.home_dir;
   // stop_cvd is located at $ANDROID_HOST_OUT/bin/stop_cvd
-  std::string android_host_out =
-      android::base::Dirname(android::base::Dirname(stopper_path));
-  stop_cvd_envs[kAndroidHostOut] = android_host_out;
-  stop_cvd_envs[kAndroidSoongHostOut] = android_host_out;
-  auto config_file_path = CF_EXPECT(GetCuttlefishConfigPath(params.home_dir));
-  stop_cvd_envs[kCuttlefishConfigEnvVarName] = config_file_path;
-  std::vector<std::string> args;
+  const std::string android_host_out =
+      android::base::Dirname(android::base::Dirname(params.bin_path));
+  const std::unordered_map<std::string, std::string> stop_cvd_envs = {
+      std::make_pair("HOME", params.home_dir),
+      std::make_pair(kAndroidHostOut, android_host_out),
+      std::make_pair(kAndroidSoongHostOut, android_host_out),
+      std::make_pair(kCuttlefishConfigEnvVarName,
+                     CF_EXPECT(GetCuttlefishConfigPath(params.home_dir))),
+  };
+
   std::string wait_flag =
       fmt::format("--wait_for_launcher={}", params.wait_for_launcher_secs);
-  args.push_back(wait_flag);
+  std::vector<std::string> args = {wait_flag};
   if (params.clear_runtime_dirs) {
-    args.push_back("--clear_instance_dirs=true");
+    args.emplace_back("--clear_instance_dirs=true");
   }
   if (!params.instance_nums.empty()) {
-    args.push_back(fmt::format("--instance_nums={}",
-                               absl::StrJoin(params.instance_nums, ",")));
+    args.emplace_back(fmt::format("--instance_nums={}",
+                                  fmt::join(params.instance_nums, ",")));
   }
-  Result<void> cmd_res = RunStopCvdCmd(stopper_path, stop_cvd_envs, args);
+
+  Result<std::string> cmd_res = RunAndCaptureStdout(
+      CreateStopCvdCommand(params.bin_path, stop_cvd_envs, args));
   if (cmd_res.ok()) {
     return {};
   }
   /**
-   * --clear_instance_dirs may not be available in old branches. This causes
-   * stop_cvd to terminate with a non-zero exit code due to a parsing error. Try
-   * again without that flag.
+   * --clear_instance_dirs or --instance_nums may not be available in old
+   * branches. This causes stop_cvd to terminate with a non-zero exit code due
+   * to a parsing error. Try again without that flag.
    */
-  if (!params.clear_runtime_dirs) {
+  if (!params.clear_runtime_dirs && !params.instance_nums.empty()) {
     CF_EXPECT(std::move(cmd_res));
   }
-  // TODO(kwstephenkim): deletes manually if `stop_cvd --clear_instance_dirs`
-  // failed.
-  LOG(ERROR) << "Perhaps --clear_instance_dirs is not supported.";
+  LOG(ERROR) << "--clear_instance_dirs or --instance_nums is not supported.";
   LOG(ERROR) << "Trying again without it";
 
-  CF_EXPECT(RunStopCvdCmd(stopper_path, stop_cvd_envs, {wait_flag}));
+  CF_EXPECT(RunAndCaptureStdout(
+      CreateStopCvdCommand(params.bin_path, stop_cvd_envs, {wait_flag})));
   return {};
 }
 }  // namespace cuttlefish