Skip to content

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

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
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

@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
}

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
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
Author

kzys commented Jul 25, 2020 •
edited
Loading

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?

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 <[email protected]>
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
Merge pull request firecracker-microvm#253 from kzys/add-id
6508c2d 
Use Config.VMID as Firecracker's instance ID
Signed-off-by: Kazuyoshi Kato <[email protected]>
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 <[email protected]>
@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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: xibz <[email protected]>
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@xibz xibz xibz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kzys @xibz