Martin Mucha has posted comments on this change. Change subject: core: refactored HostSetupNetworksValidator to use HostInterfaceValidator ......................................................................
Patch Set 32: (6 comments) https://gerrit.ovirt.org/#/c/35978/32/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java: Line 237: candidateAttachments.addAll(newAttachments); Line 238: return candidateAttachments; Line 239: } Line 240: Line 241: private boolean validModifiedBonds() { > s/validModifiedBonds/validModifiedOrNewBonds already renamed by some previous patch.Done Line 242: boolean passed = true; Line 243: for (Bond modifiedOrNewBond : params.getBonds()) { Line 244: String bondName = modifiedOrNewBond.getName(); Line 245: Line 255: continue; Line 256: } Line 257: Line 258: //count of bond slaves must be at least two. Line 259: if (modifiedOrNewBond.getSlaves().size() < 2) { > Why don't you use the HostInterfaceValidator.isValidBond validation? theoretically I can; implementation in HostInterfaceValidator operates on all existing nics (or given nics) and VdsNetworkInterface, while this just checks bonds collection for size. Do you really want to solve this duplicity, it probably won't be that straighforward. Line 260: addViolation(VdcBllMessages.NETWORK_BONDS_INVALID_SLAVE_COUNT, bondName); Line 261: passed = false; Line 262: continue; Line 263: } Line 282: if (slave.isBondSlave() && Line 283: /* we're creating new bond, and it's definition contains reference to slave already assigned Line 284: to a different bond. */ Line 285: (!slaveBondName.equals(bondName) Line 286: //…but this bond is also removed in this request, so it's ok. > Why did you add the spaces? sorry, which spaces? If ones preceding comment, it seems to be fixed from preceding patch as well. Otherwise please clarify. Line 287: || !slaveUsedByRemovedBond(slaveBondName))) { Line 288: addViolation(VdcBllMessages.NETWORK_INTERFACE_ALREADY_IN_BOND, slaveName); Line 289: passed = false; Line 290: continue; Line 283: /* we're creating new bond, and it's definition contains reference to slave already assigned Line 284: to a different bond. */ Line 285: (!slaveBondName.equals(bondName) Line 286: //…but this bond is also removed in this request, so it's ok. Line 287: || !slaveUsedByRemovedBond(slaveBondName))) { > Why not allowing move slave form one bond to another (just make sure it was I think this was covered by previous patch as well. Line 288: addViolation(VdcBllMessages.NETWORK_INTERFACE_ALREADY_IN_BOND, slaveName); Line 289: passed = false; Line 290: continue; Line 291: } Line 290: continue; Line 291: } Line 292: Line 293: /* slave has network assigned and there isn't request for unassigning it; Line 294: so this probably check, that nic is part of newly crated bond, and any previously attached network has > Please remove the word 'probably' from the comment. I wasn't unsure when I was reading it. Provided that you did not object to comment, I understand it's correct. 'Probably' removed.Done. Line 295: to be unattached. */ Line 296: if (slave.getNetworkName() != null && !nicUsedByRemovedNetwork(slave)) { Line 297: addViolation(VdcBllMessages.NETWORK_INTERFACE_ATTACHED_TO_NETWORK_CANNOT_BE_SLAVE, slave.getName()); Line 298: passed = false; https://gerrit.ovirt.org/#/c/35978/32/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/HostInterfaceValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/HostInterfaceValidator.java: Line 39: public ValidationResult interfaceInHost(Guid hostId) { Line 40: return ValidationResult.failWith(VdcBllMessages.NIC_NOT_EXISTS_ON_HOST).when(!iface.getVdsId().equals(hostId)); Line 41: } Line 42: Line 43: public ValidationResult interfaceIsBond() { > please rename to interfaceIsBondOrNull Done Line 44: return ValidationResult.failWith(VdcBllMessages.NETWORK_INTERFACE_IS_NOT_BOND) Line 45: .when(iface != null && !isBond()); Line 46: } Line 47: -- To view, visit https://gerrit.ovirt.org/35978 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I070bb5c75d635d28911f7f367051c71dbd0127d2 Gerrit-PatchSet: 32 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
