Opinionated API Changes / Rational Defaults

[jmickey]

Opinionated API Changes

• Status: proposed
• Date: 2021-11-10
• Authors: @jmickey
• Deciders: @jmickey @richardcase @Callisto13 @yitsushi

Context

Flintlock requires users to specify all aspects of the MicroVM spec. If something (e.g. metadata network interface) is not included in the MicroVM spec, then it won't be created. This proposal details some possible opinionated defaults for MicroVM specifications in cases where an explicit configuration is not provided by the user.

Scope

The scope of this proposal is not to decide on what aspects of MicroVM specifications are immutable / non-configurable. Rather it is to explore which potential aspects of the MicroVM specification can be provided by default, but also overridden/customised by the user if they so wish to.

Default Configurations

Below are some proposed default configurations, along with default values where relevant.

Name	Description	Model	Value
MicroVM Resources	Provide default values for MicroVM VCPUs and RAM. This can be difficult to get right, but is even supported in Firecracker itself. However, Firecracker defaults to 1 VCPU and 128MB of RAM, which is probably a bit small for our particular use case.	MicroVM.VCPU MicroVM.MemoryInMb	vcpu: 2 memory_in_mb: 1024
Default Namespace	If a namespace is not provided than default to using the default namespace	MicroVM.Namespace	default
Metadata network interface	Automatically generate metadata network interface with address of 169.254.0.1/16 and allow_metadata_req: true	MicroVM.NetworkInterfaces	169.254.0.1/16
Remove allow metadata requests	If the metadata network interface is created automatically then we should remove the allow_metadata_req option on the network interface model	MicroVM.NetworkInterfaces
Network interface name	Automatically generate the network interface name when one is not provided.	MicroVM.NetworkInterfaces[*].GuestDeviceName
RootVolume and AdditionalVolumes[]	Separate the Volumes field in the model into two fields. RootVolume specifically for the root volume, allowing us to hide the is_root flag, and AdditionalVolumes[] for non-root volumes.	MicroVM.Volumes	MicroVM.RootVolume and MicroVM.AdditionalVolumes[]

Unknowns/Discussion

• Can we generate volume names?
• If a volume is marked as is_root: true, can we automatically mount it at / if mount_point is not specified?

Request Example

With the defaults as described in Default Configurations, a minimal MicroVM create request would look as follows:

{
  "microvm": {
    "id": "mvm1",
    "kernel": {
      "image": "docker.io/richardcase/ubuntu-bionic-kernel:0.0.11",
      "filename": "vmlinux"
    },
    "initrd": {
      "image": "docker.io/richardcase/ubuntu-bionic-kernel:0.0.11",
      "filename": "initrd-generic"
    },
    "interfaces": [
      {
        "type": 0 // macvtap
      }
    ],
  }
}

Decision

Implement the following changes to the Flintlock API:

Name	Description	Model	Value
MicroVM Resources	Provide default values for MicroVM VCPUs and RAM. This can be difficult to get right, but is even supported in Firecracker itself. However, Firecracker defaults to 1 VCPU and 128MB of RAM, which is probably a bit small for our particular use case.	MicroVM.VCPU MicroVM.MemoryInMb	vcpu: 2 memory_in_mb: 1024
Default Namespace	If a namespace is not provided than default to using the default namespace	MicroVM.Namespace	default
Metadata network interface	Automatically generate metadata network interface with address of 169.254.0.1/16 and allow_metadata_req: true	MicroVM.NetworkInterfaces	169.254.0.1/16
Remove allow metadata requests	If the metadata network interface is created automatically then we should remove the allow_metadata_req option on the network interface model	MicroVM.NetworkInterfaces
Network interface name	Automatically generate the network interface name when one is not provided.	MicroVM.NetworkInterfaces[*].GuestDeviceName
RootVolume and AdditionalVolumes[]	Separate the Volumes field in the model into two fields. RootVolume specifically for the root volume, allowing us to hide the is_root flag, and AdditionalVolumes[] for non-root volumes.	MicroVM.Volumes	MicroVM.RootVolume and MicroVM.AdditionalVolumes[]

Note: This proposal specifically avoids diving too deep into implementation details. E.g. Automatically generating a network device name will require us to validate that there are no conflicts in the name we use. These implementation details belong within the scope of the implementation phase and PR discussion.

Consequences

[yitsushi]

memory_in_mb: 512

Our main target is to manage kubernetes clusters, 512m seems a bit small for that as a default value.

interfaces.allow_metadata_req: false

Do we want to allow users to set this flag on any devices? If we create a device that can access the metadata service, couldn't we just say "nah, don't worry, you can access it" and remove this field from from the request.

interfaces.guest_device_name: eth0

A note before we forget it, we have to validate if none of the provided network interfaces have the same name, applies to the auto-generated metadata enabled device as well, so we have to set that device name as a reserved name.

