Snapshots #2993

ricab · 2023-03-28T18:20:09Z

Fixes #208

ricab · 2023-03-28T18:22:58Z

@sharder996 Opening this so that we get CI and packages to test, at least with the stuff we already merged.

I updated this branch to be on top of the daemon refactor (this needs to wait for that to be merged).

codecov · 2023-03-28T20:07:05Z

Codecov Report

Merging #2993 (2f856e5) into main (9740dfd) will decrease coverage by 4.46%.
The diff coverage is 57.06%.

@@            Coverage Diff             @@
##             main    #2993      +/-   ##
==========================================
- Coverage   88.48%   84.03%   -4.46%     
==========================================
  Files         240      250      +10     
  Lines       12171    13464    +1293     
==========================================
+ Hits        10770    11315     +545     
- Misses       1401     2149     +748

Files	Coverage Δ
include/multipass/cli/format_utils.h	`100.00% <100.00%> (ø)`
.../multipass/exceptions/file_open_failed_exception.h	`100.00% <100.00%> (ø)`
include/multipass/snapshot.h	`100.00% <100.00%> (ø)`
include/multipass/virtual_machine_factory.h	`100.00% <ø> (ø)`
include/multipass/vm_image_vault.h	`100.00% <ø> (ø)`
src/client/cli/client.cpp	`100.00% <100.00%> (ø)`
src/client/cli/cmd/alias.cpp	`100.00% <100.00%> (ø)`
src/client/cli/cmd/delete.h	`100.00% <ø> (ø)`
src/client/cli/cmd/exec.cpp	`100.00% <100.00%> (ø)`
src/client/cli/cmd/list.cpp	`97.50% <100.00%> (+0.35%)`	⬆️
... and 53 more

townsend2010

Hey @ricab and @sharder996!

This is one BIG PR! Holy cow!!! Considering the absolute size of this, this is looking good overall. Although I spent the better part of Friday and today reviewing this, it is not complete. I have not reviewed the following files:

The bash completions
info.cpp
All the formatters
base_snapshot.*
base_virtual_machine.*
And some of the test changes.

I'm waiting on reviewing info and the formatters since I'd like to review in conjunction with the latest snap package which hasn't built as of this writing.

Also, even though I've spent a number of hours reviewing this, I would call what I've done so far a fairly complete review, but not a fined toothed comb type of review 💦

include/multipass/cli/format_utils.h

include/multipass/exceptions/snapshot_exceptions.h

src/daemon/daemon.cpp

ricab · 2023-10-24T10:57:50Z

I am going to reply to items individually, but I acknowledge this must be very difficult to go through and I just want to thank you for the review to begin with 🌟 👏

ricab · 2023-10-24T19:45:28Z

@townsend2010, I think I have addressed all the items I said I would. Others I am waiting feedback on. Please let me know if I missed anything.

townsend2010 · 2023-10-24T20:44:01Z

src/client/cli/cmd/restore.cpp

+bool cmd::Restore::confirm_destruction(const std::string& instance_name)
+{
+    static constexpr auto prompt_text =
+        "Do you want to take a snapshot of {} before discarding its current state? (Yes/no)";
+    static constexpr auto invalid_input = "Please answer Yes/no";
+    mp::PlainPrompter prompter(term);
+
+    auto answer = prompter.prompt(fmt::format(prompt_text, instance_name));
+    while (!answer.empty() && !std::regex_match(answer, yes_answer) && !std::regex_match(answer, no_answer))
+        answer = prompter.prompt(invalid_input);
+
+    return std::regex_match(answer, no_answer);
+}


Nothing needs to be done at this moment, but this is quite similar to the code in launch for it's prompt around asking for permission to create the bridge. I know @luis4a0 had done some refactoring in #3195, so we'll just need to consider how we may be able to refactor some of this once this PR is merged.

townsend2010

We are getting really close 😅 Just a couple of inline things below.

Also, I'm still reviewing delete and restore, but wanted to go ahead and get this quick feedback out.

include/multipass/vm_specs.h

src/platform/backends/shared/base_snapshot.cpp

src/client/cli/cmd/snapshot.cpp

townsend2010

Hey @ricab and @sharder996!

I'm good with in it's current state and approving! Let's wait for feedback from @andrei-toterman before merging.

andrei-toterman

Just a few small remarks :)

src/platform/backends/shared/base_virtual_machine.h

andrei-toterman · 2023-10-27T09:42:52Z

src/platform/backends/shared/base_virtual_machine.cpp

+std::string trim(const std::string& s)
+{
+    return std::regex_replace(s, std::regex{R"(^\s+|\s+$)"}, "");
+}


I think this would be useful as part of utils. Also, maybe declaring the regex as static inside this function would be a little more efficient, as it wouldn't have to parse and compile it from scratch on every invocation.

Yup. Both improvements coming.

For this one I ended up falling into a bit of a rabbit hole. I saw that there was already a trim_end that was able to trim in place. I thought I'd better be consistent, reuse it, and avoid copying strings (which is nice on full file contents...). So I dropped the regex approach entirely. From there, it bugged me that I could not use the new utils with rvalues and went on to generalize those trimmers with templates, but I wanted to be careful to avoid any dangling refs or other pitfalls (I wouldn't mind another pair of attentive eyes on that code, just to be sure). The templates also allowed me to easily drop the std::function wrappers. Please let me know if you're good with it.

First, one little mistake here

template <typename Str, typename Filter> Str&& multipass::utils::trim(Str&& s, Filter&& filter) { return trim_begin(trim_end(std::forward<Str>(s), std::forward<Filter>(filter))); }

I think filter should be passed to both trim_begin and trim_end. Now it is passed only to trim_end and trim_begin will always use is_space.

Also, there is one minuscule detail about

inline constexpr auto is_space = static_cast<int (*)(int)>(std::isspace);

since the docs say that it's 'wrong' to use static_cast<int (*)(int)>(std::isspace) when using it on std::string and it should be instead used as [](unsigned char c){ return std::isspace(c); }. I'm not really sure if this matters much, but I'd like to hear your opinion as well.

Ahh, I knew I needed you to have a look. Fixes coming.

src/platform/backends/shared/base_snapshot.h

include/multipass/exceptions/file_not_found_exception.h

Store the (now immutable) ID string in a snapshot field and provide an accessor to it, rather than deriving it each time it is needed. Adapt client code accordingly.

Move throwing default implementation of `make_specific_snapshot` overloads to the base VM class.

Generalize trim utilities to operate on both lvalues and rvalues. Take the chance to drop the `std::function` wrapper for the filter predicate.

For better readability.

townsend2010 · 2023-10-30T18:36:05Z

Manual integration test is running at https://github.com/canonical/multipass/actions/runs/6697249710.

townsend2010

Ok, integration tests passed and I'm final approving this. Let's merge this ASAP!

🚀 🎉 🍾 🥳

townsend2010 · 2023-10-31T07:31:42Z

This will fail, but want to get the commit message...

bors merge

bors · 2023-10-31T07:31:48Z

👎 Rejected by label

townsend2010 · 2023-10-31T07:32:43Z

Try again...

bors merge

2993: Snapshots r=townsend2010 a=ricab Fixes #208 Co-authored-by: Ricardo Abreu <[email protected]> Co-authored-by: ScottH <[email protected]>

bors · 2023-10-31T12:09:45Z

This PR was included in a batch that successfully built, but then failed to merge into main (it was a non-fast-forward update). It will be automatically retried.

ricab · 2023-10-31T12:15:45Z

bors cancel

bors · 2023-10-31T12:15:47Z

Canceled.

2993: Snapshots r=townsend2010 a=ricab Fixes #208 Co-authored-by: Ricardo Abreu <[email protected]> Co-authored-by: ScottH <[email protected]>

ricab added the no-merge label Mar 28, 2023

bors bot changed the base branch from daemon-refactor-instance-selection to main May 9, 2023 19:37

ricab force-pushed the snapshots branch 2 times, most recently from 1cf8855 to 8e55166 Compare May 30, 2023 13:19

ricab mentioned this pull request Jun 14, 2023

support for snapshots/backups #208

Closed

ricab force-pushed the snapshots branch 2 times, most recently from a30ab9b to 9e749f9 Compare June 28, 2023 18:31

ricab force-pushed the snapshots branch from 9e749f9 to 8c68cc7 Compare July 10, 2023 18:31

ricab force-pushed the snapshots branch from 42d592b to 38f90ee Compare July 26, 2023 18:19

ricab force-pushed the snapshots branch from f2ad7a6 to 7546c56 Compare August 29, 2023 18:02

townsend2010 linked an issue Oct 6, 2023 that may be closed by this pull request

support for snapshots/backups #208

Closed

ricab force-pushed the snapshots branch from e7c71cd to 25869b8 Compare October 16, 2023 11:53

ricab marked this pull request as ready for review October 17, 2023 17:46

ricab marked this pull request as draft October 17, 2023 18:03

townsend2010 suggested changes Oct 23, 2023

View reviewed changes

townsend2010 reviewed Oct 24, 2023

View reviewed changes

ricab force-pushed the snapshots branch 2 times, most recently from ef4a85e to 8b3c7a6 Compare October 25, 2023 18:20

townsend2010 reviewed Oct 25, 2023

View reviewed changes

include/multipass/vm_specs.h Show resolved Hide resolved

src/platform/backends/shared/base_snapshot.cpp Outdated Show resolved Hide resolved

sharder996 reviewed Oct 26, 2023

View reviewed changes

src/client/cli/cmd/snapshot.cpp Outdated Show resolved Hide resolved

townsend2010 previously approved these changes Oct 26, 2023

View reviewed changes

townsend2010 requested a review from andrei-toterman October 26, 2023 20:39

andrei-toterman reviewed Oct 27, 2023

View reviewed changes

ricab dismissed townsend2010’s stale review via b26af05 October 27, 2023 15:50

ricab force-pushed the snapshots branch from 7852899 to 93bbdeb Compare October 27, 2023 23:01

ricab marked this pull request as ready for review October 27, 2023 23:07

ricab added 10 commits October 30, 2023 14:49

