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.

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

Implement PauseVM, ResumeVM, and CreateSnapshot #278

Merged
merged 4 commits into from
Nov 11, 2020

Conversation

plamenmpetrov
Copy link
Contributor

Hello, Firecracker 0.23.0 adds new API calls for managing snapshots of a microVM and changing the microVM state. The goal of this PR is to port the PauseVM, ResumeVM, and CreateSnapshot calls to the SDK.

Issue #, if available:
#239

Description of changes

  • Added relevant PauseVM, ResumeVM, and CreateSnapshot definitions to client/swagger.yaml.
  • Generated the updated API client.
  • Added wrappers for new API calls in firecracker.go.
  • Added new API calls to machine.go

We have not added tests to the PR yet. We are open to feedback on what tests we should add and any other comments in general. We would be glad to finalize and contribute this code.

Authored by @plamenmpetrov and @ustiugov

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -398,6 +398,56 @@ paths:
description: Internal server error
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left out the parts relevant to snapshot load and tracking dirty pages. I was not going to include snapshot load support yet as per our discussion so I decided to leave the relevant API definitions altogether. That being said, copying as-is would not interfere in any way. It is perhaps best to copy as-is so that the swagger file does not need to be updated for version 0.23.0 of the SDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. It is hard to track what we've copied and what we've left out. Tracking tasks can be done in GitHub issues or somewhere else, rather than this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have copied over the swagger file from firecracker v0.23.0 and updated the API.

Copy link
Contributor

@xibz xibz left a comment

Choose a reason for hiding this comment

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

Thank you @plamenmpetrov for submitting this change. The changes look good with some minor nit comments. Also, these changes need integration tests. Once those are added, this can be merged.

machine.go Outdated
return nil
}

// CreateSnapshot Creates a snapshot of the VM
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: CreateSnapshot Creates a snapshot of the VM -> CreateSnapshot creates a snapshot of the VM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

machine.go Outdated
return nil
}

// ResumeVM Resumes the VM
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Resumes -> resumes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

machine.go Outdated
@@ -1002,3 +1002,50 @@ func (m *Machine) setupSignals() {
close(sigchan)
}()
}

// PauseVM Pauses the VM
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Pauses -> pauses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@plamenmpetrov
Copy link
Contributor Author

Thank you @plamenmpetrov for submitting this change. The changes look good with some minor nit comments. Also, these changes need integration tests. Once those are added, this can be merged.

Hello @xibz , I have added preliminary tests for pause, resume and create snapshot, which seem to pass in the build. I also updated the firecracker version used in tests to 0.23.0. However, TestNetworkMachineCNIWithConfFile times out. In my understanding, this should not be related to pause, resume, or create snapshot. Perhaps it is caused by another change in firecracker 0.23.0?

Copy link
Contributor

@xibz xibz left a comment

Choose a reason for hiding this comment

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

Awesome, this looks great! Thank you again for this contribution and updating this to have tests. I reran the tests and they passed. Something upstream must've been broken at the time it was ran

@xibz xibz merged commit 82818b2 into firecracker-microvm:master Nov 11, 2020
@austinvazquez austinvazquez mentioned this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants