[ISSUE] Failed to find "enabled" option.
by Xin Long
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.
4 years, 7 months
[PATCHv3] teamd: add a default value 1000 for link_watch.interval
by Xin Long
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
4 years, 11 months
[patch libteam] teamd: add a default value 100 for link_watch.interval
by Hangbin Liu
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
5 years
[PATCH] teamd: use enabled option_changed to sync enabled to link_up for lb runner
by Xin Long
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
5 years
[PATCH] teamd: tdport has to exist if item->per_port is set in __find_by_item_path
by Xin Long
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
5 years
[PATCH] teamd: remove port if adding fails
by Hangbin Liu
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
5 years