libteam April 2019

libteam@lists.fedorahosted.org

3 participants
6 discussions

[ISSUE] Failed to find "enabled" option.
by Xin Long 16 Sep '19

16 Sep '19

I found this err in "loadbalance" and "lacp" runner when adding ports. It's caused by trying to set "enabled" option in .port_link_changed() or .port_changed(). When a new port is added, the first 'port changed event' process is earlier than CMD TEAM_CMD_OPTIONS_GET, in this CMD, all the options are synchronized from kernel. It means there's no 'enabled' option yet when calling port_link_changed in the first 'port changed event' process. In lb_event_watch_port_link_changed and lacp_event_watch_port_changed, they call teamd_port_check_enable to set 'enabled' option. this err is triggered. I'm not sure why teamd_port_check_enable needs to check if 'enabled' option exists. I checked the ab's .port_link_changed(), it just sets it by calling team_set_port_enabled(), instead of checking 'enabled' option first. can we just use team_set_port_enabled to set it directly in .port(_link)_changed OR improve teamd_port_check_enable to avoid this err ? Thanks.

3 5

[PATCHv3] teamd: add a default value 1000 for link_watch.interval
by Xin Long 02 May '19

02 May '19

From: Hangbin Liu <liuhangbin(a)gmail.com> As we don't have a default value for link_watch.interval. If a user forgets to set this parameter, teamd will fail to init ports' priv and exit in the end. e.g. teamd -g -c '{"runner":{"name":"activebackup"}, "link_watch":{"name":"arp_ping","target_host":"198.51.100.1"}}' teamdctl team0 port add p5p1 teamdctl team0 port add p5p2 teamd debug log shows: p5p2: Got link watch from global config. p5p2: Using sticky "0". Failed to get "interval" link-watch option. Failed to load options. Failed to init port priv. Callback named "lw_periodic" not found. Callback named "lw_socket" not found. Loop callback failed with: Invalid argument Failed loop callback: libteam_events, 0x5624c28b9410 select() failed. Exiting... Removed loop callback: usock_acc_conn, 0x5624c28bab60 Removed loop callback: usock, 0x5624c28b9410 Removed loop callback: workq, 0x5624c28b9410 Removed loop callback: libteam_events, 0x5624c28b9410 Removed loop callback: daemon, 0x5624c28b9410 Failed: Bad file descriptor Fix it by adding a default value for link_watch.interval. v2: update default value to 1000, as Jamie Bainbridge suggested. v3: fix the changelog to pass checkpatch.pl. Reported-by: LiLiang <liali(a)redhat.com> Reviewed-by: Jamie Bainbridge <jamie.bainbridge(a)gmail.com> Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com> --- man/teamd.conf.5 | 10 ++++++++++ teamd/teamd_lw_psr.c | 11 ++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/man/teamd.conf.5 b/man/teamd.conf.5 index 5b0f3e9..9090b4a 100644 --- a/man/teamd.conf.5 +++ b/man/teamd.conf.5 @@ -308,6 +308,11 @@ Default: .TP .BR "link_watch.interval "| " ports.PORTIFNAME.link_watch.interval " (int) Value is a positive number in milliseconds. It is the interval between ARP requests being sent. +.RS 7 +.PP +Default: +.BR "1000" +.RE .TP .BR "link_watch.init_wait "| " ports.PORTIFNAME.link_watch.init_wait " (int) Value is a positive number in milliseconds. It is the delay between link watch initialization and the first ARP request being sent. @@ -371,6 +376,11 @@ Default: .TP .BR "link_watch.interval "| " ports.PORTIFNAME.link_watch.interval " (int) Value is a positive number in milliseconds. It is the interval between sending NS packets. +.RS 7 +.PP +Default: +.BR "1000" +.RE .TP .BR "link_watch.init_wait "| " ports.PORTIFNAME.link_watch.init_wait " (int) Value is a positive number in milliseconds. It is the delay between link watch initialization and the first NS packet being sent. diff --git a/teamd/teamd_lw_psr.c b/teamd/teamd_lw_psr.c index c0772db..ad6e56b 100644 --- a/teamd/teamd_lw_psr.c +++ b/teamd/teamd_lw_psr.c @@ -28,6 +28,7 @@ */ static const struct timespec lw_psr_default_init_wait = { 0, 1 }; +#define LW_PSR_DEFAULT_INTERVAL 1000 #define LW_PSR_DEFAULT_MISSED_MAX 3 #define LW_PERIODIC_CB_NAME "lw_periodic" @@ -77,9 +78,13 @@ static int lw_psr_load_options(struct teamd_context *ctx, int tmp; err = teamd_config_int_get(ctx, &tmp, "@.interval", cpcookie); - if (err) { - teamd_log_err("Failed to get \"interval\" link-watch option."); - return -EINVAL; + if (!err) { + if (tmp < 0) { + teamd_log_err("\"interval\" must not be negative number."); + return -EINVAL; + } + } else { + tmp = LW_PSR_DEFAULT_INTERVAL; } teamd_log_dbg("interval \"%d\".", tmp); ms_to_timespec(&psr_ppriv->interval, tmp); -- 2.1.0

