Martin Mucha has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 26: (9 comments) 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 292: Line 293: return passed; Line 294: } Line 295: Line 296: private boolean nicUsedByRemovedNetwork(VdsNetworkInterface slave) { > 1. maybe nicUseByRemovedNetworkAtachment. It is confusing, since the networ both renamed. I opted for "isNetworkAttachmentRemoved" to be consistent with following method renamed to "isBondRemoved". (note: wrong name of parameter was caused by IDE automatic refactoring; method was evidently extracted from above overgrown, where given nic is slave. This is the down side of using automatic refactoring. I rushed too much and forget to rename parameter.) however, it may be little bit weird, that we're ok with any network attachment related to the slave. I'm not super-sure about it, since there can be multiple network attachments on given nic. Neverthe less, it seems that I rewrite this code in patch named 'override configuration' and it seems to be more interrested in what's going on. Maybe there was an error I fix that way, I really cannot remember, sorry. Line 297: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 298: if (Objects.equals(slave.getName(), attachment.getNicName())) { Line 299: return true; Line 300: } Line 302: Line 303: return false; Line 304: } Line 305: Line 306: private boolean slaveUsedByRemovedBond(String slaveBondName) { > Maybe- shouldRemoveBond(String bondName) same as above. This is not properly understood extracted method. It has this meaning for caller, but when observer separately it's: "isBondRemoved". Renaming. Done. Line 307: for (VdsNetworkInterface removedBond : removedBondVdsNetworkInterface) { Line 308: if (slaveBondName.equals(removedBond.getName())) { Line 309: return true; Line 310: } Line 312: Line 313: return false; Line 314: } Line 315: Line 316: private boolean validModifiedNetworkAttachments(Map<Guid, NetworkAttachment> attachmentsById) { > General question- what is the correct way to move network from one nic to a I think moti designed it like this: network attachment can have nic ID updated, but cannot have network ID updated. That being said, it's possible to reattach network via updating network attachment, changing it's nic ID. However, that would make UI code harder I believe. So following is allowed as well — it's also ok in that case to remove network attachment and create fresh new one. so I think both is allowed. Done. Line 317: boolean passed = true; Line 318: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 319: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachment, host, managementNetworkUtil); Line 320: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 316: private boolean validModifiedNetworkAttachments(Map<Guid, NetworkAttachment> attachmentsById) { Line 317: boolean passed = true; Line 318: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 319: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachment, host, managementNetworkUtil); Line 320: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); > In the next line you check if 'networkAttachmentIsSet' (is not null), so I I don't know why it's there... From REST I do not see a way how null can be smuggled into network attachments list. Maybe from UI. But maybe null validation was added 'just to be sure'. Null value handled.Done Line 321: if (!(validate(validator.networkAttachmentIsSet()) Line 322: && validate(validator.networkExists(), networkId) Line 323: && validate(validator.notExternalNetwork(), networkId) Line 324: && validate(validator.networkAttachedToCluster(), networkId) Line 318: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 319: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachment, host, managementNetworkUtil); Line 320: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 321: if (!(validate(validator.networkAttachmentIsSet()) Line 322: && validate(validator.networkExists(), networkId) > 1. Shouldn't you first check the networkId is not null? 1— I think this is correct; when network id is null, this method ("org.ovirt.engine.core.bll.validator.NetworkAttachmentValidator#getNetwork") will return null, so network validator will be created with null-valued network, and following "org.ovirt.engine.core.bll.validator.NetworkValidator#networkIsSet" will produce "VdcBllMessages.NETWORK_NOT_EXISTS" which is correct. 2— I have to overlook some call; they're indeed the same; I'm removing it from the latter patch 3 — if network id is null, we do not have any id to inform about in first place, if it's just a inexisting network, pair error message X violatingEntity will be stored and translated into error messages using this method: "org.ovirt.engine.core.bll.network.host.ValidatorWithViolationMessages#translateViolations". So I'm not sure if it's not sufficient this way ... Or what is not covered? Line 323: && validate(validator.notExternalNetwork(), networkId) Line 324: && validate(validator.networkAttachedToCluster(), networkId) Line 325: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 326: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 323: && validate(validator.notExternalNetwork(), networkId) Line 324: && validate(validator.networkAttachedToCluster(), networkId) Line 325: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 326: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 327: && validate(validator.nicExists(), networkId) > Regarding the nicActuallyExistsOrReferencesNewBond method, which is not in I think it's the same as above; message itself does not contain ID, but it will be provided via the 'violation entity'. 'referencedNetworkAttachmentActuallyExists' moved here. 'nicActuallyExistsOrReferencesNewBond' seems more problematic, don't want to spend time on that. Done. Line 328: && validate(validator.networkIpAddressWasSameAsHostnameAndChanged(getExistingIfaces())) Line 329: && validate(validator.networkNotChanged(attachmentsById.get(attachment.getId()))) Line 330: && validate(validator.validateGateway()))) { Line 331: passed = false; Line 323: && validate(validator.notExternalNetwork(), networkId) Line 324: && validate(validator.networkAttachedToCluster(), networkId) Line 325: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 326: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 327: && validate(validator.nicExists(), networkId) > The validator.nicExists() only checks if the attachment.nicName is not null Done Line 328: && validate(validator.networkIpAddressWasSameAsHostnameAndChanged(getExistingIfaces())) Line 329: && validate(validator.networkNotChanged(attachmentsById.get(attachment.getId()))) Line 330: && validate(validator.validateGateway()))) { Line 331: passed = false; Line 339: boolean passed = true; Line 340: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 341: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 342: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 343: if (!(validate(validator.networkAttachmentIsSet()) > The message you get in this validation is- Given Network Attachment does no I've added attachment Id. However I think, that after my latest changes, if you like them, this call is not needed. Request is validated, so user does not try to remove inexisting attachments. So here we loop over removedNetworkAttachments, all already queried from DB guaranteed to be existant. I think we can remove this altogether. Line 344: && validate(validator.notExternalNetwork()) Line 345: && validate(validator.notManagementNetwork()) Line 346: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 347: passed = false; Line 340: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 341: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 342: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 343: if (!(validate(validator.networkAttachmentIsSet()) Line 344: && validate(validator.notExternalNetwork()) > Is it possible to have an attachment with external network? I don't think so. External means 'not-managed', right? If I'm right, I think "validate(validator.notExternalNetwork())" is correct. Line 345: && validate(validator.notManagementNetwork()) Line 346: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 347: passed = false; Line 348: } -- 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
