Alona Kaplan has posted comments on this change. Change subject: engine: update AddVdsGroupCommand ......................................................................
Patch Set 17: (24 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 <[email protected]> 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. 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. 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- AddVdsGroupCommand- Now it's possible to pass the management network id for the new created cluster. The command would use the supplied id or would try to guess the default management network. AttachNetworkToVdsGroupCommand, UpdateNetworkNetworkOnClusterCommands- added validation checks for the new management network (if it was changed). 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. 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 line. 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 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 redundant. Please notice that now you remove the migration validation there is no need to create the networkCluster in this stage and you'll get NPE at the execution stage (since it assumes the networkCluster is initialized). I konw I told you to add the network cluster as a d.m. But now after rethinking it again and having this NPE I think it is better to delete it and create the networkCluster locally in the canDo and the execution. 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. 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 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/AddClusterNetworkClusterValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AddClusterNetworkClusterValidator.java: Line 9: super(networkCluster, version); Line 10: } Line 11: Line 12: @Override Line 13: protected boolean isManagementNetworkChangeInvalid() { Please see the comments in the base class. This method shouldn't be overridden. Instead you should override isManagementNetworkChanged. Line 14: throw new UnsupportedOperationException(); Line 15: } Line 16: 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 overridden. 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 properties which can be not set on the parameter network. 2. You don't need to pass the network as a parameter. Just use the getPersistedNetwork in the implementation of validateAttachment in the super class. 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... 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 need to pass it via the parameters. 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 pass the wrong one in the attach class), you just should use here getPersistedNetwork. 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 54: * Make sure the management network change is valid - that is allowed in an empty cluster only Line 55: * Line 56: * @return Error if the management network change is not allowed. Line 57: */ Line 58: public ValidationResult managementNetworkChange() { This check is relevant just for setting a network to be management. So maybe you can call it- managementNetworkSet. Line 59: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_CHANGED). Line 60: when(isManagementNetworkChangeInvalid()); Line 61: } Line 62: 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- isManagementNetworkChanged() && isManagementNetworkChangeForbidden() The isManagementNetworkChanged() can be abstract. 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 you display- ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_CHANGED=".. Changing management network in a non-empty cluster is not allowed" You will get this error even the cluster is empty and you unset the mgmgt network. 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. 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. 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/AddClusterNetworkClusterValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/AddClusterNetworkClusterValidatorTest.java: Line 3: import org.junit.runner.RunWith; Line 4: import org.mockito.runners.MockitoJUnitRunner; Line 5: Line 6: @RunWith(MockitoJUnitRunner.class) Line 7: public class AddClusterNetworkClusterValidatorTest extends Please format with the regular formatter. Line 8: NetworkClusterValidatorTestBase<AddClusterNetworkClusterValidator> { Line 9: Line 10: @Override Line 11: protected AddClusterNetworkClusterValidator createValidator() { 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. 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. 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. 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 <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
