Liron Aravot has posted comments on this change.
Change subject: core: add the image block alignment scan
......................................................................
Patch Set 4: (18 inline comments)
Fede, what if the LUN is used also as storage domain?
....................................................
File backend/manager/dbscripts/upgrade/03_02_0470_base_disks_alignment.sql
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 agree, we already have disk_image_dynamic ..though we need to think where
should we put it for luns - base_disk might not be the best place
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ScanDiskAlignmentCommand.java
Line 38: private static final long serialVersionUID = -7266894047095142486L;
Line 39:
Line 40: private Disk diskToScan;
Line 41: private List<VM> vmList;
Line 42: private List<PermissionSubject> permsList = null;
the = null might be removed..
Line 43:
Line 44: public ScanDiskAlignmentCommand(T parameters) {
Line 45: super(parameters);
Line 46: }
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);
+1, BTW - fede, perhaps we want to lock it anyway? IIUC, isImageLockNeeded()
refers to the DB lock which should be done anyway when performing vdsm calls in
my opinion for better user experience.
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;
I'd suggest return here null or super.getExcl - as the default implementation.
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()) {
+1
Line 63: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_IS_NOT_VM_DISK);
Line 64: }
Line 65:
Line 66: if (getDiskType() == DiskStorageType.IMAGE) {
Line 68:
Line 69: if (diskImage.getImageStatus() != ImageStatus.OK) {
Line 70: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_LOCKED);
Line 71: }
Line 72: }
fede, what if the lun is also used as a storage domain? seems to me that it can
cause issues.
Line 73:
Line 74: if (isImageLockNeeded()) {
Line 75: for (VM vm : getVms()) {
Line 76: if (vm.isRunningOrPaused()) {
Line 70: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_LOCKED);
Line 71: }
Line 72: }
Line 73:
Line 74: if (isImageLockNeeded()) {
can you please elaborate on why this part related to the need lock method?
Line 75: for (VM vm : getVms()) {
Line 76: if (vm.isRunningOrPaused()) {
Line 77: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_RUNNING);
Line 78: }
Line 72: }
Line 73:
Line 74: if (isImageLockNeeded()) {
Line 75: for (VM vm : getVms()) {
Line 76: if (vm.isRunningOrPaused()) {
no problem in case we run vm exactly after this is executed?
Line 77: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_RUNNING);
Line 78: }
Line 79: }
Line 80: }
Line 77: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_RUNNING);
Line 78: }
Line 79: }
Line 80: }
Line 81:
shouldn't we check also the domain/pool status in the CDA method?
Line 82: return true;
Line 83: }
Line 84:
Line 85: @Override
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();
Allon, this lock is in the DB will reflect to the user that the image is locked
which is good in my opinion in terms of user experience (will appear as locked
in the gui) - the in memory lock is done regardless, in my opinion this should
stay.
Line 96:
Line 97: if (getDiskType() == DiskStorageType.IMAGE) {
Line 98: DiskImage diskImage = (DiskImage) getDisk();
Line 99:
Line 122: }
Line 123:
Line 124: Boolean isDiskAligned = (Boolean)
getBackend().getResourceManager()
Line 125: .RunVdsCommand(VDSCommandType.ScanDiskAlignment,
parameters).getReturnValue();
Line 126:
can you use runVdsCommand instead of Backend.get.. ?
Line 127: getDisk().setAlignment(isDiskAligned ? DiskAlignment.Aligned
: DiskAlignment.Misaligned);
Line 128: getDisk().setLastAlignmentScan(new Date());
Line 129:
Line 130: getBaseDiskDao().update(getDisk());
Line 144: return permsList;
Line 145: }
Line 146:
Line 147: protected void acquireDiskLocks() {
Line 148: if (isImageLockNeeded()) {
I think that the lock should be acquired for each image disk, vdsm call might
take few minutes so we will provide better user experience.
Line 149: final DiskImage diskImage = (DiskImage) getDisk();
Line 150: final ImageStatus imgStatus = diskImage.getImageStatus();
Line 151:
Line 152: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
Line 171: }
Line 172:
Line 173: protected boolean isImageLockNeeded() {
Line 174: return (getDiskType() == DiskStorageType.IMAGE &&
Line 175: ((DiskImage) getDisk()).getVolumeFormat() !=
VolumeFormat.RAW);
+1
Line 176: }
Line 177:
Line 178: protected Guid getVdsInPool(Guid poolId) {
Line 179: return getVdsDAO().getAllForStoragePoolAndStatus(poolId,
VDSStatus.Up).get(0).getId();
Line 175: ((DiskImage) getDisk()).getVolumeFormat() !=
VolumeFormat.RAW);
Line 176: }
Line 177:
Line 178: protected Guid getVdsInPool(Guid poolId) {
Line 179: return getVdsDAO().getAllForStoragePoolAndStatus(poolId,
VDSStatus.Up).get(0).getId();
there's a possible NPE here..
Line 180: }
Line 181:
Line 182: protected DiskDao getDiskDao() {
Line 183: return getDbFacade().getDiskDao();
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskAlignment.java
Line 5:
Line 6: public enum DiskAlignment {
Line 7:
Line 8: Unknown(0),
Line 9: NotApplicable(1),
is this one used somewhere?
Line 10: Misaligned(2),
Line 11: Aligned(3);
Line 12:
Line 13: private int intValue;
Line 10: Misaligned(2),
Line 11: Aligned(3);
Line 12:
Line 13: private int intValue;
Line 14: private static Map<Integer, DiskAlignment> mappings;
please use the final modifier here.
Line 15:
Line 16: static {
Line 17: mappings = new HashMap<Integer, DiskAlignment>();
Line 18:
Line 13: private int intValue;
Line 14: private static Map<Integer, DiskAlignment> mappings;
Line 15:
Line 16: static {
Line 17: mappings = new HashMap<Integer, DiskAlignment>();
please use EnumMap instead
Line 18:
Line 19: for (DiskAlignment enumValue : values()) {
Line 20: mappings.put(enumValue.getValue(), enumValue);
Line 21: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ScanDiskLunAlignmentVDSCommandParameters.java
Line 6: import org.ovirt.engine.core.compat.*;
Line 7:
Line 8:
Line 9: public class ScanDiskLunAlignmentVDSCommandParameters extends
ScanDiskAlignmentVDSCommandParameters {
Line 10: private String lunGuid;
in my opinion this should be Guid, when creating the map you could use toString.
Line 11:
Line 12: public ScanDiskLunAlignmentVDSCommandParameters(Guid vdsId, Guid
vmId) {
Line 13: super(vdsId, vmId);
Line 14: }
--
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: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
