Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
design-proposal: Generating disk serial numbers #305
base: main
Are you sure you want to change the base?
design-proposal: Generating disk serial numbers #305
Changes from all commits
d0d0377
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reference (doc or implemention) how this is addressed in other virtualization management system, especially oVirt and OpenStack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
oVirt uses the disk GUID as a serial number. It would be similar if we used the UID of a PVC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking the ID can be scoped in two ways:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will require some component to have PVC write permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we just persist all serial numbers in the
Status
of theVirtualMachine
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theSpec
etc.https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. It could work. I will add it to the design document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires additional permissions to write PVCs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to restore / transfer a VM with the serial number? IIUC you are not allowed to write to the status of a K8S object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked the code, and the
.status
of a VM is not stored into the snapshot.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach the most. Basically a stateless approach without configurables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be the best choice, if it can be compatible with storage migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we do not "migrate" PVCs (aka modify them in-place).
Instead new PVCs are created. @alicefr @awels what does storage class migration do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec.template.spec.domain.devices.generateDiskSerials
of theVirtualMachine
IMHOThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.