design-proposal: Generating disk serial numbers #305

akrejcir · 2024-07-02T12:50:24Z

What this PR does / why we need it:
This design introduces automatic generation of serial numbers for disks.

Issue:
https://issues.redhat.com/browse/CNV-39082

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note:

Added proposal for disk serial number generation.

kubevirt-bot · 2024-07-02T12:50:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vladikr for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

akrejcir · 2024-07-02T12:54:12Z

@aglitke , can someone from the storage team take a look, if this approach is good?

design-proposals/generated-disk-serial.md

dominikholler · 2024-07-05T12:02:11Z

design-proposals/generated-disk-serial.md

+## Motivation
+Some applications running in the VM require the disk serial number to be defined and persistent across reboots.
+Generating the serial reduces manual configuration, and in the future it can be part of a `VirtualMachinePreference` for the OS. 
+


Is there a reference (doc or implemention) how this is addressed in other virtualization management system, especially oVirt and OpenStack?

I will check oVirt code.

@lyarwood , do you know how this is done in OpenStack? Or can you point me to docs or source, where I could find it?

@akrejcir we talked about introducing persistent serials for the longest time but could never implement it due to a mountain of tech debt we had in the codebase. The following design was as far as we got:

https://specs.openstack.org/openstack/nova-specs/specs/stein/approved/local-disk-serial-numbers.html

Thanks!

oVirt uses the disk GUID as a serial number. It would be similar if we used the UID of a PVC.

design-proposals/generated-disk-serial.md

lyarwood · 2024-07-16T11:23:54Z

design-proposals/generated-disk-serial.md

+
+One possibility is to store it in an annotation on the PVC object.
+The annotation would be created when the VMI is created. 
+Then it will be read by `virt-handler` when creating libvirt domain XML.


Why don't we just persist all serial numbers in the Status of the VirtualMachine?

I think the status may be emptied by a GitOps controller, when it creates a new version of the VM. I will check.

I will add this possibility to the document for discussion.

We should be ignoring any updates of Status outside of the /Status subresource IIRC so I don't think a GitOps workflow would overwrite it here. That's why we are looking to move instance type metadata out of the Spec etc.

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status

The PUT and POST verbs on objects MUST ignore the status values, to avoid accidentally overwriting the status in read-modify-write scenarios. A /status subresource MUST be provided to enable system components to update statuses of resources they manage.

Thanks. It could work. I will add it to the design document.

Same question as for instancetypes. What if the status needs to be recreated? Where is the ID derived from? Can we recreate the same ID if the status needs to be recreated from scratch?

That is the main issue with this approach.

If we could deterministically generate the serial number, then we don't need to store it in .status.
In this case, the serial number is randomly generated, so it needs to be stored somewhere. If .status is recreated, then a new number is generated.

lyarwood · 2024-07-16T11:25:54Z

design-proposals/generated-disk-serial.md

+
+- Do we want the serials to be generated by default on all VMs,
+  or do we want a configuration option to enable it?
+- Where is a good place for the `generateDiskSerials` field?


spec.template.spec.domain.devices.generateDiskSerials of the VirtualMachine IMHO

lyarwood · 2024-07-16T11:28:55Z

design-proposals/generated-disk-serial.md

+## Motivation
+Some applications running in the VM require the disk serial number to be defined and persistent across reboots.
+Generating the serial reduces manual configuration, and in the future it can be part of a `VirtualMachinePreference` for the OS. 
+


@akrejcir we talked about introducing persistent serials for the longest time but could never implement it due to a mountain of tech debt we had in the codebase. The following design was as far as we got:

https://specs.openstack.org/openstack/nova-specs/specs/stein/approved/local-disk-serial-numbers.html

design-proposals/generated-disk-serial.md

This design introduces automatic generation of serial numbers for disks. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

0xFelix · 2024-07-24T15:27:15Z

design-proposals/generated-disk-serial.md

+
+# Design
+
+A new boolean option `generateDiskSerials` will be added to the VMI `.spec`. 


Do we really need an option for it? What's the default right now? Is it 123456789, which means nobody cares about it anyway? Can't we just enable it if the ID was not set manually?

The default right now is no serial number. The <serial> tag in the libvirt domain XML is not specified.

We could enable it by default for all disks. That is one of the open questions in this design.
I'm unsure if it may confuse guests on existing VMs, if the disk will have a serial that it didn't have before.

0xFelix · 2024-07-24T15:28:28Z

design-proposals/generated-disk-serial.md

+
+One possibility is to store it in an annotation on the PVC object.
+The annotation would be created when the VMI is created. 
+Then it will be read by `virt-handler` when creating libvirt domain XML.


Same question as for instancetypes. What if the status needs to be recreated? Where is the ID derived from? Can we recreate the same ID if the status needs to be recreated from scratch?

0xFelix · 2024-07-24T15:29:13Z

design-proposals/generated-disk-serial.md

+
+
+### Storing it in the `status` of the VM
+Serial numbers will be stored in the `.status` of the VM.


How to restore / transfer a VM with the serial number? IIUC you are not allowed to write to the status of a K8S object.

I will look into it.

I've checked the code, and the .status of a VM is not stored into the snapshot.

I think @0xFelix 's point was that we then need a way of persisting the serial in the core spec of a VirtualMachine during a snapshot so that we can then restore the status (and the state of the VM) later.

0xFelix · 2024-07-24T15:32:15Z

design-proposals/generated-disk-serial.md

+- It may be an unexpected place to store persistent information that cannot be regenerated if `.status` is wiped.
+
+
+### Deterministic generation


I like this approach the most. Basically a stateless approach without configurables.

It would be the best choice, if it can be compatible with storage migration.

fabiand · 2024-07-31T08:17:44Z

design-proposals/generated-disk-serial.md

+Then the rest of the document describes details of option: **Annotation on the PVC object**.
+
+### Annotation on the PVC object
+The serial number will be stored as an annotation on the PVC object. It would be created when the VMI is created.


This will require some component to have PVC write permissions.

fabiand · 2024-07-31T08:23:43Z

design-proposals/generated-disk-serial.md

+so it has to be stored somewhere, or generated deterministically.
+
+This section lists several options how to store serial numbers.
+Then the rest of the document describes details of option: **Annotation on the PVC object**.


Generally speaking the ID can be scoped in two ways:

VM/I Internal - serial is only defined/kept within the VM/I bject

VM/I external - serial is sourced from an external object - this is the case with the PVC annotation approach

If it's internal, then usually the requirements of a persistent serial number are met, however, it could lead to trouble if a volume is changed on a VMI, or if a volume is used on multiple VMIs.

If it is external, then we also meet the requirement of a persistent serial number, the same volume will get the same serial whenever attached to a VM that's good for vols attached to multi VMs, however, we need to think about cloning, and restore, and what it means for disk serial numbers. Something also requires permissionsto write the PVC.

Just to name a few things.

fabiand · 2024-07-31T08:25:42Z

design-proposals/generated-disk-serial.md

+- Serial is clearly visible to anyone who can read PVC.
+
+#### Disadvantages:
+- Does not store serials for other volume types except PVC.


Requires additional permissions to write PVCs

fabiand · 2024-07-31T08:28:51Z

design-proposals/generated-disk-serial.md

+- We don't need to store anything.
+
+#### Disadvantages:
+- If the storage is migrated to a PVC with different metadata, it would change the serial number.


Usually we do not "migrate" PVCs (aka modify them in-place).
Instead new PVCs are created. @alicefr @awels what does storage class migration do?

Storage class migration requires one to create new PVCs as the target of the migration. So there will be new PVCs. PVCs themselves are mostly immutable, in particular the name is immutable.

fabiand · 2024-07-31T08:30:35Z

design-proposals/generated-disk-serial.md

+metadata:
+  name: example-pvc
+  annotations:
+    kubevirt.io/disk-serial: 123456789 # <-- new annotation


please use vm.kubevirt.io/disk-serial

cc @EdDev @alaypatel07 can we have api conventions? Possibly worth to standardize how we prefix annotations accross kubevirt.

fabiand · 2024-07-31T08:31:34Z

design-proposals/generated-disk-serial.md