3 2

[patch libteam] teamd: add a default value 100 for link_watch.interval
by Hangbin Liu 17 Apr '19

17 Apr '19

As we don't have a default value for link_watch.interval. If user forgot to set this parameter, teamd will failed to init port priv and exit at the end. e.g. teamd -g -c '{"runner":{"name":"activebackup"},"link_watch":{"name":"arp_ping","target_host":"198.51.100.1"}}' teamdctl team0 port add p5p1 teamdctl team0 port add p5p2 teamd debug log shows: p5p2: Got link watch from global config. p5p2: Using sticky "0". Failed to get "interval" link-watch option. Failed to load options. Failed to init port priv. Callback named "lw_periodic" not found. Callback named "lw_socket" not found. Loop callback failed with: Invalid argument Failed loop callback: libteam_events, 0x5624c28b9410 select() failed. Exiting... Removed loop callback: usock_acc_conn, 0x5624c28bab60 Removed loop callback: usock, 0x5624c28b9410 Removed loop callback: workq, 0x5624c28b9410 Removed loop callback: libteam_events, 0x5624c28b9410 Removed loop callback: daemon, 0x5624c28b9410 Failed: Bad file descriptor Fix it by adding a default value for link_watch.interval. Reported-by: LiLiang <liali(a)redhat.com> Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com> --- man/teamd.conf.5 | 10 ++++++++++ teamd/teamd_lw_psr.c | 11 ++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/man/teamd.conf.5 b/man/teamd.conf.5 index 9bdf46a..6d5374a 100644 --- a/man/teamd.conf.5 +++ b/man/teamd.conf.5 @@ -308,6 +308,11 @@ Default: .TP .BR "link_watch.interval "| " ports.PORTIFNAME.link_watch.interval " (int) Value is a positive number in milliseconds. It is the interval between ARP requests being sent. +.RS 7 +.PP +Default: +.BR "100" +.RE .TP .BR "link_watch.init_wait "| " ports.PORTIFNAME.link_watch.init_wait " (int) Value is a positive number in milliseconds. It is the delay between link watch initialization and the first ARP request being sent. @@ -371,6 +376,11 @@ Default: .TP .BR "link_watch.interval "| " ports.PORTIFNAME.link_watch.interval " (int) Value is a positive number in milliseconds. It is the interval between sending NS packets. +.RS 7 +.PP +Default: +.BR "100" +.RE .TP .BR "link_watch.init_wait "| " ports.PORTIFNAME.link_watch.init_wait " (int) Value is a positive number in milliseconds. It is the delay between link watch initialization and the first NS packet being sent. diff --git a/teamd/teamd_lw_psr.c b/teamd/teamd_lw_psr.c index c0772db..3ddf295 100644 --- a/teamd/teamd_lw_psr.c +++ b/teamd/teamd_lw_psr.c @@ -29,6 +29,7 @@ static const struct timespec lw_psr_default_init_wait = { 0, 1 }; #define LW_PSR_DEFAULT_MISSED_MAX 3 +#define LW_PSR_DEFAULT_INTERVAL 100 #define LW_PERIODIC_CB_NAME "lw_periodic" static int lw_psr_callback_periodic(struct teamd_context *ctx, int events, void *priv) @@ -77,9 +78,13 @@ static int lw_psr_load_options(struct teamd_context *ctx, int tmp; err = teamd_config_int_get(ctx, &tmp, "@.interval", cpcookie); - if (err) { - teamd_log_err("Failed to get \"interval\" link-watch option."); - return -EINVAL; + if (!err) { + if (tmp < 0) { + teamd_log_err("\"interval\" must not be negative number."); + return -EINVAL; + } + } else { + tmp = LW_PSR_DEFAULT_INTERVAL; } teamd_log_dbg("interval \"%d\".", tmp); ms_to_timespec(&psr_ppriv->interval, tmp); -- 2.19.2

3 11

[PATCH] teamd: use enabled option_changed to sync enabled to link_up for lb runner
by Xin Long 17 Apr '19

17 Apr '19

