Liron Ar has posted comments on this change.
Change subject: core, restapi: Introducing support for attaching disk snapshot
......................................................................
Patch Set 27:
(22 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java
Line 202: if (diskImagesFromConfiguration == null) {
Line 203: diskImagesFromConfiguration =
Line 204:
ImagesHandler.filterImageDisks(vmFromConfiguration.getDiskMap().values(),
Line 205: false,
Line 206: true, true);
Done
Line 207: }
Line 208: return diskImagesFromConfiguration;
Line 209: }
Line 210:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java
Line 42: private Disk disk;
Line 43:
Line 44: public AttachDiskToVmCommand(T parameters) {
Line 45: super(parameters);
Line 46: disk = loadDisk((Guid)
getParameters().getEntityInfo().getId());
I agree, but that's a change unrelated to this patch :)
Line 47: }
Line 48:
Line 49: protected AttachDiskToVmCommand(T parameters, Disk disk) {
Line 50: super(parameters);
Line 56: if (isOperationPerformedOnDiskSnapshot()
Line 57: &&
(!validate(getSnapshotsValidator().snapshotExists(getSnapshot())) ||
!validate(getSnapshotsValidator().snapshotTypeSupported(getSnapshot(),
Line 58:
Collections.singletonList(SnapshotType.REGULAR))))) {
Line 59: return false;
Line 60: }
This validation checks for the snapshot existence and type so this is a non
issue IMO, changed as you preferred.
Line 61:
Line 62: if (disk == null) {
Line 63: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IMAGE_DOES_NOT_EXIST);
Line 64: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachDiskFromVmCommand.java
Line 57: }
Line 58:
Line 59: // Check if disk has no snapshots before detaching it.
Line 60: if (retValue && DiskStorageType.IMAGE ==
disk.getDiskStorageType()) {
Line 61: // if a disk snapshot is attached, it can be detached as
it's snapshots are snapshots taken to it's "original"
Changed to:
a "regular" disk cannot be detached if it's part of the vm snapshots
when a disk snapshot is being detached, it will always be part of snapshots -
but of it's "original" vm, therefore for attached disk snapshot it shouldn't be
checked whether it has snapshots or not.
Line 62: // vm.
Line 63: if (vmDevice.getSnapshotId() == null &&
getDiskImageDao().getAllSnapshotsForImageGroup(disk.getId()).size() > 1) {
Line 64:
addCanDoActionMessage(VdcBllMessages.ERROR_CANNOT_DETACH_DISK_WITH_SNAPSHOT);
Line 65: retValue = false;
Line 61: // if a disk snapshot is attached, it can be detached as
it's snapshots are snapshots taken to it's "original"
Line 62: // vm.
Line 63: if (vmDevice.getSnapshotId() == null &&
getDiskImageDao().getAllSnapshotsForImageGroup(disk.getId()).size() > 1) {
Line 64:
addCanDoActionMessage(VdcBllMessages.ERROR_CANNOT_DETACH_DISK_WITH_SNAPSHOT);
Line 65: retValue = false;
Done
Line 66: }
Line 67: }
Line 68: return retValue;
Line 69: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExtendImageSizeCommand.java
Line 202: List<Pair<VM, VmDevice>> attachedVmsInfo =
getVmDAO().getVmsWithPlugInfo(getImage().getId());
Line 203: vmsDiskPluggedTo = new LinkedList<>();
Line 204:
Line 205: for (Pair<VM, VmDevice> pair : attachedVmsInfo) {
Line 206: if (pair.getSecond().getIsPlugged() == Boolean.TRUE
&& pair.getSecond().getSnapshotId() == null) {
Done
Line 207: vmsDiskPluggedTo.add(pair.getFirst());
Line 208: }
Line 209: }
Line 210: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java
Line 51: getParameters().isFiltered());
Line 52:
Line 53: if (disksVmDevices.isEmpty()) {
Line 54: return Collections.emptyMap();
Line 55: }
in this case i don't create a new Hashmap..a matter of preference i assume
Line 56:
Line 57: Map<Guid, VmDevice> toReturn = new HashMap<>();
Line 58: for (VmDevice diskVmDevice : disksVmDevices) {
Line 59: toReturn.put(diskVmDevice.getDeviceId(), diskVmDevice);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java
Line 38: if (images == null) {
Line 39: images =
Line 40:
ImagesHandler.filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(getVmId()),
Line 41: true,
Line 42: false, true);
Done
Line 43: }
Line 44: for (DiskImage image : images) {
Line 45: if (Boolean.TRUE.equals(image.getActive())) {
Line 46: mImagesToBeRemoved.add(image.getImageId());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java
Line 86: private boolean removeVm() {
Line 87: final List<DiskImage> diskImages =
ImagesHandler.filterImageDisks(getVm().getDiskList(),
Line 88: true,
Line 89: false, true);
Line 90:
Done
Line 91: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
Line 92: @Override
Line 93: public Void runInTransaction() {
Line 94: removeVmFromDb();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmTemplateCommand.java
Line 135: } else {
Line 136: List<DiskImage> vmDIsks =
Line 137:
ImagesHandler.filterImageDisks(getDbFacade().getDiskDao().getAllForVm(vm.getId()),
Line 138: false,
Line 139: false, true);
Done
Line 140: Set<Guid> domainsIds =
getStorageDoaminsByDisks(vmDIsks, false);
Line 141: for (Guid domainId : domainsIds) {
Line 142: if
(!getParameters().getStorageDomainsList().contains(domainId)) {
Line 143: problematicVmNames.add(vm.getName());
Line 159: if (imageTemplates == null) {
Line 160: imageTemplates =
Line 161:
ImagesHandler.filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(getVmTemplateId()),
Line 162: false,
Line 163: false, true);
Done
Line 164: }
Line 165: }
Line 166:
Line 167: /**
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SPMAsyncTask.java
Line 403:
Line 404: public void stopTask() {
Line 405: stopTask(false);
Line 406: }
Line 407:
Done
Line 408: public void clearAsyncTask() {
Line 409: // if we are calling updateTask on a task which has not been
submitted,
Line 410: // to vdsm there is no need to clear the task. The task is
just deleted
Line 411: // from the database
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 48: implements QuotaStorageDependent {
Line 49:
Line 50: private List<PermissionSubject> listPermissionSubjects;
Line 51: private Map<Guid, List<Disk>> otherVmDisks = new HashMap<Guid,
List<Disk>>();
Line 52: // vms that the disk (active image) is plugged to
Done
Line 53: private List<VM> vmsDiskPluggedTo;
Line 54: // vms that a snapshot of the disk is plugged to.
Line 55: private List<VM> vmsDiskSnapshotPluggedTo;
Line 56: // vms that a disk or it's sansphot is plugged to.
Line 60: private Disk oldDisk;
Line 61:
Line 62: public UpdateVmDiskCommand(T parameters) {
Line 63: super(parameters);
Line 64: loadVmDiskAttachedToInfo();
this method populates 4 lists, what do you suggest the getter to return? the
only option i see is a map and then an enum to get the needed list by the
caller would be required..the solution i prefer is an init method for commands
that'll run after ctor. i'll push a follow up patch to add that method, as we
have dao calls in the ctor of many commands - i think that for this patch it's
good enough, let me know what's your take on that.
Line 65: }
Line 66:
Line 67: /**
Line 68: * This constructor is mandatory for activation of the
compensation process
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
Line 252
Line 253
Line 254
Line 255
Line 256
because now the vm may have attached disks that are snapshot disks (the image
isn't the active one)
Line 251: }
Line 252:
Line 253: private static void clearVmDisks(VM vm) {
Line 254: vm.getDiskList().clear();
Line 255: vm.getDiskMap().clear();
Done
Line 256: }
Line 257:
Line 258: public static void updateDisksForVm(VM vm, Collection<? extends
Disk> diskList) {
Line 259: for (Disk disk : diskList) {
Line 269:
Line 270: public static void filterDisksForVM(VM vm,
Line 271: boolean
allowOnlyNotShareableDisks,
Line 272: boolean
allowOnlySnapableDisks,
Line 273: boolean
allowOnlyActiveDisks) {
Done
Line 274: List<DiskImage> filteredDisks =
ImagesHandler.filterImageDisks(vm.getDiskList(), allowOnlyNotShareableDisks,
allowOnlySnapableDisks, allowOnlyActiveDisks);
Line 275: Collection<DiskImage> vmDisksToRemove =
CollectionUtils.subtract(vm.getDiskList(), filteredDisks);
Line 276: // done so that lun disks would be included as well
Line 277: Collection<Disk> vmDisksAfterFilter =
CollectionUtils.disjunction(vm.getDiskMap().values(), vmDisksToRemove);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsValidator.java
Line 93: public ValidationResult snapshotExists(Snapshot snapshot) {
Line 94: return createSnapshotExistsResult(snapshot != null);
Line 95: }
Line 96:
Line 97: /***
Done
Line 98: * Checks if the given snapshot's tyoe is within the given types.
Line 99: * @param snapshot
Line 100: * @param supportedtypes
Line 101: * @return
Line 94: return createSnapshotExistsResult(snapshot != null);
Line 95: }
Line 96:
Line 97: /***
Line 98: * Checks if the given snapshot's tyoe is within the given types.
Done
Line 99: * @param snapshot
Line 100: * @param supportedtypes
Line 101: * @return
Line 102: */
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskImagesValidator.java
Line 109: for (VmDevice device : devices) {
Line 110: if (device.getSnapshotId() != null) {
Line 111: VM vm = getVmDAO().get(device.getVmId());
Line 112: Snapshot snapshot =
getSnapshotDAO().get(device.getSnapshotId());
Line 113:
pluggedDiskSnapshotInfo.add(diskImage.getDiskAlias()+" (from Snapshot: " +
snapshot.getDescription() + " VM plugged to: " + vm.getName() + ")" + "\n");
Done
Line 114: }
Line 115: }
Line 116: }
Line 117:
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilderBase.java
Line 230: if (disk1.isBoot() == disk2.isBoot()) {
Line 231: return 0;
Line 232: }
Line 233:
Line 234: return disk1.isBoot() ? 1 : -1;
replaced with BooleanComparator instead.
Line 235: }
Line 236: });
Line 237: return diskImages;
Line 238: }
....................................................
File packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 206: select fn_db_add_config_value('NetworkQosSupported','false','3.2');
Line 207: select
fn_db_add_config_value('HotPlugDiskSnapshotSupported','false','3.0');
Line 208: select
fn_db_add_config_value('HotPlugDiskSnapshotSupported','false','3.1');
Line 209: select
fn_db_add_config_value('HotPlugDiskSnapshotSupported','false','3.2');
Line 210: select
fn_db_add_config_value('HotPlugDiskSnapshotSupported','true','3.3');
AFAIK i need, take a look on the other config values..it's present for all of
them
Line 211:
Line 212: -- by default use no proxy
Line 213: select fn_db_add_config_value('SpiceProxyDefault','','general');
Line 214:
--
To view, visit http://gerrit.ovirt.org/17679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02579bf1a91cd294a5040acf432f1fdb87eb18c1
Gerrit-PatchSet: 27
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
