Liron Aravot has posted comments on this change. Change subject: core: Force detach a storage domain ......................................................................
Patch Set 6: (10 comments) http://gerrit.ovirt.org/#/c/29021/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddExistingFileStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddExistingFileStorageDomainCommand.java: Line 9 Line 10 Line 11 Line 12 Line 13 this is related to the previous patch in the series as it seems http://gerrit.ovirt.org/#/c/29021/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AttachStorageDomainToPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AttachStorageDomainToPoolCommand.java: Line 81: } Line 82: }); Line 83: connectAllHostsToPool(); Line 84: @SuppressWarnings("unchecked") Line 85: Pair<StorageDomainStatic, Guid> domainFromIrs = (Pair<StorageDomainStatic, Guid>) runVdsCommand( this should be moved to be within the if block on line 93, you know the domain type from the db. Line 86: VDSCommandType.HSMGetStorageDomainInfo, Line 87: new HSMGetStorageDomainInfoVDSCommandParameters(getVdsId(), getParameters().getStorageDomainId()) Line 88: ).getReturnValue(); Line 89: StorageDomainStatic storageDomainToAttach = domainFromIrs.getFirst(); Line 83: connectAllHostsToPool(); Line 84: @SuppressWarnings("unchecked") Line 85: Pair<StorageDomainStatic, Guid> domainFromIrs = (Pair<StorageDomainStatic, Guid>) runVdsCommand( Line 86: VDSCommandType.HSMGetStorageDomainInfo, Line 87: new HSMGetStorageDomainInfoVDSCommandParameters(getVdsId(), getParameters().getStorageDomainId()) getVdsId() afaik will be null here. Line 88: ).getReturnValue(); Line 89: StorageDomainStatic storageDomainToAttach = domainFromIrs.getFirst(); Line 90: Line 91: // Detach Storage Domain only for data storage. Line 84: @SuppressWarnings("unchecked") Line 85: Pair<StorageDomainStatic, Guid> domainFromIrs = (Pair<StorageDomainStatic, Guid>) runVdsCommand( Line 86: VDSCommandType.HSMGetStorageDomainInfo, Line 87: new HSMGetStorageDomainInfoVDSCommandParameters(getVdsId(), getParameters().getStorageDomainId()) Line 88: ).getReturnValue(); not sure about that part, it means that now each time when we will attach domain even if it's already attached to other pool it'll be attached forcibly. Are we fine with that? Line 89: StorageDomainStatic storageDomainToAttach = domainFromIrs.getFirst(); Line 90: Line 91: // Detach Storage Domain only for data storage. Line 92: if (storageDomainToAttach.getStorageDomainType() != StorageDomainType.ImportExport) { Line 87: new HSMGetStorageDomainInfoVDSCommandParameters(getVdsId(), getParameters().getStorageDomainId()) Line 88: ).getReturnValue(); Line 89: StorageDomainStatic storageDomainToAttach = domainFromIrs.getFirst(); Line 90: Line 91: // Detach Storage Domain only for data storage. /s/*/forcibly detach only data storage domains Line 92: if (storageDomainToAttach.getStorageDomainType() != StorageDomainType.ImportExport) { Line 93: Guid storagePoolId = domainFromIrs.getSecond(); Line 94: Line 95: // If the storage domain is already related to another Storage Pool, detach it by force. Line 91: // Detach Storage Domain only for data storage. Line 92: if (storageDomainToAttach.getStorageDomainType() != StorageDomainType.ImportExport) { Line 93: Guid storagePoolId = domainFromIrs.getSecond(); Line 94: Line 95: // If the storage domain is already related to another Storage Pool, detach it by force. /s/related/attached Line 96: if (storagePoolId != null && !storagePoolId.toString().isEmpty()) { Line 97: Line 98: // Master domain version is not relevant since force remove at DetachStorageDomainVdsCommand Line 99: // does not use it. Line 92: if (storageDomainToAttach.getStorageDomainType() != StorageDomainType.ImportExport) { Line 93: Guid storagePoolId = domainFromIrs.getSecond(); Line 94: Line 95: // If the storage domain is already related to another Storage Pool, detach it by force. Line 96: if (storagePoolId != null && !storagePoolId.toString().isEmpty()) { replace with StringUtils.isEmpty Line 97: Line 98: // Master domain version is not relevant since force remove at DetachStorageDomainVdsCommand Line 99: // does not use it. Line 100: // Storage pool id can be empty Line 106: detachParams.setForce(true); Line 107: VDSReturnValue returnValue = Line 108: runVdsCommand(VDSCommandType.DetachStorageDomain, detachParams); Line 109: if (!returnValue.getSucceeded()) { Line 110: log.warnFormat("Detaching Storage Domain {0} from it's previous storage pool has failed. " let's log here also the other pool id if we already have it. Line 111: + Line 112: "The meta data of the Storage Domain might still indicate that it is attached to a different Storage Pool.", Line 113: getParameters().getStorageDomainId()); Line 114: } Line 109: if (!returnValue.getSucceeded()) { Line 110: log.warnFormat("Detaching Storage Domain {0} from it's previous storage pool has failed. " Line 111: + Line 112: "The meta data of the Storage Domain might still indicate that it is attached to a different Storage Pool.", Line 113: getParameters().getStorageDomainId()); at this point we should fail the command, no need to continue and try to attach Line 114: } Line 115: } Line 116: } Line 117: http://gerrit.ovirt.org/#/c/29021/6/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/AttachStorageDomainToPoolCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/AttachStorageDomainToPoolCommandTest.java: Line 107: any(VdcActionParametersBase.class), Line 108: any(CommandContext.class))).thenReturn(vdcReturnValue); Line 109: StorageDomainStatic storageDomain = new StorageDomainStatic(); Line 110: storageDomain.setId(Guid.newGuid()); Line 111: storageDomain.setStorageDomainType(StorageDomainType.ImportExport); this is unreadable imo, please seperate and document each part Line 112: Pair<StorageDomainStatic, Guid> pairResult = new Pair<>(storageDomain, Guid.newGuid()); Line 113: VDSReturnValue returnValueForGetStorageDomainInfo = new VDSReturnValue(); Line 114: returnValueForGetStorageDomainInfo.setSucceeded(true); Line 115: returnValueForGetStorageDomainInfo.setReturnValue(pairResult); -- To view, visit http://gerrit.ovirt.org/29021 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie75293678ab08616208128ac157426d3d98c1b77 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
