Generate new firmware UUID during restore #317

alromeros · 2025-01-07T11:57:50Z

What this PR does / why we need it:

This Pull Request implements a new label, "velero.kubevirt.io/generate-new-firmware-uuid" that when used in the restore object, the velero plugin will generate new firmware UUIDS for all restored VMs.

Which issue(s) this PR fixes:
Fixes # #199 (comment)

Special notes for your reviewer:

Release note:

Add label to optionally generate new firmware UUID during restore

alromeros · 2025-01-07T13:10:20Z

/test pull-kvp-unit-test

mhenriks · 2025-01-07T14:17:31Z

pkg/util/util.go

+// GenerateNewFirmwareUUID generates a new firmware UUID for the restored VM, ensuring it will be different than the one
+// Kubevirt generates for the original VM by using a different magicUUID and hashing name+namespace.
+func GenerateNewFirmwareUUID(vmiSpec *kvv1.VirtualMachineInstanceSpec, name, namespace string) {
+	vmiSpec.Domain.Firmware.UUID = types.UID(uuid.NewSHA1(firmwareUUIDns, []byte(name+namespace)).String())


Did you get this code from kubevirt? Shouldn't there be / between namespace and name? Otherwise hash(foo, bar) = hash(f, oobar)?

Yeah, used this PR as reference kubevirt/kubevirt#12885. This can be improved with your suggestion, will add the / and fix the test later today.

See what they do here: kubevirt/kubevirt#13150

Should've looked at this sooner: kubevirt/community#347

namespace/name was abandoned for object uid. Unfortunately we cannot use that here so may as well just generate new random uids

Interesting, but we can't do much until it merges. Are we still ok with this implementation?

I think random uuid is better, one of the goals of the proposal is to never have two VMs with the same firmware ID and the current scheme does not give us that

I'm not against generating a random one but currently I'm hashing name/namespace/UID, isn't that enough to secure unique IDs per VM?

No because VMs with same name/namespace is deleted+recreated it will have the same id

ShellyKa13 · 2025-01-08T12:22:40Z

pkg/plugin/vm_restore_item_action.go

its not strictly related to this PR.. but there is a pattern of

if [should do something since there is the label on the restore] { do something accordingly }

and for Run strategy it is different, and it bugs me a bit lol
do you mind changing

if runStrategy, ok := util.GetRestoreRunStrategy(input.Restore); ok { p.log.Infof("Setting virtual machine run strategy to %s", runStrategy) vm.Spec.RunStrategy = ptr.To(runStrategy) vm.Spec.Running = nil }

to something like:

if util.ShouldUpdateVmRunStrategy(input.Restore) { p.log.Infof("Setting virtual machine run strategy to %s", runStrategy) util.UpdateVMRunStrategy(input.Restore, &vm.Spec) }

and do the actual update in a function in util? as done for MacAddress and now FirmwareUUID
Ill appreciate it :)

That one is different because it's not used in both VM and VMI restore files so there's no redundant code.

But yeah I agree that it might look more organized if I add the function. I'm planning to also support annotations for all these operations soon, so I can handle it in a follow-up if you don't mind :)

dasionov · 2025-01-09T12:49:47Z

/cc @dasionov

ShellyKa13 · 2025-01-15T10:38:34Z

pkg/util/util.go

+}
+
+// GenerateNewFirmwareUUID generates a new firmware UUID for the restored VM, ensuring it will be different than the one
+// Kubevirt generates for the original VM by using a different magicUUID and hashing name+namespace.


seems like the comment is outdated?

Right, good catch

ShellyKa13

/approve

kubevirt-bot · 2025-01-15T10:39:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ShellyKa13

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ShellyKa13]

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

This commit implements a new label, "velero.kubevirt.io/generate-new-firmware-uuid", that when used in the restore object, the velero plugin will generate new firmware UUIDS for restored VMs. Signed-off-by: Alvaro Romero <[email protected]>

…uuid" label Signed-off-by: Alvaro Romero <[email protected]>

mhenriks · 2025-01-17T16:08:48Z

