Firecracker v1.4.1 vhive integration #43
Conversation
runtime/service.go
Outdated
| return errors.New("failed to load snapshot: snapshot path or container snapshot path was not provided") | ||
| } | ||
| if request.MemFilePath == "" && request.MemBackend.BackendType != "Uffd" { | ||
| return errors.New("failed to load snapshot: memroy file path was not provided") |
There was a problem hiding this comment.
typo
| return errors.New("failed to load snapshot: memroy file path was not provided") | |
| return errors.New("failed to load snapshot: memory file path was not provided") |
|
you need to squash the commits and sign them as described here: https://github.com/vhive-serverless/vHive/blob/main/docs/contributing_to_vhive.md |
There was a problem hiding this comment.
It may be a good idea to just drop the request.MemFilePath field in favor of the request.MemBackend field (our hands are not tied with backward compatibility support).
Please make sure the diff does not contain unnecessary changes (like blank lines).
Please make sure this patchset works by testing it manually with a simple setup.
I would prefer squashing the commits into one and supplying it with a small commit message describing the changes (basically, what you wrote in the PR description).
If there is no urgency in merging this PR, it would be great to see a PoC of how this patchset will be integrated with vHive (you can temporarily point to your fork's branch).
proto/types.proto
Outdated
| message MemoryBackend { | ||
| string BackendType = 1; // Either File or Uffd | ||
| string BackendPath = 2; // Either path to the file that contains the guest memory to be loaded or path to the UDS where a process is listening for a UFFD initialization | ||
| } |
There was a problem hiding this comment.
Nit: please restore the blank line at the end of the file.
proto/firecracker.proto
Outdated
| // Path to the disk device backing the container snapshot. | ||
| string ContainerSnapshotPath = 19; | ||
|
|
There was a problem hiding this comment.
Nit: please drop the blank lines here and above from the diff.
proto/types.proto
Outdated
|
|
||
| message MemoryBackend { | ||
| string BackendType = 1; // Either File or Uffd | ||
| string BackendPath = 2; // Either path to the file that contains the guest memory to be loaded or path to the UDS where a process is listening for a UFFD initialization |
There was a problem hiding this comment.
| string BackendPath = 2; // Either path to the file that contains the guest memory to be loaded or path to the UDS where a process is listening for a UFFD initialization | |
| string BackendPath = 2; // Either path to the file that contains the guest memory to be loaded or path to the UDS where a process is listening for a UFFD initialization control payload and open file descriptor that it can use to serve this process's guest memory page faults |
|
lgtm, please squash the commits into one and sign it as described in our developer guide |
Signed-off-by: char-1ee <[email protected]>
Issue #, if available:
Refer to vhive-serverless/vHive#807
Description of changes:
To support UPF of vanilla firecracker snapshot and keep consistent with firecracker Golang SDK design, add
MemBackendas new parameter in CreateVM request.MemBackendcontainsBackendTypeandBackendPath, whenBackendTypeis "Uffd",MemFilePathis ignored. For details of memory backend, refer to firecracker-microvm/firecracker-go-sdk@2e09014By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.