Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Complain on snapshot operations if unsupported #3329

Merged
merged 15 commits into from
Dec 14, 2023
Merged

Complain on snapshot operations if unsupported #3329

merged 15 commits into from
Dec 14, 2023

Conversation

ricab
Copy link
Collaborator

@ricab ricab commented Dec 12, 2023

And omit the number of snapshots in info.

Fixes #3328

ricab added 6 commits December 12, 2023 20:50
@ricab
[vm] Throw on snapshot operations when unsupported
2e13e40 
(Warning: breaks the daemon.)
@ricab
[vm] Verify supported snapshots before asserting
bed6947 
(Warning: daemon still broken.)
@ricab
[qemu] Implement check_snapshots_supported()
0b78255 
(Warning: daemon still broken.)
@ricab
[daemon] Verify snapshot first on restore
906c411 
(Warning: daemon still broken.)
@ricab
[daemon] Omit number of snapshots if unsupported
aa5d1a7 
Report info on the number of snapshots of instances only where
supported.

(Fixes daemon.)
@ricab
[cli] Omit number of snapshots if unsupported
f1b5956
@codecov Codecov
Copy link

codecov bot commented Dec 13, 2023 •
edited
Loading

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (c4e27da) 83.88% compared to head (e8a718e) 83.92%.
Report is 32 commits behind head on main.

Files Patch % Lines
.../platform/backends/shared/base_virtual_machine.cpp 0.00% 7 Missing ⚠️
src/daemon/daemon.cpp 66.66% 6 Missing ⚠️
...rc/platform/backends/shared/base_virtual_machine.h 0.00% 5 Missing ⚠️
src/platform/backends/qemu/qemu_virtual_machine.h 0.00% 1 Missing ⚠️
...tform/backends/qemu/qemu_virtual_machine_factory.h 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3329      +/-   ##
==========================================
+ Coverage   83.88%   83.92%   +0.03%     
==========================================
  Files         250      251       +1     
  Lines       13554    13678     +124     
==========================================
+ Hits        11370    11479     +109     
- Misses       2184     2199      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ricab ricab marked this pull request as ready for review December 13, 2023 18:37
@townsend2010 townsend2010 self-requested a review December 13, 2023 18:52
townsend2010
townsend2010 suggested changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ricab!

Thanks for this! Works well and the code looks good. I just have a couple of consistency comments below.

src/platform/backends/shared/base_virtual_machine.h Outdated Show resolved Hide resolved
src/platform/backends/shared/base_virtual_machine_factory.h Outdated Show resolved Hide resolved
townsend2010
townsend2010 previously approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, LGTM, thanks!

@ricab ricab marked this pull request as draft December 14, 2023 11:20
@ricab
Copy link
Collaborator Author

ricab commented Dec 14, 2023

I found an issue...

@ricab
Copy link
Collaborator Author

ricab commented Dec 14, 2023

Fixed. The issue was in snapshot settings, for example:

$ multipass get --keys
get failed: The snapshots feature is not implemented on this backend.

@ricab ricab marked this pull request as ready for review December 14, 2023 15:24
townsend2010
townsend2010 approved these changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, LGTM! Thanks!

@townsend2010 townsend2010 merged commit 4b38e63 into main Dec 14, 2023
21 of 22 checks passed
@townsend2010 townsend2010 deleted the clarify-unsupported-snapshots branch December 14, 2023 17:46
townsend2010 pushed a commit that referenced this pull request Dec 14, 2023
Merge #3329
ef361f9 
Complain on snapshot operations if unsupported

a=ricab r=townsend2010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@townsend2010 townsend2010 townsend2010 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Snapshot operations do not fail when snapshots are unsupported
2 participants
@ricab @townsend2010