Skip to content

Conversation

@frankdavid
Copy link
Contributor

@frankdavid frankdavid commented May 22, 2025

A few motivators of the new implementation:

  • Typesafe implementation.
  • The XML generation now happens in Rust using Jinja-like templates which are more powerful than sed (e.g. loops, conditionals are supported) and are checked at compile-time. The templates are easier to read.
  • Tests were added including golden files for the XML output.
  • Previously we had two shell scripts (generate-guestos-config.sh and dev-generate-guestos-config.sh) which only differed in a few lines and had to be otherwise kept in sync. This is no longer needed, since we can use rust feature flags to enable dev behavior conditionally.
  • Bonus: it works faster (0.7s -> 0.03s)

I manually verified that the behavior of the new tool is the same as the old bash script. The only difference is that we create the guestos config in a temp file instead of /boot/config/config-guestos.json. Since this file is only used to create the bootstrap config image, it's not necessary to store it on the config partition. In a later PR, we can completely remove the temporary file and just pass around a Rust struct.

@frankdavid frankdavid changed the base branch from master to frankdavid/bootstrap-config May 22, 2025 19:48
@frankdavid frankdavid added this pull request to the merge queue May 26, 2025
@frankdavid frankdavid removed this pull request from the merge queue due to a manual request May 26, 2025
…-vm-config

# Conflicts:
#	Cargo.Bazel.Fuzzing.json.lock
#	Cargo.Bazel.json.lock
@frankdavid frankdavid enabled auto-merge May 26, 2025 12:23
@frankdavid frankdavid added this pull request to the merge queue May 26, 2025
Merged via the queue into master with commit a87bc0b May 26, 2025
23 checks passed
@frankdavid frankdavid deleted the frankdavid/generate-vm-config branch May 26, 2025 12:57
github-merge-queue bot pushed a commit that referenced this pull request May 27, 2025
The golden tests were not submitted as part of #5264 because the golden
files messed up the git history.
Comment on lines +67 to +70
bail!(
"GuestOS configuration file already exists: {}",
args.output.display()
);
Copy link
Contributor

@andrewbattat andrewbattat May 27, 2025

Choose a reason for hiding this comment

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

If the configuration file already exists, will the generate-guestos-config service fail here? Don't we just want to early retun, not fail?

I supposed this can easily be tested just by rebooting HostOS

@frankdavid frankdavid restored the frankdavid/generate-vm-config branch June 16, 2025 11:42
@frankdavid frankdavid deleted the frankdavid/generate-vm-config branch June 16, 2025 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants