Allow QEMU scriptlets to read VM structs and edit QEMU config

[bensmrs]

raw.qemu* configuration keys override the ones defined in previous profiles. Sometimes, I’d like to write a profile that defines several keys in raw.qemu.conf, adds some things in raw.qemu and does additional processing in raw.qemu.qmp.scriptlet.
And sometimes I’d also like to further modify these QEMU options in my VM settings, without having to repeat everything the profile says.
Could we imagine a mechanism allowing, rather than replacing existing data, appending to them for the raw.qemu, raw.qemu.conf and raw.qemu.qmp.{early,{post,pre}-start} keys? (For raw.qemu.qmp.scriptlet, it gets trickier and I’m not sure what to propose…)

[stgraber]

We've so far stayed away from trying to do anything like this, despite it being equally useful for things like cloud-init. Though in the cloud-init case you at least do have the benefit of having both cloud-init.vendor-data and cloud-init.user-data with cloud-init itself merging the two together.

We obviously don't want to change the general behavior of config key overriding through profiles and local config, even just for a few select keys as we really want consistency there.

What we could do in theory is provide something like raw.qemu.conf.NAME and then have those be processed in alphabetical order with suitable merging/overriding. But we've also stayed away from doing that for cloud-init in the past as it adds quite a bit of complexity, makes it a fair bit harder for a user to get a good overview of their configuration and could be a bit of a slippery slope where the moment we do it for a few keys, we'll have request to do that for basically every single key in the API.
This approach also has the downside of effectively nuking the namespace. So if we allow say raw.qemu.conf.XYZ, we won't be able to define raw.qemu.conf.disks or whatever for ourselves as a new config key as now everything under raw.qemu.conf is effectively reserved.

For the scriptlet case, my recommended solution would be to change the scriptlet so it's being fed the instance definition, then a single scriptlet could get through varying behavior based on the instance, its devices or config keys (including user.XYZ config keys). I think that may allow many cases to still have the one scriptlet but have it get sufficiently customized per instance.

Obviously that wouldn't really help you with raw.qemu and raw.qemu.conf, but it would at least cover the QMP case.

[bensmrs]

We obviously don't want to change the general behavior of config key overriding through profiles and local config, even just for a few select keys as we really want consistency there.

Yeah obviously

What we could do in theory is provide something like raw.qemu.conf.NAME and then have those be processed in alphabetical order with suitable merging/overriding. But we've also stayed away from doing that for cloud-init in the past as it adds quite a bit of complexity

Oh I’m not a fan of that AT ALL :)

I have two suggestions, both of them having the same pros and cons:

• Introduce <key>+: <value> syntax, adding data to previously defined properties
• Introduce a templating engine for property definitions ({{ value }}, ${value} or whatever), allowing notably to recall the previously defined value

Pros: doesn’t pollute the namespace, processes properties in definition order
Cons:

the moment we do it for a few keys, we'll have request to do that for basically every single key in the API.

But… is it that bad?

For the +: notation, we would only mark “extensible” keys, and as soon as there’s an actual need, define an “extension handler” doing what’s the most appropriate for the given key (e.g. concatenate with a space for raw.qemu, concatenate with \n\n for raw.qemu.conf, unmarshal > concatenate list > marshal for raw.qemu.qmp.{early,{post,pre}-start}…)

For the templating engine, this would open plenty of possibilities, such as renaming (single) network cards lan.{{ instance_name[:12] }}, and come with a one-time cost; if a key is a string, add a call to a template parser with a context (but the context is not trivial to define, as we’ll want some keys to be available before others).

To be honest, I kinda like both solutions, and they are not incompatible. The second one is pretty hard though (e.g. which template parameters do we allow, and for which keys?).

makes it a fair bit harder for a user to get a good overview of their configuration

That’s what --expanded is here for :)

[stgraber]

Anything that can't be easily computed through database queries is going to become quite expensive pretty quickly. We have quite a few places, mostly around devices and networks where we need to pretty quickly go over every single instance in the database, pull specific configs (applying all profiles in the process) and then get a list of instances with a specific final value.

That's the logic we need to run through to compute things like quotas, current project usage and the used_by field we have just about everywhere in the API.

