From 78e4c3d3d06b411a1bc9e60ee8bf0c460d4453b2 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 25 May 2021 16:58:28 +0200 Subject: [PATCH 1/2] core,libnm: don't touch device TC configuration by default NetworkManager supports a very limited set of qdiscs. If users want to configure a unsupported qdisc, they need to do it outside of NetworkManager using tc. The problem is that NM also removes all qdiscs and filters during activation if the connection doesn't contain a TC setting. Therefore, setting TC configuration outside of NM is hard because users need to do it *after* the connection is up (for example through a dispatcher script). Let NM consider the presence (or absence) of a TC setting in the connection to determine whether NM should configure (or not) qdiscs and filters on the interface. We already do something similar for SR-IOV configuration. Since new connections don't have the TC setting, the new behavior (ignore existing configuration) will be the default. The impact of this change in different scenarios is: - the user previously configured TC settings via NM. This continues to work as before; - the user didn't set any qdiscs or filters in the connection, and expected NM to clear them from the interface during activation. Here there is a change in behavior, but it seems unlikely that anybody relied on the old one; - the user didn't care about qdiscs and filters; NM removed all qdiscs upon activation, and so the default qdisc from kernel was used. After this change, NM will not touch qdiscs and the default qdisc will be used, as before; - the user set a different qdisc via tc and NM cleared it during activation. Now this will work as expected. So, the new default behavior seems better than the previous one. https://bugzilla.redhat.com/show_bug.cgi?id=1928078 (cherry picked from commit a48edd0410c878d65fc5adcd5192b116ab6f8afc) (cherry picked from commit 2a8181bcd78d055b7cb9e6c0e026bc3b08231b5a) --- .../generate-docs-nm-settings-nmcli.xml.in | 4 +-- clients/common/settings-docs.h.in | 4 +-- libnm-core/nm-setting-tc-config.c | 16 ++++++++++++ src/core/devices/nm-device.c | 26 +++++++++---------- 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/clients/cli/generate-docs-nm-settings-nmcli.xml.in b/clients/cli/generate-docs-nm-settings-nmcli.xml.in index 1044ae0d38..0a75a0e681 100644 --- a/clients/cli/generate-docs-nm-settings-nmcli.xml.in +++ b/clients/cli/generate-docs-nm-settings-nmcli.xml.in @@ -914,9 +914,9 @@ + description="Array of TC queueing disciplines. When the "tc" setting is present, qdiscs from this property are applied upon activation. If the property is empty, all qdiscs are removed and the device will only have the default qdisc assigned by kernel according to the "net.core.default_qdisc" sysctl. If the "tc" setting is not present, NetworkManager doesn't touch the qdiscs present on the interface." /> + description="Array of TC traffic filters. When the "tc" setting is present, filters from this property are applied upon activation. If the property is empty, NetworkManager removes all the filters. If the "tc" setting is not present, NetworkManager doesn't touch the filters present on the interface." /> Date: Tue, 25 May 2021 18:00:27 +0200 Subject: [PATCH 2/2] ifcfg-rh: preserve an empty tc configuration If the TC setting contains no qdiscs and filters, it is lost after a write-read cycle. Fix this by adding a new property to indicate the presence of the (empty) setting. (cherry picked from commit 6a88d4e55cf031da2b5a8458d21487a011357da4) (cherry picked from commit acf0c4df2b0fb0dc332aa929131953390998828f) --- Makefile.am | 1 + libnm-core/nm-setting-tc-config.c | 14 +++- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 3 +- .../plugins/ifcfg-rh/nms-ifcfg-rh-utils.c | 1 + .../plugins/ifcfg-rh/nms-ifcfg-rh-utils.h | 2 +- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 38 +++++---- .../ifcfg-test-tc-write-empty.cexpected | 15 ++++ .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 80 +++++++++++++++++++ 8 files changed, 128 insertions(+), 26 deletions(-) create mode 100644 src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected diff --git a/Makefile.am b/Makefile.am index 9279672c1f..c8e417729b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3300,6 +3300,7 @@ EXTRA_DIST += \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-static-routes-legacy.cexpected \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write.cexpected \ + src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-team-master-1 \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-team-master-2 \ src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-team-master-invalid \ diff --git a/libnm-core/nm-setting-tc-config.c b/libnm-core/nm-setting-tc-config.c index 2fad98b1e8..31e829c1d2 100644 --- a/libnm-core/nm-setting-tc-config.c +++ b/libnm-core/nm-setting-tc-config.c @@ -1822,8 +1822,11 @@ nm_setting_tc_config_class_init(NMSettingTCConfigClass *klass) **/ /* ---ifcfg-rh--- * property: qdiscs - * variable: QDISC1(+), QDISC2(+), ... - * description: Queueing disciplines + * variable: QDISC1(+), QDISC2(+), ..., TC_COMMIT(+) + * description: Queueing disciplines to set on the interface. When no + * QDISC1, QDISC2, ..., FILTER1, FILTER2, ... keys are present, + * NetworkManager doesn't touch qdiscs and filters present on the + * interface, unless TC_COMMIT is set to 'yes'. * example: QDISC1=ingress, QDISC2="root handle 1234: fq_codel" * ---end--- */ @@ -1853,8 +1856,11 @@ nm_setting_tc_config_class_init(NMSettingTCConfigClass *klass) **/ /* ---ifcfg-rh--- * property: qdiscs - * variable: FILTER1(+), FILTER2(+), ... - * description: Traffic filters + * variable: FILTER1(+), FILTER2(+), ..., TC_COMMIT(+) + * description: Traffic filters to set on the interface. When no + * QDISC1, QDISC2, ..., FILTER1, FILTER2, ... keys are present, + * NetworkManager doesn't touch qdiscs and filters present on the + * interface, unless TC_COMMIT is set to 'yes'. * example: FILTER1="parent ffff: matchall action simple sdata Input", ... * ---end--- */ diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 209957d9b8..a42c418884 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -2707,7 +2707,8 @@ make_tc_setting(shvarFile *ifcfg) } if (nm_setting_tc_config_get_num_qdiscs(s_tc) > 0 - || nm_setting_tc_config_get_num_tfilters(s_tc) > 0) + || nm_setting_tc_config_get_num_tfilters(s_tc) > 0 + || svGetValueBoolean(ifcfg, "TC_COMMIT", FALSE)) return NM_SETTING(s_tc); g_object_unref(s_tc); diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c index 8da5de473b..ada1942acb 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c @@ -1026,6 +1026,7 @@ const NMSIfcfgKeyTypeInfo nms_ifcfg_well_known_keys[] = { _KEY_TYPE("STABLE_ID", NMS_IFCFG_KEY_TYPE_IS_PLAIN), _KEY_TYPE("STP", NMS_IFCFG_KEY_TYPE_IS_PLAIN), _KEY_TYPE("SUBCHANNELS", NMS_IFCFG_KEY_TYPE_IS_PLAIN), + _KEY_TYPE("TC_COMMIT", NMS_IFCFG_KEY_TYPE_IS_PLAIN), _KEY_TYPE("TEAM_CONFIG", NMS_IFCFG_KEY_TYPE_IS_PLAIN), _KEY_TYPE("TEAM_MASTER", NMS_IFCFG_KEY_TYPE_IS_PLAIN), _KEY_TYPE("TEAM_MASTER_UUID", NMS_IFCFG_KEY_TYPE_IS_PLAIN), diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h index 36ec922514..04a1b63d3e 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h @@ -33,7 +33,7 @@ typedef struct { NMSIfcfgKeyTypeFlags key_flags; } NMSIfcfgKeyTypeInfo; -extern const NMSIfcfgKeyTypeInfo nms_ifcfg_well_known_keys[247]; +extern const NMSIfcfgKeyTypeInfo nms_ifcfg_well_known_keys[248]; const NMSIfcfgKeyTypeInfo *nms_ifcfg_well_known_key_find_info(const char *key, gssize *out_idx); diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index a968fce0ba..65bacb293a 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -2511,46 +2511,46 @@ write_sriov_setting(NMConnection *connection, shvarFile *ifcfg) } } -static gboolean -write_tc_setting(NMConnection *connection, shvarFile *ifcfg, GError **error) +static void +write_tc_setting(NMConnection *connection, shvarFile *ifcfg) { NMSettingTCConfig *s_tc; - guint i, num, n; + guint num_qdiscs; + guint num_filters; + guint i; + guint n; char tag[64]; s_tc = nm_connection_get_setting_tc_config(connection); if (!s_tc) - return TRUE; + return; - num = nm_setting_tc_config_get_num_qdiscs(s_tc); - for (n = 1, i = 0; i < num; i++) { + num_qdiscs = nm_setting_tc_config_get_num_qdiscs(s_tc); + for (n = 1, i = 0; i < num_qdiscs; i++) { NMTCQdisc * qdisc; gs_free char *str = NULL; qdisc = nm_setting_tc_config_get_qdisc(s_tc, i); - str = nm_utils_tc_qdisc_to_str(qdisc, error); - if (!str) - return FALSE; - + str = nm_utils_tc_qdisc_to_str(qdisc, NULL); + nm_assert(str); svSetValueStr(ifcfg, numbered_tag(tag, "QDISC", n), str); n++; } - num = nm_setting_tc_config_get_num_tfilters(s_tc); - for (n = 1, i = 0; i < num; i++) { + num_filters = nm_setting_tc_config_get_num_tfilters(s_tc); + for (n = 1, i = 0; i < num_filters; i++) { NMTCTfilter * tfilter; gs_free char *str = NULL; tfilter = nm_setting_tc_config_get_tfilter(s_tc, i); - str = nm_utils_tc_tfilter_to_str(tfilter, error); - if (!str) - return FALSE; - + str = nm_utils_tc_tfilter_to_str(tfilter, NULL); + nm_assert(str); svSetValueStr(ifcfg, numbered_tag(tag, "FILTER", n), str); n++; } - return TRUE; + if (num_qdiscs == 0 && num_filters == 0) + svSetValueBoolean(ifcfg, "TC_COMMIT", TRUE); } static void @@ -3373,9 +3373,7 @@ do_write_construct(NMConnection * connection, write_match_setting(connection, ifcfg); write_hostname_setting(connection, ifcfg); write_sriov_setting(connection, ifcfg); - - if (!write_tc_setting(connection, ifcfg, error)) - return FALSE; + write_tc_setting(connection, ifcfg); route_path_is_svformat = utils_has_route_file_new_syntax(route_path); diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected new file mode 100644 index 0000000000..4df768b463 --- /dev/null +++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected @@ -0,0 +1,15 @@ +TYPE=Ethernet +PROXY_METHOD=none +BROWSER_ONLY=no +TC_COMMIT=yes +BOOTPROTO=none +IPADDR=1.1.1.3 +PREFIX=24 +GATEWAY=1.1.1.1 +DEFROUTE=yes +IPV4_FAILURE_FATAL=no +IPV6INIT=no +NAME="Test Write TC config" +UUID=${UUID} +DEVICE=eth0 +ONBOOT=yes diff --git a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 59127d0103..9d9ed62653 100644 --- a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -11108,6 +11108,85 @@ test_tc_read(void) g_object_unref(connection); } +static void +test_tc_write_empty(void) +{ + nmtst_auto_unlinkfile char *testfile = NULL; + gs_unref_object NMConnection *connection = NULL; + gs_unref_object NMConnection *reread = NULL; + NMSettingConnection * s_con; + NMSettingIPConfig * s_ip4; + NMSettingIPConfig * s_ip6; + NMSettingWired * s_wired; + NMSettingTCConfig * s_tc; + NMIPAddress * addr; + GError * error = NULL; + + connection = nm_simple_connection_new(); + + /* Connection setting */ + s_con = (NMSettingConnection *) nm_setting_connection_new(); + nm_connection_add_setting(connection, NM_SETTING(s_con)); + + g_object_set(s_con, + NM_SETTING_CONNECTION_ID, + "Test Write TC config", + NM_SETTING_CONNECTION_UUID, + nm_utils_uuid_generate_a(), + NM_SETTING_CONNECTION_AUTOCONNECT, + TRUE, + NM_SETTING_CONNECTION_INTERFACE_NAME, + "eth0", + NM_SETTING_CONNECTION_TYPE, + NM_SETTING_WIRED_SETTING_NAME, + NULL); + + /* Wired setting */ + s_wired = (NMSettingWired *) nm_setting_wired_new(); + nm_connection_add_setting(connection, NM_SETTING(s_wired)); + + /* IP4 setting */ + s_ip4 = (NMSettingIPConfig *) nm_setting_ip4_config_new(); + nm_connection_add_setting(connection, NM_SETTING(s_ip4)); + + g_object_set(s_ip4, + NM_SETTING_IP_CONFIG_METHOD, + NM_SETTING_IP4_CONFIG_METHOD_MANUAL, + NM_SETTING_IP_CONFIG_GATEWAY, + "1.1.1.1", + NM_SETTING_IP_CONFIG_MAY_FAIL, + TRUE, + NULL); + + addr = nm_ip_address_new(AF_INET, "1.1.1.3", 24, &error); + g_assert_no_error(error); + nm_setting_ip_config_add_address(s_ip4, addr); + nm_ip_address_unref(addr); + + /* IP6 setting */ + s_ip6 = (NMSettingIPConfig *) nm_setting_ip6_config_new(); + nm_connection_add_setting(connection, NM_SETTING(s_ip6)); + + g_object_set(s_ip6, NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NULL); + + /* TC setting */ + s_tc = (NMSettingTCConfig *) nm_setting_tc_config_new(); + nm_connection_add_setting(connection, NM_SETTING(s_tc)); + + nm_connection_add_setting(connection, nm_setting_proxy_new()); + + nmtst_assert_connection_verifies_without_normalization(connection); + + _writer_new_connec_exp(connection, + TEST_SCRATCH_DIR, + TEST_IFCFG_DIR "/ifcfg-test-tc-write-empty.cexpected", + &testfile); + + reread = _connection_from_file(testfile, NULL, TYPE_BOND, NULL); + + nmtst_assert_connection_equals(connection, FALSE, reread, FALSE); +} + static void test_tc_write(void) { @@ -11848,6 +11927,7 @@ main(int argc, char **argv) g_test_add_func(TPATH "tc/read", test_tc_read); g_test_add_func(TPATH "tc/write", test_tc_write); + g_test_add_func(TPATH "tc/write_empty", test_tc_write_empty); g_test_add_func(TPATH "utils/test_well_known_keys", test_well_known_keys); g_test_add_func(TPATH "utils/test_utils_has_route_file_new_syntax", test_utils_has_route_file_new_syntax); -- 2.31.1