[feat] Add wait-ready command to check Multipass daemon readiness #4145
Conversation
|
Hey @rluna319, thank you for this! We'll review when we get a chance. |
|
Hey @levkropp, would you mind doing a secondary review of this one? Than I can do a very light "tertiary" review. |
There was a problem hiding this comment.
Great work, @rluna319! I appreciate the work you've put into this. I was on the verge of implementing a similar feature for the upcoming CLI tests I'm working on, and you can't imagine how happy I was to see this PR :)
Overall, the code looks good! I built and tested your changes locally, and everything works as expected. I have a small request in line, and also, having some unit tests would be nice.
There was a problem hiding this comment.
Excellent work @rluna319, especially for a first contribution! I have a number of requests, but they are for polishing and small things. The bulk of the feature looks solid. Just a few tweaks and we should be able to merge.
Many thanks for your involvement and feel free to keep more of these coming 😃
|
@rluna319, could you also rebase on the latest main, to see if we can get some CI to run on this? Appreciate it. |
|
@ricab I had not included your requested changes on my latest commit. I had not seen them before I made the push, my fault. I did rebase on the latest main however. I will work on your requested changes ASAP. Would you like me to amend your requested changes to my latest commit or have it in a separate commit? |
|
Hey @rluna319 no worries. We generally prefer additional commits, to make it easy to identify new changes. Thanks! |
|
@ricab I've added all of your requested changes to the recent commit. I also rebased on the latest main as well. |
There was a problem hiding this comment.
Getting real close @rluna319! I am going to let CI go through and I think the Lint workflow is going to request some formatting changes. Other than that, I have mostly minor requests. There's only one for real behavior, but I think we can discuss and split off as a separate task if we need it.
|
Hey @rluna319, it looks like you haven't signed our CLA yet. Would you mind doing so, please? https://ubuntu.com/legal/contributors Also, there are a few linting problems. Editors often have options to strip trailing whitespace and keep a single newline at the end. You can check whitespace sanity on a diff with
|
|
@ricab I signed the CLA, went through linter errors (and other CI build/test errors) and fixed them to the best of my knowledge, ran the code through |
|
I removed some commented out includes that shouldn't have been in that commit. |
|
I see that I still missed some linter errors. I thought I ran everything through |
|
@ricab Ive addressed the issues. It says some of the workflows are awaiting approval to be run. |
Approved the workflow. |
|
Thank you @xmkg. I think that the remaining workflows that fail are due to some internal CI issues. Something about a missing token. Is this something I need to fix or can fix on my end? |
Yeah, unfortunately, our CI is not happy with PRs coming from external forks. We're working on making this process smoother, but the necessary machinery is not yet in place. Would you mind if I create a PR for your PR, just for the sake of running the pipeline? We can track the progress and make changes here. |
|
Oh ok. Sure thing. |
|
Opened #4304. |
|
@rluna319, we've landed a bunch of CI-related fixes to |
This commit introduces the `multipass wait-ready` command. This new
command allows clients to block until the Multipass daemon is fully
initialized and ready to accept requests.
The command first polls the daemon until its gRPC socket is available.
Once the daemon is reachable and receives the `WaitReady` request, it
verifies its readiness by attempting to update image manifests, which
implicitly checks connectivity to image servers.
Users can specify a `--timeout` (in seconds), after which the command
will terminate with an error if the daemon is not yet ready. If no
timeout is specified, the command will wait indefinitely until the
daemon is ready or some other error occurs.
Implementation Details:
- Client-side (`wait_ready.cpp`, `wait_ready.h`):
- Created the `WaitReady` command logic and its header file.
- Implemented a retry loop to poll for daemon socket availability.
- Utilized the existing `mp::cmd::parse_timeout()` function from
`common_cli.h` to handle the `--timeout` option.
- Integrated `AnimatedSpinner` for visual feedback during the
waiting period.
- Added error handling for various failure conditions (e.g., timeout,
daemon connection errors).
- Protocol (`multipass.proto`):
- Defined the `WaitReadyRequest` and `WaitReadyReply` messages and
the `WaitReady` RPC method within the `Multipass` service.
- Daemon-side:
- Added the `wait_ready` gRPC service handler function in
`daemon_rpc.cpp` to process incoming requests.
- Implemented the core `wait_ready` logic in `daemon.cpp`, which
leverages the existing
`wait_update_manifests_all_and_optionally_applied_force()`
function to check image server connectivity.
- Testing (`tests/mock_client_rpc.h`):
- Added mock methods for `wait_readyRaw`, `Asyncwait_readyRaw`,
and `PrepareAsyncwait_readyRaw` to `MockRpcStub` to support
testing of the new RPC endpoint.
This commit introduces unit test for the `wait-ready` command along with improvements to its client-side and daemon-side command logic. The daemon now avoids forcing manifest downloads during `wait-ready`. The daemon's `wait-ready` gRPC handler now uses the correct gRPC status code when there is an issue verifying connection to the image servers. Unit tests for `wait-ready` have been added. Implementation Details: - Daemon (`daemon.cpp::wait_ready()`): - Set `force_manifest_network_download` to `false` to prevent forcing unnecessary manifest downloads while verifing imager server connection. - Change `grpc::StatusCode` from `UNAVAILABLE` to `FAILED PRECONDITION` to avoid client retry loop from continuing when there is an exception thrown during the `update_manifests_all_task`. - Client (`wait_ready.cpp`): - Remove redundant cerr output. - Add logic to stop `timer` and `spinner` if the `--timeout` option is enabled. - Unit Testing: - Added `test_daemon_wait_ready.cpp` test suite to verify daemon-side `wait-ready` logic. - Added `wait-ready` cli tests to `test_cli_client.cpp` to verify client- side command parsing and failure handling. - Added `wait-ready` to the existing timeout and client logging test suites.
Refactors how the `--timeout` option is added to commands to improve code reuse and provide more context-specific help text. A private helper function, `add_timeout_option`, is introduced in an anonymous namespace to handle the core logic of creating the option. This is now used by two public functions: - `add_timeout`: Provides a generic description, now used by `wait-ready`. - `add_instance_timeout`(NEW): Provides a more detailed description for commands that operate on instances. The `wait-ready` command implementation has also been improved: - The `spinner` and `timer` are now local variables within the `run()` method to minimize their scope. - Associated headers (`chrono`, `thread`, `timer.h`, `animated_spinner.h`) have been moved from the header into `wait_ready.cpp`.
The daemon will now catch any DownloadExceptions and set the grpc status to NOT_FOUND with a specific error message to let the client know that the daemon was not able to reach the image servers. The client will retry (send another dispatch) on this error status until the `--timeout` specified is reached or the daemon successfully verifies connection with the image servers. Implementation Details: - Daemon (`daemon.cpp::wait_ready()`) - When a `DownloadException` is caught, the status code is set to `NOT_FOUND`, instead of `FAILED_PRECONDITION`, with a specific error message. - Client (`wait_ready.cpp`) - The `on_failure` lambda now checks for the specific error message alongside the `NOT_FOUND` status code. - This allows for differentiation between `NOT_FOUND` being returned by the dispatcher when the daemon's socket is unavailable, and `NOT_FOUND` being returned by the daemon when it catches a `DownloadException`. - Removed unused `this` capture from `on_success` lambda to satisfy macOS build error. - Testing (`test_daemon_wait_ready.cpp`) - Refactored the test case where a `DownloadException` is thrown by `update_mainfests()` to allow additional calls due to the new client retry logic. - Added EXPECT_CALL for mock_settings.get(winterm_key) to address uninetersting mock call error for windows platform tests. - Testing (`test_cli_client.cpp`) - Added a test case to check proper handling of the new grpc status set by the daemon when it catches a `DownloadException`. Other: - Fixed linter errors - Addressed improper log levels being used - Fixed formating issues - Removed unnecessary includes format and fix exception format with clang-format remove commented out includes
Details: - Remove checking of grpc status error strings in the `on_failure` lambda in `wait_ready.cpp`. - Resolve missed linter errors. - Resolve 3rd party jsoncpp submodule pointing to wrong version.
|
@xmkg I have rebased the PR. Workflows awaiting approval. |
|
@xmkg Great thank you. I see all the CI tests have passed in #4304. Should I mind the coverage report? I've looked at it already and I can cover the line in However, i'm having trouble covering the other one in |
That would be nice.
I'd say don't worry about it. There's a way to trigger that exception catcher... but it's a bit cheeky, though. We could use a logger that throws an exception on |
- added a new test in `test_cli_client.cpp` which covers the conditional `if (timer)` in the `on_failure` lambda.
|
@xmkg I added the coverage for I tried to implement your suggestion, throwing and exception on |
github-merge-queue
bot
closed this pull request by merging all changes
into
canonical:main
in
c50c359
|
Our pleasure @rluna319! This was a great contribution, very well pondered. Feel free to send more in! |
Feature: Add
multipass wait-readycommandDescription:
This PR introduces a new CLI command,
multipass wait-ready. This command allows users and scripts to pause execution until the Multipass daemon is fully initialized and ready to accept requests. This is particularly beneficial for automation workflows that need to ensure the daemon is operational before proceeding with other Multipass operations.The
wait-readycommand works by:WaitReadyrequest to the daemon.--timeoutoption (in seconds). If the daemon isn't ready within the specified timeout, the command exits with an error. If no timeout is given, it waits indefinitely.Motivation:
Related Issues:
Fixes #452
Fixes #3723
Key Implementation Changes:
wait_ready.cpp): New command logic, including retry for socket connection, timeout handling (reusingmp::cmd::parse_timeout), and spinner integration.multipass.proto): AddedWaitReadyRequest/WaitReadyReplymessages andWaitReadyRPC method.daemon_rpc.cpp,daemon.cpp):daemon_rpc.cpp.daemon.cppleverages the existingwait_update_manifests_all_and_optionally_applied_force()function.How to Test:
$ cd build.--timeout, simply run thewait-readycommand with the--timeoutoption:wait-ready), start up the daemon and immediately usemultipass launch:wait-readycommand solves the issue, fit thewait-readycommand between daemon startup andmultipass launch:multipass launchprocess. You may Ctrl+C here to terminate the launch process.Points for Reviewers:
wait_update_manifests_all_and_optionally_applied_force(true)is an appropriate and robust proxy for overall daemon readiness.