From 10f10d534c501adcdd264fc5237366125b2de607 Mon Sep 17 00:00:00 2001 From: Pim van Pelt Date: Sat, 14 Aug 2021 09:40:43 +0200 Subject: [PATCH] Ensure the plugin works well with namespaces I've made a few cosmetic adjustments: - introduce debug, info, notice, warn and err loggers - remove redundant logging, and set correct (conservative) log levels - turn the sync-state error into a warning And a little debt paydown: - refactor sync-state into its own function, to be called instead of all the spot fixes elsewhere. It's going to be the case that sync-state is "the reconsiliation function". - Fix a bug in lip->lip_namespace copy: vec_dup() there doesn't seem to work for me, use strdup() instead and make a mental note to reviist. The plugin now works with 'lcpng default netns dataplane' in its startup.conf; and with 'lcp default netns dataplane' as its first command. A few of these fixes should go upstream, too, which I'll do next. --- lcpng_if_sync.c | 51 ++++++++++++++++++++++-------------------- lcpng_interface.c | 56 +++++++++++++++++++++++------------------------ lcpng_interface.h | 16 +++++++++++++- 3 files changed, 69 insertions(+), 54 deletions(-) diff --git a/lcpng_if_sync.c b/lcpng_if_sync.c index afcb6b8..a054b6f 100644 --- a/lcpng_if_sync.c +++ b/lcpng_if_sync.c @@ -36,28 +36,23 @@ #include #include -/* walk function to copy forward all sw interface link state flags +/* helper function to copy forward all sw interface link state flags * MTU, and IP addresses into their counterpart LIP interface. * * This is called upon MTU changes and state changes. */ -static walk_rc_t -lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx) +void +lcp_itf_pair_sync_state (lcp_itf_pair_t *lip) { - lcp_itf_pair_t *lip; vnet_sw_interface_t *sw; vnet_sw_interface_t *sup_sw; int curr_ns_fd = -1; int vif_ns_fd = -1; - lip = lcp_itf_pair_get (lipi); - if (!lip) - return WALK_CONTINUE; - sw = vnet_get_sw_interface_or_null (vnet_get_main (), lip->lip_phy_sw_if_index); if (!sw) - return WALK_CONTINUE; + return; sup_sw = vnet_get_sw_interface_or_null (vnet_get_main (), sw->sup_sw_if_index); @@ -69,9 +64,9 @@ lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx) clib_setns (vif_ns_fd); } - LCP_ITF_PAIR_DBG ("walk_sync_state: %U flags %u mtu %u sup-mtu %u", - format_lcp_itf_pair, lip, sw->flags, sw->mtu[VNET_MTU_L3], - sup_sw->mtu[VNET_MTU_L3]); + LCP_ITF_PAIR_INFO ("sync_state: %U flags %u mtu %u sup-mtu %u", + format_lcp_itf_pair, lip, sw->flags, sw->mtu[VNET_MTU_L3], + sup_sw->mtu[VNET_MTU_L3]); lcp_itf_set_link_state (lip, (sw->flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP)); /* Linux will clamp MTU of children when the parent is lower. VPP is fine @@ -79,7 +74,7 @@ lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx) */ if (sup_sw->mtu[VNET_MTU_L3] < sw->mtu[VNET_MTU_L3]) { - LCP_ITF_PAIR_ERR ("walk_sync_state: %U flags %u mtu %u sup-mtu %u: " + LCP_ITF_PAIR_WARN ("sync_state: %U flags %u mtu %u sup-mtu %u: " "clamping to sup-mtu to satisfy netlink", format_lcp_itf_pair, lip, sw->flags, sw->mtu[VNET_MTU_L3], sup_sw->mtu[VNET_MTU_L3]); @@ -89,13 +84,9 @@ lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx) } /* Linux will remove IPv6 addresses on children when the master state - * goes down, so we ensure all IPv4/IPv6 addresses are set when the phy - * comes back up. + * goes down, so we ensure all IPv4/IPv6 addresses are synced. */ - if (sw->flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP) - { - lcp_itf_set_interface_addr (lip); - } + lcp_itf_set_interface_addr (lip); if (vif_ns_fd != -1) close (vif_ns_fd); @@ -106,6 +97,18 @@ lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx) close (curr_ns_fd); } + return; +} + +static walk_rc_t +lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx) +{ + lcp_itf_pair_t *lip; + lip = lcp_itf_pair_get (lipi); + if (!lip) + return WALK_CONTINUE; + + lcp_itf_pair_sync_state (lip); return WALK_CONTINUE; } @@ -171,7 +174,7 @@ static clib_error_t * lcp_itf_mtu_change (vnet_main_t *vnm, u32 sw_if_index, u32 flags) { const lcp_itf_pair_t *lip; - vnet_sw_interface_t *si; + vnet_sw_interface_t *sw; int curr_ns_fd = -1; int vif_ns_fd = -1; @@ -183,8 +186,8 @@ lcp_itf_mtu_change (vnet_main_t *vnm, u32 sw_if_index, u32 flags) if (!lip) return NULL; - si = vnet_get_sw_interface_or_null (vnm, sw_if_index); - if (!si) + sw = vnet_get_sw_interface_or_null (vnm, sw_if_index); + if (!sw) return NULL; if (lip->lip_namespace) @@ -196,8 +199,8 @@ lcp_itf_mtu_change (vnet_main_t *vnm, u32 sw_if_index, u32 flags) } LCP_ITF_PAIR_INFO ("mtu_change: %U mtu %u", format_lcp_itf_pair, lip, - si->mtu[VNET_MTU_L3]); - vnet_netlink_set_link_mtu (lip->lip_vif_index, si->mtu[VNET_MTU_L3]); + sw->mtu[VNET_MTU_L3]); + vnet_netlink_set_link_mtu (lip->lip_vif_index, sw->mtu[VNET_MTU_L3]); if (vif_ns_fd != -1) close (vif_ns_fd); diff --git a/lcpng_interface.c b/lcpng_interface.c index 85f0045..3f1b23e 100644 --- a/lcpng_interface.c +++ b/lcpng_interface.c @@ -129,8 +129,7 @@ lcp_itf_pair_show (u32 phy_sw_if_index) vm = vlib_get_main (); ns = lcp_get_default_ns(); - vlib_cli_output (vm, "lcpng netns '%s'\n", - ns ? (char *) ns : ""); + vlib_cli_output (vm, "lcp netns %s\n", ns ? (char *) ns : ""); if (phy_sw_if_index == ~0) { @@ -242,7 +241,7 @@ lcp_itf_pair_add (u32 host_sw_if_index, u32 phy_sw_if_index, u8 *host_name, lipi = lcp_itf_pair_find_by_phy (phy_sw_if_index); - LCP_ITF_PAIR_INFO ("add: host:%U phy:%U, host_if:%v vif:%d ns:%v", + LCP_ITF_PAIR_INFO ("pair_add: host:%U phy:%U, host_if:%v vif:%d ns:%s", format_vnet_sw_if_index_name, vnet_get_main (), host_sw_if_index, format_vnet_sw_if_index_name, vnet_get_main (), phy_sw_if_index, host_name, host_index, @@ -269,7 +268,11 @@ lcp_itf_pair_add (u32 host_sw_if_index, u32 phy_sw_if_index, u8 *host_name, lip->lip_host_name = vec_dup (host_name); lip->lip_host_type = host_type; lip->lip_vif_index = host_index; - lip->lip_namespace = vec_dup (ns); + + // TODO(pim) - understand why vec_dup(ns) returns 'nil' here + lip->lip_namespace = 0; + if (ns && ns[0] != 0) + lip->lip_namespace = (u8 *) strdup ((const char *) ns); if (lip->lip_host_sw_if_index == ~0) return 0; @@ -629,6 +632,9 @@ lcp_itf_set_interface_addr (const lcp_itf_pair_t *lip) int vif_ns_fd = -1; int curr_ns_fd = -1; + if (!lip) + return; + if (lip->lip_namespace) { curr_ns_fd = clib_netns_open (NULL /* self */); @@ -685,11 +691,13 @@ lcp_itf_pair_find_walk (vnet_main_t *vnm, u32 sw_if_index, void *arg) sw = vnet_get_sw_interface_or_null (vnm, sw_if_index); if (sw && (sw->sub.eth.inner_vlan_id == 0) && (sw->sub.eth.outer_vlan_id == match->vlan) && (sw->sub.eth.flags.dot1ad == match->dot1ad)) { - LCP_ITF_PAIR_DBG ("pair_create: found match %U outer %d dot1ad %d inner-dot1q %d", - format_vnet_sw_if_index_name, vnet_get_main (), sw_if_index, - sw->sub.eth.outer_vlan_id, sw->sub.eth.flags.dot1ad, sw->sub.eth.inner_vlan_id); - match->matched_sw_if_index = sw_if_index; - return WALK_STOP; + LCP_ITF_PAIR_DBG ( + "find_walk: found match %U outer %d dot1ad %d inner-dot1q %d", + format_vnet_sw_if_index_name, vnet_get_main (), sw_if_index, + sw->sub.eth.outer_vlan_id, sw->sub.eth.flags.dot1ad, + sw->sub.eth.inner_vlan_id); + match->matched_sw_if_index = sw_if_index; + return WALK_STOP; } return WALK_CONTINUE; @@ -752,7 +760,7 @@ lcp_itf_pair_create (u32 phy_sw_if_index, u8 *host_if_name, u32 vif_index = 0, host_sw_if_index; const vnet_sw_interface_t *sw; const vnet_hw_interface_t *hw; - const lcp_itf_pair_t *lip; + lcp_itf_pair_t *lip; if (!vnet_sw_if_index_is_api_valid (phy_sw_if_index)) { LCP_ITF_PAIR_ERR ("pair_create: invalid phy index %u", phy_sw_if_index); @@ -936,7 +944,9 @@ lcp_itf_pair_create (u32 phy_sw_if_index, u8 *host_if_name, } if (ns && ns[0] != 0) - args.host_namespace = ns; + { + args.host_namespace = ns; + } vm = vlib_get_main (); tap_create_if (vm, &args); @@ -983,18 +993,14 @@ lcp_itf_pair_create (u32 phy_sw_if_index, u8 *host_if_name, if (!vif_index) { - LCP_ITF_PAIR_INFO ("failed pair add (no vif index): {%U, %U, %s}", - format_vnet_sw_if_index_name, vnet_get_main (), - phy_sw_if_index, format_vnet_sw_if_index_name, - vnet_get_main (), host_sw_if_index, host_if_name); + LCP_ITF_PAIR_ERR ( + "pair_create: failed pair add (no vif index): {%U, %U, %s}", + format_vnet_sw_if_index_name, vnet_get_main (), phy_sw_if_index, + format_vnet_sw_if_index_name, vnet_get_main (), host_sw_if_index, + host_if_name); return -1; } - LCP_ITF_PAIR_INFO ("pair create: {%U, %U, %s}", - format_vnet_sw_if_index_name, vnet_get_main (), phy_sw_if_index, - format_vnet_sw_if_index_name, vnet_get_main (), host_sw_if_index, - host_if_name); - lcp_itf_pair_add (host_sw_if_index, phy_sw_if_index, host_if_name, vif_index, host_if_type, ns); @@ -1005,15 +1011,7 @@ lcp_itf_pair_create (u32 phy_sw_if_index, u8 *host_if_name, */ vnet_sw_interface_admin_up (vnm, host_sw_if_index); lip = lcp_itf_pair_get (lcp_itf_pair_find_by_vif(vif_index)); - LCP_ITF_PAIR_INFO ("pair_create: copy link state %u from %U to %U", - sw->flags, - format_vnet_sw_if_index_name, vnet_get_main (), sw->sw_if_index, - format_lcp_itf_pair, lip); - lcp_itf_set_link_state (lip, sw->flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP); - - /* Copy L3 addresses from VPP into the host side, if any. - */ - lcp_itf_set_interface_addr (lip); + lcp_itf_pair_sync_state (lip); if (host_sw_if_indexp) *host_sw_if_indexp = host_sw_if_index; diff --git a/lcpng_interface.h b/lcpng_interface.h index 8daf2d9..4dd0eb1 100644 --- a/lcpng_interface.h +++ b/lcpng_interface.h @@ -26,11 +26,17 @@ extern vlib_log_class_t lcp_itf_pair_logger; #define LCP_ITF_PAIR_DBG(...) \ - vlib_log_notice (lcp_itf_pair_logger, __VA_ARGS__); + vlib_log_debug (lcp_itf_pair_logger, __VA_ARGS__); #define LCP_ITF_PAIR_INFO(...) \ + vlib_log_info (lcp_itf_pair_logger, __VA_ARGS__); + +#define LCP_ITF_PAIR_NOTICE(...) \ vlib_log_notice (lcp_itf_pair_logger, __VA_ARGS__); +#define LCP_ITF_PAIR_WARN(...) \ + vlib_log_warn (lcp_itf_pair_logger, __VA_ARGS__); + #define LCP_ITF_PAIR_ERR(...) \ vlib_log_err (lcp_itf_pair_logger, __VA_ARGS__); @@ -187,6 +193,14 @@ void lcp_itf_ip6_add_del_interface_addr (ip6_main_t *im, uword opaque, u32 address_length, u32 if_address_index, u32 is_del); +/* Sync all state from VPP to Linux device + * + * Note: in some circumstances, this syncer will (have to) make changes to + * the VPP interface, for example if its MTU is greater than its parent. + * See the function for rationale. + */ +void lcp_itf_pair_sync_state (lcp_itf_pair_t *lip); + /* * fd.io coding-style-patch-verification: ON *