+
+## Update/Rollback Compatibility
+The feature is backward compatible. Existing VMs remain unaffected unless the new field is set.
+Rollback ignores the new field.


We should check todays state:

If a serial is not specified, between VM start, stop, start, does the serial of the disk inside the VM change?

If it is the same serial, then we might need a migraiton plan.

kubevirt-bot added the dco-signoff: no Indicates the PR's author has not DCO signed all their commits. label Jul 2, 2024

kubevirt-bot requested review from aburdenthehand and jean-edouard July 2, 2024 12:50

kubevirt-bot added the size/L label Jul 2, 2024

akrejcir force-pushed the disk-serial-proposal branch from 2ac6a4a to 0522553 Compare July 2, 2024 12:52

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jul 2, 2024

codingben reviewed Jul 3, 2024

View reviewed changes

design-proposals/generated-disk-serial.md Show resolved Hide resolved

design-proposals/generated-disk-serial.md Outdated Show resolved Hide resolved

design-proposals/generated-disk-serial.md Show resolved Hide resolved

akrejcir force-pushed the disk-serial-proposal branch from 0522553 to 9aaec11 Compare July 4, 2024 15:13

dominikholler suggested changes Jul 5, 2024

View reviewed changes

kubevirt-bot assigned dominikholler Jul 5, 2024

lyarwood reviewed Jul 16, 2024

View reviewed changes

akrejcir force-pushed the disk-serial-proposal branch from 9aaec11 to 735359c Compare July 17, 2024 15:26

design-proposal: Generating disk serial numbers

d0d0377

This design introduces automatic generation of serial numbers for disks. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

akrejcir force-pushed the disk-serial-proposal branch from 735359c to d0d0377 Compare July 24, 2024 13:30

0xFelix reviewed Jul 24, 2024

View reviewed changes

fabiand reviewed Jul 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

design-proposal: Generating disk serial numbers #305

design-proposal: Generating disk serial numbers #305

akrejcir commented Jul 2, 2024 •

edited

Loading

kubevirt-bot commented Jul 2, 2024

akrejcir commented Jul 2, 2024

dominikholler Jul 5, 2024

akrejcir Jul 16, 2024 •

edited

Loading

lyarwood Jul 16, 2024

akrejcir Jul 18, 2024

lyarwood Jul 16, 2024

akrejcir Jul 16, 2024

lyarwood Jul 17, 2024 •

edited

Loading

akrejcir Jul 17, 2024

0xFelix Jul 24, 2024

akrejcir Jul 25, 2024

lyarwood Jul 16, 2024

akrejcir Jul 16, 2024

lyarwood Jul 16, 2024

0xFelix Jul 24, 2024

akrejcir Jul 25, 2024 •

edited

Loading

0xFelix Jul 24, 2024

0xFelix Jul 24, 2024

akrejcir Jul 25, 2024

akrejcir Jul 29, 2024

lyarwood Aug 20, 2024

0xFelix Jul 24, 2024

akrejcir Jul 25, 2024

fabiand Jul 31, 2024

fabiand Jul 31, 2024

fabiand Jul 31, 2024

fabiand Jul 31, 2024

awels Jul 31, 2024

fabiand Jul 31, 2024

fabiand Jul 31, 2024


		# Design

		A new boolean option `generateDiskSerials` will be added to the VMI `.spec`.



		### Storing it in the `status` of the VM
		Serial numbers will be stored in the `.status` of the VM.

		- It may be an unexpected place to store persistent information that cannot be regenerated if `.status` is wiped.


		### Deterministic generation

design-proposal: Generating disk serial numbers #305

Are you sure you want to change the base?

design-proposal: Generating disk serial numbers #305

Conversation

akrejcir commented Jul 2, 2024 • edited Loading

kubevirt-bot commented Jul 2, 2024

akrejcir commented Jul 2, 2024

Choose a reason for hiding this comment

akrejcir Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyarwood Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir commented Jul 2, 2024 •

edited

Loading

akrejcir Jul 16, 2024 •

edited

Loading

lyarwood Jul 17, 2024 •

edited

Loading

akrejcir Jul 25, 2024 •

edited

Loading