minimal MicroVM

• In theory, initrd is not a required field if root volume is specified and the root volume has init.
• For metadata, it's an optional value, in a minimum configuration it can be omitted.

interfaces.type: 0

Maybe it's out of scope, but I think we should let the user set string based enum values like "tap" or "macvtap" instead of a non-descriptive int value.

[richardcase]

we should let the user set string based enum values like "tap" or "macvtap" instead of a non-descriptive int value

Yes completely agree with this. In the webinar this became apparent as i had to mention that 0/1 actuals means "tap"/"macvtap"

[jmickey]

Our main target is to manage kubernetes clusters, 512m seems a bit small for that as a default value.

Yeah that is fair. I kinda picked 512mb out of a hat with the hope that we would discuss it a bit further. I don't think the default value should be considered reasonable for a production system, rather something that is enough for the purposes of getting started quickly in a local or test environment. Maybe an increase to 1GB?

interfaces.allow_metadata_req: false. Do we want to allow users to set this flag on any devices?

I would be supportive of that.

so we have to set that device name as a reserved name.

Definitely think we should come up with a reserved name for the metadata device. I imagine in most use cases users would be fine with auto-generated networks device names as well.

[Callisto13]

I don't think the default value should be considered reasonable for a production system, rather something that is enough for the purposes of getting started quickly in a local or test environment.

yes i agree

[Callisto13]

Maybe it's out of scope, but I think we should let the user set string based enum values like "tap" or "macvtap" instead of a non-descriptive int value.

I think this sounds really sensible, but yeh out of scope I feel.

Are we going to have a separate ADR to discuss opinionated things which a user cannot alter?

[jmickey]

Are we going to have a separate ADR to discuss opinionated things which a user cannot alter?

I don't see why not!

[richardcase]

I propose that we also change how we declare volumes and separate out the root volume so you end up with

• RootVolume Volume
• AdditionalVolumes []Volume

I like this clean separation and it allows us to hide is_root (or any other microvm provider specific options)

[jmickey]

I would be supportive of this! It would also make it clearer when you're using a Initrd and RootVolume at the same time (which should be illegal according to Firecracker).

[jmickey]

Do you think this change would be within the scope of this ADR?

[richardcase]

I think it would be as its a change in how its represented in the Firecracker API, we are abstratcting away the is_root from the user.

[yitsushi]

Updated:

- Date: 2021-11-010
+ Date: 2021-11-10

[Callisto13]

Would it be reasonable to provide defaults for MicroVM IDs and Namespaces? e.g. Automatically generate the ID (mvm#) and default the namespace to default.

for this I feel like defaulting the namespace to default may make sense, but i am not sure about the name. i feel that is a fine thing to always required people to set

[jmickey]

@weaveworks/quick-silver Updated ADR. There are still two unknowns that would be good to discuss. I've drafted the proposed final decision, but it's still open to discussion!

[Callisto13]

"Network interface name" is in the table twice

If a volume is marked as is_root: true, can we automatically mount it at / if mount_point is not specified?

The (admittedly small) risk would be if they specified 2 volumes and set one non-root to be mounted at / (for whatever reason) and the root one with no explicit mount point. That is a special level of 'duh' but not impossible. I don't think it hurts people (the CAPI provider API 😄 ) to make them include that field.

Can we generate volume names?

By volume name do you mean the volume id field?

[jmickey]

"Network interface name" is in the table twice

Fixed, thank you!

I don't think it hurts people (the CAPI provider API 😄 ) to make them include that field.

You bring up a great point and I am inclined to agree.

By volume name do you mean the volume id field?

Yes

[Callisto13]

Can we generate volume names?

Yeh I think so, though I am not sure who would write a spec without an id because that feels weird. We would only have to worry about conflicting names within the scope of the mvm itself, so that is less effort, and we will end up recording the eventual name/id in the status/spec somewhere

[jmickey]

• If a volume is marked as is_root: true, can we automatically mount it at / if mount_point is not specified?

@Callisto13 brings up a good point regarding possible mount_point conflicts, so I don't think defaulting this is a good idea.

• Can we generate volume names?

Rethinking this as I am not sure it makes much sense, hard to imagine cases where a user wouldn't provide an ID for volumes.

Unless there are any other concerns then I would like to propose we accept this decision. Need two +1's (not including myself) to accept and merge into the repo. Please reply to this comment with +1 if you're happy with it.

[yitsushi]

Filed an issue for the collision: #258
+1 from me (:heart: reaction)

[richardcase]

I think we should include the RootVolume / AdditionalVolumes change as well.

[jmickey]

Haha literally just added it! Great minds etc...

[richardcase]

+1 from me as well

[jmickey]

This ADR has received two approvals, and has been merged in #262. Locking conversation!