Martin Mucha has posted comments on this change. Change subject: engine: Introduce network attachments command ......................................................................
Patch Set 28: (7 comments) https://gerrit.ovirt.org/#/c/34971/28/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java: Line 34: Line 35: @Override Line 36: protected boolean canDoAction() { Line 37: // VDS host = getVds(); Line 38: // > You have to add validation to make sure the passed attachment doesn't conta Done Line 39: // List<String> hostValidationMessages = new HostValidator(host, isInternalExecution()).validate(); Line 40: // if (!hostValidationMessages.isEmpty()) { Line 41: // getReturnValue().getCanDoActionMessages().addAll(hostValidationMessages); Line 42: // return false; Line 51: Line 52: @Override Line 53: protected void executeCommand() { Line 54: HostSetupNetworksParameters params = new HostSetupNetworksParameters(getParameters().getVdsId()); Line 55: params.getNetworkAttachments().add(getParameters().getNetworkAttachment()); > If you're planning to use the parameter later in this class, I would pass a true. I'm not creating copy constructor just for that, solved differently Done. Line 56: VdcReturnValueBase returnValue = runInternalAction(VdcActionType.HostSetupNetworks, params); Line 57: Guid createdAttachmentId = resolveCreatedAttachmentId(); Line 58: getReturnValue().setActionReturnValue(createdAttachmentId); Line 59: propagateFailure(returnValue); Line 53: protected void executeCommand() { Line 54: HostSetupNetworksParameters params = new HostSetupNetworksParameters(getParameters().getVdsId()); Line 55: params.getNetworkAttachments().add(getParameters().getNetworkAttachment()); Line 56: VdcReturnValueBase returnValue = runInternalAction(VdcActionType.HostSetupNetworks, params); Line 57: Guid createdAttachmentId = resolveCreatedAttachmentId(); > Should be done just if the return value is returnValue.getSucceeded()) Done Line 58: getReturnValue().setActionReturnValue(createdAttachmentId); Line 59: propagateFailure(returnValue); Line 60: setSucceeded(returnValue.getSucceeded()); Line 61: } Line 60: setSucceeded(returnValue.getSucceeded()); Line 61: } Line 62: Line 63: private Guid resolveCreatedAttachmentId() { Line 64: String configuredNicName = getParameters().getNetworkAttachment().getNicName(); > What if getParameters().getNetworkAttachment() contains the nicId and not t completer for nic returned back in place. also added (in following patch) network completer Done. Line 65: VdsNetworkInterface configuredNic = Entities.entitiesByName(getHostInterfaces()).get(configuredNicName); Line 66: List<NetworkAttachment> attachmentsOnNic = Line 67: getDbFacade().getNetworkAttachmentDao().getAllForNic(configuredNic.getId()); Line 68: Line 63: private Guid resolveCreatedAttachmentId() { Line 64: String configuredNicName = getParameters().getNetworkAttachment().getNicName(); Line 65: VdsNetworkInterface configuredNic = Entities.entitiesByName(getHostInterfaces()).get(configuredNicName); Line 66: List<NetworkAttachment> attachmentsOnNic = Line 67: getDbFacade().getNetworkAttachmentDao().getAllForNic(configuredNic.getId()); > Since HostSetupNetworks invoke refreshVdsCaps, you may find out the nic is in that case, would be operation successful? I'm adding attachment to nic, which ceases to exist. This should not be successful. Is it or is it not? Line 68: Line 69: Line 70: NetworkAttachment createNetworkAttachment = LinqUtils.firstOrNull(attachmentsOnNic, Line 71: new Predicate<NetworkAttachment>() { Line 77: return networkAttachment.getNetworkId().equals(requiredNetworkId); Line 78: } Line 79: }); Line 80: Line 81: return createNetworkAttachment.getId(); > Please add null safety check for createNetworkAttachment (it shouldn't be n already solved differently. If something should be valid, we expect it to be valid, but it's not, this is the right time for assertion (for assertion fans unfamiliar with its unusability) or exception. Exception was already added.Done. Line 82: } Line 83: Line 84: // private boolean validateCrossAttachments() { Line 85: // List<NetworkAttachment> expectedAttachments = getDbFacade().getNetworkAttachmentDao().getAllForHost(getVdsId()); Line 119: // && validate(hostInterfaceValidator.validBond(getHostInterfaces())) Line 120: // && validate(hostInterfaceValidator.networkCanBeAttached()); Line 121: // } Line 122: Line 123: private List<VdsNetworkInterface> getHostInterfaces() { > If you follow my comment in line 64, this method will becode redundant. it seems that this method is needed to initialize completer.Done. Line 124: if (hostNics == null) { Line 125: hostNics = getDbFacade().getInterfaceDao().getAllInterfacesForVds(getVdsId()); Line 126: } Line 127: -- To view, visit https://gerrit.ovirt.org/34971 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3c1b7a4e0b5ceacf6ea436c4ede84e414817dbdd Gerrit-PatchSet: 28 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: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
