Federico Simoncelli has posted comments on this change.
Change subject: core: add the image block alignment scan
......................................................................
Patch Set 5: (15 inline comments)
....................................................
File backend/manager/dbscripts/upgrade/03_02_0480_base_disks_alignment.sql
Line 1: select fn_db_add_column('base_disks', 'alignment', 'smallint default
0');
Line 2: select fn_db_add_column('base_disks', 'last_alignment_scan', 'timestamp
with time zone');
Line 3: insert into action_version_map values (231, '3.3', '*');
Done
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ScanDiskAlignmentCommand.java
Line 82: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_NOT_EXIST);
Line 83: }
Line 84:
Line 85: if (getDiskType() == DiskStorageType.IMAGE) {
Line 86: if (((DiskImage) getDisk()).getImageStatus() !=
ImageStatus.OK) {
The cast is safe (if the disk type is an IMAGE, then it's a DiskImage). About
the format, I wanted to create a section where to put all the additional checks
about the DiskImage (safely casted), but at the moment there's only one.
Line 87: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_LOCKED);
Line 88: }
Line 89: }
Line 90:
Line 99: if (getVdsIdInPool() == null) {
Line 100: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NO_VDS_IN_POOL);
Line 101: }
Line 102:
Line 103: if (!validate(new
StoragePoolValidator(getStoragePoolDao().get(getStoragePoolId())).isUp())) {
no, the error message is already added by CommandBase.validate.
Line 104: return false;
Line 105: }
Line 106:
Line 107: return true;
Line 134: lunParameters.setLunGuid(lunDisk.getLun().getLUN_id());
Line 135:
Line 136: parameters = lunParameters;
Line 137: } else {
Line 138: throw new RuntimeException("Unknown DiskStorageType " +
getDiskType().toString());
Done
Line 139: }
Line 140:
Line 141: Boolean isDiskAligned = (Boolean) runVdsCommand(
Line 142: VDSCommandType.ScanDiskAlignment,
parameters).getReturnValue();
Line 191: protected boolean isImageLockNeeded() {
Line 192: /* In case the volume format is RAW (same as a direct LUN)
the image lock is not
Line 193: * needed since the alignment scan can run without any
interference by a concurrent
Line 194: * running VM.
Line 195: */
This looks like a job for the shared locks then. I'll add that.
Line 196: return (getDiskType() == DiskStorageType.IMAGE &&
Line 197: ((DiskImage) getDisk()).getVolumeFormat() !=
VolumeFormat.RAW);
Line 198: }
Line 199:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskAlignment.java
Line 1: package org.ovirt.engine.core.common.businessentities;
Actually this patch passes the compilation and it works, I'm not sure if you
tried it. That entry is added later on in the second patch when it will be
actually used in the gwt part (not relevant to this patch).
Line 2:
Line 3: import java.util.HashMap;
Line 4: import java.util.Map;
Line 5:
Line 10: Misaligned(2),
Line 11: Aligned(3);
Line 12:
Line 13: private int intValue;
Line 14: final private static Map<Integer, DiskAlignment> mappings;
Done
Line 15:
Line 16: static {
Line 17: mappings = new HashMap<Integer, DiskAlignment>();
Line 18:
Line 12:
Line 13: private int intValue;
Line 14: final private static Map<Integer, DiskAlignment> mappings;
Line 15:
Line 16: static {
I agree but this is a common practice in these enums, see ActionGroup,
AsyncTaskStatusEnum, BootSequence, (many others ~32 classes).
Line 17: mappings = new HashMap<Integer, DiskAlignment>();
Line 18:
Line 19: for (DiskAlignment enumValue : values()) {
Line 20: mappings.put(enumValue.getValue(), enumValue);
Line 28: public int getValue() {
Line 29: return intValue;
Line 30: }
Line 31:
Line 32: public static DiskAlignment forValue(int value) {
I may agree with you but the forValue method is widely used in the rest of the
code, check the other businessentities (e.g.: ActionGroup, DiskStorageType,
many other...)
Line 33: return mappings.get(value);
Line 34: }
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/AbstractBaseDiskRowMapper.java
Line 31: if (!StringUtils.isEmpty(propagateErrors)) {
Line 32:
disk.setPropagateErrors(PropagateErrors.valueOf(propagateErrors));
Line 33: }
Line 34:
Line 35: disk.setShareable(rs.getBoolean("shareable"));
I don't think it's relevant to this patch.
Line 36: disk.setBoot(rs.getBoolean("boot"));
Line 37:
disk.setAlignment(DiskAlignment.forValue(rs.getInt("alignment")));
Line 38:
disk.setLastAlignmentScan(DbFacadeUtils.fromDate(rs.getTimestamp("last_alignment_scan")));
Line 39:
Line 34:
Line 35: disk.setShareable(rs.getBoolean("shareable"));
Line 36: disk.setBoot(rs.getBoolean("boot"));
Line 37:
disk.setAlignment(DiskAlignment.forValue(rs.getInt("alignment")));
Line 38:
disk.setLastAlignmentScan(DbFacadeUtils.fromDate(rs.getTimestamp("last_alignment_scan")));
Yes, I already commented/fixed this in setLastAlignmentScan (and
getLastAlignmentScan too).
Line 39:
Line 40: return disk;
Line 41: }
Line 42:
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/AlignmentScanReturnForXmlRpc.java
Line 4:
Line 5: import org.ovirt.engine.core.vdsbroker.irsbroker.StatusReturnForXmlRpc;
Line 6:
Line 7: public final class AlignmentScanReturnForXmlRpc extends
StatusReturnForXmlRpc {
Line 8: private static final String ALIGNMENT = "alignment";
I'm fine with your comment but here I'm trying to stick to what's common in
these vdsbroker commands. Look at the other classes (CODE = "code", MESSAGE =
"message", STATUS = "status", and so on...)
Line 9:
Line 10: private Map<String, Object> mAlignment;
Line 11:
Line 12: public AlignmentScanReturnForXmlRpc(Map<String, Object> innerMap) {
Line 6:
Line 7: public final class AlignmentScanReturnForXmlRpc extends
StatusReturnForXmlRpc {
Line 8: private static final String ALIGNMENT = "alignment";
Line 9:
Line 10: private Map<String, Object> mAlignment;
Done, but we should consider to change also all the other members in the
vdsbroker classes.
Line 11:
Line 12: public AlignmentScanReturnForXmlRpc(Map<String, Object> innerMap) {
Line 13: super(innerMap);
Line 14: mAlignment = (Map<String, Object>) innerMap.get(ALIGNMENT);
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ScanDiskAlignmentVDSCommand.java
Line 2:
Line 3: import
org.ovirt.engine.core.common.vdscommands.ScanDiskAlignmentVDSCommandParameters;
Line 4:
Line 5: public class ScanDiskAlignmentVDSCommand<P extends
ScanDiskAlignmentVDSCommandParameters> extends VdsBrokerCommand<P> {
Line 6: private AlignmentScanReturnForXmlRpc mDiskAlignment;
Done, but we should consider to change also all the other members in the
vdsbroker classes.
Line 7:
Line 8: public ScanDiskAlignmentVDSCommand(P parameters) {
Line 9: super(parameters);
Line 10: }
....................................................
Commit Message
Line 44:
Line 45: The result (with the scan execution timestamp) is stored in the
Line 46: base_disks table (as other similar properties such as the bootable
Line 47: and shareable flags).
Line 48:
Done
Line 49: Change-Id: I4858b7bbfa453230fcafecfbc5358c715d5d825b
--
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: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
