Maor Lipchuk has posted comments on this change.
Change subject: core: Add snapshot validation for empty guid.
......................................................................
Patch Set 1: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java
Line 298: }
Line 299:
Line 300: @Override
Line 301: protected boolean canDoAction() {
Line 302: if (Guid.Empty.equals(getParameters().getDstSnapshotId())) {
I also thought about it but I preferred not to mix those values together.
Empty GUID is a bug for sure, there is no scenario that we initialize an empty
GUID.
Null GUID on the other hand is used when we detach a disk.
I know that detached disk should not be related to snapshot operation, but
there is also no reason to check it here, besides the CDA message is not
related and could be confusing and also looking forward we might want to
support single disk snapshot.
Line 303: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CORRUPTED_VM_SNAPSHOT_ID);
Line 304: }
Line 305:
Line 306: SnapshotsValidator snapshotValidator =
createSnapshotValidator();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TryBackToAllSnapshotsOfVmCommand.java
Line 198: }
Line 199:
Line 200: @Override
Line 201: protected boolean canDoAction() {
Line 202: if (Guid.Empty.equals(getParameters().getDstSnapshotId())) {
I also thought about it but I preferred not to mix those values together.
Empty GUID is a bug for sure, there is no scenario that we initialize an empty
GUID.
Null GUID on the other hand is used when we detach a disk.
I know that detached disk should not be related to snapshot operation, but
there is also no reason to check it here, besides the CDA message is not
related and could be confusing and also looking forward we might want to
support single disk snapshot.
Line 203: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CORRUPTED_VM_SNAPSHOT_ID);
Line 204: }
Line 205:
Line 206: Snapshot snapshot =
getSnapshotDao().get(getParameters().getDstSnapshotId());
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 12: VMT_CANNOT_REMOVE_DETECTED_DERIVED_VM=Cannot delete Template. Template
is being used by the following VMs: ${vmsList}.
Line 13: VMT_CANNOT_REMOVE_DOMAINS_LIST_MISMATCH=Cannot delete Template. The
Template does not exist on the following Storage Domains:
${domainsList}.\nEither verify that Template exists on all Storage Domains
listed on the domains list,\nor do not send domains list in order to delete all
instances of the Template from the system.
Line 14: ACTION_TYPE_FAILED_VM_IMAGE_DOES_NOT_EXIST=Cannot ${action} ${type}.
VM's Image does not exist.
Line 15: ACTION_TYPE_FAILED_VM_SNAPSHOT_DOES_NOT_EXIST=Cannot ${action}
${type}. VM's Snapshot does not exist.
Line 16: ACTION_TYPE_FAILED_CORRUPTED_VM_SNAPSHOT_ID=Cannot ${action} ${type}.
The snapshot configuration has been corrupted (snapshot id is empty). Please
contact the system administrator.
done for the switch comment.
This message was discussed in the bug, I saw that other messages used it also.
If you think of a better alternative I will be happy to re-think about it.
Line 17: ACTION_TYPE_FAILED_VM_SNAPSHOT_HAS_NO_CONFIGURATION=Cannot ${action}
${type}. The snapshot ${SnapshotName} of VM ${VmName} has no configuration
available. Please choose a snapshot with configuration available.
Line 18: ACTION_TYPE_FAILED_VM_SNAPSHOT_IS_BROKEN=Cannot ${action} ${type}. The
snapshot is broken, and no further work can be done on it. Please remove this
snapshot from the VM.
Line 19: IMAGE_REPOSITORY_NOT_FOUND=Storage Domain cannot be accessed.\n\
Line 20: Possible reasons:\n\
....................................................
Commit Message
Line 7: core: Add snapshot validation for empty guid.
Line 8:
Line 9: Currently when virt-v2v is used to convert a vm from a foreign
Line 10: hypervisor (Xen, VMware and etc), it sets the vm_snapshot_id of
Line 11: all disks of that vm to empty guid.
It is related to a virt-v2v bug, which was already solved upstream
Line 12: This later causes issues doing different tasks on snapshots.
Line 13:
Line 14: The proposed fix is to add a validation whether the
Line 15: snapshot id is empty and provides an appropriate message.
Line 11: all disks of that vm to empty guid.
Line 12: This later causes issues doing different tasks on snapshots.
Line 13:
Line 14: The proposed fix is to add a validation whether the
Line 15: snapshot id is empty and provides an appropriate message.
I assume you are referring to the virt-v2v bug.
We can't be sure if the user already imported the VM with the virt-v2v before
the fix and it is already in the setup.
Line 16:
Line 17: Change-Id: I19bc2982adecca198f8dc38ca011f4528dd58db9
--
To view, visit http://gerrit.ovirt.org/15701
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I19bc2982adecca198f8dc38ca011f4528dd58db9
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Cheryn Tan <cheryn...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
