Lior Vernia has uploaded a new change for review. Change subject: engine: Ensure that all networks on NIC have QoS ......................................................................
engine: Ensure that all networks on NIC have QoS Becuase of various damaging edge cases where some networks have QoS configured on top of one logical interface while others don't, this blocks a setup networks operation if only some of the networks on the interface have it configured. Change-Id: I59bf1bd6fcd26c3229cf0068d06e643b1abe1003 Signed-off-by: Lior Vernia <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java M backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties M backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommand.java M frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java M frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties 8 files changed, 102 insertions(+), 22 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/32/34132/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java index 7ec2715..9ee7a92 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java @@ -250,21 +250,42 @@ * with it are valid. */ private void validateNetworkQos() { - for (VdsNetworkInterface iface : params.getInterfaces()) { - if (iface.isQosOverridden()) { - if (!hostNetworkQosSupported) { - addViolation(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NETWORK_QOS_NOT_SUPPORTED, iface.getNetworkName()); - } + Set<String> someSubInterfacesHaveQos = new HashSet<>(); + Set<String> notAllSubInterfacesHaveQos = new HashSet<>(); - HostNetworkQosValidator qosValidator = new HostNetworkQosValidator(iface.getQos()); - if (qosValidator.requiredValuesPresent() != ValidationResult.VALID) { - addViolation(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_MISSING_VALUES, - iface.getNetworkName()); + for (VdsNetworkInterface iface : params.getInterfaces()) { + String networkName = iface.getNetworkName(); + if (networkName == null) { + continue; + } + + Network network = existingClusterNetworks.get(networkName); + String baseIfaceName = NetworkUtils.stripVlan(iface); + if (NetworkUtils.qosConfiguredOnInterface(iface, network)) { + someSubInterfacesHaveQos.add(baseIfaceName); + if (iface.isQosOverridden()) { + if (!hostNetworkQosSupported) { + addViolation(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NETWORK_QOS_NOT_SUPPORTED, networkName); + } + + HostNetworkQosValidator qosValidator = new HostNetworkQosValidator(iface.getQos()); + if (qosValidator.requiredValuesPresent() != ValidationResult.VALID) { + addViolation(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_MISSING_VALUES, + networkName); + } + if (qosValidator.valuesConsistent() != ValidationResult.VALID) { + addViolation(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_INCONSISTENT_VALUES, + networkName); + } } - if (qosValidator.valuesConsistent() != ValidationResult.VALID) { - addViolation(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_INCONSISTENT_VALUES, - iface.getNetworkName()); - } + } else { + notAllSubInterfacesHaveQos.add(baseIfaceName); + } + } + + for (String ifaceName : someSubInterfacesHaveQos) { + if (notAllSubInterfacesHaveQos.contains(ifaceName)) { + addViolation(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INTERFACES_WITHOUT_QOS, ifaceName); } } } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java index 8cca010..ff911f5 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java @@ -371,6 +371,7 @@ VdsNetworkInterface iface = createNicSyncedWithNetwork("eth0", network); mockExistingIfaces(iface); iface.setQosOverridden(true); + iface.setQos(createQos()); SetupNetworksHelper helper = createHelper(createParametersForNics(iface)); @@ -407,6 +408,49 @@ validateAndAssertNetworkModified(helper, network); } + private SetupNetworksHelper setupCompositeQosConfiguration(String nicName, boolean qosOnAll) { + Network net1 = createNetwork("net1"); + net1.setVlanId(100); + Network net2 = createNetwork("net2"); + net2.setVlanId(200); + Network net3 = createNetwork("net3"); + net3.setVmNetwork(false); + + Guid qosId = Guid.newGuid(); + when(qosDao.get(qosId)).thenReturn(createQos()); + net1.setQosId(qosId); + net2.setQosId(qosId); + net3.setQosId(qosOnAll ? qosId : null); + + VdsNetworkInterface nic = createNic(nicName, "net3"); + VdsNetworkInterface vlan1 = createVlan(nic.getName(), net1.getVlanId(), net1.getName()); + VdsNetworkInterface vlan2 = createVlan(nic.getName(), net2.getVlanId(), net2.getName()); + + mockExistingNetworks(net1, net2, net3); + mockExistingIfaces(nic); + + return createHelper(createParametersForNics(nic, vlan1, vlan2), Version.v3_4); + } + + private SetupNetworksHelper setupCompositeQosConfiguration(boolean qosOnAll) { + return setupCompositeQosConfiguration("nic0", qosOnAll); + } + + @Test + public void qosConfiguredOnAllNetworks() { + SetupNetworksHelper helper = setupCompositeQosConfiguration(true); + validateAndExpectNoViolations(helper); + } + + @Test + public void qosNotConfiguredOnAllNetworks() { + String nicName = "nic0"; + SetupNetworksHelper helper = setupCompositeQosConfiguration(nicName, false); + validateAndExpectViolation(helper, + VdcBllMessages.ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INTERFACES_WITHOUT_QOS, + nicName); + } + @Test public void customPropertiesNotSupported() { Network network = createNetwork(MANAGEMENT_NETWORK_NAME); diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java index 57a40c7..61e6031 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java @@ -1002,6 +1002,7 @@ ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_MISSING_VALUES(ErrorType.BAD_PARAMETERS), ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INCONSISTENT_VALUES(ErrorType.BAD_PARAMETERS), ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_INCONSISTENT_VALUES(ErrorType.BAD_PARAMETERS), + ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INTERFACES_WITHOUT_QOS(ErrorType.BAD_PARAMETERS), // Alignment scan ERROR_CANNOT_RUN_ALIGNMENT_SCAN_VM_IS_RUNNING(ErrorType.CONFLICT), diff --git a/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties b/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties index 27548c2..6aa075f 100644 --- a/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties +++ b/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties @@ -1148,6 +1148,7 @@ ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_MISSING_VALUES=Cannot ${action} ${type}. Weighted share must be specified to complete QoS configuration, but the following network(s) are missing it: ${ACTION_TYPE_FAILED_HOST_NETWORK_QOS_MISSING_VALUES_LIST}. ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INCONSISTENT_VALUES=Cannot ${action} ${type}. If both are provided, rate limit must not be lower than committed rate. ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_INCONSISTENT_VALUES=Cannot ${action} ${type}. If both are provided, rate limit must not be lower than committed rate. However, this is not the case with the following network(s): ${ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_INCONSISTENT_VALUES_LIST}. +ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INTERFACES_WITHOUT_QOS=Cannot ${action} ${type}. All or none of the networks attached to an interface must have QoS configured, but on the following interface(s) some of the networks are missing QoS: ${ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INTERFACES_WITHOUT_QOS_LIST}. QOS_NAME_NOT_NULL=QoS name cannot be empty. QOS_NAME_INVALID=Invalid QoS name (name must be formed of "a-z0-9A-Z" or "-_ ") QOS_NAME_TOO_LONG=QoS name length must be under 50 characters. diff --git a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java index 749483c..c9f35b0 100644 --- a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java +++ b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java @@ -136,6 +136,23 @@ } } + /** + * Check whether the interface has any QoS configured on it, whether via its attached network or overridden. + * + * @param iface + * The network interface. + * @param network + * The network attached to the interface. + * @return true iff any QoS is applied to the interface (though it might be unlimited). + */ + public static boolean qosConfiguredOnInterface(VdsNetworkInterface iface, Network network) { + if (iface.isQosOverridden()) { + return iface.getQos() != null; + } else { + return network != null && network.getQosId() != null; + } + } + public static boolean isNetworkInSync(VdsNetworkInterface iface, Network network, HostNetworkQos qos) { return ((network.getMtu() == 0 && iface.getMtu() == getDefaultMtu()) || iface.getMtu() == network.getMtu()) && Objects.equals(iface.getVlanId(), network.getVlanId()) diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommand.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommand.java index 2625fe0..84fbaa6 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommand.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommand.java @@ -84,7 +84,7 @@ opts.put(VdsProperties.STP, network.getStp() ? "yes" : "no"); } - if (hostNetworkQosSupported && qosConfiguredOnInterface(iface, network)) { + if (hostNetworkQosSupported && NetworkUtils.qosConfiguredOnInterface(iface, network)) { HostNetworkQosMapper qosMapper = new HostNetworkQosMapper(opts); qosMapper.serialize(iface.isQosOverridden() ? iface.getQos() : qosDao.get(network.getQosId())); } @@ -109,14 +109,6 @@ } return networks; - } - - private boolean qosConfiguredOnInterface(VdsNetworkInterface iface, Network network) { - if (iface.isQosOverridden()) { - return iface.getQos() != null; - } else { - return network.getQosId() != null; - } } private void addBootProtocol(Map<String, Object> opts, VdsNetworkInterface iface) { diff --git a/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java b/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java index dacef7a..a8b4872 100644 --- a/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java +++ b/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java @@ -3092,6 +3092,9 @@ @DefaultStringValue("Cannot ${action} ${type}. If both are provided, rate limit must not be lower than committed rate. However, this is not the case with the following network(s): ${ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_INCONSISTENT_VALUES_LIST}.") String ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_INCONSISTENT_VALUES(); + @DefaultStringValue("Cannot ${action} ${type}. All or none of the networks attached to an interface must have QoS configured, but on the following interface(s) some of the networks are missing QoS: ${ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INTERFACES_WITHOUT_QOS_LIST}.") + String ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INTERFACES_WITHOUT_QOS(); + @DefaultStringValue("Cannot ${action} ${type}. Values are out of range.") String ACTION_TYPE_FAILED_QOS_OUT_OF_RANGE_VALUES(); diff --git a/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties b/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties index 2b9b45e..350fe34 100644 --- a/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties +++ b/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties @@ -1117,6 +1117,7 @@ ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_MISSING_VALUES=Cannot ${action} ${type}. Weighted share must be specified to complete QoS configuration, but the following network(s) are missing it: ${ACTION_TYPE_FAILED_HOST_NETWORK_QOS_MISSING_VALUES_LIST}. ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INCONSISTENT_VALUES=Cannot ${action} ${type}. If both are provided, rate limit must not be lower than committed rate. ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_INCONSISTENT_VALUES=Cannot ${action} ${type}. If both are provided, rate limit must not be lower than committed rate. However, this is not the case with the following network(s): ${ACTION_TYPE_FAILED_HOST_NETWORK_QOS_SETUP_NETWORKS_INCONSISTENT_VALUES_LIST}. +ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INTERFACES_WITHOUT_QOS=Cannot ${action} ${type}. All or none of the networks attached to an interface must have QoS configured, but on the following interface(s) some of the networks are missing QoS: ${ACTION_TYPE_FAILED_HOST_NETWORK_QOS_INTERFACES_WITHOUT_QOS_LIST}. QOS_NAME_NOT_NULL=QoS name cannot be empty. QOS_NAME_INVALID=Invalid QoS name (name must be formed of "a-z0-9A-Z" or "-_ ") QOS_NAME_TOO_LONG=QoS name length must be under 50 characters. -- To view, visit http://gerrit.ovirt.org/34132 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I59bf1bd6fcd26c3229cf0068d06e643b1abe1003 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
