Maor Lipchuk 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 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 some OVF handler instead of CommandBase? 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 CreateOvfVolumeForStorageDomainCommand is 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 getCreateOvfVolumeForStorageDomainParameters (same as you done for createProcessOvfUpdateForDomainParams) 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 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 is up, what if the Data Center will be in a problematic state? 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 the patch be more readable 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()) 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 image. This way it is confusing. 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 rebase) 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 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 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 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 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 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 CreateOvfVolumeForStorageDomainCommandParameters.java and just rename it differently? 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 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? Line 456 Line 457 Line 458 Line 459 Line 460 why removing this? -- 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: 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
