Allon Mureinik has posted comments on this change.
Change subject: core, restapi: Introducing support for attaching disk snapshot
......................................................................
Patch Set 27:
(26 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);
please adhere to the given format - this new "true" should be on it's own line.
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());
this cast looks redundant.
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: }
seems as though this should be done much later, after the basic validations
(like the disk's existence)
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/CreateAllSnapshotsFromVmCommand.java
Line 99: protected List<DiskImage> getDisksList() {
Line 100: if (cachedSelectedActiveDisks == null) {
Line 101: cachedSelectedActiveDisks =
ImagesHandler.filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(getVmId()),
Line 102: true,
Line 103: true, true);
please adhere to the given format - this new "true" should be on it's own line.
Line 104: }
Line 105: return cachedSelectedActiveDisks;
Line 106: }
Line 107:
Line 234:
getSnapshotDao().updateStatus(createdSnapshot.getId(), SnapshotStatus.OK);
Line 235:
Line 236: if (isLiveSnapshotApplicable()) {
Line 237: performLiveSnapshot(createdSnapshot);
Line 238: } else {
unrelated to the patch
Line 239: // If the created snapshot contains memory,
remove the memory volumes as
Line 240: // they are not in use since no live snapshot
was created
Line 241: if
(!createdSnapshot.getMemoryVolume().isEmpty()) {
Line 242: logMemorySavingFailed();
....................................................
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"
I did not understand this comment.
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;
please fix the indentation.
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) {
use equals, not ==
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: }
This is redundant - just return the new HashMap...
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/HotPlugDiskToVmCommand.java
Line 39: }
Line 40:
Line 41: @Override
Line 42: protected boolean canDoAction() {
Line 43: performDbLoads();
really, REALLY, not a fan of loading data from the DB in CDAs.
But I guess it's already there...
Line 44:
Line 45: return
Line 46: isVmExist() &&
Line 47: isVmInUpPausedDownStatus() &&
....................................................
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);
please adhere to the given format - this new "true" should be on it's own line.
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:
please adhere to the given format - this new "true" should be on it's own line.
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);
please adhere to the given format - this new "true" should be on it's own line.
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);
please adhere to the given format - this new "true" should be on it's own line.
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:
why move it?
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
please use javadoc syntax - /** */
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();
why in the ctor instead of a proper getter?
DB code in the ctor is an opening to a world of problems.
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
why is it OK to drop this condition?
Line 251: }
Line 252:
Line 253: private static void clearVmDisks(VM vm) {
Line 254: vm.getDiskList().clear();
Line 255: vm.getDiskMap().clear();
why isn't this a utility method in VM?
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) {
indentation
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: /***
s/***/**/
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.
s/tyoe/type/
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");
use string.format instead of +.
specifically, you should be using %n instead of \n
Line 114: }
Line 115: }
Line 116: }
Line 117:
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskDaoTest.java
Line 189
Line 190
Line 191
Line 192
Line 193
WAT
....................................................
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;
The entire implementation of this method should just be
Boolean.compare(disk1.isBoot(), disk2.isBoot())
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');
IIUC, you don't need to explicitly specify the default value for the current
version.
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
