Alona Kaplan has posted comments on this change. Change subject: engine: Refactor AttachNetworksToClusterCommand ......................................................................
Patch Set 22: (22 comments) http://gerrit.ovirt.org/#/c/33684/22//COMMIT_MSG Commit Message: Line 5: CommitDate: 2014-11-13 17:42:21 +0200 Line 6: Line 7: engine: Refactor AttachNetworksToClusterCommand Line 8: Line 9: Make network management network attacments be processed first. Please update the comment according to the updates in the code- -Attaching the management network first is the responsibility of the client. Otherwise, the command will fail. Line 10: Refactor AttachNetworkToVdsGroupCommand: split the command into Line 11: 2 internal commands: Line 12: * AttachNetworkToClusterCommand Line 13: * PropagateLabeledNetworksToClusterHostsCommand Line 8: Line 9: Make network management network attacments be processed first. Line 10: Refactor AttachNetworkToVdsGroupCommand: split the command into Line 11: 2 internal commands: Line 12: * AttachNetworkToClusterCommand Same here- the name of this class was changed. Line 13: * PropagateLabeledNetworksToClusterHostsCommand Line 14: Now AttachNetworkToVdsGroupCommand and AttachNetworksToClusterCommand Line 15: are implemented as a compositions of the 2 internal ones. Line 16: http://gerrit.ovirt.org/#/c/33684/22/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java: All the changes in this class have to be in a separate infra patch and also reviewed by an infra maintainter. Line 1: package org.ovirt.engine.core.bll; Line 2: Line 3: import java.io.Serializable; Line 4: import java.lang.reflect.Method; Line 2298: return MacPoolPerDcSingleton.getInstance().poolForDataCenter(getStoragePoolId()); Line 2299: } Line 2300: Line 2301: protected void runInternalActionAsynchronously(VdcActionType actionType, T parameter) { Line 2302: runInternalMultipleActions(actionType, wrapParametersWithList(parameter)); 1. Why not just calling- runInternalMultipleActions(actionType, new ArrayList<VdcActionParametersBase>(Collections.singletonList(parameter))); Why do you need the wrap method? 2. If you'll listen to my advices and will decide not to call PropagateLabeledNetworksToClusterHostsCommand in a separate thread (see the comments on AttachNetworksToClusterCommand and AttachNetworkToVdsGroupCommand) this method won't have any use case, a therefore should be removed. Line 2303: } Line 2304: Line 2305: private ArrayList<VdcActionParametersBase> wrapParametersWithList(VdcActionParametersBase parameters) { Line 2306: final ArrayList<VdcActionParametersBase> resultList = new ArrayList<>(1); Line 2317: * @param parameters Line 2318: * parameters list. The size of the {@link List} defines the number of iteration on the action. Line 2319: * @return <code>TRUE</code> on success otherwise <code>FALSE</code>. Line 2320: */ Line 2321: protected <P extends VdcActionParametersBase> boolean runMultipleInternalActionsSequentiallyInNewTransaction( This method is never used. I don't see any reason to keep it. Line 2322: final VdcActionType actionType, Line 2323: final List<P> parameters) { Line 2324: return TransactionSupport.executeInNewTransaction(new TransactionMethod<Boolean>() { Line 2325: Line 2346: * @param parameters Line 2347: * parameters list. The size of the {@link List} defines the number of iteration on the action. Line 2348: * @return the {@link List} of parameters that were processed successfully Line 2349: */ Line 2350: protected <P extends VdcActionParametersBase> List<P> runMultipleInternalActionsSequentially( I think you should implement this method internally wherever you need it. It is not a general use case. Generally, you don't know if the internal actions modify the parameters list. And if it is, it is problematic to later rely on this list. Line 2351: final VdcActionType actionType, Line 2352: final List<P> parameters) { Line 2353: Line 2354: final List<P> successfullParametersList = new ArrayList<>(parameters.size()); http://gerrit.ovirt.org/#/c/33684/22/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachNetworkFromClusterMultipleActionRunner.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachNetworkFromClusterMultipleActionRunner.java: Line 22: Line 23: public DetachNetworkFromClusterMultipleActionRunner(VdcActionType actionType, Line 24: List<VdcActionParametersBase> parameters, Line 25: boolean isInternal, Line 26: CommandContext commandContext) { Please format with the regular formatter. Line 27: super(actionType, parameters, commandContext, isInternal); Line 28: } Line 29: Line 30: protected void runCommands() { Line 48: super.runCommands(); Line 49: return; Line 50: } Line 51: Line 52: // multiple networks can be either attached or detached from a single cluster Please fix the comment (remove the attached...) Line 53: if (!params.isEmpty()) { Line 54: Backend.getInstance().runInternalAction(VdcActionType.DetachNetworksFromCluster, Line 55: new ClusterNetworksParameters(params.get(0).getVdsGroupId(), params), commandContext); Line 56: } http://gerrit.ovirt.org/#/c/33684/22/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterInternalCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterInternalCommand.java: Line 54: } Line 55: Line 56: private boolean validateAttachment() { Line 57: final AttachNetworkClusterValidator networkClusterValidator = createNetworkClusterValidator(); Line 58: return validate(networkClusterValidator.networkBelongsToClusterDataCenter(getVdsGroup(), getPersistedNetwork())); You should also call to super.validateAttachment(networkClusterValidator, network), the are lot of validations that need to be performed. Line 59: } Line 60: Line 61: private AttachNetworkClusterValidator createNetworkClusterValidator() { Line 62: return new AttachNetworkClusterValidator(getNetworkCluster(), getClusterVersion()); http://gerrit.ovirt.org/#/c/33684/22/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java: Line 36: propagateFailure(returnValue); Line 37: } Line 38: } Line 39: Line 40: private void attachLabeledNetworks() { I think the previous name attachNetworkToHosts was better. Line 41: final AttachNetworkToVdsGroupParameter parameters = getParameters(); Line 42: Line 43: // AttachLabeledNetworks should be run asynchronously Line 44: runInternalMultipleActions( Line 40: private void attachLabeledNetworks() { Line 41: final AttachNetworkToVdsGroupParameter parameters = getParameters(); Line 42: Line 43: // AttachLabeledNetworks should be run asynchronously Line 44: runInternalMultipleActions( 1. You can use the runInternalActionAsynchronously.runInternalActionAsynchronously. A new method you introduce in this patch. But- 2. I'm not sure there's a real need for a new thread here. Since PropagateLabeledNetworksToClusterHosts calls PersistentSetupNetworks in a multiple action each PersistentSetupNetworks will run in a separate thread (see ParallelMultipleActionsRunner). I'm not sure the overhead of creating a new thread is not bigger than just running ParallelMultipleActionsRunner in the same thread. Except of the PersistentSetupNetworks call there in just a small calculation which in my opinion doesn't worse a thread. But as you wish... Line 45: VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 46: createParametersArrayList(parameters)); Line 47: } Line 48: Line 45: VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 46: createParametersArrayList(parameters)); Line 47: } Line 48: Line 49: private ArrayList<VdcActionParametersBase> createParametersArrayList(final AttachNetworkToVdsGroupParameter parameters) { See the previous comment. This method can be removed. Line 50: final ArrayList<VdcActionParametersBase> resultList = new ArrayList<>(); Line 51: resultList.add(parameters); Line 52: Line 53: return resultList; http://gerrit.ovirt.org/#/c/33684/22/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworksToClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworksToClusterCommand.java: Line 27: Line 28: setSucceeded(true); Line 29: Line 30: // PropagateLabeledNetworksToClusterHosts should be run asynchronously Line 31: runInternalActionAsynchronously( Please see the comment in AttachNetworkToVdsGroupCommand.java. I'm not sure there is a real need for a new thread here. Line 32: VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 33: new ClusterNetworksParameters(getParameters().getVdsGroupId(), successfullyProcessedParameters)); Line 34: } Line 35: http://gerrit.ovirt.org/#/c/33684/22/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/PropagateLabeledNetworksToClusterHostsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/PropagateLabeledNetworksToClusterHostsCommand.java: Line 25: import org.ovirt.engine.core.compat.Guid; Line 26: Line 27: @InternalCommandAttribute Line 28: @NonTransactiveCommandAttribute Line 29: public class PropagateLabeledNetworksToClusterHostsCommand<T extends ClusterNetworksParameters> extends Please format Line 30: VdsGroupCommandBase<T> { Line 31: Line 32: public PropagateLabeledNetworksToClusterHostsCommand(T parameters) { Line 33: super(parameters); Line 80: Line 81: return networks; Line 82: } Line 83: Line 84: private void configureNetworksOnHosts(Map<Guid, List<Network>> networksByHost, Please format Line 85: Map<Guid, Map<String, VdsNetworkInterface>> labelsToNicsByHost) { Line 86: ArrayList<VdcActionParametersBase> parameters = new ArrayList<>(); Line 87: for (Entry<Guid, List<Network>> entry : networksByHost.entrySet()) { Line 88: AddNetworksByLabelParametersBuilder builder = new AddNetworksByLabelParametersBuilder(getContext()); http://gerrit.ovirt.org/#/c/33684/22/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java: Line 1209: } Line 1210: Line 1211: } Line 1212: Line 1213: private static class ReversePredicate<T> implements IPredicate<T> { Why reverse? Maybe notPredicate or NegativePredicate? Line 1214: Line 1215: private final IPredicate<T> predicate; Line 1216: Line 1217: private ReversePredicate(IPredicate<T> predicate) { http://gerrit.ovirt.org/#/c/33684/22/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterNetworkManageModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterNetworkManageModel.java: Line 188: doFinish(); Line 189: } Line 190: Line 191: private void attachNetworks(final ArrayList<AttachNetworkToVdsGroupParameter> toAttach) { Line 192: final List<AttachNetworkToVdsGroupParameter> managementNetworkAtachements = What if the management network is set via the update. You should do the set of management network first, whether is is set via attach or update. On onSuccess of mgmgt set you should do all the other attached, updates and detaches. Line 193: Linq.where(toAttach, IS_MANAGEMENT_NETWORK_PARAMETER_PREDICATE); Line 194: final List<AttachNetworkToVdsGroupParameter> nonManagementNetworkAtachements = Line 195: Linq.where(toAttach, Linq.reversePredicate(IS_MANAGEMENT_NETWORK_PARAMETER_PREDICATE)); Line 196: attachManagementNetworks(managementNetworkAtachements); Line 196: attachManagementNetworks(managementNetworkAtachements); Line 197: attachNonManagementNetworksCommand(nonManagementNetworkAtachements); Line 198: } Line 199: Line 200: private void attachManagementNetworks(final List<AttachNetworkToVdsGroupParameter> parameters) { runAction is asynchronous. Calling one run action before the other doesn't mean the first action will be performed before the second. Most of the chances they will be performed at the same time in different threads. So, you don't guarantee here the mgmt attached will be performed first. To guarantee this you should call the second run from the first action's onSuccess method. Line 201: for (AttachNetworkToVdsGroupParameter param : parameters) { Line 202: Frontend.getInstance().runAction(VdcActionType.AttachNetworkToVdsGroup, param); Line 203: } Line 204: } Line 198: } Line 199: Line 200: private void attachManagementNetworks(final List<AttachNetworkToVdsGroupParameter> parameters) { Line 201: for (AttachNetworkToVdsGroupParameter param : parameters) { Line 202: Frontend.getInstance().runAction(VdcActionType.AttachNetworkToVdsGroup, param); what about the doFinish()? Line 203: } Line 204: } Line 205: Line 206: private void attachNonManagementNetworksCommand(final List<AttachNetworkToVdsGroupParameter> parameters) { Line 204: } Line 205: Line 206: private void attachNonManagementNetworksCommand(final List<AttachNetworkToVdsGroupParameter> parameters) { Line 207: if (!parameters.isEmpty()) { Line 208: Frontend.getInstance().runAction( Same here, what about the doFinish()? Line 209: VdcActionType.AttachNetworksToCluster, Line 210: new ClusterNetworksParameters(null, parameters)); Line 211: } Line 212: } Line 206: private void attachNonManagementNetworksCommand(final List<AttachNetworkToVdsGroupParameter> parameters) { Line 207: if (!parameters.isEmpty()) { Line 208: Frontend.getInstance().runAction( Line 209: VdcActionType.AttachNetworksToCluster, Line 210: new ClusterNetworksParameters(null, parameters)); ClusterNetworksParameters gets the clusterId. It means it should be used for networks on the same cluster. This is not the case here. So whether remove the clusterId from the parameters class or use a new class for your command. Line 211: } Line 212: } Line 213: Line 214: private void doFinish() { http://gerrit.ovirt.org/#/c/33684/22/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/predicate/IsManagementNetworkParameterPredicateTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/predicate/IsManagementNetworkParameterPredicateTest.java: Line 32: } Line 33: Line 34: @Test Line 35: public void notMatch() throws Exception { Line 36: Mockito.when(mockNetworkClusterParameters.getNetworkCluster()).thenReturn(mockNetworkCluster); Please do static import to- import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.when; Line 37: Assert.assertFalse(underTest.match(mockNetworkClusterParameters)); Line 38: } Line 39: Line 40: @Test -- To view, visit http://gerrit.ovirt.org/33684 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a0b86d8bb018a6172891cb357a4402cfef135d1 Gerrit-PatchSet: 22 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