Typically that doesn't impact the raw.XYZ config keys as those are by definition just for raw data we pass to something else, but that's why I wouldn't want this kind of concept to become more generalized as we absolutely don't want this anywhere near any of the limits.XYZ keys or any key that's used to refer to another object (network, pool, ...).

So it's one of those cases where I think it's probably a better idea to take a big step back and look at exactly what the current problem is and see if there isn't a better way to solve that problem :)

[stgraber]

One example of something that could be done here is to just make more use of raw.qemu.scriptlet.

The name of that key means that it's not necessarily tied to QMP.

So we could for example introduce a config stage which runs prior to QEMU being spawned and allows for mangling of the QEMU command line and QEMU conf itself.

[stgraber]

That would then in theory let you do everything that you currently want through a single scriptlet.

Speaking of, it would probably make sense to expose the instance itself to the QEMU scriptlet, currently we're only passed a bunch of functions and the stage for the hook, but we don't even know the name of the VM we're working against :)

[bensmrs]

We have quite a few places, mostly around devices and networks where we need to pretty quickly go over every single instance in the database, pull specific configs (applying all profiles in the process) and then get a list of instances with a specific final value.

Typically that doesn't impact the raw.XYZ config keys as those are by definition just for raw data we pass to something else, but that's why I wouldn't want this kind of concept to become more generalized as we absolutely don't want this anywhere near any of the limits.XYZ keys or any key that's used to refer to another object (network, pool, ...).

I mean… sure, but it makes very little sense in most cases where extending an existing key doesn’t have an obvious meaning.

So it's one of those cases where I think it's probably a better idea to take a big step back and look at exactly what the current problem is and see if there isn't a better way to solve that problem :)

I think you actually summarized it quite well: we want a mechanism like this one at some places, not all of them!

One example of something that could be done here is to just make more use of raw.qemu.scriptlet.

So we could for example introduce a config stage which runs prior to QEMU being spawned and allows for mangling of the QEMU command line and QEMU conf itself.

Hey I’ll not be one to say no to a nice little scriptlet, but I feel like it’ll get very hard for people who just want to add something to the QEMU command-line (and there are cases where it cannot be done in the configuration file).
That’s obviously a solution, but a quite complex one, and it is itself subject to the initial problem: what if a profile includes the one where we had a scriptlet modify some configuration keys?

Speaking of, it would probably make sense to expose the instance itself to the QEMU scriptlet, currently we're only passed a bunch of functions and the stage for the hook, but we don't even know the name of the VM we're working against :)

For the name, I’m sure we can get it from QMP :)
But yeah, more information about the VM could be nice.

[stgraber]

That’s obviously a solution, but a quite complex one, and it is itself subject to the initial problem: what if a profile includes the one where we had a scriptlet modify some configuration keys?

I wouldn't let a scriptlet modify the instance configuration keys, but I would let the QEMU scriptlet specifically modify the QEMU command line and config files right before it's written to disk and executed (only in stage == config).

So that would run after any existing raw.qemu or raw.qemu.conf and would be able to get the command line string and the INI qemu.conf data and provide new values for those.

This combined with having the instance struct exposed to the scriptlet means that you can write a generic enough scriptlet (in your case the MacOS stuff) which can vary based on the device list, based on instance config and have its own behavior altered by using user.XYZ config keys on the instance.

[stgraber]

Basically I'm thinking of a way where you can just ship a macos.py scriptlet that one can incus config set raw.qemu.scriptlet - < macos.py and then be good to go with all the needed QEMU alterations handled by it in one go as well as the ability for it to validate unsupported cases in the instance config and log as needed.

[bensmrs]

Well now I’m hyped :) (although we’re veeeeeery far from the original message)
I sure can work on that (and maybe finally be able to solve my macOS networking problems, although I’m really not sure).
About the instance struct, do we “just” add a new scriptlet parameter? what about existing scriptlets: is there a migration mechanism in place for configuration keys? What would the struct be populated with?

[stgraber]

We should provide an api.Instance.

Dealing with existing scriptlets is a bit more problematic indeed.
Is there an easy way for us to inspect the scriptlet and know what that function expects as far as arguments? (or really, just how many arguments it expects)

[bensmrs]

We should provide an api.Instance.

Ok, pretty simple then (unfortunately not much introspection into devices, so it won’t help with my macOS side quest).

