Liron Aravot has posted comments on this change. Change subject: core: perform ovf update for domain when it's deactivated ......................................................................
Patch Set 6: (19 comments) http://gerrit.ovirt.org/#/c/33158/6//COMMIT_MSG Commit Message: Line 12: Line 13: Ovf stores are created for a domain according to the config Line 14: value StorageDomainOvfStoreCount on each OvfDataUpdater run, so the same Line 15: case should be supported on deactivation of domain (OVF stores will be Line 16: created if needed). > remove redundant spaces Done Line 17: Line 18: Change-Id: I54cc929937d3b14dc49a2edab9e763cbe603a9c3 Line 19: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1138124 http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java: Line 736: } Line 737: Line 738: protected boolean hasParentCommand() { Line 739: return getParameters().getParentCommand() != VdcActionType.Unknown; Line 740: } > This is only used for the OVF, won't it be more appropriate to put it in so it's used in multiple commands and it's a general 'utility' method, hence it belongs here imo. Line 741: Line 742: private boolean internalCanDoAction() { Line 743: boolean returnValue = false; Line 744: try { http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateOvfStoresForStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateOvfStoresForStorageDomainCommand.java: Line 18: import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector; Line 19: import org.ovirt.engine.core.dao.StorageDomainOvfInfoDao; Line 20: Line 21: @InternalCommandAttribute Line 22: @NonTransactiveCommandAttribute > I think it will be better it will extend StorageDomainCommandBase as Create let's discuss it out of this patch scope, as the hierarchy wasn't changed in this patch. Line 23: public class CreateOvfStoresForStorageDomainCommand<T extends CreateOvfStoresForStorageDomainCommandParameters> extends CommandBase<T> { Line 24: Line 25: public CreateOvfStoresForStorageDomainCommand(T parameters) { Line 26: this(parameters, null); Line 41: parameters.setParentParameters(getParameters().getParentParameters()); Line 42: } else { Line 43: parameters.setParentCommand(getActionType()); Line 44: parameters.setParentParameters(getParameters()); Line 45: } > Please extract this to a seperate method, called getCreateOvfVolumeForStora Done Line 46: Line 47: VdcReturnValueBase vdcReturnValueBase = Line 48: runInternalAction(VdcActionType.CreateOvfVolumeForStorageDomain, Line 49: parameters); Line 113: private ProcessOvfUpdateForStorageDomainCommandParameters createProcessOvfUpdateForDomainParams() { Line 114: ProcessOvfUpdateForStorageDomainCommandParameters params = new ProcessOvfUpdateForStorageDomainCommandParameters(getParameters().getStoragePoolId(), getParameters().getStorageDomainId()); Line 115: params.setSkipDomainChecks(getParameters().isSkipDomainChecks()); Line 116: params.setParentCommand(getParameters().getParentCommand()); Line 117: params.setSkipDomainChecks(getParameters().isSkipDomainChecks()); > This set was done already in line 115 Done Line 118: params.setParentParameters(getParameters().getParentParameters()); Line 119: return params; Line 120: } Line 121: http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImageSpmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImageSpmCommand.java: Line 53 Line 54 Line 55 Line 56 Line 57 > What about uploadStreamCommand, doesn't it need to verify the storage pool the caller is responsible for doing that. http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunnersFactory.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunnersFactory.java: Line 20: ArrayList<VdcActionParametersBase> parameters, Line 21: boolean isInternal, CommandContext commandContext) { Line 22: MultipleActionsRunner runner; Line 23: switch (actionType) { Line 24: case DeactivateStorageDomainWithOvfUpdate: { > can this (and the GUI part) be seperated into a different patch? to make th i prefer to keep it in the same patch..the changes are minimal (just name change). Line 25: runner = new DeactivateStorageDomainsMultipleActionRunner(actionType, parameters, commandContext, isInternal); Line 26: break; Line 27: } Line 28: case AttachStorageDomainToPool: { http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DeactivateStorageDomainWithOvfUpdateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DeactivateStorageDomainWithOvfUpdateCommand.java: Line 55: getParameters().getStoragePoolId())); Line 56: changeDomainStatusWithCompensation(map, StorageDomainStatus.Unknown, StorageDomainStatus.Locked); Line 57: Line 58: if (shouldPerformOvfUpdate()) { Line 59: Backend.getInstance().runInternalAction(VdcActionType.ProcessOvfUpdateForStoragePool, new StoragePoolParametersBase(getStoragePoolId())); > use runInternalAction (remove Backend.getInstance()) Done Line 60: Line 61: VdcReturnValueBase tmpRetValue = runInternalActionWithTasksContext(VdcActionType.ProcessOvfUpdateForStorageDomain, Line 62: createProcessOvfUpdateForDomainParams(), getLock()); Line 63: getReturnValue().getVdsmTaskIdList().addAll(tmpRetValue.getInternalVdsmTaskIdList()); Line 112: Line 113: List<Guid> createdTasks = new LinkedList<>(); Line 114: Line 115: for (VdcActionParametersBase parametersBase : getParameters().getImagesParameters()) { Line 116: if (parametersBase.getCommandType() == VdcActionType.AddImageFromScratch) { > Please move this if before the for loop, and get the type from the first im i prefer to have it that way than assume that there's always an element (which is breakable). therefore i leave this as is. Line 117: CreateOvfStoresForStorageDomainCommandParameters parameters = new CreateOvfStoresForStorageDomainCommandParameters(getParameters().getStoragePoolId(), Line 118: getParameters().getStorageDomainId(), 0); Line 119: parameters.getImagesParameters().addAll(getParameters().getImagesParameters()); Line 120: parameters.setParentParameters(getParameters()); Line 154: waitForTasksToBeCleared(); Line 155: executeDeactivateCommnad(); Line 156: } catch (Exception e) { Line 157: setSucceeded(false); Line 158: log.errorFormat("Error when attempting to deactivate storage domain {0}", getStorageDomainId(), e); > This should be changed to log.error instead of log.errorFormat (after rebas why? Line 159: compensate(); Line 160: } Line 161: } Line 162: }); Line 166: } Line 167: Line 168: protected void waitForTasksToBeCleared() throws InterruptedException { Line 169: while (true) { Line 170: Thread.sleep(3000); > please use TimeUnit.SECONDS.sleep(x), it is more readable that way Done Line 171: List<Guid> tasks = getDbFacade().getAsyncTaskDao().getAsyncTaskIdsByEntity(getStorageDomainId()); Line 172: if (tasks.isEmpty()) { Line 173: return; Line 174: } Line 169: while (true) { Line 170: Thread.sleep(3000); Line 171: List<Guid> tasks = getDbFacade().getAsyncTaskDao().getAsyncTaskIdsByEntity(getStorageDomainId()); Line 172: if (tasks.isEmpty()) { Line 173: return; > Please add a log here Done Line 174: } Line 175: } Line 176: } http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AddDiskParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AddDiskParameters.java: Line 8: private Guid vmSnapshotId; Line 9: private Guid storageDomainId; Line 10: private Boolean plugDiskToVm; Line 11: private boolean shouldRemainIllegalOnFailedExecution; Line 12: private boolean skipDomainCheck; > /s/skipDomainCheck/skipDomainValidation i prefer to keep the name as is. Line 13: Line 14: public AddDiskParameters() { Line 15: storageDomainId = Guid.Empty; Line 16: } http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/CreateOvfStoresForStorageDomainCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/CreateOvfStoresForStorageDomainCommandParameters.java: Line 3: import org.ovirt.engine.core.compat.Guid; Line 4: Line 5: public class CreateOvfStoresForStorageDomainCommandParameters extends StorageDomainParametersBase { Line 6: private int storesCount; Line 7: private boolean skipDomainChecks; > /s/skipDomainChecks/skipDomainValidation prefer to keep as is Line 8: Line 9: public CreateOvfStoresForStorageDomainCommandParameters() { Line 10: } Line 11: http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/CreateOvfVolumeForStorageDomainCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/CreateOvfVolumeForStorageDomainCommandParameters.java: Line 2: Line 3: import org.ovirt.engine.core.compat.Guid; Line 4: Line 5: public class CreateOvfVolumeForStorageDomainCommandParameters extends StorageDomainParametersBase { Line 6: private boolean skipDomainChecks; > /s/skipDomainChecks/skipDomainValidation prefer to keep as is Line 7: Line 8: public CreateOvfVolumeForStorageDomainCommandParameters() { Line 9: super(); Line 10: } http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ProcessOvfUpdateForStorageDomainCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ProcessOvfUpdateForStorageDomainCommandParameters.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 ProcessOvfUpdateForStorageDomainCommandParameters extends StorageDomainParametersBase { > Why add a new class, why not using the same parameter class as CreateOvfVol I prefer to have different classes for the different commands. Line 6: private boolean skipDomainChecks; Line 7: Line 8: public ProcessOvfUpdateForStorageDomainCommandParameters() { Line 9: super(); http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/StorageDomainPoolParametersBase.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/StorageDomainPoolParametersBase.java: Line 6: private static final long serialVersionUID = 6248101174739394633L; Line 7: Line 8: private boolean runSilent; Line 9: private boolean inactive; Line 10: private boolean skipChecks; > /s/skipChecks/skipValidation prefer to keep as is Line 11: Line 12: public boolean isInactive() { Line 13: return inactive; Line 14: } http://gerrit.ovirt.org/#/c/33158/6/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties: Line 455 Line 456 Line 457 Line 458 Line 459 > why removing this? because we don't have the user name after the tasks execution end (in the end method), which leads to audit log with placerholder. Line 456 Line 457 Line 458 Line 459 Line 460 > why removing this? because we don't have the user name after the tasks execution end (in the end method), which leads to audit log with placerholder. -- To view, visit http://gerrit.ovirt.org/33158 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I54cc929937d3b14dc49a2edab9e763cbe603a9c3 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