/lgtm

alromeros · 2025-01-17T16:22:36Z

/cherrypick release-v0.7

kubevirt-bot · 2025-01-17T16:23:13Z

@alromeros: #317 failed to apply on top of branch "release-v0.7":

Applying: Add label to generate new firmware UUIDS for restored VMs
Using index info to reconstruct a base tree...
M	pkg/plugin/vm_restore_item_action.go
M	pkg/plugin/vm_restore_item_action_test.go
M	pkg/plugin/vmi_restore_item_action.go
M	pkg/util/util.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/util/util.go
Auto-merging pkg/plugin/vmi_restore_item_action.go
Auto-merging pkg/plugin/vm_restore_item_action_test.go
CONFLICT (content): Merge conflict in pkg/plugin/vm_restore_item_action_test.go
Auto-merging pkg/plugin/vm_restore_item_action.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Add label to generate new firmware UUIDS for restored VMs

In response to this:

/cherrypick release-v0.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Jan 7, 2025

kubevirt-bot requested review from mhenriks and ShellyKa13 January 7, 2025 11:58

mhenriks reviewed Jan 7, 2025

View reviewed changes

ShellyKa13 reviewed Jan 8, 2025

View reviewed changes

alromeros force-pushed the new-firmware-uuid branch 6 times, most recently from 7679c31 to 4211ea4 Compare January 8, 2025 21:12

kubevirt-bot requested a review from dasionov January 9, 2025 12:49

alromeros force-pushed the new-firmware-uuid branch from 4211ea4 to 5232e76 Compare January 9, 2025 17:02

ShellyKa13 reviewed Jan 15, 2025

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2025

alromeros added 2 commits January 15, 2025 11:56

Add func test coverage for "velero.kubevirt.io/generate-new-firmware-…

087b8a6

…uuid" label Signed-off-by: Alvaro Romero <[email protected]>

alromeros force-pushed the new-firmware-uuid branch from 5232e76 to 087b8a6 Compare January 15, 2025 10:56

alromeros requested a review from mhenriks January 15, 2025 19:37

kubevirt-bot assigned mhenriks Jan 17, 2025

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2025

kubevirt-bot merged commit a07ca1a into kubevirt:main Jan 17, 2025
5 checks passed

alromeros mentioned this pull request Jan 20, 2025

[release-v0.7] Generate new firmware UUID during restore #323

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate new firmware UUID during restore #317

Generate new firmware UUID during restore #317

alromeros commented Jan 7, 2025

alromeros commented Jan 7, 2025

mhenriks Jan 7, 2025 •

edited

Loading

alromeros Jan 7, 2025

mhenriks Jan 7, 2025

alromeros Jan 8, 2025

mhenriks Jan 8, 2025

alromeros Jan 9, 2025

mhenriks Jan 9, 2025

alromeros Jan 9, 2025

mhenriks Jan 9, 2025 •

edited

Loading

alromeros Jan 9, 2025

ShellyKa13 Jan 8, 2025 •

edited

Loading

alromeros Jan 8, 2025

alromeros Jan 8, 2025

dasionov commented Jan 9, 2025

ShellyKa13 Jan 15, 2025

alromeros Jan 15, 2025

ShellyKa13 left a comment

kubevirt-bot commented Jan 15, 2025

mhenriks commented Jan 17, 2025

alromeros commented Jan 17, 2025

kubevirt-bot commented Jan 17, 2025

Generate new firmware UUID during restore #317

Generate new firmware UUID during restore #317

Conversation

alromeros commented Jan 7, 2025

alromeros commented Jan 7, 2025

mhenriks Jan 7, 2025 • 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

mhenriks Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyKa13 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasionov commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyKa13 left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jan 15, 2025

mhenriks commented Jan 17, 2025

alromeros commented Jan 17, 2025

kubevirt-bot commented Jan 17, 2025

mhenriks Jan 7, 2025 •

edited

Loading

mhenriks Jan 9, 2025 •

edited

Loading

ShellyKa13 Jan 8, 2025 •

edited

Loading