As usual, my 200 questions. How do we want to handle the modification of QEMU command line/config file? Make the struct readonly and have the scriptlet in the config stage return a tuple (qemu_cli, qemu_conf)? Make the struct r/w and allow the scriptlet to modify it? only modify ExpandedConfig["raw.qemu*"]? Use set_qemu_cli/set_qemu_conf functions? (should the changes be reflected into the instance struct seen by the scriptlet?)

Dealing with existing scriptlets is a bit more problematic indeed.
Is there an easy way for us to inspect the scriptlet and know what that function expects as far as arguments? (or really, just how many arguments it expects)

Oh yeah, that’s pretty easy. It will only break the last PR that prevents just that :D
Either we define the struct as a global variable (I really don’t like that), or we propose a get_instance function (I hate that), or we accept to break existing scriptlets (I somewhat don’t like that) but it will have a nasty behavior with the last PR (and the fact that QEMU scriptlets are the LTS version means that we should be extra careful).

The real question feels more like “are there enough people that use the feature to actually do more than putting a red alert box in the Incus announcement message and in the forums?”

[stgraber]

Ok, pretty simple then (unfortunately not much introspection into devices, so it won’t help with my macOS side quest).

What's missing in your case?

The real question feels more like “are there enough people that use the feature to actually do more than putting a red alert box in the Incus announcement message and in the forums?”

I think we can break it this time around. I believe you and I are likely the only two "production" users at this point :)

As usual, my 200 questions. How do we want to handle the modification of QEMU command line/config file? Make the struct readonly and have the scriptlet in the config stage return a tuple (qemu_cli, qemu_conf)? Make the struct r/w and allow the scriptlet to modify it? only modify ExpandedConfig["raw.qemu*"]? Use set_qemu_cli/set_qemu_conf functions? (should the changes be reflected into the instance struct seen by the scriptlet?)

I'd provide a set of functions:

• get_qemu_cmdline
• set_qemu_cmdline
• get_qemu_conf
• set_qemu_conf

So you can get the current value, modify whatever you want and send it back.

[bensmrs]

What's missing in your case?

The thing is that I don’t know, hence my forum post…

I think we can break it this time around. I believe you and I are likely the only two "production" users at this point :)

Understandable; using it in prod is pretty cursed.

I'd provide a set of functions:

• get_qemu_cmdline
• set_qemu_cmdline
• get_qemu_conf
• set_qemu_conf

So you can get the current value, modify whatever you want and send it back.

LGTM. We should trigger an error for the set_* functions outside of stage="config", right? How should the conf be presented to the Starlark scriptlet? Raw text feels a bit rough given the fact that we have very few functions available within scriptlets to parse that…

---

Starlark-go dicts preserve insertion order, so I can propose two transformations for the following:

[device "sata0"]
driver = "virtio-blk-pci"
drive = "devzero"
share-rw = "on"

[device "sata1"]
driver = "virtio-blk-pci"
drive = "devzero"
share-rw = "on"

Option 1

This first option massively uses dicts, but reordering can be tricky, as it involves removing the key and reinserting it. Also, while Starlark-go guarantees to respect insertion order within scriptlets, unmarshalling can get tricky…

{
  'device "sata0"': {
    "driver": "virtio-blk-pci",
    "drive": "devzero",
    "share-rw": "on"
  },
  'device "sata1"': {
    "driver": "virtio-blk-pci",
    "drive": "devzero",
    "share-rw": "on"
  }
}

Option 2

Feels easier to implement, but loses the nice dictionary lookup feature…

[
  {
    "key": 'device "sata0"',
    "parameters": {
      "driver": "virtio-blk-pci",
      "drive": "devzero",
      "share-rw": "on"
    }
  },
  {
    "key": 'device "sata1"',
    "parameters": {
      "driver": "virtio-blk-pci",
      "drive": "devzero",
      "share-rw": "on"
    }
  }
]

---

I’ll let you be the judge. And while we’re at it, we might as well do the same to present cmdline args nicely to the scriptlet (e.g. a list of -{key} {value} tuples), but I won’t push the idea as hard…

Also, do we completely throw away the idea of having a function to recall previously defined scriptlets? OT1H, it could allow people to compose scriptlets and achieve some kind of inheritance; OTOH, it would require revamping quite some code handling configuration keys. I don’t mind giving up on that :)

