Martin Mucha has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 26: (36 comments) https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java: Line 64: private static final Logger log = LoggerFactory.getLogger(HostSetupNetworksCommand.class); Line 65: private Map<Guid, Network> clusterNetworks; Line 66: private Set<String> removedNetworks; Line 67: private Set<String> removedBondNames; Line 68: private List<VdsNetworkInterface> existingBondVdsInterfacesToBeRemoved; > why not just- removedBonds? Done Line 69: private List<VdsNetworkInterface> existingNics; Line 70: private List<NetworkAttachment> existingAttachments; Line 71: private List<HostNetwork> networksToConfigure; Line 72: Line 94: @Override Line 95: protected boolean canDoAction() { Line 96: VDS host = getVds(); Line 97: Line 98: if (host == null) { > Consider adding these two validations (host exists and in a supported statu Done Line 99: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST); Line 100: return false; Line 101: } Line 102: Line 160: runVdsCommand(VDSCommandType.GetCapabilities, Line 161: new VdsIdAndVdsVDSCommandParametersBase(getVds())); Line 162: VDS updatedHost = (VDS) returnValue.getReturnValue(); Line 163: persistNetworkChanges(updatedHost); Line 164: } > Please add logMonitorLockReleased("Host setup networks"); I can, but wouldn't that be really cumbersome and non-OO? Assume that we want logging of locking. We've introduced EngineLock with Autoclosable capability. Then this lock should log his actions himself; it's his responsibility, not commads. So here it would make sense, if we provide "locking" and "unlocking" message to overriden lock creation method (acquireMonitorLock), if needed along with commands logger(but that smells). That way, we've got locking/unlocking messages everywhere for free. If messages are not provided, we can calculate them using stacktrace (lock acquired for $(callee) ) or log generic message (but that defies logging purpose quite a lot). Line 165: Line 166: setSucceeded(true); Line 167: } Line 168: } Line 206: : Config.<Integer> getValue(ConfigValues.NetworkConnectivityCheckTimeoutInSeconds); Line 207: } Line 208: Line 209: private boolean defaultRouteRequired(Network network, IpConfiguration ipConfiguration) { Line 210: return managementNetworkUtil.isManagementNetwork(network.getId()) > you should use- managementNetworkUtil.isManagementNetwork(network.getId(), Done Line 211: && ipConfiguration != null Line 212: && (ipConfiguration.getBootProtocol() == NetworkBootProtocol.DHCP Line 213: || ipConfiguration.getBootProtocol() == NetworkBootProtocol.STATIC_IP Line 214: && StringUtils.isNotEmpty(ipConfiguration.getGateway())); Line 220: && getParameters().getBonds().isEmpty() Line 221: && getRemovedBondNames().isEmpty(); Line 222: } Line 223: Line 224: private List<VdsNetworkInterface> getExistingBondVdsInterfaceToBeRemoved() { > why not just- getRemovedBonds? Done Line 225: if (existingBondVdsInterfacesToBeRemoved == null) { Line 226: Line 227: Set<Guid> removedBondIds = getParameters().getRemovedBonds(); Line 228: int removedBondsSize = removedBondIds.size(); Line 227: Set<Guid> removedBondIds = getParameters().getRemovedBonds(); Line 228: int removedBondsSize = removedBondIds.size(); Line 229: List<VdsNetworkInterface> removedBondInterfaces = new ArrayList<>(removedBondsSize); Line 230: Line 231: if (!Entities.entitiesById(removedBondIds, existingNics, removedBondInterfaces)) { > please call getExistingNics() instead of directly passing existingNics. Done Line 232: throw new IllegalArgumentException("Unable to find requested bond"); Line 233: } else { Line 234: this.existingBondVdsInterfacesToBeRemoved = removedBondInterfaces; Line 235: } Line 228: int removedBondsSize = removedBondIds.size(); Line 229: List<VdsNetworkInterface> removedBondInterfaces = new ArrayList<>(removedBondsSize); Line 230: Line 231: if (!Entities.entitiesById(removedBondIds, existingNics, removedBondInterfaces)) { Line 232: throw new IllegalArgumentException("Unable to find requested bond"); > It passed the canDo, so IIUC, there is no way to get this exception. yes, here it's not needed. Removed. Done. Line 233: } else { Line 234: this.existingBondVdsInterfacesToBeRemoved = removedBondInterfaces; Line 235: } Line 236: } Line 278: return existingAttachments; Line 279: } Line 280: Line 281: private Set<String> getRemovedNetworks() { Line 282: if (removedNetworks == null) { > Same comment as for getExistingBondVdsInterfaceToBeRemoved() Done Line 283: Set<Guid> removedNetworkAttachmentIds = getParameters().getRemovedNetworkAttachments(); Line 284: int removedNetworkAttachmentIdsSize = removedNetworkAttachmentIds.size(); Line 285: removedNetworks = new HashSet<>(removedNetworkAttachmentIdsSize); Line 286: Line 363: Line 364: return userConfiguredNics; Line 365: } Line 366: Line 367: private List<Network> getModifiedNetworks() { > Please cache it, like you do in all the other getters. Done Line 368: List<Network> modifiedNetworks = new ArrayList<>(getParameters().getNetworkAttachments().size()); Line 369: for (NetworkAttachment attachment : getParameters().getNetworkAttachments()) { Line 370: modifiedNetworks.add(getClusterNetworks().get(attachment.getNetworkId())); Line 371: } Line 366: Line 367: private List<Network> getModifiedNetworks() { Line 368: List<Network> modifiedNetworks = new ArrayList<>(getParameters().getNetworkAttachments().size()); Line 369: for (NetworkAttachment attachment : getParameters().getNetworkAttachments()) { Line 370: modifiedNetworks.add(getClusterNetworks().get(attachment.getNetworkId())); > Please extract getClusterNetworks().get(attachment.getNetworkId()) to a met I'm not sure if I understand. I renamed method "getNetworks" to "getNetworksToConfigure" so it's more clear, and extracted new method "existingNetworkRelatedToAttachment" to get existing network for given networksattachments networkd id. Line 371: } Line 372: Line 373: return modifiedNetworks; Line 374: } https://gerrit.ovirt.org/#/c/33333/26/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 83: Line 84: if (validateAndInitRemovedBondInterfaces() Line 85: && validateAndInitRemovedNetworkAttachments() Line 86: && validModifiedNetworkAttachments(attachmentsById) Line 87: && validRemovedNetworkAttachments(attachmentsById) > You have a method called validateAndInitRemovedNetworkAttachments() and va I redoed it entirely. This method was wrong. It was done with good intentions, but clearly methods having 'and' in its name does (at least) two things and are overgrown. Done. Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() Line 90: && validateNotRemovingUsedNetworkByVms()) { Line 91: Line 85: && validateAndInitRemovedNetworkAttachments() Line 86: && validModifiedNetworkAttachments(attachmentsById) Line 87: && validRemovedNetworkAttachments(attachmentsById) Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() > same comment as the previous. Done Line 90: && validateNotRemovingUsedNetworkByVms()) { Line 91: Line 92: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 93: if (networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 86: && validModifiedNetworkAttachments(attachmentsById) Line 87: && validRemovedNetworkAttachments(attachmentsById) Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() Line 90: && validateNotRemovingUsedNetworkByVms()) { > Shouldn't it be part of validRemovedNetworkAttachments? I can do it. Notice that this will make validRemovedNetworkAttachments bigger and harder to test. Do we really need it? Yes, it will make this method more clear, but ... Line 91: Line 92: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 93: if (networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 94: && validate(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() Line 90: && validateNotRemovingUsedNetworkByVms()) { Line 91: Line 92: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); > Shouldn't this block (except the customProperties validation) of validation I can do it. Notice that this will make validModifiedNetworkAttachments bigger and harder to test. Do we really need it? Yes, it will make this method more clear, but ... Line 93: if (networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 94: && validate(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 95: && validateMtu(attachmentsToConfigure) Line 96: && validateCustomProperties()) { Line 109: public boolean validateAndInitRemovedBondInterfaces() { Line 110: Set<Guid> removedBondIds = params.getRemovedBonds(); Line 111: List<VdsNetworkInterface> removedBondInterfaces = new ArrayList<>(removedBondIds.size()); Line 112: if (!Entities.entitiesById(removedBondIds, existingIfaces.values(), removedBondInterfaces)) { Line 113: addViolation(VdcBllMessages.NETWORK_BOND_NOT_EXISTS, null); > The bond id should be specified in the message. method removed entirelly. In redoed code, bond is specified in respective violation.Done Line 114: return false; Line 115: } Line 116: Line 117: this.removedBondVdsNetworkInterface = removedBondInterfaces; Line 110: Set<Guid> removedBondIds = params.getRemovedBonds(); Line 111: List<VdsNetworkInterface> removedBondInterfaces = new ArrayList<>(removedBondIds.size()); Line 112: if (!Entities.entitiesById(removedBondIds, existingIfaces.values(), removedBondInterfaces)) { Line 113: addViolation(VdcBllMessages.NETWORK_BOND_NOT_EXISTS, null); Line 114: return false; > Consider moving this validation to validRemovedBonds() and calling validRe Done Line 115: } Line 116: Line 117: this.removedBondVdsNetworkInterface = removedBondInterfaces; Line 118: removedBondVdsNetworkInterfaceMap = new BusinessEntityMap<>(removedBondInterfaces); Line 113: addViolation(VdcBllMessages.NETWORK_BOND_NOT_EXISTS, null); Line 114: return false; Line 115: } Line 116: Line 117: this.removedBondVdsNetworkInterface = removedBondInterfaces; > The this is redundant. Done Line 118: removedBondVdsNetworkInterfaceMap = new BusinessEntityMap<>(removedBondInterfaces); Line 119: return true; Line 120: } Line 121: Line 122: public boolean validateAndInitRemovedNetworkAttachments() { Line 123: Set<Guid> removedNetworkAttachmentIds = params.getRemovedNetworkAttachments(); Line 124: List<NetworkAttachment> removedNetworkAttachments = new ArrayList<>(removedNetworkAttachmentIds.size()); Line 125: if (!Entities.entitiesById(removedNetworkAttachmentIds, existingAttachments, removedNetworkAttachments)) { Line 126: addViolation(VdcBllMessages.NETWORK_ATTACHMENT_NOT_EXISTS, null); > Same comment as for validateAndInitRemovedBondInterfaces() Done Line 127: return false; Line 128: } Line 129: Line 130: this.removedNetworkAttachments = removedNetworkAttachments; Line 173: Set<String> requiredNicsNames = getRemovedBondsUsedByNetworks(); Line 174: Line 175: boolean passed = true; Line 176: for (VdsNetworkInterface removedBond : removedBondVdsNetworkInterface) { Line 177: if (removedBond.getName() == null) { > How is it possible for the bond name to be null? You fetch the bonds from t this is probably the artefact from older code, where removed entities were specified by whole objets.Done. Line 178: addViolation(VdcBllMessages.HOST_NETWORK_INTERFACE_NOT_EXIST, null); Line 179: passed = false; Line 180: continue; Line 181: } Line 196: Line 197: return passed; Line 198: } Line 199: Line 200: private Set<String> getRemovedBondsUsedByNetworks() { > How is this method related to bonds? I look what would it take to move it here, but it seems not that super trivial.Done. Line 201: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 202: Set<String> requiredNicsNames = new HashSet<>(); Line 203: for (NetworkAttachment attachment : attachmentsToConfigure) { Line 204: requiredNicsNames.add(attachment.getNicName()); Line 209: Line 210: private Collection<NetworkAttachment> getAttachmentsToConfigure() { Line 211: Map<Guid, NetworkAttachment> attachmentsToConfigure = new HashMap<>(Entities.businessEntitiesById(existingAttachments)); Line 212: // ignore removed attachments Line 213: for (NetworkAttachment removedAttachment : this.removedNetworkAttachments) { > The this is redundant. with what? Line 214: attachmentsToConfigure.remove(removedAttachment.getId()); Line 215: } Line 216: Line 217: List<NetworkAttachment> newAttachments = new ArrayList<>(); Line 224: newAttachments.add(attachment); Line 225: continue; Line 226: } Line 227: Line 228: if (removedBondVdsNetworkInterfaceMap.containsKey(attachment.getNicName())) { > I really don't understand this bond related code. As I understand the metho to be completely frank, I don't understand it 100% either. Removed attachments are not part of attachmentToConfigure list, while attachments related to removed bonds are. I don't know why. Line 229: attachmentsToConfigure.put(attachment.getId(), attachment); Line 230: } else { Line 231: attachmentsToConfigure.remove(attachment.getId()); Line 232: } Line 236: candidateAttachments.addAll(newAttachments); Line 237: return candidateAttachments; Line 238: } Line 239: Line 240: private boolean validModifiedBonds() { > Should be validNewOrModifiedBonds- It was fixed in a later patch? If you ca it was fixed later. Since it's not only modified, but also new. I cannot move all fixes here, that's FAR harder. Even simple block of code removal creates dozen of conflicts. renamed both validModifiedBonds and validModifiedNetworkAttachment methods. Done. Line 241: boolean passed = true; Line 242: for (Bond modifiedBond : params.getBonds()) { Line 243: if (modifiedBond.getName() == null) { Line 244: addViolation(VdcBllMessages.HOST_NETWORK_INTERFACE_NOT_EXIST, null); Line 378: private boolean validateMtu(Collection<NetworkAttachment> attachmentsToConfigure) { Line 379: boolean passed = true; Line 380: Map<String, List<Network>> nicsToNetworks = getNetworksOnNics(attachmentsToConfigure); Line 381: Line 382: for (Entry<String, List<Network>> nicToNetwork : nicsToNetworks.entrySet()) { > s/nicToNetwork/nicToNetworks Done Line 383: List<Network> networksOnNic = nicToNetwork.getValue(); Line 384: if (!networksOnNicMatchMtu(networksOnNic)) { Line 385: reportMtuDifferences(networksOnNic); Line 386: passed = false; Line 408: Line 409: return true; Line 410: } Line 411: Line 412: private Map<String, List<Network>> getNetworksOnNics(Collection<NetworkAttachment> attachmentsToConfigure) { > Maybe- getNicToNetworks(..) renamed to "getNicNameToNetworksMap". Done Line 413: Map<String, List<Network>> nicsToNetworks = new HashMap<>(); Line 414: for (NetworkAttachment attachment : attachmentsToConfigure) { Line 415: String nicName = attachment.getNicName(); Line 416: if (!nicsToNetworks.containsKey(nicName)) { Line 444: version))); Line 445: Map<String, String> validPropertiesNonVm = new HashMap<>(validProperties); Line 446: validPropertiesNonVm.remove("bridge_opts"); Line 447: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 448: Network network = clusterNetworks.get(attachment.getNetworkId()); > Saw this more than once, please extract to a method. Done Line 449: if (attachment.hasProperties()) { Line 450: if (!networkCustomPropertiesSupported) { Line 451: addViolation(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_CUSTOM_PROPERTIES_NOT_SUPPORTED, Line 452: network.getName()); Line 489: Line 490: return validationResult.isValid(); Line 491: } Line 492: Line 493: private boolean validate(ValidationResult validationResult) { > You can just call- validate(validationResult, null) I've rewritten gathering errors in following patch. This specific code (with passing null) is not present there any more.Done. Line 494: if (!validationResult.isValid()) { Line 495: addViolation(validationResult.getMessage(), null); Line 496: } Line 497: https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Entities.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Entities.java: Line 206: * Line 207: * @param requestedIds id to find. Line 208: * @param existingEntities all existing entities Line 209: * @param resultCollection result collection Line 210: * @param <E> > Why does it part of the java doc? Done Line 211: * @param <I> Line 212: * Line 213: * @return true if all entities for given ids were found. Line 214: * @throws IllegalArgumentException when result collection is not empty.14.1. Line 207: * @param requestedIds id to find. Line 208: * @param existingEntities all existing entities Line 209: * @param resultCollection result collection Line 210: * @param <E> Line 211: * @param <I> > same Done Line 212: * Line 213: * @return true if all entities for given ids were found. Line 214: * @throws IllegalArgumentException when result collection is not empty.14.1. Line 215: */ Line 212: * Line 213: * @return true if all entities for given ids were found. Line 214: * @throws IllegalArgumentException when result collection is not empty.14.1. Line 215: */ Line 216: public static <E extends BusinessEntity<I>, I extends Serializable> boolean entitiesById(Collection<I> requestedIds, > I agree, i think that the signature of this method should be: revisiting code after a while I can easily agree with both of you. I don't like this code either; it was written just to save some time, but it's pointless. what I have now is method extracting invalid ids, not related to existing interfaces. That would work for all similar usecases. Size cannot be used to locating problems, since we want report them specifically (although I already changed datatype to Set to avoid duplicates in input while processing some patch). This should fix all readability problems. note: Having both this class (Entities) and BusinessEntityMap is probably wrong. I'd like to wipe this class away, since when I have map instances in multiple ways, I have to call this numerous times. Or when I have something more complex operation performed on more maps, I have no place to put this method Wrapper named "Instances", offering common mappings (byName, byId, withoutIdSet, ...) as well as acess to original collection, would be better and it could be reused in wider context. Line 217: Collection<E> existingEntities, Line 218: Collection<E> resultCollection) { Line 219: Line 220: if (!resultCollection.isEmpty()) { Line 223: Line 224: Map<I, E> existingEntitiesMap = businessEntitiesById(existingEntities); Line 225: for (I requestedId : requestedIds) { Line 226: if (requestedId == null) { Line 227: log.warn("Request to find entity by null valued ID; ignoring."); > It is a utility method, it shouldn't contain logs. this method was overgrown. I threw it away as a whole a did it differently.Done. Line 228: continue; Line 229: } Line 230: Line 231: if (existingEntitiesMap.containsKey(requestedId)) { https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java: Line 34: this.hostId = hostId; Line 35: this.userNetworkAttachments = userNetworkAttachments; Line 36: this.clusterNetworks = new BusinessEntityMap<>(clusterNetworks); Line 37: nicsByName = Entities.entitiesByName(nics); Line 38: configuredNetworkAttachments = new ArrayList<>(); > I'm not sure, why do you need this list? I don't know, why it was introduced. And do not want (if possible) to spent time on this. I removed this field in latter patch as a part of bigger refactoring (see refactor&test for HostNetworkAttachmentsPersister and patch preceding that one). Looking at them it may be relatively easy to move them close to this patch and squash them. Do you want me to do that? Line 39: Line 40: reportedNetworksById = new HashMap<>(); Line 41: reportedNicsByNetworkId = new HashMap<>(); Line 42: for (VdsNetworkInterface nic : nics) { Line 73: attachment.setId(Guid.newGuid()); Line 74: attachment.setNetworkId(clusterNetworks.get(networkName).getId()); Line 75: Guid nicId = nic.getBaseInterface() == null ? nic.getId() : nicsByName.get(nic.getBaseInterface()).getId(); Line 76: attachment.setNicId(nicId); Line 77: attachment.setProperties(nic.getCustomProperties()); > You're not initializing the bootProtocol and IpConfiguration data. Why? added bootProtocol, address, subnet, and gateway. Done. Line 78: networkAttachmentDao.save(attachment); Line 79: } Line 80: } Line 81: Line 75: Guid nicId = nic.getBaseInterface() == null ? nic.getId() : nicsByName.get(nic.getBaseInterface()).getId(); Line 76: attachment.setNicId(nicId); Line 77: attachment.setProperties(nic.getCustomProperties()); Line 78: networkAttachmentDao.save(attachment); Line 79: } > The user defined attachments passed to this class are taken from HostSetupN to be completely frank, I don't know where and how did I 'fix' it, and cannot find it by few clicks. I can try to move it closer, but please tell me which patch it is, so I do not break anything. Thanks. Line 80: } Line 81: Line 82: Line 83: private boolean networkAttachmentExistsForNetwork(String networkName) { https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkInterfacesPersisterImpl.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkInterfacesPersisterImpl.java: Line 53: private void updateModifiedInterfaces() { Line 54: List<VdsNetworkInterface> nicsForUpdate = prepareNicsForUpdate(); Line 55: if (!nicsForUpdate.isEmpty()) { Line 56: interfaceDao.massUpdateInterfacesForVds(getNicsForUpdate()); Line 57: > ? ? empty line? If so, Done. Line 58: } Line 59: } Line 60: Line 61: private void createNewInterfaces() { https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkTopologyPersisterImpl.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkTopologyPersisterImpl.java: Line 255: * the host for which the network topology should be persisted and contains the list of the reported nics Line 256: * @param dbNics Line 257: * network interfaces from the database prior to vdsm report Line 258: * @param clusterNetworks Line 259: * the networks which assigned to the host's clusters > s/clusters/cluster Done Line 260: * @param userConfiguredNetworkData Line 261: * The network configuration as provided by the user, for which engine managed data will be preserved. Line 262: */ Line 263: private void persistTopology(VDS host, -- To view, visit https://gerrit.ovirt.org/33333 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I60077019f308f371f21fb7b5545ba48adb38bd06 Gerrit-PatchSet: 26 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
