-
Notifications
You must be signed in to change notification settings - Fork 642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multipass clone #3264
Open
georgeliao
wants to merge
175
commits into
main
Choose a base branch
from
multipass_clone
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Multipass clone #3264
Changes from all commits
Commits
Show all changes
175 commits
Select commit
Hold shift + click to select a range
e40376c
[grpc] Added multipass clone protobuf message place holder.
georgeliao 4d8c1a1
[grpc] Added missing fields of clone message
georgeliao 0fbed45
[cli] Added clone command line class and functions interfaces.
georgeliao fc6a33f
[cli] Added parse_args implementation and invocation.
georgeliao fd17fef
[cli] Added grpc code on the frond end.
georgeliao 96b42ff
[grpc] Create the rpc framework for daemon::clone function.
georgeliao 49b0056
Added rpc_request fill-in code on the front end.
georgeliao 2d602c6
Added source_name validity check.
georgeliao 370ca51
Added specified destination name check.
georgeliao cddeca1
Added generating destination name.
georgeliao 47e28ee
Added the source instance state check.
georgeliao 57478bf
Small renaming for the vm instance pointer variable
georgeliao cc99eb3
Formatted legacy code diff on this branch based on the new .clang-for…
georgeliao d6a6a72
[daemon] Added clone_spec_item_and_write_to_instance_db lambda functi…
georgeliao 6b4abef
[daemon] Wrap the destination name related code into generate_destina…
georgeliao 7b5c083
[imageVault] Added clone method for DefaultVMImageVault class
georgeliao b8ae251
[daemon] Added vault clone method interface and the caller of that.
georgeliao 25a934e
[daemon] Added the code of virtual machine instance folder copy.
georgeliao c155026
[file_ops] Added copy file wrapper function.
georgeliao 97fc1bd
[daemon] replace the std file copy function with the wrapper one.
georgeliao fd732fd
[daemon] added qemu_iso and the meta_data and network-config parts.
georgeliao ba99097
[daemon] Added mount directory creation.
georgeliao 4704c20
[daemon] Added mount command for the iso file.
georgeliao a4a7b41
[daemon] add the loading the same parts of iso file and write to the …
georgeliao 524c04b
[daemon] Added unmount command.
georgeliao fba93ca
[daemon] Added removing directory.
georgeliao 447af7e
[daemon] added create_virtual_machine and init_mount code for destina…
georgeliao f75e054
[daemon] fix the compiler warning which caused the build failure.
georgeliao 716eb5f
[daemon] small fix for rebase conflict.
georgeliao 26d3b83
[daemon, cli] Added the reply message of clone and improved the clone…
georgeliao 45e60a9
[snapshot] modified the find_parent function and added call to snapsh…
georgeliao 1e3fda3
[snapshot] added qemu_snapshot constructor that is based on another s…
georgeliao 95e6540
[snapshot] added clone function interface.
georgeliao 8179527
[virtual_machine] added clone method of qemu virtual machine
georgeliao 6cf74e0
[virtual machine] added clone_snapshots method place holder.
georgeliao d3a67fc
[virtual machine] small name improvement for the snapshot clone funct…
georgeliao a688d9b
[snapshot] fix the constness of snapshot classes
georgeliao c3e5702
[virtual machine] fix the constness of snapshot in clone_one_snapshot…
georgeliao cc5cab5
[snapshot] reverted the in RAM clone snapshot related code.
georgeliao 8bda1a1
[snapshot] added new constructor for for constructing from file and r…
georgeliao 06e1503
[snapshot] added qemu_snapshot constructor.
georgeliao a77db55
[virtual machine] added make_specific_snapshot function place holder
georgeliao 3e6d45b
[virtual machine] added make_specific_snapshot qemu implementation.
georgeliao 28065bc
[virtual machine] added public method load_snapshots_and_update_uniqu…
georgeliao 23b17f3
[virtual machine] added load one file to snapshot function. PS: refac…
georgeliao fab405d
[daemon] added the loading and updating snapshots calls, now the whol…
georgeliao 4d7f13a
[json] added update_unique_identifiers_of_metadata function and refac…
georgeliao 4878dc8
[virtual machine] added load_snapshots_common to reduce the repeated …
georgeliao 593196f
[virtual machine] added variadic templated function load_snapshot_and…
georgeliao eab612f
[virtual machine] changed load_snapshots_common function to variadic …
georgeliao 13958eb
[virtual machine] added explanatory comment for load_snapshots_common…
georgeliao 3dc81b2
[cli] replaced the destination name positional argument with option.
georgeliao ec189fe
[cli] fix the warning (compilation error) after upgrading to ubuntu 2…
georgeliao 4cb037a
[cli] fixed and refined the destination name description.
georgeliao 6e0f108
[daemon] added destination name validity check.
georgeliao 643fe95
[virtual machine] improved the name generator based on the team discu…
georgeliao a36a3e0
[utils] move some yaml helpers functions into utils.cpp to set up the…
georgeliao 9c7a9f6
[daemon] small refactoring, localize the lambda function further.
georgeliao 566ece9
[vm factory] added create_vm_and_instance_disk_data function for clon…
georgeliao f076464
[vm factory] added create_vm_and_instance_disk_data interface and cal…
georgeliao e035496
[vm factory] using the new read_from function to read the cloud-init-…
georgeliao e1f2a69
[cloud init] added at operator, it is similar with std::unordered_map…
georgeliao b93250c
[vm factory] added load string and replace name functionality of mak…
georgeliao 638e6c8
[cloud init] added contains function.
georgeliao 23dd54f
[vm factory] refine the client code of make_cloud_init_network_config…
georgeliao 77ed82c
[cloud init] clean up some CloudInitIso utility functions/
georgeliao 4171139
[vm factory] added early back-end check for the clone operation.
georgeliao 609b3c7
[json util] refined on the source name to destination name string re…
georgeliao 7b81f5c
[daemon] added run_at_boot commands unique identifiers update for clo…
georgeliao 54bccb3
[daemon] moved cloned_instance_count from virtual machine class to vm…
georgeliao 1f91f74
[daemon] fix a small error from the previous commit.
georgeliao 47cd576
[daemon] refined the clone_spec_item function.
georgeliao 68be40c
[daemon] use find_instance_and_react to make the error report of clon…
georgeliao 50c7822
[daemon] correct the extra interfaces update in clone.
georgeliao 85fe30a
[daemon] removed update_extra_interfaces_mac_address_of_run_at_boot s…
georgeliao 9b2c3d0
[vm factory] added scope_guard clean up function for the create_vm_an…
georgeliao 4019ea2
[cloud init] rebase fix.
georgeliao b069450
[json utils] fix formatting.
georgeliao eb0a6c9
fixed the post-merge compilation.
georgeliao 54e5d0c
[bash completion] added clone bash completion
georgeliao 48de54a
[unit test] added clone cli tests.
georgeliao c1f648b
[unit test][daemon] added the daemon clone unit test frame work.
georgeliao 3e4092c
[unit test][daemon] added unusual unit test case.
georgeliao 8cbe7b2
[unit test][daemon] added failsOnCloneOnNonStoppedInstance test.
georgeliao e4e69e4
rebase fix.
georgeliao 85065c9
[yaml utils] made the make_cloud_init_meta_config to update instance_…
georgeliao 7dbeaaa
[json utils] added updating cloud init instance id in snapshot file c…
georgeliao 2815630
[unit test][json utils] added unit tests for update_cloud_init_instan…
georgeliao 2093e42
[json utils] changed update_unique_identifiers_of_metadata parameter …
georgeliao ea81690
[clone] small comment correction.
georgeliao 4e15104
[clone] small cleanup for some comments.
georgeliao 13b9e34
[daemon] refined the the roll back mechanism in clone.
georgeliao 97ba9b4
[unit test] added more unit tests for client failed command line args…
georgeliao 2845d38
[unit test] added clone invalid option unit test.
georgeliao efd0cd3
[cmd] added AnimatedSpinner to clone command.
georgeliao 3e6f277
[client][unit test] added command fail error to cover action_on_failu…
georgeliao c364287
[vm factory] use in class data member instances_dir to derive the sou…
georgeliao 1bee362
[vm image] improve one function parameter name.
georgeliao 2032a3d
[clone] function name improvement.
georgeliao 90a59e6
[clone] added update_cloned_cloud_init utility function.
georgeliao d36c380
[cloud init] [unit test] added unit test for update_cloned_cloud_init…
georgeliao 718973e
[vm factory] use update_cloned_cloud_init function to prepare for uni…
georgeliao 9ccfc5e
[unit test] added FileOps::copy unit test.
georgeliao e3a36fd
[image vault] added unit test for DefaultVMImageVault::clone.
georgeliao 5c339c3
[image vault] [unit test] added imageCloneFailOnNonExistSrcImage case.
georgeliao 7477df7
[image vault] [unit test] added imageCloneFailOnAlreadyExistDestImage…
georgeliao 9274626
[lxd image vault][unit test] added lxdImageVaultCloneThrow unit test.
georgeliao 6389e79
[unit test] added coverage for DaemonRpc::clone.
georgeliao c0d18a1
[daemon] small refactor on generate_next_clone_name function.
georgeliao 621f499
[cloud init] small function name improvement and typo correction.
georgeliao 3f81305
[vm image] improves the image_path sub-string replacement, get it con…
georgeliao 255d6ce
[qemu][vm factory] fixed rollback instance directory removal did not …
georgeliao 57b8fc8
[clone] added metadata non-empty check for the non-qemu backend cases.
georgeliao f452dd7
[unit test] removed assertion line coverage unit test because windows…
georgeliao 2752fa6
[qemu][vm factory] using the QemuVirtualMachine constructor directly …
georgeliao 8b8222d
[qemu][vm factory] added copy_instance_dir_without_snapshot_files fun…
georgeliao 1ed3178
[qemu] remove all load_snapshots_and_update_unique_identifiers relate…
georgeliao 61d5648
[virtual machine] added the placeholder of the remove_all_snapshots_f…
georgeliao 139d271
[qemu][virtual machine] added qemu remove_all_snapshots_from_the_imag…
georgeliao 6955469
[vm] removed get_vm_name because it is not needed.
georgeliao fb09fec
[cli] improve the command description based on review comments.
georgeliao 0ea8eb8
[daemon] use release_resources function to simplify the roll back mec…
georgeliao 08d66b1
[daemon] moving is_name_already_used from local lambda to private fun…
georgeliao 4469a9b
[daemon] moving generate_destination_name and clone_spec to private f…
georgeliao dab1ad6
[json utils] refined the update_cloud_init_instance_id function.
georgeliao a99038d
[lxd][vm image] Using NotImplementedOnThisBackendException to be more…
georgeliao 7491d6b
[qemu factory] simplified the copy_instance_dir_without_snapshot_file…
georgeliao 884832d
[daemon] using instance_trail to get the source_vm_ptr pointer to sav…
georgeliao 532dc9e
[daemon] added cloneInvalidNameException and set the corresponding gr…
georgeliao 3636b01
[daemon][cli] small wording improvement.
georgeliao 82b138a
[daemon] added preparing_instances for clone operation, followed the …
georgeliao 92109f0
[lxd] fixed one unit test.
georgeliao b2a9f0c
[daemon] polishing some daemon::clone code
georgeliao 386ce53
[daemon] added invalidDestVmName test.
georgeliao fa3af0c
[unit test] change the instance_name for better clarity.
georgeliao d7b6dc3
[unit test][daemon] added alreadyExistDestVmName unit test.
georgeliao 8613150
[unit test] added unit test for qemu removeAllSnapshotsFromTheImage.
georgeliao 42acc3e
[qemu][unit test] added createVmAndCloneInstanceDirData test.
georgeliao c647843
[unit test][daemon] added the extra interface so the daemon::clone su…
georgeliao cfba218
[qemu][unit test] added mocked snapshot_list output stream to cover s…
georgeliao 5d40b1c
Update src/daemon/daemon.cpp
georgeliao 1b0c647
[daemon] small format correction.
georgeliao 89591be
[daemon] small wording improvement.
georgeliao 85dd29b
[daemon] fix the format.
georgeliao 06308e4
[daemon][unit test] added successfulCloneSpecifyDestNameOkStatus unit…
georgeliao 9aa8840
Update src/client/cli/cmd/clone.cpp
georgeliao 767249e
renaming the function name from remove_all_snapshots_from_the_image t…
georgeliao 142fbf3
[unit test][daemon] removed the unused data member.
georgeliao 24ad520
[unit test][qemu] improved unit test createVmAndCloneInstanceDirData …
georgeliao f672a75
[daemon][unit test] small header cleanup.
georgeliao c3b3da4
Update src/platform/backends/lxd/lxd_vm_image_vault.h
georgeliao e585b54
[daemon][qemu unit test] small format improvement.
georgeliao fcc4904
[daemon] added preparing_instances removal to the rollback_clean_up_a…
georgeliao 71daf4a
[unit test] fix the unit test.
georgeliao bf3ed13
[qemu][unit test] improved the removeAllSnapshotsFromTheImage unit test.
georgeliao b3c2044
Use char* variable for cloud-init-config.iso string literal
georgeliao 5a46eca
[unit test] small tuning of the string construction.
georgeliao c30f674
Update src/daemon/daemon.cpp
georgeliao 4af1dcf
[daemon] changing throwing to early return.
georgeliao a644d2d
[daemon] move the srt vm state before the rollback and generating des…
georgeliao 35695ee
[daemon] converted from regular try-catch to function try-catch
georgeliao 84c5b64
[yaml utils] small header format improvement.
georgeliao 0247452
[yaml utils] make default interface node always present in the case o…
georgeliao 9bed6a3
[yaml utils] refactored the create_default_interface_node code into a…
georgeliao a224c69
[yaml utils] added dhcp-identifier: mac to all network interfaces.
georgeliao 30ea5b4
[cloud init] fixed the update_cloned_cloud_init_unique_identifiers di…
georgeliao 92363db
[qemu img utils] move snapshot -l into a function
georgeliao df8cd7d
[qemu img utils] move snapshot -d code into a function.
georgeliao dfe6e98
[qemu img utils] removed repeated qemu img utils function.
georgeliao 96a7664
[yaml utils] added comments for the new make_cloud_init_network_confi…
georgeliao 6ec5836
[qemu][unit test] corrected and improved the unit test removeAllSnaps…
georgeliao 75b21c9
[test cloud_init] change std::string_view to char* to adhere to the s…
georgeliao 4332fa1
[virtual machine] improve the exception message.
georgeliao cb40bca
[vm factory] created the instance folder copy utility function and us…
georgeliao 54fcf58
[cmd] improved the clone help text based the documentation review.
georgeliao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/* | ||
* Copyright (C) Canonical, Ltd. | ||
* | ||
* This program is free software; you can redistribute it and/or modify | ||
* it under the terms of the GNU General Public License as published by | ||
* the Free Software Foundation; version 3. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
* | ||
*/ | ||
|
||
#ifndef MULTIPASS_CLONE_EXCEPTIONS_H | ||
#define MULTIPASS_CLONE_EXCEPTIONS_H | ||
|
||
#include <stdexcept> | ||
|
||
namespace multipass | ||
{ | ||
class CloneInvalidNameException : public std::runtime_error | ||
{ | ||
public: | ||
using std::runtime_error::runtime_error; | ||
}; | ||
} // namespace multipass | ||
|
||
#endif // MULTIPASS_CLONE_EXCEPTIONS_H |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
/* | ||
* Copyright (C) Canonical, Ltd. | ||
* | ||
* This program is free software; you can redistribute it and/or modify | ||
* it under the terms of the GNU General Public License as published by | ||
* the Free Software Foundation; version 3. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
* | ||
*/ | ||
|
||
#include "clone.h" | ||
|
||
#include "animated_spinner.h" | ||
#include "common_cli.h" | ||
|
||
#include <multipass/cli/argparser.h> | ||
|
||
namespace mp = multipass; | ||
namespace cmd = multipass::cmd; | ||
|
||
mp::ReturnCode cmd::Clone::run(ArgParser* parser) | ||
{ | ||
const auto parscode = parse_args(parser); | ||
if (parscode != ParseCode::Ok) | ||
{ | ||
return parser->returnCodeFrom(parscode); | ||
} | ||
|
||
AnimatedSpinner spinner{cout}; | ||
auto action_on_success = [this, &spinner](CloneReply& reply) -> ReturnCode { | ||
spinner.stop(); | ||
cout << reply.reply_message(); | ||
|
||
return ReturnCode::Ok; | ||
}; | ||
|
||
auto action_on_failure = [this, &spinner](grpc::Status& status, CloneReply& reply) -> ReturnCode { | ||
spinner.stop(); | ||
return standard_failure_handler_for(name(), cerr, status, reply.reply_message()); | ||
}; | ||
|
||
spinner.start("Cloning " + rpc_request.source_name()); | ||
return dispatch(&RpcMethod::clone, rpc_request, action_on_success, action_on_failure); | ||
} | ||
|
||
std::string cmd::Clone::name() const | ||
{ | ||
return "clone"; | ||
} | ||
|
||
QString cmd::Clone::short_help() const | ||
{ | ||
return QStringLiteral("Clone an instance"); | ||
} | ||
|
||
QString cmd::Clone::description() const | ||
{ | ||
return QStringLiteral( | ||
"Create an independent copy of an existing instance. The instance must be stopped before you proceed."); | ||
} | ||
|
||
mp::ParseCode cmd::Clone::parse_args(ArgParser* parser) | ||
{ | ||
parser->addPositionalArgument("source_name", "The name of the source instance to be cloned", "<source_name>"); | ||
|
||
const QCommandLineOption destination_name_option{ | ||
{"n", "name"}, | ||
"Specify a custom name for the cloned instance. The name must follow the same validity rules as instance names " | ||
"(see \"help launch\"). Default: \"<source_name>-cloneN\", where N is the Nth cloned instance.", | ||
"destination-name"}; | ||
|
||
parser->addOption(destination_name_option); | ||
|
||
const auto status = parser->commandParse(this); | ||
if (status != ParseCode::Ok) | ||
{ | ||
return status; | ||
} | ||
|
||
const auto number_of_positional_arguments = parser->positionalArguments().count(); | ||
if (number_of_positional_arguments < 1) | ||
{ | ||
cerr << "Please provide the name of the source instance.\n"; | ||
return ParseCode::CommandLineError; | ||
} | ||
|
||
if (number_of_positional_arguments > 1) | ||
{ | ||
cerr << "Too many arguments.\n"; | ||
return ParseCode::CommandLineError; | ||
} | ||
|
||
const auto source_name = parser->positionalArguments()[0]; | ||
rpc_request.set_source_name(source_name.toStdString()); | ||
rpc_request.set_verbosity_level(parser->verbosityLevel()); | ||
if (parser->isSet(destination_name_option)) | ||
{ | ||
rpc_request.set_destination_name(parser->value(destination_name_option).toStdString()); | ||
} | ||
|
||
return ParseCode::Ok; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/* | ||
* Copyright (C) Canonical, Ltd. | ||
* | ||
* This program is free software; you can redistribute it and/or modify | ||
* it under the terms of the GNU General Public License as published by | ||
* the Free Software Foundation; version 3. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
* | ||
*/ | ||
|
||
#ifndef MULTIPASS_CLONE_H | ||
#define MULTIPASS_CLONE_H | ||
|
||
#include <multipass/cli/command.h> | ||
|
||
namespace multipass | ||
{ | ||
namespace cmd | ||
{ | ||
class Clone final : public Command | ||
{ | ||
public: | ||
using Command::Command; | ||
ReturnCode run(ArgParser* parser) override; | ||
|
||
std::string name() const override; | ||
QString short_help() const override; | ||
QString description() const override; | ||
|
||
private: | ||
ParseCode parse_args(ArgParser* parser); | ||
|
||
CloneRequest rpc_request; | ||
}; | ||
} // namespace cmd | ||
} // namespace multipass | ||
#endif // MULTIPASS_CLONE_H |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you create a string template that takes the vm name and clone name and formats them into an appropriate error string. Otherwise, there's no real point in having this class other than the class name...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we look at the function body of
generate_destination_instance_name_for_clone
, there are three error messages of invalid name and they all have different formats. First, I think it is informative to users to have these specific error messages. With this in place, it is hard to patternize the error message into an exception.The only role of this exception is to let the daemon::clone catch so he can set
grpc::StatusCode::INVALID_ARGUMENT
uniformly for these three cases. If we do not use an exception here thengenerate_destination_instance_name_for_clone
needs to return the error string, which will make the function signature more cumbersome.std::expected
from C++23 which is similar to Ruststd::result
will be a better choice here than an exception. With that, we can do an early return in theif(status.ok())
block ofdaemon::clone
likereturn status_promise->set_value(grpc::Status{grpc::INVALID_ARGUMENT, <error msg>});
, as a result, the branchcatch (const mp::CloneInvalidNameException& e)
can be removed. Butstd::expected
is not available yet in Multipass code base.Be free to share any other possible code design you might have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is necessary to have
generate_destination_instance_name_for_clone
code as a function in terms of modulization, expressiveness and not cluttering thedaemon::clone
further.With that said, throwing an exception is one way and returning the error message from the function is another way to propagate the error message. Returning the error message requires changing the return type from
std::string
to either a struct orstd::pair
.That is right.
@ricab in case you have any better idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception name is crucial. It is what allows you to catch and handle specific errors in a different way. The idea of a separate function throwing a specific exception sounds right to me. However...
I wonder if a common error prefix would be more informative. Something like
"Could not clone: {}"
? I don't know, this also depends on how things are printed on the client side. I guess we should look into what we do in other cases (i.e. failure to start, to stop, to snapshot, etc.) and try to stick with that.