[snapshot] Avoid deriving the ID string each time

acf850a

Store the (now immutable) ID string in a snapshot field and provide an accessor to it, rather than deriving it each time it is needed. Adapt client code accordingly.

[exceptions] Rename file to match type

1bdc9c1

[vm] Move default implementation to base VM

a917147

Move throwing default implementation of `make_specific_snapshot` overloads to the base VM class.

[utils] Implement remaining string trimmers

e24e524

[vm] Use generic trim util on file contents

a0f1612

[utils] Generalize trimmers

bc29248

Generalize trim utilities to operate on both lvalues and rvalues. Take the chance to drop the `std::function` wrapper for the filter predicate.

[utils] Avoid repeating isspace lambda

5eb6460

[utils] Separate template declaration/definition

d40e8e6

For better readability.

[utils] Bind std::isspace safely

bc394d8

[utils] Fix ignored custom filter in trim

6baa7a4

ricab dismissed andrei-toterman’s stale review via 6baa7a4 October 30, 2023 13:03

ricab force-pushed the snapshots branch from d7703bf to 6baa7a4 Compare October 30, 2023 13:03

[utils] Fix formatting

2f856e5

townsend2010 approved these changes Oct 31, 2023

View reviewed changes

townsend2010 removed the no-merge label Oct 31, 2023

bors bot added a commit that referenced this pull request Oct 31, 2023

Merge #2993

6186bc1

2993: Snapshots r=townsend2010 a=ricab Fixes #208 Co-authored-by: Ricardo Abreu <[email protected]> Co-authored-by: ScottH <[email protected]>

townsend2010 closed this Oct 31, 2023

townsend2010 reopened this Oct 31, 2023

townsend2010 merged commit 7e4dc40 into main Oct 31, 2023
28 of 30 checks passed

bors bot deleted the snapshots branch October 31, 2023 07:34

luis4a0 pushed a commit that referenced this pull request Nov 7, 2023

Merge #2993

6c84e07

2993: Snapshots r=townsend2010 a=ricab Fixes #208 Co-authored-by: Ricardo Abreu <[email protected]> Co-authored-by: ScottH <[email protected]>

luis4a0 pushed a commit that referenced this pull request Nov 7, 2023

Merge #2993

2e79ee1

2993: Snapshots r=townsend2010 a=ricab Fixes #208 Co-authored-by: Ricardo Abreu <[email protected]> Co-authored-by: ScottH <[email protected]>

luis4a0 pushed a commit that referenced this pull request Nov 8, 2023

Merge #2993

faa8a97

2993: Snapshots r=townsend2010 a=ricab Fixes #208 Co-authored-by: Ricardo Abreu <[email protected]> Co-authored-by: ScottH <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snapshots #2993

Snapshots #2993

ricab commented Mar 28, 2023 •

edited

Loading

ricab commented Mar 28, 2023

codecov bot commented Mar 28, 2023 •

edited

Loading

townsend2010 left a comment

ricab commented Oct 24, 2023

ricab commented Oct 24, 2023

townsend2010 Oct 24, 2023

townsend2010 left a comment

townsend2010 left a comment

andrei-toterman left a comment

andrei-toterman Oct 27, 2023

ricab Oct 27, 2023

ricab Oct 27, 2023

andrei-toterman Oct 30, 2023

ricab Oct 30, 2023

townsend2010 commented Oct 30, 2023

townsend2010 left a comment

townsend2010 commented Oct 31, 2023

bors bot commented Oct 31, 2023

townsend2010 commented Oct 31, 2023

bors bot commented Oct 31, 2023

ricab commented Oct 31, 2023

bors bot commented Oct 31, 2023

Snapshots #2993

Snapshots #2993

Conversation

ricab commented Mar 28, 2023 • edited Loading

ricab commented Mar 28, 2023

codecov bot commented Mar 28, 2023 • edited Loading

Codecov Report

townsend2010 left a comment

Choose a reason for hiding this comment

ricab commented Oct 24, 2023

ricab commented Oct 24, 2023

townsend2010 Oct 24, 2023

Choose a reason for hiding this comment

townsend2010 left a comment

Choose a reason for hiding this comment

townsend2010 left a comment

Choose a reason for hiding this comment

andrei-toterman left a comment

Choose a reason for hiding this comment

andrei-toterman Oct 27, 2023

Choose a reason for hiding this comment

ricab Oct 27, 2023

Choose a reason for hiding this comment

ricab Oct 27, 2023

Choose a reason for hiding this comment

andrei-toterman Oct 30, 2023

Choose a reason for hiding this comment

ricab Oct 30, 2023

Choose a reason for hiding this comment

townsend2010 commented Oct 30, 2023

townsend2010 left a comment

Choose a reason for hiding this comment

townsend2010 commented Oct 31, 2023

bors bot commented Oct 31, 2023

townsend2010 commented Oct 31, 2023

bors bot commented Oct 31, 2023

ricab commented Oct 31, 2023

bors bot commented Oct 31, 2023

ricab commented Mar 28, 2023 •

edited

Loading

codecov bot commented Mar 28, 2023 •

edited

Loading