Anyway, I’m changing the issue title, and I can commit to implementing it for Incus 6.9, so feel free to assign me the issue.

[stgraber]

LGTM. We should trigger an error for the set_* functions outside of stage="config", right? How should the conf be presented to the Starlark scriptlet? Raw text feels a bit rough given the fact that we have very few functions available within scriptlets to parse that…

Yeah, the set_ should fail if not in the config stage, get_ should be safe from any stage.

For what to expose, it may make sense to expose the config as a []cfgSection, so effectively the same type that's used internally when putting it together. It's pretty close to option 2. I'd be happy to have that type tweaked if needed so it's more suitable for use by the scriptlet but still remains shared with the internal logic.

Making that code/type shared between the actual config file building and the scriptlet would also make it easy for us to expose a variation of qemuRawCfgOverride to the scriptlet, so the scriptlet can read the current config, apply an override using a similar syntax to what's possible in raw.qemu.conf then read back the altered config if further changes are needed. I don't know if that'd be easier than just altering the struct and giving it back in full.

I’ll let you be the judge. And while we’re at it, we might as well do the same to present cmdline args nicely to the scriptlet (e.g. a list of -{key} {value} tuples), but I won’t push the idea as hard…

Yeah, for this one it would make sense to return the args part of qemuCmd (everything except first field) and expect a similar value to be provide back by the user. That will save us from having to send stuff through shellquote again to parse it.

Also, do we completely throw away the idea of having a function to recall previously defined scriptlets? OT1H, it could allow people to compose scriptlets and achieve some kind of inheritance; OTOH, it would require revamping quite some code handling configuration keys. I don’t mind giving up on that :)

I think so, at least for now, lets wait for a use case that we can't fix cleanly otherwise :)

Anyway, I’m changing the issue title, and I can commit to implementing it for Incus 6.9, so feel free to assign me the issue.

Excellent. I'll triage this as a new feature for 6.9!

[bensmrs]

Any particular reason why cfgSection.entries is a []cfgEntry instead of a map[string]string? I don’t think the order matters to QEMU. Is it rather to have a consistent ordering to ensure predictable marshaling, thus making tests simple string comparisons as it is the case now?

I can either convert the entries to a map (and back) in the scriptlet definition, or remove the cfgEntry type (implying a bit of work to ensure everything doesn’t fall apart). Your call!

[bensmrs]

Also, I propose to prevent messing with -bios or -kernel through set_qemu_cmdline, because generateQemuConfigFile checks these and I really don’t want to change the logic of the function (just rewire it a bit so I can plug the scriptlet before WriteFile). So basically, we shall check the presence of -bios and -kernel before and after, and both should match. generateQemuConfigFile only checks their presence to load the NVRAM and setup boot priorities, so we can mess with the values as long as it’s already there.

[stgraber]

Also, I propose to prevent messing with -bios or -kernel through set_qemu_cmdline, because generateQemuConfigFile checks these and I really don’t want to change the logic of the function (just rewire it a bit so I can plug the scriptlet before WriteFile). So basically, we shall check the presence of -bios and -kernel before and after, and both should match. generateQemuConfigFile only checks their presence to load the NVRAM and setup boot priorities, so we can mess with the values as long as it’s already there.

Sounds good

Any particular reason why cfgSection.entries is a []cfgEntry instead of a map[string]string? I don’t think the order matters to QEMU. Is it rather to have a consistent ordering to ensure predictable marshaling, thus making tests simple string comparisons as it is the case now?

I don't think so. It may have helped with tests but that sounds like a poor reason for it.
The current logic was contributed by a LXD contributor quite a while back so I don't recall his exact motivation for that (or if we even asked about it ;)). If moving to a map makes things easier, I'd be happy with us doing that.

[bensmrs]

I don’t think it’ll make things any easier, but having clean code feels like a good incentive for change!
This PR is definitely getting huge :)

[bensmrs]

Well I’m getting cold feet… The override mechanism code is lacking precious comments; I can try to patch it, but IMO it will need to be rewritten from scratch. Coming from a safety and formal methods background, I’m not at ease having a configuration parser relying on a big uncommented regex and ad-hoc if-then-else statements…
I’m not gonna do it for this PR, but we should have a real lexer/parser there, IMO.