feat: [NODE-1683] Make VM memory configuration a dev only feature

[Bownairo]

No description provided.

[github-actions]

This pull request modifies the IC-OS configuration types library (rs/ic_os/config_types/src/lib.rs).

Please ensure you have followed the Configuration Update Protocol guidelines——particularly if adding a new enum or enum variants:

Enum Variant Forward Compatibility Guidelines: If adding a new enum or new variants to an enum, ensure older versions can handle unknown variants gracefully by using #[serde(other)] on a fallback variant. See examples: GuestVMType::Unknown and Ipv6Config::Unknown.

To acknowledge this reminder and unblock the PR, dismiss this code review by:

• Going to the bottom of the pull request page
• Finding where this bot is requesting changes
• Clicking the three dots on the right
• Selecting "Dismiss review"

For complete guidelines, see the documentation at the top of rs/ic_os/config_types/src/lib.rs.

CONFIG_TYPES_COMPATIBILITY_REMINDER_DEDUP

[Bownairo]

This could be moved even closer to SetupOSConfig, but there's not really any logic in the types crate yet, so that can wait.

[Bownairo]

I tried to remove references to vm_memory, and 490 wherever possible, just so it doesn't trip us up fuzzy searching.

[Bownairo]

As above, this should probably be closer to the VM crates, but I'm avoiding circular dependencies for now.

[Bownairo]

memory from the deployment JSON is now optional, and vm_memory in HostOSSettings is still always set. If memory is specified on dev, that value is used, else PROD_GUEST_VM_MEMORY.

I'd like to take it further and always have HostOSSettings read back PROD_GUEST_VM_MEMORY on prod, but that's difficult while we still access struct fields directly.

[andrewbattat]

Do we plan to update vm_cpu and vm_nr_of_vcpus to also be dev features in a follow-up?

[Bownairo]

That's a great point. Maybe we can take the whole thing to vm_resource_overrides?

[andrewbattat]

Sounds good to me!

[andrewbattat]

I feel a little uncomfortable removing memory from deployment.json.template. We worked hard to get rid of all the "hidden" deployment.json.template config fields that weren't in the template but were being injected in for testing, and now we're adding one back in.

If I understand this change correctly, we want vm_memory configurable for dev, but hard-coded in the config_tool for prod, and because the default value is hard-coded in the config_tool for prod, we don't also want the field default value here.

WDYT of renaming the field in deployment.json.template to vm_memory_override and then we leave the value blank in the template like: "memory_override": "",

Then, if the memory_override deployment.json value is empty, the default vm_memory is used and if non-empty and dev, the memory_override is used? I'm sorta thinking out loud here.

[andrewbattat]

After reading more, this would be my suggestion:

1. rename the deployment.json.template field AND the config types field from vm_memory to vm_memory_override
2. Leave "memory_override": "" in deployment.json
3. Make vm_memory_override optional in config types (and in deployment_json.rs, like it is now)
4. Always allow vm_memory_override to be written to the config object, whether it's prod or dev (i.e. remove the dev check in SetupOS)
   ** ^ (this one i'm if-y about. Maybe it's better to have the SetupOS check. Or maybe it's unnecessary complexity. idk)
5. In HostOS, when configuring the guest xml, set the vm memory to (vm_memory_override if dev and Some) OR the default

WDYT? We've been placing the prod checks as close to the actual configuration as possible, so that was my thought process for moving it to HostOS.

I'm also open to just the following suggestion and leaving everything else mostly the same:

WDYT of renaming the field in deployment.json.template to vm_memory_override and then we leave the value blank in the template like: "memory_override": "",
Then, if the memory_override deployment.json value is empty, the default vm_memory is used and if it's non-empty and dev, the memory_override is used

[Bownairo]

Thanks for working through this with me!

Always allow vm_memory_override to be written to the config object, whether it's prod or dev (i.e. remove the dev check in SetupOS)

Sounds good. I wanted to avoid inconsistencies in config and reality, but the "override" naming allows for that. I prefer the check in HostOS, too.

---

Thinking through this for compatibility - SetupOS naming has no issues, as the config does not leave the system. HostOS holds its config from the time it launches, right? For prod we can "drop" any of this config, since we will ignore it anyway. For dev, we need to think about upgrades (From mainnet tests), and downgrades (To mainnet tests), so that the overrides are used.

That makes me think, can we make the whole of hostos_settings dev only? How does verbose work? If we cheat a little and leave hostos_settings the same, the internal struct could be made HostOSDevSettings and parse fine since JSON is untagged...

If we want to do it the "right" way, I think we should try to get to:

struct HostOSSettings {
    verbose: bool,
    hostos_dev_settings: HostOSDevSettings,
}

struct HostOSDevSettings {
    vm_memory: u32,
    vm_cpu: String,
    vm_nr_of_vcpus: u32,
}

What do you think:

1. Add and use field hostos_dev_settings with default values matching dev, begin populating both values, ignore both completely on prod.
   time passes
2. Remove old values completely

It's a slight variation on Removing Fields where we populate both instead of setting a default, since we still have the information around.

The double population supports downgrades (to a version without new variant), the defaults support upgrades (from a version without new variant populated). We won't be able to properly test outside of this range, but that is the case with the update protocol as well. If anything it is less of an issue here as prod will not use these values (and we hope they never have been used - we can throw them away if they have).