LiLiang found an issue that after adding two ports into a team device with lb mode their enabled option sometimes is false. It was caused by the unexpected events order: 0. team_port_add() in kernel. 1. port_change event A1 sent to userspace. 2. option_change event B1 sent to userspace. 3. port_change event A2 sent to userspace IF port is up now. 4. process port_change event A1 and set port's enabled option 'false'. 5. option_change event B2 sent to userspace. 6. process option_change event B1 and sync enabled option (value = 1). 7. process port_change event A2 and do nothing as enabled option is 1. 8. process option_change event B2 and sync enabled option (value = 0). In kernel, when the port is still down after dev_open(), which happens more often since it changed to use netif_oper_up() to check the state instead of netif_carrier_ok(), the event A2 in Step 3 can be sent at any moment. When it's ahead of Step 4, Step 7 won't set enabled option to 1 as Step 8 comes late. As the port up event can be triggered by dev_watchdog at anytime in kernel, the port_change and option_change events order can not be controlled. What can only be done here is to correct it at Step 8, to sync enabled option to link_up. So this patch is to add enabled option_changed for lb mode to do this sync. Reported-by: LiLiang <liali(a)redhat.com> Signed-off-by: Xin Long <lucien.xin(a)gmail.com> --- teamd/teamd_runner_loadbalance.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/teamd/teamd_runner_loadbalance.c b/teamd/teamd_runner_loadbalance.c index b9bfc13..a581472 100644 --- a/teamd/teamd_runner_loadbalance.c +++ b/teamd/teamd_runner_loadbalance.c @@ -109,12 +109,27 @@ static int lb_event_watch_port_hwaddr_changed(struct teamd_context *ctx, return err; } +static int lb_event_watch_enabled_option_changed(struct teamd_context *ctx, + struct team_option *option, + void *priv) +{ + struct teamd_port *tdport; + + tdport = teamd_get_port(ctx, team_get_option_port_ifindex(option)); + if (!tdport) + return 0; + + return lb_event_watch_port_link_changed(ctx, tdport, priv); +} + static const struct teamd_event_watch_ops lb_port_watch_ops = { .hwaddr_changed = lb_event_watch_hwaddr_changed, .port_hwaddr_changed = lb_event_watch_port_hwaddr_changed, .port_added = lb_event_watch_port_added, .port_removed = lb_event_watch_port_removed, .port_link_changed = lb_event_watch_port_link_changed, + .option_changed = lb_event_watch_enabled_option_changed, + .option_changed_match_name = "enabled", }; static int lb_init(struct teamd_context *ctx, void *priv) -- 2.1.0

2 1

[PATCH] teamd: tdport has to exist if item->per_port is set in __find_by_item_path
by Xin Long 17 Apr '19

17 Apr '19

The issue can be reproduced by: # teamd -c '{"device":"team0", "runner":{"name":"lacp"}}' & # teamdctl team0 state item set runner.aggregator.selected true teamd process will abort in lacp_port_state_aggregator_selected_set() as gsc->info.tdport was set to NULL in teamd_state_item_value_set() The item 'runner.aggregator.selected' is of per_port = true, and it shouldn't allow to call its setter/getter(). This patch is to add the check for it in __find_by_item_path() called by teamd_state_item_value_get/set(). Fixes: 6c00aaf02553 ("teamd: add support for state item write operation") Signed-off-by: Xin Long <lucien.xin(a)gmail.com> --- teamd/teamd_state.c | 1 + 1 file changed, 1 insertion(+) diff --git a/teamd/teamd_state.c b/teamd/teamd_state.c index ab64db9..0714880 100644 --- a/teamd/teamd_state.c +++ b/teamd/teamd_state.c @@ -333,6 +333,7 @@ static int __find_by_item_path(struct teamd_state_val_item **p_item, list_for_each_node_entry(item, &ctx->state_val_list, list) { /* item->subpath[0] == '.' */ if (!strcmp(item->subpath + 1, subpath) && + (!item->per_port || tdport) && (!item->tdport || item->tdport == tdport)) { *p_item = item; *p_tdport = tdport; -- 2.1.0

2 1

[PATCH] teamd: remove port if adding fails
by Hangbin Liu 17 Apr '19

17 Apr '19

When we add a port to team via teamdctl, some drivers do not support changing mac address after dev opened, which would lead to the failure of port_obj_create(). Currently we only destroy the port object if adding port fails. But the port is still enslaved to team in kernel. The interface shows it's a team_slave via ip link cmd, but teamdctl state shows nothing. This may make users confused. Fix it by removing the port if adding fails. Reported-by: Vladimir Benes <vbenes(a)redhat.com> Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com> --- teamd/teamd_per_port.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c index 09d1dc7..7f0a45d 100644 --- a/teamd/teamd_per_port.c +++ b/teamd/teamd_per_port.c @@ -42,6 +42,8 @@ struct port_obj { }; #define _port(port_obj) (&(port_obj)->port) +static int teamd_port_remove(struct teamd_context *ctx, + struct teamd_port *tdport); int teamd_port_priv_create_and_get(void **ppriv, struct teamd_port *tdport, const struct teamd_port_priv *pp, @@ -203,6 +205,7 @@ static int port_obj_create(struct teamd_context *ctx, teamd_event_port_removed: teamd_event_port_removed(ctx, tdport); list_del: + teamd_port_remove(ctx, tdport); port_obj_destroy(ctx, port_obj); port_obj_free(port_obj); return err; -- 2.19.2

2 5

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

libteam April 2019