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

Use Config.VMID as Firecracker's instance ID #253

Merged
merged 1 commit into from
Jul 27, 2020
Merged

Use Config.VMID as Firecracker's instance ID #253

merged 1 commit into from
Jul 27, 2020

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Jul 24, 2020

Firecracker is internally has an instance ID, but the SDK didn't have
the way to configure the ID. This change connects Config.VMID to the
instance ID.

Issue #, if available:

NA

Description of changes:

  • Connects Config.VMID to Firecracker's Instance ID.
  • Alternatively we can add Config.InstanceID but I think that would be too confusing.
  • Renaming VMID to InstanceID is another option and has less impedance mismatch, but I don't want to break existing customers, including firecracker-containerd.

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

@kzys kzys force-pushed the add-id branch 2 times, most recently from 2fd202c to d73940e Compare July 24, 2020 23:23
xibz
xibz reviewed Jul 25, 2020
View reviewed changes
machine_test.go
@@ -420,13 +420,17 @@ func TestStartVMM(t *testing.T) {
}

func TestLogAndMetrics(t *testing.T) {
const logLevel = "DEBUG"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be Debug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The input log level became case-insensitive (firecracker-microvm/firecracker@75587b8) recently and the output is always uppercase (https://github.com/firecracker-microvm/firecracker/blob/db84b948461bddcc89c0b69aa5639368a7d7c3ef/src/logger/src/logger.rs#L82).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, cool!

@kzys
Copy link
Contributor Author

kzys commented Jul 25, 2020 •
edited

As always, this is more complicated than I thought.

  1. Firecracker's Instance ID doesn't have to be unique because it is only used inside a micro VM.
  2. Firecracker Jailer's ID has to be unique, but the jailer passes the ID as its micro VM's instance ID.
  3. The SDK's VMID has to be unique.

Using SDK's VMID as a micro VM's instance ID limits what Firecracker can technically do (using the same Instance ID for multiple VMs), while the limitation is enforced by Firecracker's Jailer as well though.

So I think the limitation above is fine. What do you folks think?

@kzys
Use Config.VMID as Firecracker's instance ID
01ba3cd 
Firecracker is internally has an instance ID, but the SDK didn't have
the way to configure the ID. This change connects Config.VMID to the
instance ID.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
xibz
xibz approved these changes Jul 27, 2020
View reviewed changes
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.

LGTM

@kzys kzys merged commit d93b040 into firecracker-microvm:master Jul 27, 2020
@kzys kzys mentioned this pull request Jul 28, 2020
Merged
dreadl0ck pushed a commit to dreadl0ck/firecracker-go-sdk that referenced this pull request Aug 15, 2020
@kzys
Merge pull request firecracker-microvm#253 from kzys/add-id
6508c2d 
Use Config.VMID as Firecracker's instance ID
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
xibz pushed a commit to xibz/firecracker-go-sdk that referenced this pull request Oct 9, 2020
Release v0.22.0
9de3342 
* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263)
* Fix Benchmark Goroutine (firecracker-microvm#259)
* Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255)
* Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253)
* Fixed error that was not being test against in `TestWait` (firecracker-microvm#251)
* Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230)
* Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225)
* Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218)

Signed-off-by: Debian <admin@ip-172-31-27-214.us-east-2.compute.internal>
@xibz xibz mentioned this pull request Oct 9, 2020
Merged
xibz pushed a commit to xibz/firecracker-go-sdk that referenced this pull request Oct 20, 2020
Release v0.22.0
981b72c 
* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263)
* Fix Benchmark Goroutine (firecracker-microvm#259)
* Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255)
* Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253)
* Fixed error that was not being test against in `TestWait` (firecracker-microvm#251)
* Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230)
* Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225)
* Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218)

Signed-off-by: Debian <admin@ip-172-31-27-214.us-east-2.compute.internal>
xibz pushed a commit to xibz/firecracker-go-sdk that referenced this pull request Oct 23, 2020
Release v0.22.0
c959b3c 
* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263)
* Fix Benchmark Goroutine (firecracker-microvm#259)
* Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255)
* Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253)
* Fixed error that was not being test against in `TestWait` (firecracker-microvm#251)
* Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230)
* Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225)
* Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218)

Signed-off-by: Debian <admin@ip-172-31-27-214.us-east-2.compute.internal>
xibz pushed a commit to xibz/firecracker-go-sdk that referenced this pull request Oct 23, 2020
@xibz
Release v0.22.0
f800b77 
* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263)
* Fix Benchmark Goroutine (firecracker-microvm#259)
* Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255)
* Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253)
* Fixed error that was not being test against in `TestWait` (firecracker-microvm#251)
* Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230)
* Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225)
* Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218)

Signed-off-by: Debian <admin@ip-172-31-27-214.us-east-2.compute.internal>
Signed-off-by: xibz <impactbchang@gmail.com>
xibz pushed a commit to xibz/firecracker-go-sdk that referenced this pull request Oct 23, 2020
@xibz
Release v0.22.0
2206275 
* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263)
* Fix Benchmark Goroutine (firecracker-microvm#259)
* Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255)
* Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253)
* Fixed error that was not being test against in `TestWait` (firecracker-microvm#251)
* Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230)
* Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225)
* Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218)

Signed-off-by: xibz <impactbchang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xibz xibz xibz 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.

None yet
2 participants
@kzys @xibz