Skip to content

Conversation

@norwoodj
Copy link

πŸ“‘ Description

Implements vsock for firecracker VMs spawned by firepilot by adding the configuration and API call to the firecracker server.

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code is documented
  • I provided unitary tests or procedure to test my code
  • My PR have a clear description of what my code is supposed to do

@norwoodj norwoodj requested a review from alexandrebrg as a code owner August 26, 2025 17:46
@norwoodj
Copy link
Author

Hey @alexandrebrg, is this repository maintained? I have this and a couple of other branches that I'd like to get merged in here. For now we're maintaining an internal fork, but I'd like to use the public version if we can get these changes in.

@alexandrebrg
Copy link
Member

Hi @norwoodj, my apologies for the delay.
Not actively maintained, however, now that I know you've got work on this project, I'd be happy to review your PRs.

Thank you for the contribution :)

Copy link
Member

@alexandrebrg alexandrebrg left a comment

Choose a reason for hiding this comment

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

Small changes otherwise look good to me πŸ‘

Comment on lines +56 to +60
VsockBuilder::new()
.with_guest_cid(3)
.with_uds_path("/tmp/fc.sock".to_string())
.try_build()
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Rather than having a panic with no context (if it fails), please add the expected behaviour in a expect

Suggested change
VsockBuilder::new()
.with_guest_cid(3)
.with_uds_path("/tmp/fc.sock".to_string())
.try_build()
.unwrap();
VsockBuilder::new()
.with_guest_cid(3)
.with_uds_path("/tmp/fc.sock".to_string())
.try_build()
.expect("Minimal build should be valid")

#[test]
#[should_panic]
fn partial_kernel() {
VsockBuilder::new().with_guest_cid(3).try_build().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Comment on lines +105 to 111
impl Default for Executor {
fn default() -> Self {
Self::new()
}
}

impl Executor {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
impl Default for Executor {
fn default() -> Self {
Self::new()
}
}
impl Executor {
#[derive(Default)]
impl Executor {

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.

2 participants