Alona Kaplan has posted comments on this change. Change subject: engine: Introduce network attachments command ......................................................................
Patch Set 28: (16 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 33: } Line 34: Line 35: @Override Line 36: protected boolean canDoAction() { Line 37: // VDS host = getVds(); Please remove the commented code. Line 38: // Line 39: // List<String> hostValidationMessages = new HostValidator(host, isInternalExecution()).validate(); Line 40: // if (!hostValidationMessages.isEmpty()) { Line 41: // getReturnValue().getCanDoActionMessages().addAll(hostValidationMessages); 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 contain id (you actually adding new one and not updating an existing one). 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 copy of the network attachment and not the same instance. You don't know what the inner command does to the parameters. 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()) Line 58: getReturnValue().setActionReturnValue(createdAttachmentId); Line 59: propagateFailure(returnValue); Line 60: setSucceeded(returnValue.getSucceeded()); Line 61: } Line 64: configuredNicName What if getParameters().getNetworkAttachment() contains the nicId and not the name (you cannot assume the HostSetupNetworkCommand will complete it). I suggest you to use the completer to complete the parameter and then getting the configuredNic by getDbFacade().getInterfaceDao().get(nicId). 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 not existing anymore. So please add null safety check for configuredNic. Line 68: Line 69: Line 70: NetworkAttachment createNetworkAttachment = LinqUtils.firstOrNull(attachmentsOnNic, Line 71: new Predicate<NetworkAttachment>() { Line 69: Line 70: NetworkAttachment createNetworkAttachment = LinqUtils.firstOrNull(attachmentsOnNic, Line 71: new Predicate<NetworkAttachment>() { Line 72: Line 73: private Guid requiredNetworkId = getParameters().getNetworkAttachment().getNetworkId(); Same here, maybe the id isn't passed to the parameter. You have to use the completer to be sure the id and name are set. Line 74: Line 75: @Override Line 76: public boolean eval(NetworkAttachment networkAttachment) { Line 77: return networkAttachment.getNetworkId().equals(requiredNetworkId); 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 null, but better be on the safe side). Line 82: } Line 83: Line 84: // private boolean validateCrossAttachments() { Line 85: // List<NetworkAttachment> expectedAttachments = getDbFacade().getNetworkAttachmentDao().getAllForHost(getVdsId()); Line 80: Line 81: return createNetworkAttachment.getId(); Line 82: } Line 83: Line 84: // private boolean validateCrossAttachments() { Please remove the commented code. Line 85: // List<NetworkAttachment> expectedAttachments = getDbFacade().getNetworkAttachmentDao().getAllForHost(getVdsId()); Line 86: // expectedAttachments.add(getParameters().getNetworkAttachment()); Line 87: // NetworkAttachmentsValidator crossAttachmentsValidator = Line 88: // new NetworkAttachmentsValidator(expectedAttachments, 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. Line 124: if (hostNics == null) { Line 125: hostNics = getDbFacade().getInterfaceDao().getAllInterfacesForVds(getVdsId()); Line 126: } Line 127: https://gerrit.ovirt.org/#/c/34971/28/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/RemoveNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/RemoveNetworkAttachmentCommand.java: Line 12: import org.ovirt.engine.core.common.businessentities.network.NetworkAttachment; Line 13: import org.ovirt.engine.core.common.validation.group.RemoveEntity; Line 14: Line 15: @NonTransactiveCommandAttribute Line 16: public class RemoveNetworkAttachmentCommand<T extends NetworkAttachmentParameters> extends VdsCommand<T> { Please refer to the comment on patchset 21 line 56- the parameter should be changed to IdParameters. Line 17: Line 18: @Inject Line 19: private ManagementNetworkUtil managementNetworkUtil; Line 20: Line 24: } Line 25: Line 26: @Override Line 27: protected boolean canDoAction() { Line 28: // VDS host = getVds(); Please remove the commented code. Line 29: // Line 30: // List<String> hostValidationMessages = new HostValidator(host, isInternalExecution()).validate(); Line 31: // if (!hostValidationMessages.isEmpty()) { Line 32: // getReturnValue().getCanDoActionMessages().addAll(hostValidationMessages); Line 45: Line 46: @Override Line 47: protected void executeCommand() { Line 48: HostSetupNetworksParameters params = new HostSetupNetworksParameters(getParameters().getVdsId()); Line 49: NetworkAttachment networkAttachmentForRemove = As I wrote in patchset 21, the query is redundant, please remove it. Just do- params.getRemovedNetworkAttachments().add(getParameters().getNicId()); Line 50: getDbFacade().getNetworkAttachmentDao().get(getParameters().getNetworkAttachment().getId()); Line 51: params.getRemovedNetworkAttachments().add(networkAttachmentForRemove.getId()); Line 52: VdcReturnValueBase returnValue = runInternalAction(VdcActionType.HostSetupNetworks, params); Line 53: propagateFailure(returnValue); https://gerrit.ovirt.org/#/c/34971/28/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java: Line 28: addValidationGroup(UpdateEntity.class); Line 29: } Line 30: Line 31: @Override Line 32: protected boolean canDoAction() { You have to add validation to make sure the passed attachment contains id (you actually updating and not and not adding a new one). Line 33: // VDS host = getVds(); Line 34: // Line 35: // List<String> hostValidationMessages = new HostValidator(host, isInternalExecution()).validate(); Line 36: // if (!hostValidationMessages.isEmpty()) { Line 29: } Line 30: Line 31: @Override Line 32: protected boolean canDoAction() { Line 33: // VDS host = getVds(); Please remove the commented code. Line 34: // Line 35: // List<String> hostValidationMessages = new HostValidator(host, isInternalExecution()).validate(); Line 36: // if (!hostValidationMessages.isEmpty()) { Line 37: // getReturnValue().getCanDoActionMessages().addAll(hostValidationMessages); Line 61: propagateFailure(returnValue); Line 62: setSucceeded(returnValue.getSucceeded()); Line 63: } Line 64: Line 65: // private boolean validateCrossAttachments() { Please remove the commented code. Line 66: // List<NetworkAttachment> expectedAttachments = getExpectedNetworkAttachments(); Line 67: // NetworkAttachmentsValidator crossAttachmentsValidator = Line 68: // new NetworkAttachmentsValidator(expectedAttachments, Line 69: // Entities.businessEntitiesById(getNetworkDAO().getAllForCluster(getVdsGroupId()))); -- 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
