Allon Mureinik has posted comments on this change.
Change subject: core: add the image block alignment scan
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(18 inline comments)
....................................................
File backend/manager/dbscripts/upgrade/03_02_0470_base_disks_alignment.sql
Line 1: select fn_db_add_column('base_disks', 'alignment', 'smallint');
also add a default value (0), IMHO
Line 1: select fn_db_add_column('base_disks', 'alignment', 'smallint');
Line 2: select fn_db_add_column('base_disks', 'last_alignment_scan', 'timestamp
with time zone');
I'm not sure base disks is the correct place for these columns - do we really
want to perform an alignment scan on a LUN disk we attach?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ScanDiskAlignmentCommand.java
Line 47:
Line 48: @Override
Line 49: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 50: if (isImageLockNeeded()) {
Line 51: return
Collections.singletonMap(getDisk().getId().toString(),
LockMessagesMatchUtil.DISK);
This method is called before canDoAction(), so if getDisk() is indeed null -
which you check in CDA, this will result in a NullPointerException.
Instead, just get the ID from the parameters
Line 52: }
Line 53: return Collections.EMPTY_MAP;
Line 54: }
Line 55:
Line 49: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 50: if (isImageLockNeeded()) {
Line 51: return
Collections.singletonMap(getDisk().getId().toString(),
LockMessagesMatchUtil.DISK);
Line 52: }
Line 53: return Collections.EMPTY_MAP;
should be Collections.emptyMap() [which supports generics]
Line 54: }
Line 55:
Line 56: @Override
Line 57: protected boolean canDoAction() {
Line 58: if (getDisk() == null) {
Line 59: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_NOT_EXIST);
Line 60: }
Line 61:
Line 62: if (getVms() == null || getVms().isEmpty()) {
IIRC, this should never be null
Line 63: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_IS_NOT_VM_DISK);
Line 64: }
Line 65:
Line 66: if (getDiskType() == DiskStorageType.IMAGE) {
Line 83: }
Line 84:
Line 85: @Override
Line 86: protected void executeCommand() {
Line 87: ScanDiskAlignmentVDSCommandParameters parameters;
why is this here?
Line 88:
Line 89: // At the moment we select the first VM. This could become a
live operation
Line 90: // (only relevant for non-shareabale images) and we might want
to pick one
Line 91: // of the running VM.
Line 91: // of the running VM.
Line 92: final VM vm = getVms().get(0);
Line 93: final Guid storagePoolId = vm.getStoragePoolId();
Line 94:
Line 95: acquireDiskLocks();
Locking should be done before the CDA, not in the execute.
And with the new locking mechanism, it should also be persisted.
The getExclusiveLocks should be enough
Line 96:
Line 97: if (getDiskType() == DiskStorageType.IMAGE) {
Line 98: DiskImage diskImage = (DiskImage) getDisk();
Line 99:
Line 121: throw new RuntimeException("Unknown DiskStorageType " +
getDiskType().toString());
Line 122: }
Line 123:
Line 124: Boolean isDiskAligned = (Boolean)
getBackend().getResourceManager()
Line 125: .RunVdsCommand(VDSCommandType.ScanDiskAlignment,
parameters).getReturnValue();
It's a read only disk - it's either aligned or unaligned in all of them.
Line 126:
Line 127: getDisk().setAlignment(isDiskAligned ? DiskAlignment.Aligned
: DiskAlignment.Misaligned);
Line 128: getDisk().setLastAlignmentScan(new Date());
Line 129:
Line 138: if (permsList == null && getDisk() != null) {
Line 139: permsList = new ArrayList<PermissionSubject>();
Line 140: permsList.add(new PermissionSubject(getDisk().getId(),
Line 141: VdcObjectType.Disk,
Line 142: ActionGroup.ATTACH_DISK));
Don't we have a manipulate disk permission?
Line 143: }
Line 144: return permsList;
Line 145: }
Line 146:
Line 171: }
Line 172:
Line 173: protected boolean isImageLockNeeded() {
Line 174: return (getDiskType() == DiskStorageType.IMAGE &&
Line 175: ((DiskImage) getDisk()).getVolumeFormat() !=
VolumeFormat.RAW);
why not always take the lock?
can you explain this mehod?
Line 176: }
Line 177:
Line 178: protected Guid getVdsInPool(Guid poolId) {
Line 179: return getVdsDAO().getAllForStoragePoolAndStatus(poolId,
VDSStatus.Up).get(0).getId();
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ScanDiskAlignmentParameters.java
Line 1: package org.ovirt.engine.core.common.action;
Line 2:
Line 3: import org.ovirt.engine.core.compat.Guid;
Line 4:
Line 5: public class ScanDiskAlignmentParameters extends
VdcActionParametersBase {
If we aren't adding any more data memebers, perhaps this whole class is
redundent?
Line 6: private static final long serialVersionUID = -6587274019503875891L;
Line 7:
Line 8: public ScanDiskAlignmentParameters(Guid diskId) {
Line 9: setEntityId(diskId);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/BaseDisk.java
Line 185: alignment = value;
Line 186: }
Line 187:
Line 188: public Date getLastAlignmentScan() {
Line 189: return lastAlignmentScan;
Date is mutable, and you'd be providing a reference to the calss's member.
This should be cloned (or just you unixtime instead)
Line 190: }
Line 191:
Line 192: public void setLastAlignmentScan(Date value) {
Line 193: lastAlignmentScan = value;
Line 189: return lastAlignmentScan;
Line 190: }
Line 191:
Line 192: public void setLastAlignmentScan(Date value) {
Line 193: lastAlignmentScan = value;
same here
Line 194: }
Line 195:
Line 196: @Override
Line 197: public int hashCode() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ScanDiskAlignmentVDSCommandParameters.java
Line 1: package org.ovirt.engine.core.common.vdscommands;
Line 2:
Line 3: import java.util.Map;
Line 4:
Line 5: import org.ovirt.engine.core.compat.*;
Use FQCN imports, not *
Line 6:
Line 7:
Line 8: public abstract class ScanDiskAlignmentVDSCommandParameters extends
VdsAndVmIDVDSParametersBase {
Line 9: public ScanDiskAlignmentVDSCommandParameters(Guid vdsId, Guid vmId)
{
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ScanDiskImageAlignmentVDSCommandParameters.java
Line 2:
Line 3: import java.util.HashMap;
Line 4: import java.util.Map;
Line 5:
Line 6: import org.ovirt.engine.core.compat.*;
Use FQCN imports, not *
Line 7:
Line 8:
Line 9: public class ScanDiskImageAlignmentVDSCommandParameters extends
ScanDiskAlignmentVDSCommandParameters {
Line 10: private Guid poolId, domainId, imageGroupId, imageId;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ScanDiskLunAlignmentVDSCommandParameters.java
Line 2:
Line 3: import java.util.HashMap;
Line 4: import java.util.Map;
Line 5:
Line 6: import org.ovirt.engine.core.compat.*;
Use FQCN imports, not *
Line 7:
Line 8:
Line 9: public class ScanDiskLunAlignmentVDSCommandParameters extends
ScanDiskAlignmentVDSCommandParameters {
Line 10: private String lunGuid;
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/AlignmentScanReturnForXmlRpc.java
Line 1: package org.ovirt.engine.core.vdsbroker.vdsbroker;
Line 2:
Line 3: import java.util.Map;
Line 4:
Use FQCN imports, not *
Line 5: import org.ovirt.engine.core.vdsbroker.irsbroker.*;
Line 6:
Line 7: public final class AlignmentScanReturnForXmlRpc extends
StatusReturnForXmlRpc {
Line 8: private static final String ALIGNMENT = "alignment";
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ScanDiskAlignmentVDSCommand.java
Line 1: package org.ovirt.engine.core.vdsbroker.vdsbroker;
Line 2:
Line 3: import org.ovirt.engine.core.common.asynctasks.*;
Line 4: import org.ovirt.engine.core.common.vdscommands.*;
Line 5: import org.ovirt.engine.core.compat.*;
Use FQCN imports, not *
Line 6:
Line 7: public class ScanDiskAlignmentVDSCommand<P extends
ScanDiskAlignmentVDSCommandParameters> extends VdsBrokerCommand<P> {
Line 8: private AlignmentScanReturnForXmlRpc mDiskAlignment;
Line 9:
--
To view, visit http://gerrit.ovirt.org/11946
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4858b7bbfa453230fcafecfbc5358c715d5d825b
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
