Alona Kaplan has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 35: (33 comments) https://gerrit.ovirt.org/#/c/33333/35/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 91: } Line 92: Line 93: @Override Line 94: protected boolean canDoAction() { Line 95: qosDao = getDbFacade().getHostNetworkQosDao(); The use of DbFacade should be avoided. You can inject the HostNetworkQosDao (same as you injected managementNetworkUtil). Line 96: Line 97: VDS host = getVds(); Line 98: Line 99: NetworkAttachmentCompleter completer = new NetworkAttachmentCompleter(getExistingNics()); Line 105: getExistingNics(), Line 106: getExistingAttachments(), Line 107: getClusterNetworks(), Line 108: managementNetworkUtil Line 109: ); Why did you move it to a new line? Line 110: Line 111: getReturnValue().getCanDoActionMessages().addAll(validator.validate()); Line 112: getReturnValue().getCanDoActionMessages().addAll(new HostValidator(host, isInternalExecution()).validate()); Line 113: Line 108: managementNetworkUtil Line 109: ); Line 110: Line 111: getReturnValue().getCanDoActionMessages().addAll(validator.validate()); Line 112: getReturnValue().getCanDoActionMessages().addAll(new HostValidator(host, isInternalExecution()).validate()); The HostValidator validations are very basic (at least the host existence one) and should be executed before the completer logic (the place they were executed in patchset 26, just via the HostValidator). Line 113: Line 114: boolean noErrorMessageRecorded = getReturnValue().getCanDoActionMessages().isEmpty(); Line 115: return noErrorMessageRecorded; Line 116: } Line 204: Line 205: private List<VdsNetworkInterface> getRemovedBonds() { Line 206: if (removedBonds == null) { Line 207: Set<Guid> removedBondIds = getParameters().getRemovedBonds(); Line 208: this.removedBonds = Entities.entitiesById(removedBondIds, getExistingNics()); The 'this' is redundant. Line 209: } Line 210: Line 211: return removedBonds; Line 212: } Line 289: VdsNetworkInterface iface = nics.get(attachment.getNicId()); Line 290: boolean qosConfiguredOnInterface = NetworkUtils.qosConfiguredOnInterface(iface, network); Line 291: networkToConfigure.setQosConfiguredOnInterface(qosConfiguredOnInterface); Line 292: if (qosConfiguredOnInterface) { Line 293: networkToConfigure.setRelevantQos( Why relvantQos and not just networkToConfigure.setQos(..)? Of cousre it is the relevant, why would you set irrelevant property. I know this was introduced in a previous patch, so if you can fix it there, if not fix here. Line 294: iface.isQosOverridden() ? iface.getQos() : qosDao.get(network.getQosId())); Line 295: } Line 296: Line 297: networksToConfigure.add(networkToConfigure); Line 290: boolean qosConfiguredOnInterface = NetworkUtils.qosConfiguredOnInterface(iface, network); Line 291: networkToConfigure.setQosConfiguredOnInterface(qosConfiguredOnInterface); Line 292: if (qosConfiguredOnInterface) { Line 293: networkToConfigure.setRelevantQos( Line 294: iface.isQosOverridden() ? iface.getQos() : qosDao.get(network.getQosId())); Currently, IIUC, HostSetupNetworks doesn't support overriding the QoS (The is no way to pass the overriden qos via the parameters). The 'isQosOverridden' should be moved from the iface to the networkAttachment. Also the networkAttachment should contain HostNetworkQos (so you will be able to find out that iface.qos in not in sync with the attachment.qos). Is this part of your TODO list? Line 295: } Line 296: Line 297: networksToConfigure.add(networkToConfigure); Line 298: } https://gerrit.ovirt.org/#/c/33333/35/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 155: existingIfacesById); Line 156: if (!invalidBondIds.isEmpty()) { Line 157: for (Guid problematicId : invalidBondIds) { Line 158: addViolation(VdcBllMessages.NETWORK_BOND_NOT_EXISTS, problematicId.toString()); Line 159: This AppErrors message of 'NETWORK_BOND_NOT_EXISTS' doesn't contain a place holder for the id. Since this message is used by RemoveBondCommand you shouldn't change it, so please add a new message with a place holder for the id. Line 160: } Line 161: return false; Line 162: } Line 163: Line 221: candidateAttachments.addAll(newAttachments); Line 222: return candidateAttachments; Line 223: } Line 224: Line 225: private boolean validNewOrModifiedBonds() { The final version of this method has a comment- that each violation should have its own method, and this one to aggregate all the small methods. I agree. Line 226: boolean passed = true; Line 227: for (Bond modifiedBond : params.getBonds()) { Line 228: if (modifiedBond.getName() == null) { Line 229: addViolation(VdcBllMessages.HOST_NETWORK_INTERFACE_NOT_EXIST, null); Line 258: passed = false; Line 259: continue; Line 260: } Line 261: Line 262: String currentSlavesBondName = slave.getBondName(); What about my second comment in patchset 26- "2. You should verify the same slave is not attached to more than one bond." You won't get a validation in case the user attaches the same slave to two or more bonds (The case it is still attached to the previous one is covered, in mean it is attached to two or more new bonds). Line 263: if (slave.isBondSlave() && (!currentSlavesBondName.equals(modifiedBond.getName()) Line 264: && !isBondRemoved(currentSlavesBondName) Line 265: Line 266: //slaveRemovedFromItsFormerBond Line 259: continue; Line 260: } Line 261: Line 262: String currentSlavesBondName = slave.getBondName(); Line 263: if (slave.isBondSlave() && (!currentSlavesBondName.equals(modifiedBond.getName()) 1. In my opinion- 'slave.isBondSlave()' is little bit confusing. I would change it to- 'currentSlavesBondName!=null' 2. In the final revision of this 'if' block there is a comment- /* definition of currently processed bond references this slave, but this slave already 'slaves' for another bond. This is ok only when this bond will be removed as a part of this request. */ You should update it according to the changes in this patch- "This is ok only when this bond will be removed or the slave will be removed from this bond as a part of this request" Line 264: && !isBondRemoved(currentSlavesBondName) Line 265: Line 266: //slaveRemovedFromItsFormerBond Line 267: && !bondIsUpdatedAndDoesNotContainCertainSlave(slaveName, currentSlavesBondName))) { Line 269: passed = false; Line 270: continue; Line 271: } Line 272: Line 273: if (slave.getNetworkName() != null && !isNetworkAttachmentRemoved(slave)) { 1. As you wrote in patchset 26- the same nic can have more than one attachment. You have to check all of them were removed (this is not covered even in the final revision in the method) or 2. moved to another nic (this is covered in the final revision, but just for the non vlan network. If the slave has vlan networks attached to it, even in the final revision you won't get a validation error). Please at least copy to here the final revision of this method (validNewOrModifiedBonds). It is impossible to review it this way. I have the final revision in my eclipse and it has bugs, but since the code of the method is spread between ~7 patches it is impossible to add comments. Line 274: addViolation(VdcBllMessages.NETWORK_INTERFACE_ATTACHED_TO_NETWORK_CANNOT_BE_SLAVE, slave.getName()); Line 275: passed = false; Line 276: continue; Line 277: } Line 283: Line 284: /** Line 285: * @return true if there's network attachment related to given nic, which gets removed upon request. Line 286: */ Line 287: private boolean isNetworkAttachmentRemoved(VdsNetworkInterface nic) { I am assuming you will copy to this patch the final revision of validNewOrModifiedBonds. So, in the final revision, this method (isNetworkAttachmentRemoved) still exists nut never used. Line 288: for (NetworkAttachment attachment : removedNetworkAttachments) { Line 289: if (Objects.equals(nic.getName(), attachment.getNicName())) { Line 290: return true; Line 291: } Line 301: * @param bondName name of bond we're examining Line 302: * Line 303: * @return true if bond specified by name is present in request and does not contain given slave. Line 304: */ Line 305: private boolean bondIsUpdatedAndDoesNotContainCertainSlave(String slaveName, String bondName) { Maybe isSlaveRemovedFromBond Line 306: for (Bond bond : params.getBonds()) { Line 307: boolean isRequiredBond = bond.getName().equals(bondName); Line 308: if (isRequiredBond) { Line 309: boolean updatedBondDoesNotContainGivenSlave = !bond.getSlaves().contains(slaveName); Line 303: * @return true if bond specified by name is present in request and does not contain given slave. Line 304: */ Line 305: private boolean bondIsUpdatedAndDoesNotContainCertainSlave(String slaveName, String bondName) { Line 306: for (Bond bond : params.getBonds()) { Line 307: boolean isRequiredBond = bond.getName().equals(bondName); Why aren't you using the bondsMap? Line 308: if (isRequiredBond) { Line 309: boolean updatedBondDoesNotContainGivenSlave = !bond.getSlaves().contains(slaveName); Line 310: return updatedBondDoesNotContainGivenSlave; Line 311: } Line 328: boolean passed = true; Line 329: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 330: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachment, host, managementNetworkUtil); Line 331: Line 332: if (!validate(validator.networkAttachmentIsSet())) { validator.networkAttachmentIsSet() checks if the attachment is null. In this case you're iterating over the attachments list, so you can assume it is not null. (If you don't assume it, it means that for consistency you have to verify all the parameters lists (removedAttachment, removedBonds, bonds) doesn't contain null values). note: I didn't notice it when reviewed patchset 26, that's why I asked you to meve this check upper. But as you wrote in one of you comment answers, there is no way to get null here. Line 333: passed = false; Line 334: continue; Line 335: } Line 336: Line 335: } Line 336: Line 337: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 338: if (!(validate(validator.networkExists(), networkId) Line 339: && validate(referencedNetworkAttachmentActuallyExists(attachment.getId()), networkId) The error message is- "Cannot ${action} ${type}. Given Network Attachment does not exist" You are in the context of HostSetupNetworks. What is the given network attachment? I see that you passed the networkId to the addViolation. But it is meaningless, the message doesn't have placeholder for the networkId. And if IIUC, the addViolation(*, violatingEntity) is used to fill the following format- {message_name}_LIST. So you have to add a corresponding place holder to the message. Line 340: && validate(validator.notExternalNetwork(), networkId) Line 341: && validate(validator.networkAttachedToCluster(), networkId) Line 342: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 343: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 340: notExternalNetwork same Line 341: networkAttachedToCluster same Line 338: if (!(validate(validator.networkExists(), networkId) Line 339: && validate(referencedNetworkAttachmentActuallyExists(attachment.getId()), networkId) Line 340: && validate(validator.notExternalNetwork(), networkId) Line 341: && validate(validator.networkAttachedToCluster(), networkId) Line 342: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) same Line 343: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 344: && validate(validator.nicExists(), networkId) Line 345: && validate(validator.networkIpAddressWasSameAsHostnameAndChanged(getExistingIfaces())) Line 346: && validate(validator.networkNotChanged(attachmentsById.get(attachment.getId()))) Line 339: && validate(referencedNetworkAttachmentActuallyExists(attachment.getId()), networkId) Line 340: && validate(validator.notExternalNetwork(), networkId) Line 341: && validate(validator.networkAttachedToCluster(), networkId) Line 342: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 343: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) same Line 344: && validate(validator.nicExists(), networkId) Line 345: && validate(validator.networkIpAddressWasSameAsHostnameAndChanged(getExistingIfaces())) Line 346: && validate(validator.networkNotChanged(attachmentsById.get(attachment.getId()))) Line 347: && validate(validator.validateGateway()))) { Line 340: && validate(validator.notExternalNetwork(), networkId) Line 341: && validate(validator.networkAttachedToCluster(), networkId) Line 342: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 343: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 344: && validate(validator.nicExists(), networkId) same Line 345: && validate(validator.networkIpAddressWasSameAsHostnameAndChanged(getExistingIfaces())) Line 346: && validate(validator.networkNotChanged(attachmentsById.get(attachment.getId()))) Line 347: && validate(validator.validateGateway()))) { Line 348: passed = false; Line 351: Line 352: return passed; Line 353: } Line 354: Line 355: private ValidationResult referencedNetworkAttachmentActuallyExists(Guid networkAttachmentId) { maybe- modifiedAttachmentExists Line 356: boolean doesNotReferenceExistingNetworkAttachment = networkAttachmentId == null; Line 357: if (doesNotReferenceExistingNetworkAttachment) { Line 358: return ValidationResult.VALID; Line 359: } Line 352: return passed; Line 353: } Line 354: Line 355: private ValidationResult referencedNetworkAttachmentActuallyExists(Guid networkAttachmentId) { Line 356: boolean doesNotReferenceExistingNetworkAttachment = networkAttachmentId == null; maybe- isNewAttachment Line 357: if (doesNotReferenceExistingNetworkAttachment) { Line 358: return ValidationResult.VALID; Line 359: } Line 360: Line 365: } Line 366: Line 367: return new ValidationResult(VdcBllMessages.NETWORK_ATTACHMENT_NOT_EXISTS); Line 368: } Line 369: Please remove the white space. Line 370: private boolean validRemovedNetworkAttachments(Map<Guid, NetworkAttachment> attachmentsById) { Line 371: List<Guid> invalidIds = Entities.idsNotReferencingExistingRecords(params.getRemovedNetworkAttachments(), Line 372: existingIfacesById); Line 373: if (!invalidIds.isEmpty()) { Line 378: return false; Line 379: } Line 380: Line 381: boolean passed = true; Line 382: for (NetworkAttachment attachment : this.removedNetworkAttachments) { the "this" is redundant. Line 383: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 384: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 385: if (!(validate(validator.networkAttachmentIsSet(), attachment.getId().toString()) Line 386: && validate(validator.notExternalNetwork()) Line 381: boolean passed = true; Line 382: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 383: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 384: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 385: if (!(validate(validator.networkAttachmentIsSet(), attachment.getId().toString()) I agree with you, (as you answered on patchset 26) this validation should be removed. You iterating over existing attachments, it cannot be null. Line 386: && validate(validator.notExternalNetwork()) Line 387: && validate(validator.notManagementNetwork()) Line 388: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 389: passed = false; https://gerrit.ovirt.org/#/c/33333/35/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostValidator.java: Line 35: boolean hostStatusLegalForSetupNetworks = supportedStatuses.contains(host.getStatus()) Line 36: || host.getStatus() == VDSStatus.Installing && internalExecution; Line 37: Line 38: if (!hostStatusLegalForSetupNetworks) { Line 39: addViolation(VdcBllMessages.VAR__HOST_STATUS__UP_MAINTENANCE_OR_NON_OPERATIONAL, host.getName()); Adding the host.getName() is redundant as I explained in the HostSetupNetworkValidator comments. Line 40: addViolation(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL, host.getName()); Line 41: return false; Line 42: } Line 43: Line 36: || host.getStatus() == VDSStatus.Installing && internalExecution; Line 37: Line 38: if (!hostStatusLegalForSetupNetworks) { Line 39: addViolation(VdcBllMessages.VAR__HOST_STATUS__UP_MAINTENANCE_OR_NON_OPERATIONAL, host.getName()); Line 40: addViolation(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL, host.getName()); same. Line 41: return false; Line 42: } Line 43: Line 44: return true; https://gerrit.ovirt.org/#/c/33333/35/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/ValidatorWithViolationMessages.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/ValidatorWithViolationMessages.java: Line 12: public class ValidatorWithViolationMessages { Line 13: private Map<VdcBllMessages, List<String>> violations = new HashMap<>(); Line 14: Line 15: protected void addViolation(VdcBllMessages violation, String violatingEntity) { Line 16: List<String> violatingEntities = violations.get(violation); You can use the MultiValueMapUtils Line 17: if (violatingEntities == null) { Line 18: violatingEntities = new ArrayList<>(); Line 19: violations.put(violation, violatingEntities); Line 20: } https://gerrit.ovirt.org/#/c/33333/35/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 202: return ids; Line 203: } Line 204: Line 205: Line 206: public static <E extends BusinessEntity<I>, I extends Serializable> List<E> entitiesById(Collection<I> requestedIds, 1. I would call the method filterEntitiesByRequiredIds. 2. Why did you remove the java doc? Please add one. Line 207: Collection<E> existingEntities) { Line 208: Line 209: List<E> resultCollection = new ArrayList<>(requestedIds.size()); Line 210: https://gerrit.ovirt.org/#/c/33333/35/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 64: private void addNewNetworkAttachments() { Line 65: for (VdsNetworkInterface nic : nicsByName.values()) { Line 66: String networkName = nic.getNetworkName(); Line 67: if (networkName == null Line 68: || !clusterNetworks.containsKey(networkName) Please fix the formatting. Line 69: || networkAttachmentExistsForNetwork(networkName) Line 70: || nic.isBondSlave()) { Line 71: continue; Line 72: } Line 65: for (VdsNetworkInterface nic : nicsByName.values()) { Line 66: String networkName = nic.getNetworkName(); Line 67: if (networkName == null Line 68: || !clusterNetworks.containsKey(networkName) Line 69: || networkAttachmentExistsForNetwork(networkName) What if the attachment exists (there is attachment with the given networkName) but the nicId on the attachment doesn't equal the nic.getId? (It can happen if the user would manually change the nics network, via the hosts config files). I think the old attachment should be updated to contain the new nicId (we are not supporting this kind of out of sync). note: please refer to comment in line 117, it must be fixed, so the attachment will have the correct nicId here. Line 70: || nic.isBondSlave()) { Line 71: continue; Line 72: } Line 73: Line 113: if (networkConfiguredOnHost(attachment.getNetworkId())) { Line 114: VdsNetworkInterface nic = reportedNicsByNetworkId.get(attachment.getNetworkId()); Line 115: attachment.setNicId(nic.getId()); Line 116: if (dbAttachmentsById.containsKey(attachment.getId())) { Line 117: networkAttachmentDao.update(attachment); Here you call it configuredNetworkAttachments in the final version it is existingValidAttachments but the issue is the same- you should replace the old network attachment (in the list) with the updated one. Line 118: } else { Line 119: attachment.setId(Guid.newGuid()); Line 120: networkAttachmentDao.save(attachment); Line 121: configuredNetworkAttachments.add(attachment); -- 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: 35 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
