Skip to content

Upgrade Firecracker to post-v0.20.0 #383

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

Merged
merged 5 commits into from
Feb 18, 2020
Merged

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Jan 24, 2020

Issue #, if available:

Fixes #253 finally!

Description of changes:

Upgrade Firecracker to v0.20.0

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

@kzys
Copy link
Contributor Author

kzys commented Jan 24, 2020

I'm going to upgrade Firecracker Go SDK to 0.20.0 (which will be released hopefully this week) in this PR.

@kzys kzys marked this pull request as ready for review January 24, 2020 17:35
@kzys
Copy link
Contributor Author

kzys commented Jan 24, 2020

I'm going to upgrade Go SDK to untagged master for making sure it doesn't break firecracker-containerd.

@kzys
Copy link
Contributor Author

kzys commented Jan 24, 2020

The "failed to create VM: failed to start the VM: invalid character 'v' looking for beginning of value" error seems due to Firecracker. Opening a pull request...

firecracker-microvm/firecracker#1547

@kzys kzys changed the title Upgrade Firecracker to v0.20.0 Upgrade Firecracker to post-v0.20.0 Jan 28, 2020
@kzys
Copy link
Contributor Author

kzys commented Jan 28, 2020

7f1aa72 is not really related to Firecracker upgrade, but better to fix anyways.

@kzys
Copy link
Contributor Author

kzys commented Jan 28, 2020

I'm not sure the root cause of "vCPUs resume failed". The code below is sending the error and .recv_timeout(Duration::from_millis(100)) looks a bit suspicious. Would it be always 100ms?

https://github.com/firecracker-microvm/firecracker/blob/53cf1bacadc23a21df361a894ac2887cafcb7139/src/vmm/src/lib.rs#L941

"exit status 148" is firecracker-microvm/firecracker#1456 and not a regression technically.

@kzys
Copy link
Contributor Author

kzys commented Jan 28, 2020

I'm going to move Firecracker fixes to upstream. In the meantime, we can say that the Go SDK is working correctly and we can release 0.20.0!

@kzys
Copy link
Contributor Author

kzys commented Jan 28, 2020

Firecracker patches are kept in https://github.com/kzys/firecracker-containerd/tree/upgrade-fc-with-fixes for reference and a successful BuildKite build with the patches is https://buildkite.com/firecracker-microvm/firecracker-containerd/builds/1477. So I'm going to remove them from this PR.

@kzys
Copy link
Contributor Author

kzys commented Feb 13, 2020

Right now 31e7627 is referencing an open pull request. I'd like to merge this pull request after the pull request is merged into master.

@kzys
Copy link
Contributor Author

kzys commented Feb 17, 2020

@mxpv @xibz @IRCody This branch is ready for review! It will fix #253 and #358 finally.

if err != nil {
return nil, vsockAckError{cause: err}
}
if !strings.HasPrefix(line, "OK ") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do the full string compare here?

Copy link
Contributor Author

@kzys kzys Feb 17, 2020

Choose a reason for hiding this comment

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

The line contains assigned_hostside_port (see https://github.com/firecracker-microvm/firecracker/blob/master/docs/vsock.md#examples) which we don't know. Also we are not using the number by ourselves.

We can check whether the following token is a number or not, but I'm unsure how much value we could get from doing so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I thought this was the port we were passing in. Can we add the vsock.md link in the comments stating we are following a custom Firecracker protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Updated b67f582

kzys added 2 commits February 17, 2020 16:39
v0.20.0 has some issues that block firecracker-containerd to use.

Signed-off-by: Kazuyoshi Kato <[email protected]>
- Rust 1.35 cannot parse Firecracker's Cargo.lock somehow
- While Rust's latest release is 1.40.0, we'd like to keep in sync
  with Firecracker upstream.

firecracker-microvm/firecracker#1419

Signed-off-by: Kazuyoshi Kato <[email protected]>
@kzys
Copy link
Contributor Author

kzys commented Feb 18, 2020

@xibz Can you take a look the AL build? Seems I cannot retry the job.

@kzys
Copy link
Contributor Author

kzys commented Feb 18, 2020

  1. According to @xibz, we cannot retry AL builds, but we can rebuild

  2. I manually run the following commands on the AL host.

sh-4.2# ./tools/thinpool.sh remove 181
Device /dev/mapper/fcci--vg-181-snap-* not found
Command failed
  Logical volume "181" successfully removed

We may need to check our post-build scripts.

Copy link
Contributor

@IRCody IRCody left a comment

Choose a reason for hiding this comment

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

LGTM.

kzys added 3 commits February 18, 2020 08:42
Since 0.20.0, Firecracker itself writes "OK <port>" as ACK.
So we don't have to write our own "IMALIVE" message anymore.

Signed-off-by: Kazuyoshi Kato <[email protected]>
Since Firecracker 0.20.0 can report the size of a special block
device correctly, the size of the block device from devmapper
snapshotter won't be zero.

Signed-off-by: Kazuyoshi Kato <[email protected]>
The submodule is used to install a specific version of Firecracker on
our Docker-based build. Amazon Linux 2 build should use that as well.

Signed-off-by: Kazuyoshi Kato <[email protected]>
Copy link
Contributor

@mxpv mxpv left a comment

Choose a reason for hiding this comment

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

LGTM

@kzys kzys merged commit 0dd32fc into firecracker-microvm:master Feb 18, 2020
fangn2 pushed a commit to fangn2/firecracker-containerd that referenced this pull request Mar 23, 2023
…ependabot/go_modules/github.com/mdlayher/vsock-1.1.0

Bump github.com/mdlayher/vsock from 1.0.0 to 1.1.0
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.

Command inside container hangs forever
4 participants