Repository navigation
Conversation
| pub iommu: IommuType, | ||
| #[serde(default)] | ||
| pub pci_segment: u16, | ||
| #[serde(default, alias = "pci_device_id", rename = "bdf_device")] |
There was a problem hiding this comment.
hint for reviewers: my old alignment commit was wrong from the very beginning. there was no good reason to keep bdf_device, neither generally nor for libvirt.
We now have a clear old->new migration path.
| // Alias is technical debt from our fork. Once the previous version | ||
| // is fully rolled out, we can remove this for migration. However, there | ||
| // might be old snapshots that would not restore without this. | ||
| // Remove with caution. | ||
| #[serde(default, alias = "bdf_device")] |
There was a problem hiding this comment.
This can be boiled down to a oneliner, e.g. (the release is exemplary, I have not verified since when this is the case):
| // Alias is technical debt from our fork. Once the previous version | |
| // is fully rolled out, we can remove this for migration. However, there | |
| // might be old snapshots that would not restore without this. | |
| // Remove with caution. | |
| #[serde(default, alias = "bdf_device")] | |
| // `bdf_device` is the name used by gardenlinux-release-26-06-16 and later. | |
| #[serde(default, alias = "bdf_device")] |
There was a problem hiding this comment.
Thanks! I'd favor a middle ground: Say from when to when the old name is used and keep the warning
There was a problem hiding this comment.
I decided for a different more verbose wording. I want us to be very careful with removing this because of old snapshots. The comment must make this clear.
| @@ -441,7 +441,11 @@ pub struct PciDeviceCommonConfig { | |||
| pub iommu: IommuType, | |||
There was a problem hiding this comment.
In the commit message:
I made a mistake and made things more complicated than necessary.
Could you please elaborate explicitly on what was unnecessarily complicated? Is this just that we wouldn't have needed rename?
There was a problem hiding this comment.
Yes, we could have used pci_device_id right from the beginning and would have just needed the alias.
This aligns the code with upstream CH. CH accepts both names and currently still prefers bdf_device but this changes soon [0]. [0] cyberus-technology/cloud-hypervisor#223 On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
ce1c1bd to
f288810
Compare
In "vmm: pci: rename pci_device_id -> bdf_device" [0] I made things more complicated then necessary: we could have use this change right away. Next, the actual way forward is to use an alias so that old->new works but new snapshots use the new name. Once the whole fleet has this, the commit could be dropped. However, we might want to keep it for longer (half a year?) as old snapshots need to be restored as well. There is no risk here. libvirt/ch works with both variants for the time being. [0] 3580b9f On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
f288810 to
91e28d8
Compare
|
Question: If new snapshots use the new name, will that then not cause problems when migrating from a new instance to an old? (Or is that not something our customer needs to do?) |
Thanks for thinking about corner cases! We are safe, tho
|
This is a trivial quick win and aligns with our goal to remove more fork-only quirks soon.
There is no risk and this is the right thing to do.
libvirt companion OR: https://gitlab.cyberus-technology.de/cyberus/cloud/libvirt/-/merge_requests/313
(both PRs are independend tho, as CH in any way supports both names)
Ticket: https://github.com/cobaltcore-dev/cobaltcore/issues/747
Steps to Undraft