Alissa Bonas has posted comments on this change.
Change subject: engine: Enable online virtual drive resize
......................................................................
Patch Set 2: (13 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExtendImageSizeCommand.java
Line 24:
Line 25: import java.util.ArrayList;
Line 26: import java.util.List;
Line 27:
Line 28: public class ExtendImageSizeCommand<T extends
ExtendImageSizeParameters> extends BaseImagesCommand<T> {
You also need to override the method setActionMessageParameters and set there
the correct VAR__ACTION and VAR_TYPE so in case of failure a correct message
will be displayed. (It fills the "Cannot ${action} ${type}." message that is
added in AppErrors.properties)
Line 29:
Line 30: private List<PermissionSubject> permissionsList;
Line 31:
Line 32: public ExtendImageSizeCommand(T parameters) {
Line 40:
Line 41: @Override
Line 42: protected void executeCommand() {
Line 43: DiskImage diskImage = getImage();
Line 44:
getParameters().setStoragePoolId(diskImage.getStoragePoolId().getValue());
Why are class properties of parameters class set here? If this is something
that is never filled by the calling side to the command ,why not define them
and use just inside the command class itself? It can be confusing to the
caller to see this object and wrongly conclude he needs to fill those
parameters. If there is actually a need to fill those parameters from outside,
then there are wrongly overriden here, and it's better to have a validation +
appropriate error should be thrown if they are missing.
Line 45:
getParameters().setStorageDomainId(diskImage.getStorageIds().get(0));
Line 46: getParameters().setImageGroupID(diskImage.getId());
Line 47:
Line 48: lockImage();
Line 81: log.warnFormat("Failed to extend size of the volume '${0}',
user: '${1}'", getImage().getId(), getUserName());
Line 82: addCustomValue("DiskId", getImageId().toString());
Line 83: addCustomValue("UserName", getUserName());
Line 84: AuditLogDirector.log(this,
AuditLogType.USER_EXTEND_DISK_SIZE_FAILURE);
Line 85: setSucceeded(true);
if the command ended with failure, why is the setSucceeded true?
Also, what are the implications of a failed extend size - is the disk in
"healthy" condition after this operation?
Line 86: }
Line 87:
Line 88: private void updateImageSize(DiskImage diskImage) {
Line 89: getImageDao().updateImageSize(diskImage.getId(),
diskImage.getSize(), diskImage.getLastModifiedDate());
Line 137: }
Line 138:
Line 139: private boolean isImageExist() {
Line 140: if (getImage() == null || getImage().getDiskStorageType() !=
Disk.DiskStorageType.IMAGE) {
Line 141:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_NOT_EXIST);
instead of using addCanDoActionMessage and return false, you can use
failCanDoAction method that does both.
Line 142: return false;
Line 143: }
Line 144: return true;
Line 145: }
Line 143: }
Line 144: return true;
Line 145: }
Line 146:
Line 147: private boolean isExtendCurrentSize() {
I would rename the method to imply that it does comparison of new/old sizes.
Something like validateNewSize or is new size legal.
Line 148: long currentSize = getImage().getSize();
Line 149: if (currentSize > getParameters().getNewSize()) {
Line 150:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_SHRINK_IS_NOT_SUPPORTED);
Line 151: return false;
Line 147: private boolean isExtendCurrentSize() {
Line 148: long currentSize = getImage().getSize();
Line 149: if (currentSize > getParameters().getNewSize()) {
Line 150:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_SHRINK_IS_NOT_SUPPORTED);
Line 151: return false;
instead of using addCanDoActionMessage and return false, you can use
failCanDoAction method that does both.
Line 152: }
Line 153: return true;
Line 154: }
Line 155:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ExtendImageSizeParameters.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 ExtendImageSizeParameters extends
ImagesActionsParametersBase {
Why is there a need for new class just to keep the size?
Line 6:
Line 7: private long newSize;
Line 8:
Line 9: public ExtendImageSizeParameters() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 210: AddPosixFsStorageDomain(1009, ActionGroup.CREATE_STORAGE_DOMAIN,
QuotaDependency.NONE),
Line 211: LiveMigrateDisk(1010, QuotaDependency.NONE),
Line 212: LiveMigrateVmDisks(1011, false, QuotaDependency.STORAGE),
Line 213: MoveDisks(1012, false, QuotaDependency.NONE),
Line 214: ExtendImageSize(1013, false, QuotaDependency.NONE),
Why is the permission group set to false? Doesn't it need permissions to change
image size?
Line 215: // Event Notification
Line 216: AddEventSubscription(1100, QuotaDependency.NONE),
Line 217: RemoveEventSubscription(1101, QuotaDependency.NONE),
Line 218:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ExtendVmDiskSizeVDSCommandParameters.java
Line 16: public ExtendVmDiskSizeVDSCommandParameters() {
Line 17: super();
Line 18: }
Line 19:
Line 20: public ExtendVmDiskSizeVDSCommandParameters(Guid vdsId, Guid vmId,
long newSize) {
why are there storagePoolId , imageGroupId and storageDomainId members in
class, but they are not set in this constructor?
Line 21: super(vdsId, vmId);
Line 22: this.newSize = newSize;
Line 23: }
Line 24:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java
Line 14: public void updateQuotaForImageAndSnapshots(Guid imageGroupId,
NGuid quotaId);
Line 15:
Line 16: public void updateImageVmSnapshotId(Guid id, Guid vmSnapshotId);
Line 17:
Line 18: public void updateImageSize(Guid id, long size, Date lastModified);
I'd rename the parameter to explain more clear id of what entity is this.
Also, please add unitests to the new dao function.
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 204: ACTION_TYPE_FAILED_STORAGE_DOMAIN_ALREADY_EXIST=Cannot ${action}
${type}. The Storage Domain already exists.
Line 205: ACTION_TYPE_FAILED_STORAGE_POOL_NAME_ALREADY_EXIST=Cannot ${action}
${type}. The Data Center name is already in use.
Line 206: ACTION_TYPE_FAILED_TEMPLATE_NOT_FOUND_ON_DESTINATION_DOMAIN=Cannot
${action} ${type}. The selected Storage Domain does not contain the VM Template.
Line 207: ACTION_TYPE_FAILED_NO_VDS_IN_POOL=Cannot ${action} ${type}. There is
no active Host in the Data Center.
Line 208: ACTION_TYPE_FAILED_SHRINK_IS_NOT_SUPPORTED=Cannot ${action} ${type}.
The new size should be greater than the current.
There are 2 more AppErrors.properties files where the same message should be
written as well.
Also, there is AppErrors.java file where it should be added too.
And what about the resize vm failure messages that are added in audit log?
Shouldn't it be here too? Please explain.
Line 209: VAR__TYPE__HOST=$type Host
Line 210: VAR__ENTITIES__HOSTS=$entities hosts
Line 211: VAR__TYPE__NETWORK=$type Network
Line 212: VAR__TYPE__NETWORKS=$type Networks
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 159: USER_TRY_BACK_TO_SNAPSHOT_FINISH_FAILURE=Failed to complete
Snapshot-Preview ${SnapshotName} for VM ${VmName}.
Line 160: USER_UPDATE_BOOKMARK=Bookmark ${BookmarkName} was updated by
${UserName}.
Line 161: USER_UPDATE_BOOKMARK_FAILED=Failed to update bookmark:
${BookmarkName} (User: ${UserName})
Line 162: USER_EXTEND_DISK_SIZE_UPDATE_VM_FAILURE=Failed to update VM {VmName}
with the new volume size. VM restart is recommended.
Line 163: USER_EXTEND_DISK_SIZE_FAILED=Failed to extend disk size ${DiskId}.
(User: ${UserName})
I think it's better to say "Failed to extend size of disk <diskId>" , otherwise
it sounds like the diskId is the size. And why here username is mentioned in
the message, but not in the vm failure message (the other new message) ?
Line 164: USER_UPDATE_VDS=Host ${VdsName} parameters were updated by
${UserName}.
Line 165: USER_UPDATE_VM=VM ${VmName} parameters were updated by ${UserName}.
Line 166: USER_UPDATE_VM_CLUSTER_DEFAULT_HOST_CLEARED=${VmName} cluster was
updated by ${UserName}, Default host was reset to auto assign.
Line 167: USER_UPDATE_VM_POOL=VM Pool ${VmPoolName} parameters were updated by
${UserName}.
....................................................
Commit Message
Line 5: CommitDate: 2013-05-26 08:51:35 +0300
Line 6:
Line 7: engine: Enable online virtual drive resize
Line 8:
Line 9: Currently there is no option to extend virtual disk size after it's
it's creation--> its creation.
It seems from the patch that this is only backend support with no UI - correct
me if I misunderstood.
If this is the case, this patch contains new code that can't be used. Please
add either UI or REST relevant code so the new functionality can be used.
Does this patch contain code that assumes that there is a new api that doesn't
exist in all vdsm versions? It can create a problem for existing users with
older vdsm.
if this patch is related to bug or RFE (enhancement request), please state the
relevant bugzilla id in commit message.
Also, the patch is completely missing unitests - please add.
Line 10: creation. This patch is providing such option from the engine side.
Line 11:
Line 12: Change-Id: Ie702348a68a26ac02a01f66aaa1ea42c2c675ebb
--
To view, visit http://gerrit.ovirt.org/14975
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie702348a68a26ac02a01f66aaa1ea42c2c675ebb
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
