diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/main.cc b/base/cvd/cuttlefish/host/commands/run_cvd/main.cc index 9e0a1d1cbde..0f5b9fb2a0a 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/main.cc +++ b/base/cvd/cuttlefish/host/commands/run_cvd/main.cc @@ -15,6 +15,7 @@ */ #include +#include #include #include @@ -245,6 +246,7 @@ void ConfigureLogs(const CuttlefishConfig& config, } // namespace Result RunCvdMain(int argc, char** argv) { + signal(SIGPIPE, SIG_IGN); google::ParseCommandLineFlags(&argc, &argv, false); CF_EXPECT(StdinValid(), "Invalid stdin"); diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp index e658ac176f9..6cb0bc16552 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp +++ b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp @@ -24,7 +24,9 @@ #include #include #include +#include #include +#include #include #include @@ -58,6 +60,24 @@ namespace cuttlefish { namespace run_cvd_impl { +std::string_view format_as(ServerLoopImpl::DeviceStatus status) { + switch (status) { + case ServerLoopImpl::DeviceStatus::kUnknown: + return "Unknown"; + case ServerLoopImpl::DeviceStatus::kActive: + return "Active"; + case ServerLoopImpl::DeviceStatus::kSuspended: + return "Suspended"; + case ServerLoopImpl::DeviceStatus::kGuestOff: + return "GuestOff"; + } +} + +std::ostream& operator<<(std::ostream& out, + ServerLoopImpl::DeviceStatus status) { + return out << format_as(status); +} + bool ServerLoopImpl::CreateQcowOverlay(const std::string& crosvm_path, const std::string& backing_file, const std::string& output_overlay_path) { @@ -116,16 +136,25 @@ Result ServerLoopImpl::Run() { CF_EXPECT(process_monitor.StartAndMonitorProcesses()); device_status_ = DeviceStatus::kActive; + bool process_monitor_active = true; while (true) { // TODO: use select to handle simultaneous connections. SharedFDSet read_set; read_set.Set(server_); - read_set.Set(process_monitor.status()); + if (process_monitor_active) { + read_set.Set(process_monitor.status()); + } Select(&read_set, nullptr, nullptr, nullptr); - if (read_set.IsSet(process_monitor.status())) { - return CF_ERR("process monitor has died"); + if (process_monitor_active && read_set.IsSet(process_monitor.status())) { + process_monitor_active = false; + CF_EXPECT(process_monitor.StopMonitoredProcesses()); + device_status_ = DeviceStatus::kGuestOff; + LOG(INFO) + << "Process monitor has exited gracefully (guest VM shut down). " + "Server loop continuing to listen for status/restart."; + continue; } CF_EXPECT(read_set.IsSet(server_)); @@ -183,6 +212,8 @@ Result ServerLoopImpl::HandleExtended( switch (action_info.extended_action.actions_case()) { case ActionsCase::kSuspend: { VLOG(0) << "Run_cvd received suspend request."; + CF_EXPECT_NE(device_status_.load(), DeviceStatus::kGuestOff, + "Device is powered off, cannot suspend"); if (device_status_.load() == DeviceStatus::kActive) { CF_EXPECT(HandleSuspend(process_monitor)); } @@ -191,6 +222,8 @@ Result ServerLoopImpl::HandleExtended( } case ActionsCase::kResume: { VLOG(0) << "Run_cvd received resume request."; + CF_EXPECT_NE(device_status_.load(), DeviceStatus::kGuestOff, + "Device is powered off, cannot resume"); if (device_status_.load() == DeviceStatus::kSuspended) { CF_EXPECT(HandleResume(process_monitor)); } @@ -199,24 +232,30 @@ Result ServerLoopImpl::HandleExtended( } case ActionsCase::kSnapshotTake: { VLOG(0) << "Run_cvd received snapshot request."; - CF_EXPECT(device_status_.load() == DeviceStatus::kSuspended, - "The device is not suspended, and snapshot cannot be taken"); + CF_EXPECT_EQ(device_status_.load(), DeviceStatus::kSuspended, + "The device is not suspended, and snapshot cannot be taken"); CF_EXPECT( HandleSnapshotTake(action_info.extended_action.snapshot_take())); return {}; } case ActionsCase::kStartScreenRecording: { VLOG(0) << "Run_cvd received start screen recording request."; + CF_EXPECT_EQ(device_status_.load(), DeviceStatus::kActive, + "Device is not active, cannot start screen recording"); CF_EXPECT(HandleStartScreenRecording()); return {}; } case ActionsCase::kStopScreenRecording: { VLOG(0) << "Run_cvd received stop screen recording request."; + CF_EXPECT_EQ(device_status_.load(), DeviceStatus::kActive, + "Device is not active, cannot stop screen recording"); CF_EXPECT(HandleStopScreenRecording()); return {}; } case ActionsCase::kScreenshotDisplay: { VLOG(0) << "Run_cvd received screenshot display request."; + CF_EXPECT_EQ(device_status_.load(), DeviceStatus::kActive, + "Device is not active, cannot take screenshot"); const auto& request = action_info.extended_action.screenshot_display(); CF_EXPECT(HandleScreenshotDisplay(request)); return {}; @@ -261,7 +300,13 @@ void ServerLoopImpl::HandleActionWithNoData(const LauncherAction action, break; } case LauncherAction::kStatus: { - // TODO(schuffelen): Return more information on a side channel + // TODO(schuffelen): Return more information on a side channel. + // Note: When device_status_ == DeviceStatus::kGuestOff, returning + // kSuccess keeps the socket responsive so that `cvd restart` can power + // the VM back on. Because `cvd status` hardcodes "Running" upon + // receiving LauncherResponse::kSuccess, reporting a distinct + // "Powered Off" status requires extending the + // LauncherAction/LauncherResponse protocol. auto response = LauncherResponse::kSuccess; // TODO(schuffelen): Handle unused result (void)client->Write(&response, sizeof(response)); diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h index 1ec2ad1d9ca..a042b6ba61d 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h +++ b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h @@ -17,7 +17,9 @@ #pragma once #include +#include #include +#include #include #include #include @@ -62,6 +64,7 @@ class ServerLoopImpl : public ServerLoop, kUnknown = 0, kActive = 1, kSuspended = 2, + kGuestOff = 3, }; private: @@ -114,5 +117,14 @@ class ServerLoopImpl : public ServerLoop, std::atomic device_status_; }; +std::string_view format_as(ServerLoopImpl::DeviceStatus status); +std::ostream& operator<<(std::ostream& out, + ServerLoopImpl::DeviceStatus status); + +template +void AbslStringify(Sink& sink, ServerLoopImpl::DeviceStatus status) { + sink.Append(format_as(status)); +} + } // namespace run_cvd_impl } // namespace cuttlefish diff --git a/base/cvd/cuttlefish/host/libs/feature/command_source.h b/base/cvd/cuttlefish/host/libs/feature/command_source.h index f47b496782b..d4c5fcf2406 100644 --- a/base/cvd/cuttlefish/host/libs/feature/command_source.h +++ b/base/cvd/cuttlefish/host/libs/feature/command_source.h @@ -15,6 +15,8 @@ #pragma once +#include +#include #include #include @@ -26,12 +28,42 @@ namespace cuttlefish { +enum class ProcessCategory { + kNonCriticalSupport, + kCriticalSupport, + kVmm, +}; + +inline constexpr std::string_view format_as(ProcessCategory category) { + switch (category) { + case ProcessCategory::kVmm: + return "vmm"; + case ProcessCategory::kCriticalSupport: + return "critical support"; + case ProcessCategory::kNonCriticalSupport: + return "non-critical support"; + } +} + +inline std::ostream& operator<<(std::ostream& out, ProcessCategory category) { + return out << format_as(category); +} + struct MonitorCommand { Command command; bool is_critical; + ProcessCategory category; MonitorCommand(Command command, bool is_critical = true) - : command(std::move(command)), is_critical(is_critical) {} + : command(std::move(command)), + is_critical(is_critical), + category(is_critical ? ProcessCategory::kCriticalSupport + : ProcessCategory::kNonCriticalSupport) {} + + MonitorCommand(Command command, ProcessCategory category) + : command(std::move(command)), + is_critical(category != ProcessCategory::kNonCriticalSupport), + category(category) {} }; class CommandSource : public virtual SetupFeature { diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/BUILD.bazel b/base/cvd/cuttlefish/host/libs/process_monitor/BUILD.bazel index 4cf55fdc2cc..97fa5209f3f 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/BUILD.bazel +++ b/base/cvd/cuttlefish/host/libs/process_monitor/BUILD.bazel @@ -20,6 +20,7 @@ cf_cc_library( "//cuttlefish/host/libs/config:known_paths", "//cuttlefish/host/libs/feature", "//cuttlefish/posix:strerror", + "//cuttlefish/posix:temp_failure_retry", "//cuttlefish/process:command", "//cuttlefish/process:subprocess", "//cuttlefish/result", diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc index 09cdb92ff4f..66a4bcde4f0 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc +++ b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc @@ -43,6 +43,8 @@ #include "cuttlefish/host/libs/command_util/util.h" #include "cuttlefish/host/libs/config/known_paths.h" #include "cuttlefish/posix/strerror.h" +#include "cuttlefish/posix/temp_failure_retry.h" +#include "cuttlefish/process/command.h" #include "cuttlefish/process/subprocess.h" #include "cuttlefish/result/result.h" @@ -137,13 +139,35 @@ Result MonitorLoop(std::atomic_bool& running, // in the future, cmd->Start might not run exec() it->proc.reset(new Subprocess(it->cmd->Start(std::move(options)))); } else { - bool is_critical = it->is_critical; + const bool is_critical = it->is_critical; + const bool is_vmm = it->category == ProcessCategory::kVmm; + std::string name = it->cmd->GetShortName(); monitored.erase(it); if (running.load() && is_critical) { - LOG(ERROR) << "Stopping all monitored processes due to unexpected " - "exit of critical process"; running.store(false); - break; + if (is_vmm && WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) { + LOG(INFO) + << "Stopping all monitored processes due to graceful exit " + "of VMM process " + << name; + break; + } else { + LOG(ERROR) << "Stopping all monitored processes due to unexpected " + "exit of critical process " + << name; + if (WIFSIGNALED(wstatus)) { + return CF_ERRF("Critical process {} was killed by signal {}", + name, WTERMSIG(wstatus)); + } else if (WEXITSTATUS(wstatus) != 0) { + return CF_ERRF( + "Critical process {} exited with non-zero exit code {}", name, + WEXITSTATUS(wstatus)); + } else { + return CF_ERRF( + "Critical process {} exited unexpectedly with exit code 0", + name); + } + } } } } @@ -153,7 +177,15 @@ Result MonitorLoop(std::atomic_bool& running, Result StopSubprocesses(std::vector& monitored) { VLOG(0) << "Stopping monitored subprocesses"; + for (const auto& it : monitored) { + if (it.proc) { + (void)it.proc->SendSignalToGroup(SIGCONT); + } + } auto stop = [](const auto& it) { + if (!it.proc) { + return true; + } auto stop_result = it.proc->Stop(); if (stop_result == StopperResult::kFailure) { LOG(WARNING) << "Error in stopping \"" << it.cmd->GetShortName() << "\""; @@ -334,7 +366,7 @@ ProcessMonitor::Properties& ProcessMonitor::Properties::RestartSubprocesses( ProcessMonitor::Properties& ProcessMonitor::Properties::AddCommand( MonitorCommand cmd) & { - entries_.emplace_back(std::move(cmd.command), cmd.is_critical); + entries_.emplace_back(std::move(cmd.command), cmd.category); return *this; } @@ -357,18 +389,29 @@ ProcessMonitor::ProcessMonitor(ProcessMonitor::Properties&& properties, monitor_(-1) {} Result ProcessMonitor::StopMonitoredProcesses() { - CF_EXPECT(monitor_ != -1, "The monitor process has already exited."); - CF_EXPECT(parent_channel_.has_value(), - "The monitor socket is already closed"); - CF_EXPECT( - SendEmptyRequest(*parent_channel_, ParentToChildMessageType::kStop)); + if (monitor_ == -1) { + return {}; + } pid_t last_monitor = monitor_; monitor_ = -1; + + int wstatus = 0; + pid_t wait_res = TEMP_FAILURE_RETRY(waitpid(last_monitor, &wstatus, WNOHANG)); + if (wait_res == 0) { + if (parent_channel_.has_value()) { + auto send_result = + SendEmptyRequest(*parent_channel_, ParentToChildMessageType::kStop); + if (!send_result.has_value()) { + VLOG(0) << "SendEmptyRequest failed during StopMonitoredProcesses: " + << send_result.error(); + } + } + wait_res = TEMP_FAILURE_RETRY(waitpid(last_monitor, &wstatus, 0)); + } + parent_channel_.reset(); - int wstatus; - CF_EXPECT(waitpid(last_monitor, &wstatus, 0) == last_monitor, - "Failed to wait for monitor process"); + CF_EXPECT(wait_res == last_monitor, "Failed to wait for monitor process"); CF_EXPECT(!WIFSIGNALED(wstatus), "Monitor process exited due to a signal"); CF_EXPECT(WIFEXITED(wstatus), "Monitor process exited for unknown reasons"); CF_EXPECT(WEXITSTATUS(wstatus) == 0, @@ -450,16 +493,25 @@ Result ProcessMonitor::MonitorRoutine() { auto parent_comms = std::async(std::launch::async, read_monitor_socket_loop, std::ref(running)); - CF_EXPECT(MonitorLoop(running, properties_mutex_, - properties_.restart_subprocesses_, - properties_.entries_)); + auto monitor_loop_result = + MonitorLoop(running, properties_mutex_, properties_.restart_subprocesses_, + properties_.entries_); running.store(false); if (child_sock_->IsOpen()) { child_sock_->Shutdown(SHUT_RDWR); } - CF_EXPECT(parent_comms.get(), "Should have exited if monitoring stopped"); + auto parent_comms_result = parent_comms.get(); + if (!parent_comms_result.has_value()) { + LOG(WARNING) << "Parent comms thread failed: " + << parent_comms_result.error(); + } - CF_EXPECT(StopSubprocesses(properties_.entries_)); + auto stop_result = StopSubprocesses(properties_.entries_); + if (!stop_result.has_value()) { + LOG(WARNING) << "Failed to stop subprocesses: " << stop_result.error(); + } + CF_EXPECT(std::move(monitor_loop_result)); + CF_EXPECT(std::move(stop_result)); VLOG(0) << "Done monitoring subprocesses"; return {}; } diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.h b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.h index 2a6a889b9f9..da43d0ddb28 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.h +++ b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.h @@ -34,9 +34,18 @@ struct MonitorEntry { std::unique_ptr cmd; std::unique_ptr proc; bool is_critical; + ProcessCategory category; + + MonitorEntry(Command command, ProcessCategory category) + : cmd(new Command(std::move(command))), + is_critical(category != ProcessCategory::kNonCriticalSupport), + category(category) {} MonitorEntry(Command command, bool is_critical) - : cmd(new Command(std::move(command))), is_critical(is_critical) {} + : cmd(new Command(std::move(command))), + is_critical(is_critical), + category(is_critical ? ProcessCategory::kCriticalSupport + : ProcessCategory::kNonCriticalSupport) {} }; // Launches and keeps track of subprocesses, decides response if they diff --git a/base/cvd/cuttlefish/host/libs/vm_manager/crosvm_manager.cpp b/base/cvd/cuttlefish/host/libs/vm_manager/crosvm_manager.cpp index 05b04198bc0..c659da8b3b3 100644 --- a/base/cvd/cuttlefish/host/libs/vm_manager/crosvm_manager.cpp +++ b/base/cvd/cuttlefish/host/libs/vm_manager/crosvm_manager.cpp @@ -1102,11 +1102,12 @@ Result> CrosvmManager::StartCommands( gpu_capture_logs); commands.emplace_back(std::move(gpu_capture_log_tee_cmd)); - commands.emplace_back(std::move(gpu_capture_command)); + commands.emplace_back(std::move(gpu_capture_command), + ProcessCategory::kVmm); } else { crosvm_cmd.Cmd().RedirectStdIO(Command::StdIoChannel::kStdOut, crosvm_logs); crosvm_cmd.Cmd().RedirectStdIO(Command::StdIoChannel::kStdErr, crosvm_logs); - commands.emplace_back(std::move(crosvm_cmd.Cmd()), true); + commands.emplace_back(std::move(crosvm_cmd.Cmd()), ProcessCategory::kVmm); } return commands; diff --git a/base/cvd/cuttlefish/host/libs/vm_manager/gem5_manager.cpp b/base/cvd/cuttlefish/host/libs/vm_manager/gem5_manager.cpp index f363c4a420c..cc90016bd65 100644 --- a/base/cvd/cuttlefish/host/libs/vm_manager/gem5_manager.cpp +++ b/base/cvd/cuttlefish/host/libs/vm_manager/gem5_manager.cpp @@ -368,7 +368,7 @@ Result> Gem5Manager::StartCommands( gem5_cmd.AddEnvironmentVariable("M5_PATH", config.assembly_dir()); std::vector commands; - commands.emplace_back(std::move(gem5_cmd), true); + commands.emplace_back(std::move(gem5_cmd), ProcessCategory::kVmm); return commands; } diff --git a/base/cvd/cuttlefish/host/libs/vm_manager/qemu_manager.cpp b/base/cvd/cuttlefish/host/libs/vm_manager/qemu_manager.cpp index 869556097f4..89703d07af0 100644 --- a/base/cvd/cuttlefish/host/libs/vm_manager/qemu_manager.cpp +++ b/base/cvd/cuttlefish/host/libs/vm_manager/qemu_manager.cpp @@ -917,7 +917,7 @@ Result> QemuManager::StartCommands( add_hvc_sink(); } - commands.emplace_back(std::move(qemu_cmd), true); + commands.emplace_back(std::move(qemu_cmd), ProcessCategory::kVmm); return commands; }