Maor Lipchuk 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 done 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 dom done 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. Vds is being initialized in the CDA 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 d yes, detached domain will not have empty SPUUID, unless it has been imported, and if it was imported the user should be aware of those consequences 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 done 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 It does not have to be attcahed, it can be forced removed from the setup 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 This is Guid, you can't do it for GUID 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. done 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 at no, we should keep this command, so the user will get an appropriate message when it will be attached 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 re-extract to two methods 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
