clear MAC addresses on restore #235
Conversation
kubevirt-bot
added
the
dco-signoff: no
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @esteban-ee. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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/test-infra repository. |
pkg/plugin/vm_restore_item_action.go
Outdated
| @@ -63,6 +63,10 @@ func (p *VMRestorePlugin) Execute(input *velero.RestoreItemActionExecuteInput) ( | |||
| return nil, errors.WithStack(err) | |||
| } | |||
|
|
|||
| for i := 0; i < len(vm.Spec.Template.Spec.Domain.Devices.Interfaces); i++ { | |||
| vm.Spec.Template.Spec.Domain.Devices.Interfaces[i].MacAddress = "" | |||
There was a problem hiding this comment.
What about users that want to preserve the mac address?
I think a better solution would be to add an annotation to the VM like backup.kubevirt.io/clearMacAddress: true and if that's set, the plugin will clear the mac
There was a problem hiding this comment.
right, added annotation check
Signed-off-by: Eduardo Esteban <[email protected]>
Signed-off-by: Eduardo Esteban <[email protected]>
esteban-ee
force-pushed
the
clear-macaddress
branch
from
b5f03a8 to
3861c10
Compare
kubevirt-bot
added
dco-signoff: yes
|
I think the approach in #248 is preferable. When restoring to existing namespace, preserve macs otherwise clear |
|
I believe it's best to check for MAC address conflicts before restoration. If a VM is being restored into a different namespace and the original VM no longer exists in its original namespace, would it still be necessary to clear the MAC addresses? |
kubevirt-bot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
kubevirt-bot
added
the
lifecycle/stale
| @@ -365,3 +366,12 @@ func AddVMIObjectGraph(spec v1.VirtualMachineInstanceSpec, namespace string, ext | |||
| return extra | |||
|
|
|||
| } | |||
|
|
|||
| func IsMacAdressClearedByAnnotation(vm *v1.VirtualMachine) (bool) { | |||
| annotations := vm.GetAnnotations() | |||
There was a problem hiding this comment.
It may make more sense to have the annotation in the restore object so the decision about clearing mac address out can be made retroactively.
There was a problem hiding this comment.
Hey @esteban-ee, many thanks for the contribution! I'm interested in merging this but there are a couple of things we should consider first. Are you still working on this?
| @@ -28,6 +28,7 @@ import ( | |||
| ) | |||
|
|
|||
| const VELERO_EXCLUDE_LABEL = "velero.io/exclude-from-backup" | |||
| const CLEAR_MAC_ADDRESS_ANNOTATION = "restore.kubevirt.io/clear-mac-address" | |||
There was a problem hiding this comment.
I'd also say to support a label too since currently, velero client only allows adding labels to the backup/restore objects during creation. It'd be easier for users to also allow a label and we can deprecate it once the client support annotations (PR is already merged).
|
#304 was merged so it's appropriate to close this PR. @esteban-ee thank you for your contribution, I cherry-picked your commits so the contribution is still in the code base. Thanks! |
|
@alromeros: Closed this PR. In response to this:
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. |
What this PR does / why we need it:
When a virtual machine is restored to an alternate namespace, the virtual machine is restored with the same MAC address as the original virtual machine. This results in a MAC address conflict when the original virtual machine is still running on the original namespace.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
#199
Special notes for your reviewer:
Release note: