Yevgeny Zaspitsky has posted comments on this change. Change subject: engine: update commands that might change management network ......................................................................
Patch Set 17: (21 comments) http://gerrit.ovirt.org/#/c/33416/17//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-09-27 17:32:14 +0300 Line 4: Commit: Yevgeny Zaspitsky <yzasp...@redhat.com> Line 5: CommitDate: 2014-10-29 16:31:59 +0200 Line 6: Line 7: engine: update AddVdsGroupCommand > Maybe- Updated the flows that can set a network as a management. Done Line 8: Line 9: Update AddVdsGroupCommand with the new logic. Line 10: Now it's possible to pass the management network id for the new Line 11: created cluster. The command would use the supplied id or would try Line 5: CommitDate: 2014-10-29 16:31:59 +0200 Line 6: Line 7: engine: update AddVdsGroupCommand Line 8: Line 9: Update AddVdsGroupCommand with the new logic. > This can be removed. Done Line 10: Now it's possible to pass the management network id for the new Line 11: created cluster. The command would use the supplied id or would try Line 12: to guess the default management network. Line 13: Line 6: Line 7: engine: update AddVdsGroupCommand Line 8: Line 9: Update AddVdsGroupCommand with the new logic. Line 10: Now it's possible to pass the management network id for the new > The commands that were updated are- Done Line 11: created cluster. The command would use the supplied id or would try Line 12: to guess the default management network. Line 13: Line 14: Change-Id: I196a583ae7d8d7e373a1aca2a48e592232b18a5b http://gerrit.ovirt.org/#/c/33416/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsGroupCommand.java: Line 52: private StoragePoolDAO storagePoolDao; Line 53: Line 54: private Network managementNetwork; Line 55: Line 56: private NetworkCluster networkCluster; > Please rename to managementNetworkCluster to more accurate. Not relevant anymore - the variable was removed Line 57: Line 58: public AddVdsGroupCommand(T parameters) { Line 59: super(parameters); Line 60: } Line 92: private Guid getManagementNetworkId() { Line 93: return getParameters().getManagementNetworkId(); Line 94: } Line 95: Line 96: private Network getManagementNetworkById() { > Why do you need a method for this? It is called only once and can be one li 1. why not? 2. "Can be one line" - Making every code a one-liner is a bad practice IMHO. You realize that when you try debugging such code. 3. Having a name for an operation instead of someDao.get(...) is more human readable and makes the callee method shorter. 4. Having shorter methods is more human readable. 5. see http://en.wikipedia.org/wiki/Single_responsibility_principle Line 97: final Guid managementNetworkId = getManagementNetworkId(); Line 98: return networkDao.get(managementNetworkId); Line 99: } Line 100: Line 213: return networkBelongsToClusterDataCenter(); Line 214: } Line 215: } Line 216: Line 217: private boolean valdateDefaultManagementNetwork() { > s/valdateDefaultManagementNetwork/validateDefaultManagementNetwork it doesn't validate anymore, so i called it findDefaultManagementNetwork Line 218: managementNetwork = Line 219: defaultManagementNetworkFinder.findDefaultManagementNetwork(getVdsGroup().getStoragePoolId()); Line 220: if (managementNetwork == null) { Line 221: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DEFAULT_MANAGEMENT_NETWORK_NOT_FOUND); Line 221: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DEFAULT_MANAGEMENT_NETWORK_NOT_FOUND); Line 222: return false; Line 223: } Line 224: final AddClusterNetworkClusterValidator networkClusterValidator = createNetworkClusterValidator(); Line 225: return validate(networkClusterValidator.migrationPropertySupported()); > Please see the comment on patch set 8. This validation is harmful and redun Done Line 226: } Line 227: Line 228: private boolean networkBelongsToClusterDataCenter() { Line 229: managementNetwork = getManagementNetworkById(); Line 234: final NetworkClusterValidatorBase networkClusterValidator = createNetworkClusterValidator(); Line 235: return validate(networkClusterValidator.networkBelongsToClusterDataCenter(getVdsGroup(), managementNetwork)) Line 236: && validate(networkClusterValidator.managementNetworkRequired(managementNetwork)) Line 237: && validate(networkClusterValidator.managementNetworkNotExternal(managementNetwork)) Line 238: && validate(networkClusterValidator.migrationPropertySupported()); > See the previous comment. Done Line 239: } Line 240: Line 241: private AddClusterNetworkClusterValidator createNetworkClusterValidator() { Line 242: networkCluster = createNetworkCluster(); Line 242: networkCluster = createNetworkCluster(); Line 243: return new AddClusterNetworkClusterValidator(networkCluster, getVdsGroup().getcompatibility_version()); Line 244: } Line 245: Line 246: private NetworkCluster createNetworkCluster() { > Maybe- createManagementNetworkCluster Done Line 247: return new NetworkCluster( Line 248: getVdsGroupId(), Line 249: managementNetwork.getId(), Line 250: NetworkStatus.OPERATIONAL, http://gerrit.ovirt.org/#/c/33416/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkClusterValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkClusterValidator.java: Line 13: return networkCluster.isManagement(); Line 14: } Line 15: Line 16: @Override Line 17: protected boolean isManagementNetworkChangeInvalid() { > Please see the comments in the base class. This method shouldn't be overrid Done Line 18: return isManagementNetworkChanged() && isManagementNetworkChangeForbidden(); Line 19: } Line 20: http://gerrit.ovirt.org/#/c/33416/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java: Line 100: && logicalNetworkExists() Line 101: && validateAttachment(getNetwork()); Line 102: } Line 103: Line 104: private boolean validateAttachment(Network network) { > 1. You should pass the persistent network since the validations checks some Done Line 105: final NetworkClusterValidatorBase networkClusterValidator = Line 106: new AttachNetworkClusterValidator(getNetworkCluster(), getClusterVersion()); Line 107: return validate(networkClusterValidator.networkBelongsToClusterDataCenter(getVdsGroup(), getNetwork())) && Line 108: super.validateAttachment(networkClusterValidator, network); Line 104: private boolean validateAttachment(Network network) { Line 105: final NetworkClusterValidatorBase networkClusterValidator = Line 106: new AttachNetworkClusterValidator(getNetworkCluster(), getClusterVersion()); Line 107: return validate(networkClusterValidator.networkBelongsToClusterDataCenter(getVdsGroup(), getNetwork())) && Line 108: super.validateAttachment(networkClusterValidator, network); > You don't need the super... Done Line 109: } Line 110: Line 111: private boolean logicalNetworkExists() { Line 112: if (getPersistedNetwork() != null) { http://gerrit.ovirt.org/#/c/33416/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java: Line 28: public String getNetworkName() { Line 29: return getPersistedNetwork().getName(); Line 30: } Line 31: Line 32: private boolean validateExternalNetwork(NetworkClusterValidatorBase validator) { > Consider adding an abstract method getNetworkClusterValidator. so you won't I do not like that getter hierarchy. That will lead to validator instance will be hold the whole command life cycle period instead of having it on spot where it is needed and after that it coud be GC'ed. Line 33: return validate(validator.externalNetworkSupported()) Line 34: && validate(validator.externalNetworkNotDisplay(getNetworkName())) Line 35: && validate(validator.externalNetworkNotRequired(getNetworkName())); Line 36: } Line 34: && validate(validator.externalNetworkNotDisplay(getNetworkName())) Line 35: && validate(validator.externalNetworkNotRequired(getNetworkName())); Line 36: } Line 37: Line 38: protected boolean validateAttachment(NetworkClusterValidatorBase validator, Network network) { > As I wrote in the Attach class. You don't need the network parameter (and p Done Line 39: return validate(validator.managementNetworkRequired(network)) Line 40: && validate(validator.managementNetworkNotExternal(network)) Line 41: && validate(validator.managementNetworkChange()) Line 42: && validate(validator.migrationPropertySupported()) http://gerrit.ovirt.org/#/c/33416/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorBase.java: Line 59: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_CHANGED). Line 60: when(isManagementNetworkChangeInvalid()); Line 61: } Line 62: Line 63: protected abstract boolean isManagementNetworkChangeInvalid(); > You can have a default implementation- Done Line 64: Line 65: protected boolean isManagementNetworkChangeForbidden() { Line 66: return !isClusterEmpty(); Line 67: } http://gerrit.ovirt.org/#/c/33416/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterValidator.java: Line 18: private boolean isManagementNetworkChanged() { Line 19: return oldNetworkCluster.isManagement() != networkCluster.isManagement(); Line 20: } Line 21: Line 22: private boolean managementNetworkUnset() { > You should have a specific error message for this case. Now, the message yo Done Line 23: return oldNetworkCluster.isManagement() && !networkCluster.isManagement(); Line 24: } Line 25: Line 26: @Override Line 23: return oldNetworkCluster.isManagement() && !networkCluster.isManagement(); Line 24: } Line 25: Line 26: @Override Line 27: protected boolean isManagementNetworkChangeInvalid() { > Please see the previous comments- this method shouldn't be overridden. Done Line 28: return managementNetworkUnset() || (isManagementNetworkChanged() && isManagementNetworkChangeForbidden()); Line 29: } http://gerrit.ovirt.org/#/c/33416/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkOnClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkOnClusterCommand.java: Line 91: return validate(networkClusterAttachmentExists()) Line 92: && validateAttachment(getPersistedNetwork()); Line 93: } Line 94: Line 95: private boolean validateAttachment(Network network) { > As I wrote in the previous comments, you don't need the parameter. Done Line 96: final UpdateNetworkClusterValidator networkClusterValidator = createNetworkClusterValidator(); Line 97: return super.validateAttachment(networkClusterValidator, network); Line 98: } Line 99: http://gerrit.ovirt.org/#/c/33416/17/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorTestBase.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorTestBase.java: Line 75: when(network.getName()).thenReturn(NETWORK_NAME); Line 76: when(currentManagementNetwork.getId()).thenReturn(TEST_CURRENT_MANAGMENT_NETWORK_ID); Line 77: when(dbFacade.getVdsDao()).thenReturn(vdsDao); Line 78: when(dbFacade.getNetworkDao()).thenReturn(networkDao); Line 79: DbFacadeLocator.setDbFacade(dbFacade); > As we talked, try to avoid this. Done Line 80: Line 81: validator = createValidator(); Line 82: } Line 83: http://gerrit.ovirt.org/#/c/33416/17/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterValidatorTest.java: Line 54: testUpdateManagementNetworkChange(false, true, true, isValid()); Line 55: } Line 56: Line 57: @Test Line 58: public void managementNetworkChangeInvalidNonEmptyClusterUnsetManagementNet() { > After you'll fix the validator, you should have a new validation error here Done Line 59: testUpdateManagementNetworkChange( Line 60: true, Line 61: false, Line 62: false, Line 63: failsWith(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_CHANGED)); Line 64: } Line 65: Line 66: @Test Line 67: public void managementNetworkChangeInvalidEmptyClusterUnsetManagementNet() { > After you'll fix the validator, you should have a new validation error here Done Line 68: testUpdateManagementNetworkChange( Line 69: true, Line 70: false, Line 71: true, -- To view, visit http://gerrit.ovirt.org/33416 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I196a583ae7d8d7e373a1aca2a48e592232b18a5b Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@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
