Support per-instance stop in stop_cvd#2614
Conversation
SuperStrongDinosaur
force-pushed
the
backend_stop
branch
from
5159d24 to
5b674f5
Compare
SuperStrongDinosaur
changed the title
SuperStrongDinosaur
force-pushed
the
backend_stop
branch
from
5b674f5 to
a6bfd06
Compare
jemoreira
left a comment
jemoreira
left a comment
There was a problem hiding this comment.
This is not very useful unless cvd start also supports specific instances. Are you planning to implement that too?
Yes, I am currently finishing up |
SuperStrongDinosaur
force-pushed
the
backend_stop
branch
from
b0767bd to
e64329c
Compare
SuperStrongDinosaur
force-pushed
the
backend_stop
branch
from
e64329c to
f4fa5bb
Compare
SuperStrongDinosaur
force-pushed
the
backend_stop
branch
from
f4fa5bb to
52c3462
Compare
| std::vector<std::future<int>> exit_state_futures; | ||
| exit_state_futures.reserve(instances.size()); | ||
| for (const auto& instance : instances) { | ||
| unsigned id; |
There was a problem hiding this comment.
IIRC stopping the first instance causes all other instances to stop too (or crash). Did you test that case?
There was a problem hiding this comment.
I tested this myself and can confirm that stopping the first one also kills the second one (becomes unreachable). I think you're going to have to land the separate starts before being able to land this one.
You probably already know this, but just in case: The first instance runs some services that are used by all instances. To support this feature you're likely going to have to extract those to run as a standalone thing as part of the group.
There was a problem hiding this comment.
I'm aware of this issue. I plan to introduce validation in a separate PR to prevent cvd stop and cvd restart from executing on the main instance, as doing so currently crashes all other instances.
Long-term, the ideal solution is to extract those daemons from the main instance. However, that is highly complex :), and we need the ability to manipulate instances separately within a reasonable timeframe. Given that this specific limitation isn't a blocker for our use case, the validation check is the best step for now
There was a problem hiding this comment.
I plan to introduce validation in a separate PR
Would rather see it in this PR. stop/main.cc can already load the CuttlefishConfig structure, which reports what the first instance id is. Something like
CuttlefishConfig::Get()->Instances()[0].id()should produce the first instance id.
There was a problem hiding this comment.
I added the check that prevents main instance to stop. Also I created a separate pr with this change for cvd restart
#2649
SuperStrongDinosaur
force-pushed
the
backend_stop
branch
from
d53a923 to
9425378
Compare
SuperStrongDinosaur
force-pushed
the
backend_stop
branch
from
9425378 to
6dea36e
Compare
| CF_EXPECTF(!instances.empty(), "Instance '{}' not found in group '{}'", | ||
| name, group.GroupName()); | ||
| instance_nums.push_back(instances.front().id()); |
There was a problem hiding this comment.
Rather than looking only at instances.front(), can it either validate that instances.size() == 1 or apply to every member of instances?
4 participants
Summary
Currently, stop_cvd indiscriminately stops all Cuttlefish instances. This change introduces granular control, allowing users to stop a specific instance (or list of instances) in a running group without affecting others.
Changes
stop_cvd update: Introduced the --instance_nums flag to stop_cvd. It accepts a comma-separated list of instance IDs to stop.
Instance Filtering: Updated the main loop in stop_cvd to check the --instance_nums set. If populated, it only sends the termination signal to instances matching those IDs.
cvd stop Integration: Modified the cvd stop command handler to parse selectors. It resolves targeted instance names to their IDs and propagates them as --instance_nums to the stop_cvd backend.
API Propagation: Updated InstanceManager::StopInstanceGroup and RunStopCvd to accept and pass the instance_nums string.
Database Status Update Fix: Updated StopInstanceGroup in instance_manager.cpp to only set the state to STOPPED in the database for the instances that were actually targeted and stopped.
Bug
b/459780275
Document
go/cuttlefish-per-instance-control
Frontend pr
#2615