Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/platform/backends/qemu/qemu_vm_process_spec.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

This covers only the QEMU backend. What happens in other backends? Is a name-based UUID available in the new Hyper-V and AppleVZ today? If not, that's probably worth some wider team awareness and synchronizing.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be available for all off-the-shelf hypervisors already -- the existing hyper-v implementation already does that:

ubuntu@test-vm-3:~$ sudo cat /sys/class/dmi/id/product_uuid
e190e5e1-6261-40ca-a806-1a197a1d4ca0

I don't know the specifics for Apple VZ, but I think there should be a way to set it, as it's a pretty common thing for hypervisors to do. https://developer.apple.com/documentation/virtualization/vzgenericmachineidentifier looks promising, maybe @sharder996 could confirm that. I'll check the new Hyper-V implementation to make sure that it's still available.

Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ mp::QemuVMProcessSpec::QemuVMProcessSpec(const mp::VirtualMachineDescription& de
QStringList mp::QemuVMProcessSpec::arguments() const
{
QStringList args;

if (resume_data)
{
args = resume_data->arguments;
Expand All @@ -63,10 +62,12 @@ QStringList mp::QemuVMProcessSpec::arguments() const
}
else
{
// The UUID needs to be unique per VM and must be consistent across boots.
const auto vm_uuid = utils::make_uuid(desc.vm_name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

A few questions regarding effects on clones:

  • Since their name changes from the original, clones get a new UUID, correct?
  • Is that compatible with pre-existing disk contents in typical scenarios where this is relevant?
  • We cause cloud-init to rerun when cloning. Is that typically enough to cascade the UUID wherever relevant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since their name changes from the original, clones get a new UUID, correct?

Yes. It should be treated as any other instance-unique field.

Is that compatible with pre-existing disk contents in typical scenarios where this is relevant?

Yes, it is abstracted from the disk contents. In reality, it's a hardware serial-like info burned into the BIOS by the manufacturer. No disk involvement at all.

We cause cloud-init to rerun when cloning. Is that typically enough to cascade the UUID wherever relevant?

The /sys/class/dmi/id/product_uuid file is immediately available to queries without cloud-init to be re-run.

auto mem_size =
QString::number(desc.mem_size.in_megabytes()) + 'M'; /* flooring here; format documented
in `man qemu-system`, under `-m` option; including suffix to avoid relying on default unit */

// clang-format off
args << platform_args;
// The VM image itself
args << "-device"
Expand Down Expand Up @@ -97,6 +98,9 @@ in `man qemu-system`, under `-m` option; including suffix to avoid relying on de
<< "-nographic";
// Cloud-init disk
args << "-cdrom" << desc.cloud_init_iso;
// To make `/sys/class/dmi/id/product_uuid` present
args << "-uuid" << vm_uuid;
Copy link
Collaborator

Choose a reason for hiding this comment

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

To implement instance renaming, we'll probably want to keep the UUID stable. We'd probably save it along with other instance metadata, right? Do you foresee any impediment doing that? Just to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd probably save it along with other instance metadata, right?

That should be sufficient if we update the UUID on clone. I can't think of any potential gotchas on that.

// clang-format on
}

for (const auto& [_, mount_data] : mount_args)
Expand Down
4 changes: 3 additions & 1 deletion tests/unit/qemu/test_qemu_vm_process_spec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ TEST_F(TestQemuVMProcessSpec, defaultArgumentsCorrect)
#else
const auto storage_interface = "virtio-scsi-pci";
#endif

const auto expected_uuid = multipass::utils::make_uuid(desc.vm_name);
EXPECT_EQ(spec.arguments(),
QStringList({"--enable-kvm",
"-nic",
Expand All @@ -89,6 +89,8 @@ TEST_F(TestQemuVMProcessSpec, defaultArgumentsCorrect)
"-nographic",
"-cdrom",
"/path/to/cloud_init.iso",
"-uuid",
expected_uuid,
"-virtfs",
"local,security_model=passthrough,uid_map=1000:1000,gid_map=1000:1000,"
"path=path/to/target,mount_tag=m810e457178f448d9afffc9d950d726"}));
Expand Down
Loading