diff --git a/CLAUDE.md b/CLAUDE.md index 631daad..d346bed 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2584,11 +2584,10 @@ Source layout (all under `src/`): orchestrator exists, not for this parser's own unit tests, since its content is expected to keep changing as more of the orchestrator gets built on top of it. -- `compose_orchestrator.{h,cpp}` — the actual `-u/--up` orchestrator - (`-d/--down`, `commands.cpp`'s `compose_down_command()`, is still the - stub `load_and_validate_compose()`-only implementation described above — - not touched by this file yet), built as five separate steps/commits, - matching the user's own explicit sequencing request: +- `compose_orchestrator.{h,cpp}` — the `-u/--up` orchestrator (five + separate steps/commits, matching the user's own explicit sequencing + request) and `-d/--down` (`stop_compose_services()`, its own later + addition, see below): 1. `resolve_compose_images()` — matches each service's own `image:` reference against `list_oci_images(images_directory)` (`oci_image.h` — the exact function `-l/--list-images` already uses, reused as-is). @@ -2683,11 +2682,13 @@ Source layout (all under `src/`): `xdg_state_dir()/"compose"/sanitize_for_filename()`, the same state-file-naming pattern `port_forward.h`'s/`network_tap_relay.h`'s own crash-orphan records - already use — read back by both a future `-d/--down` implementation - and `--list-containers` (`list_compose_containers()`, below). - Overwrites any previous run's own record for the same file — a known, - expected limitation until `-d/--down` itself exists to keep the two - in sync (there's no way yet to tell which of an older run's services + already use — read back by both `-d/--down` (`stop_compose_services()`, + see below) and `--list-containers` (`list_compose_containers()`, + below). Overwrites any previous run's own record for the same file -- + a known, expected limitation, since `-d/--down` removes the file once + it acts on it, but nothing reconciles an *unclean* previous run (e.g. + a crash) against a fresh `-u/--up`'s own new record (there's no way + yet to tell which of an older run's services are still actually running versus already stopped by hand). **`--list-containers`** (`list_compose_containers()`, `list_containers_command()` @@ -2714,6 +2715,46 @@ Source layout (all under `src/`): `running` — confirming the status reflects real liveness, not just presence in the state file. + **`-d/--down`** (`stop_compose_services()`, `commands.cpp`'s + `compose_down_command()`) — unlike `-u/--up`, takes only a single + optional parameter (the compose file name; no images directory at all, + per the user's own explicit request), since stopping a stack needs + neither to mount nor resolve any image: everything required (service + name, container name, pid) is already in the state file `-u/--up` wrote. + Reads that file back, calls `kill_session()` (`kill_session.h` — the + exact same graceful `SIGTERM`-then-`SIGKILL`, cgroup-aware stop + `--kill ` already uses) on every recorded pid, then removes the + state file. If the compose file at the given path still exists and + parses, each service's own `stop_grace_period_seconds` (parsed since the + very first commit of `compose_file.cpp` but never actually consumed + until now) is used as that service's own grace period instead of + `kill_session()`'s hardcoded 10s default — best-effort: a service no + longer found there (edited/moved/deleted since `-u/--up` ran) just falls + back to that default, since the state file alone already has everything + strictly required. `load_and_validate_compose()` (the shared + resolve-images-directory helper `-u/--up` uses) is untouched and now + `-u/--up`-only. **Known scope limitation, not yet implemented**: doesn't + tear down the compose file's own managed networks/volumes — matching + real `docker compose down`'s own default (they survive unless + `-v`/`--volumes` is also given), but unlike real Compose, there's no + equivalent flag here yet to opt into removing them. + + This required splitting `-u`/`-d`'s previously-shared CLI parsing + (`cli_args.cpp`): `-u/--up` keeps its two-token + `required_argument`-images-directory-plus-optional-compose-file shape, + while `-d/--down` became its own `no_argument` option with a single + manually-peeked optional trailing token (same "doesn't look like the + next flag" guard as `-u`'s own second token) — `ParsedArgs::compose_images_directory` + is now only ever set for `Mode::compose_up`, left unset for + `Mode::compose_down`. + + **Verified manually, end to end**: `-u` followed by a bare `-d` (no + images directory) against a real 2-service stack correctly finds and + stops both via the recorded state file, removes it, and leaves no + processes behind (confirmed via `--list-containers`, now empty, and + `ps`); a `-d` run with nothing recorded for that compose file exits + cleanly (`"no running services found for ..."`, exit 0), not an error. + **`commands.cpp` exports needed for all of the above** (each a pure refactor out of its own former anonymous-namespace scope, no behavior change, verified via `meson test` plus manual `-r/--run` smoke tests diff --git a/src/cli_args.cpp b/src/cli_args.cpp index f6130de..a5e7162 100644 --- a/src/cli_args.cpp +++ b/src/cli_args.cpp @@ -124,7 +124,7 @@ constexpr std::array long_options = {{ {"port-forward", required_argument, nullptr, 'p'}, {"no-dns", no_argument, nullptr, options::no_dns}, {"up", required_argument, nullptr, 'u'}, - {"down", required_argument, nullptr, 'd'}, + {"down", no_argument, nullptr, 'd'}, {"list-containers", no_argument, nullptr, options::list_containers}, {nullptr, 0, nullptr, 0}, }}; @@ -154,7 +154,7 @@ void print_usage(const char* prog) { " {0} --clean-processes\n" " {0} --list-containers\n" " {0} -u|--up []\n" - " {0} -d|--down []\n" + " {0} -d|--down []\n" " {0} [-c|--config-file ] -w|--write-config\n" " {0} -t|--test [-- ]\n" " {0} -h|--help\n" @@ -305,16 +305,25 @@ void print_usage(const char* prog) { " left behind by sessions that are no longer\n" " running\n" " -u, --up []\n" - " load and validate a Compose file (default\n" - " \"compose.yaml\", resolved relative to the\n" - " current directory) against \n" - " (where its services' images are expected to\n" - " live) -- orchestration itself (actually\n" - " starting the services) isn't implemented yet\n" - " -d, --down []\n" - " same file/directory resolution as -u/--up --\n" - " orchestration itself (actually stopping the\n" - " services) isn't implemented yet\n" + " bring up a Compose file (default \"compose.yaml\",\n" + " resolved relative to the current directory):\n" + " resolves every service's image against\n" + " (where they're expected to\n" + " live), mounts all of them, provisions any\n" + " managed networks/volumes, then starts every\n" + " service, in dependency order, in the background\n" + " (as if -D/--daemonize had been given). Port-\n" + " forwarding and an explicit user/group aren't\n" + " wired up for compose services yet\n" + " -d, --down []\n" + " stop every service a previous -u/--up started\n" + " for this same compose file (same default/\n" + " resolution as -u/--up's own compose-file-name --\n" + " no images directory needed here, since nothing\n" + " is mounted or resolved). Doesn't tear down the\n" + " compose file's own managed networks/volumes\n" + " (matching real Compose's own default -- they\n" + " survive unless explicitly removed)\n" " --list-containers list containers started by -u/--up, across every\n" " compose file that's been run at least once --\n" " compose file, service name, container name, pid,\n" @@ -415,7 +424,7 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { optind = 0; opterr = 0; int opt; - while ((opt = getopt_long(argc, argv, ":hVtc:r:l:v:i:x:Dwn:p:u:d:", long_options.data(), nullptr)) != -1) { + while ((opt = getopt_long(argc, argv, ":hVtc:r:l:v:i:x:Dwn:p:u:d", long_options.data(), nullptr)) != -1) { switch (opt) { case 'h': print_usage(argv[0]); @@ -528,23 +537,22 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { ++optind; break; } - case 'u': - case 'd': { - // -u/--up and -d/--down both take the OCI images directory - // (optarg, required) plus an optional second token naming the - // compose YAML file to load, resolved relative to the cwd -- - // same manual second-token consumption -v/--volume already - // uses above, except this second token is optional: only - // consumed when present and not itself the next flag (the - // same "looks like -x" guard -v's own required second token - // uses to detect it was omitted). - Mode requested = (opt == 'u') ? Mode::compose_up : Mode::compose_down; - if (out.mode != Mode::none && out.mode != requested) { + case 'u': { + // -u/--up []: the + // images directory (optarg) is required; the second, + // optional token names the compose YAML file to load, + // resolved relative to the cwd -- same manual second-token + // consumption -v/--volume already uses above, except this + // second token is optional: only consumed when present and + // not itself the next flag (the same "looks like -x" guard + // -v's own required second token uses to detect it was + // omitted). + if (out.mode != Mode::none && out.mode != Mode::compose_up) { spdlog::error("multiple actions specified"); print_usage(argv[0]); return 1; } - out.mode = requested; + out.mode = Mode::compose_up; out.compose_images_directory = optarg; if (optind < argc && !(argv[optind][0] == '-' && argv[optind][1] != '\0')) { out.compose_file_name = argv[optind]; @@ -552,6 +560,25 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { } break; } + case 'd': { + // -d/--down []: unlike -u/--up, no + // images directory is needed at all -- stopping a running + // stack doesn't mount or resolve any image. The single + // optional token names the compose YAML file, same + // resolution/default and "looks like a flag" omission + // guard as -u/--up's own second token. + if (out.mode != Mode::none && out.mode != Mode::compose_down) { + spdlog::error("multiple actions specified"); + print_usage(argv[0]); + return 1; + } + out.mode = Mode::compose_down; + if (optind < argc && !(argv[optind][0] == '-' && argv[optind][1] != '\0')) { + out.compose_file_name = argv[optind]; + ++optind; + } + break; + } case 'n': // -n/--network takes a single token (the name), always via // getopt's own required_argument -- repeatable, so it's diff --git a/src/cli_args.h b/src/cli_args.h index 05d4ae1..ae57655 100644 --- a/src/cli_args.h +++ b/src/cli_args.h @@ -123,16 +123,20 @@ struct ParsedArgs { // (possibly -c-sourced) global config's own log-level -- see // apply_log_level()'s own doc comment below for why this matters. bool log_level_flag_given = false; - // -u/--up [] and -d/--down - // []: `compose_images_directory` - // (optarg) is required; the second, optional token names the compose - // YAML file to load -- resolved relative to the current working - // directory, same as any other plain file argument on this CLI (e.g. - // -r/--run ), *not* relative to the images directory. - // Defaults to "compose.yaml" when omitted. Consumed the same manual - // second-token technique -v/--volume already uses (cli_args.cpp), just - // with the second token optional rather than required. + // -u/--up []: `compose_images_directory` + // (optarg) is required -- only ever set for Mode::compose_up, unset + // for Mode::compose_down, which needs no images directory at all + // (stopping a running stack doesn't mount or resolve any image). std::optional compose_images_directory; + // -u/--up's own second, optional token, and -d/--down's single + // optional token: names the compose YAML file to load -- resolved + // relative to the current working directory, same as any other plain + // file argument on this CLI (e.g. -r/--run ), *not* + // relative to the images directory. Defaults to "compose.yaml" when + // omitted. Consumed the same manual optional-trailing-token technique + // in both cases (cli_args.cpp) -- similar in spirit to -v/--volume's + // own manual second-token consumption, but that one is required, not + // optional. std::string compose_file_name = "compose.yaml"; }; diff --git a/src/commands.cpp b/src/commands.cpp index 7a0ee51..203c4f6 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -638,12 +638,13 @@ int write_config_command(const std::filesystem::path& config_path, const AppConf return 0; } -// Shared by compose_up_command()/compose_down_command() below: both need -// the exact same resolve-images-directory + load_compose_file() + -// validate_compose_external_state() step before doing anything -// mode-specific -- which, for now, is nothing at all; see each caller's own -// "not implemented yet" message. `compose_file_name` is resolved relative -// to the current working directory (cli_args.h's own doc comment on +// -u/--up's own resolve-images-directory + load_compose_file() + +// validate_compose_external_state() step, shared by compose_up_command() +// below. compose_down_command() doesn't use this at all -- unlike -u/--up, +// stopping a stack needs no images directory and doesn't need the compose +// file to still exist or parse (everything it needs is already in the +// state file -u/--up wrote). `compose_file_name` is resolved relative to +// the current working directory (cli_args.h's own doc comment on // ParsedArgs::compose_file_name), independent of `images_directory`. std::optional load_and_validate_compose(const std::string& images_directory, const std::string& compose_file_name, AppConfig& config) { @@ -709,13 +710,22 @@ int compose_up_command(const std::string& images_directory, const std::string& c return started.size() == compose->services.size() ? 0 : 1; } -// Stub for -d/--down -- same idea as compose_up_command() above. -int compose_down_command(const std::string& images_directory, const std::string& compose_file_name, - AppConfig& config) { - if (!load_and_validate_compose(images_directory, compose_file_name, config)) { - return 1; +// -d/--down: unlike -u/--up, needs no images directory at all -- stopping +// a running stack doesn't mount or resolve any image, so this doesn't call +// load_and_validate_compose() (that helper stays -u/--up-only from here +// on). stop_compose_services() (compose_orchestrator.h) does all the real +// work: reads the state file -u/--up wrote for this same compose path, +// stops every recorded session, and removes the state file. A missing +// state file (no -u/--up ever ran for this file) isn't an error -- just +// nothing to do. +int compose_down_command(const std::string& compose_file_name) { + auto compose_path = std::filesystem::absolute(compose_file_name); + auto stopped = stop_compose_services(compose_path); + if (stopped.empty()) { + fmt::print("no running services found for {}\n", compose_path.string()); + } else { + fmt::print("stopped {} service(s)\n", stopped.size()); } - spdlog::warn("compose orchestration (-d/--down) isn't implemented yet -- nothing was actually stopped"); return 0; } @@ -1086,7 +1096,7 @@ int dispatch_command(const ParsedArgs& args, const std::filesystem::path& config case Mode::compose_up: return compose_up_command(*args.compose_images_directory, args.compose_file_name, config); case Mode::compose_down: - return compose_down_command(*args.compose_images_directory, args.compose_file_name, config); + return compose_down_command(args.compose_file_name); case Mode::run: { // As root, containers-storage mount doesn't need to reexec into a private // user namespace to gain privilege, so the mount is already directly diff --git a/src/compose_orchestrator.cpp b/src/compose_orchestrator.cpp index 5d693a8..3fed6e0 100644 --- a/src/compose_orchestrator.cpp +++ b/src/compose_orchestrator.cpp @@ -32,6 +32,7 @@ #include "containers_storage.h" #include "daemonize.h" +#include "kill_session.h" #include "oci_image.h" #include "pid_file.h" @@ -359,3 +360,61 @@ std::vector list_compose_containers() { return result; } + +std::vector stop_compose_services(const std::filesystem::path& compose_path) { + std::vector stopped; + + auto state_path = compose_state_file_path(compose_path); + std::ifstream in(state_path); + if (!in) { + return stopped; + } + + std::string header; + if (!std::getline(in, header)) { + return stopped; + } + + std::map grace_periods; + if (auto compose = load_compose_file(compose_path)) { + for (const auto& service : compose->services) { + if (service.stop_grace_period_seconds) { + grace_periods[service.name] = *service.stop_grace_period_seconds; + } + } + } + + std::string line; + while (std::getline(in, line)) { + std::istringstream fields(line); + std::string service_name; + std::string container_name; + pid_t pid = -1; + if (!(fields >> service_name >> container_name >> pid)) { + continue; + } + + int grace_period = 10; + auto it = grace_periods.find(service_name); + if (it != grace_periods.end()) { + grace_period = it->second; + } + + if (kill_session(pid, grace_period) == 0) { + fmt::print("stopped service '{}' (container '{}', pid {})\n", service_name, container_name, pid); + } else { + spdlog::warn("service '{}' (container '{}', pid {}) was already stopped, or failed to stop cleanly", + service_name, container_name, pid); + } + stopped.push_back(service_name); + } + in.close(); + + std::error_code ec; + std::filesystem::remove(state_path, ec); + if (ec) { + spdlog::warn("failed to remove compose state file {}: {}", state_path.string(), ec.message()); + } + + return stopped; +} diff --git a/src/compose_orchestrator.h b/src/compose_orchestrator.h index cdb6c62..811eccd 100644 --- a/src/compose_orchestrator.h +++ b/src/compose_orchestrator.h @@ -223,3 +223,40 @@ struct ComposeContainerInfo { // list_sessions() itself already uses for a malformed pid file. An empty // or missing compose/ directory yields an empty result, not an error. std::vector list_compose_containers(); + +// -d/--down: reads back the compose state file for `compose_path` +// (compose_state_file_path()/record_compose_services() -- written by the +// most recent -u/--up run against this same compose file) and stops +// (kill_session(), kill_session.h -- the exact same graceful +// SIGTERM-then-SIGKILL, cgroup-aware stop `--kill ` already uses) +// every session it recorded, then removes the state file itself once +// done. Deliberately doesn't need `images_directory` or otherwise resolve +// any image, unlike -u/--up: everything strictly required to stop a +// session (service name, container name, pid) is already in the state +// file. If the compose file at `compose_path` still exists and parses +// (load_compose_file(), compose_file.h -- best-effort, not required), +// each service's own `stop_grace_period_seconds`, if it declared one, is +// used as `kill_session()`'s own grace period instead of that function's +// hardcoded 10s default; a service no longer found there (the file was +// edited/moved/deleted since -u/--up ran) just falls back to that +// default -- the state file alone remains sufficient either way. A +// session already stopped by hand (kill_session() itself reports failure +// for a pid that isn't a tracked, running session) is logged as a warning +// and otherwise treated the same as one this call did stop -- both are +// simply "no longer running" by the time this returns. A missing or +// unreadable state file (no -u/--up has ever been run for this compose +// file, or it was already torn down) is not an error -- returns an empty +// result. +// +// **Known scope limitation**: doesn't tear down the compose file's own +// managed networks/volumes (provision_compose_networks_and_volumes()'s +// own state) -- matching real `docker compose down`'s own default +// (networks/volumes survive unless `-v`/`--volumes` is also given), but +// unlike real Compose, there's no equivalent flag here yet to opt into +// removing them either. +// +// Returns the service names actually found and attempted in the state +// file (regardless of whether kill_session() itself reported them as +// already-stopped), the same "what did this touch" reporting shape +// clean_stale_sessions() et al. already use. +std::vector stop_compose_services(const std::filesystem::path& compose_path); diff --git a/tests/unit/test_cli_args.cpp b/tests/unit/test_cli_args.cpp index 99f1ab6..aae51d2 100644 --- a/tests/unit/test_cli_args.cpp +++ b/tests/unit/test_cli_args.cpp @@ -277,18 +277,28 @@ TEST_CASE("parse_args: -u doesn't swallow a following flag as compose_file_name" CHECK(result.args.log_level_flag_given); } -TEST_CASE("parse_args: -d/--down works the same way as -u/--up", "[unit]") { - auto default_name = run_parse({"--down", "/var/lib/images"}); +TEST_CASE("parse_args: -d/--down takes a single optional compose-file-name, no images directory", "[unit]") { + auto default_name = run_parse({"--down"}); REQUIRE_FALSE(default_name.exit_code.has_value()); CHECK(default_name.args.mode == Mode::compose_down); + CHECK_FALSE(default_name.args.compose_images_directory.has_value()); CHECK(default_name.args.compose_file_name == "compose.yaml"); - auto explicit_name = run_parse({"-d", "/var/lib/images", "my-stack.yaml"}); + auto explicit_name = run_parse({"-d", "my-stack.yaml"}); REQUIRE_FALSE(explicit_name.exit_code.has_value()); CHECK(explicit_name.args.mode == Mode::compose_down); + CHECK_FALSE(explicit_name.args.compose_images_directory.has_value()); CHECK(explicit_name.args.compose_file_name == "my-stack.yaml"); } +TEST_CASE("parse_args: -d doesn't swallow a following flag as compose_file_name", "[unit]") { + auto result = run_parse({"-d", "--log-level", "debug"}); + REQUIRE_FALSE(result.exit_code.has_value()); + CHECK(result.args.mode == Mode::compose_down); + CHECK(result.args.compose_file_name == "compose.yaml"); + CHECK(result.args.log_level_flag_given); +} + TEST_CASE("parse_args: -u and -d together on the same command line is a parse error", "[unit]") { auto result = run_parse({"-u", "/var/lib/images", "-d", "/var/lib/images"}); REQUIRE(result.exit_code.has_value());