[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
[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
[PATCH] teamd: lacp: update port state according to partner's sync bit
by Hangbin Liu
1) According to 6.4.12 of IEEE 802.1AX-2008, Figure 6-18, if the partner's sync
bit is not set, the lacp port state should be set to EXPIRED.
Function lacpdu_recv() makes the state of the lacp port to PORT_STATE_CURRENT
when the teamd receives any valid lacp packet. Fix it by checking the parter's
sync bit.
2) According to 6.4.15 of IEEE 802.1AX-2008, Figure 6-22, the state that the
port is selected moves MUX state from DETACHED to ATTACHED.
But ATTACHED sate does not mean that the port can send and receive user frames.
COLLECTING_DISTRIBUTION state is the sate that the port can send and receive
user frames. To move MUX state from ATTACHED to COLLECTING_DISTRIBUTION, the
partner state should be sync as well as the port selected.
In function lacp_port_actor_update(), only INFO_STATE_SYNCHRONIZATION should
be set to the actor.state when the port is selected. INFO_STATE_COLLECTING and
INFO_STATE_DISTRIBUTING should be set to false with ATTACHED mode and set to
true when INFO_STATE_SYNCHRONIZATION of partner.state is set.
In function lacp_port_should_be_{enabled, disabled}(), we also need to check
the INFO_STATE_SYNCHRONIZATION bit of partner.state.
Reported-by: Cisco Systems
Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com>
---
teamd/teamd_runner_lacp.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
index 7b8f0a7..5e763ad 100644
--- a/teamd/teamd_runner_lacp.c
+++ b/teamd/teamd_runner_lacp.c
@@ -333,7 +333,8 @@ static int lacp_port_should_be_enabled(struct lacp_port *lacp_port)
struct lacp *lacp = lacp_port->lacp;
if (lacp_port_selected(lacp_port) &&
- lacp_port->agg_lead == lacp->selected_agg_lead)
+ lacp_port->agg_lead == lacp->selected_agg_lead &&
+ lacp_port->partner.state & INFO_STATE_SYNCHRONIZATION)
return true;
return false;
}
@@ -343,7 +344,8 @@ static int lacp_port_should_be_disabled(struct lacp_port *lacp_port)
struct lacp *lacp = lacp_port->lacp;
if (!lacp_port_selected(lacp_port) ||
- lacp_port->agg_lead != lacp->selected_agg_lead)
+ lacp_port->agg_lead != lacp->selected_agg_lead ||
+ !(lacp_port->partner.state & INFO_STATE_SYNCHRONIZATION))
return true;
return false;
}
@@ -914,9 +916,13 @@ static void lacp_port_actor_update(struct lacp_port *lacp_port)
if (lacp_port->lacp->cfg.fast_rate)
state |= INFO_STATE_LACP_TIMEOUT;
if (lacp_port_selected(lacp_port) &&
- lacp_port_agg_selected(lacp_port))
- state |= INFO_STATE_SYNCHRONIZATION |
- INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;
+ lacp_port_agg_selected(lacp_port)) {
+ state |= INFO_STATE_SYNCHRONIZATION;
+ state &= ~(INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING);
+ if (lacp_port->partner.state & INFO_STATE_SYNCHRONIZATION)
+ state |= INFO_STATE_COLLECTING |
+ INFO_STATE_DISTRIBUTING;
+ }
if (lacp_port->state == PORT_STATE_EXPIRED)
state |= INFO_STATE_EXPIRED;
if (lacp_port->state == PORT_STATE_DEFAULTED)
@@ -1095,7 +1101,10 @@ static int lacpdu_recv(struct lacp_port *lacp_port)
return err;
}
- err = lacp_port_set_state(lacp_port, PORT_STATE_CURRENT);
+ if (lacp_port->partner.state & INFO_STATE_SYNCHRONIZATION)
+ err = lacp_port_set_state(lacp_port, PORT_STATE_CURRENT);
+ else
+ err = lacp_port_set_state(lacp_port, PORT_STATE_EXPIRED);
if (err)
return err;
--
2.19.2
5 years, 1 month
[patch libteam] man: fix runner.min_ports default value
by Hangbin Liu
It should be 1 instead of 0.
Reported-by: LiLiang <liali(a)redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com>
---
man/teamd.conf.5 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/man/teamd.conf.5 b/man/teamd.conf.5
index 9bdf46a..5b0f3e9 100644
--- a/man/teamd.conf.5
+++ b/man/teamd.conf.5
@@ -240,7 +240,7 @@ Specifies the minimum number of ports that must be active before asserting carri
.RS 7
.PP
Default:
-.BR "0"
+.BR "1"
.RE
.TP
.BR "runner.agg_select_policy " (string)
--
2.19.2
5 years, 1 month
[patch libteam] teamd: lw: nsna_ping: only send ns on enabled port
by Hangbin Liu
We forget to check forced_send when using nsna_ping link_watch.
Ns is sent from all ports, which cause switch mac flapping. Some
reply packets are delivered to disabled port and dropped directly.
Fix it by checking forced_send and only send ns on enabled port.
Reported-by: LiLiang <liali(a)redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin(a)gmail.com>
---
teamd/teamd_lw_nsna_ping.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/teamd/teamd_lw_nsna_ping.c b/teamd/teamd_lw_nsna_ping.c
index 127d950..82371c4 100644
--- a/teamd/teamd_lw_nsna_ping.c
+++ b/teamd/teamd_lw_nsna_ping.c
@@ -203,6 +203,9 @@ static int lw_nsnap_send(struct lw_psr_port_priv *psr_ppriv)
struct sockaddr_in6 sendto_addr;
struct ns_packet nsp;
+ if (!(psr_ppriv->common.forced_send))
+ return 0;
+
err = teamd_getsockname_hwaddr(psr_ppriv->sock, &ll_my,
sizeof(nsp.hwaddr));
if (err)
--
2.19.2
5 years, 1 month
Re: A question about team port enable/disable
by Hangbin Liu
Hi Jiri,
On Thu, Feb 21, 2019 at 09:44:07AM +0100, Jiri Pirko wrote:
> Thu, Feb 21, 2019 at 09:26:17AM CET, liuhangbin(a)gmail.com wrote:
> >> Just confirmed the issue is not cause by this code. I will give an update after get more info.
> >> Sorry to bother you.
> >>
> >
> >Update: This issue was caused by teaming PFs and VFs .
> >
> >The customer has a topology like
> >
> >eth0 (PF link that's running LACP)
> > --> vf0 (VF that use loadbalance mode)
> >eth1 (physical link that's running LACP)
> > --> vf1 (VF that use loadbalance mode)
> >
> >After eth0/vf0 down and up. eth0 enters lacp negotiate mode and not
> >active yet. But vf0 is up immediately.
> >So vf0 will be active(cause it use loadbalance mode) and starts
> >sending packets. In the end, these packets
> >are sent from eth0 as it's PF device.
>
> It is unclear to me how your topology looks exactly. Anyway, if you run
> lacp, you don't really care about physical link up/down. So that should
> be okay for the vf, even if it gets link up.
Sorry I didn't make it clear. Hope this topo helps.
Host
vf0 -- team1 -- vf1
| |
eth0 -- team0 -- eth1
| |
--|----------------|------
switch
eth0 and eth1 are two PF devices, with team0(LACP mode) on them.
vf0 and vf1 are two VF devices, with team1(loadbalance mode) on them.
Here is the process:
1) The switch port eth0 conected down/unplugged. Then eth0/vf0 also down.
2) eth1/vf1 enabled and start to send/reply messages.
3) After a while, the switch port up/plugged. eth0/vf0 up at the same time.
4) eth0 goto lacp negotiate state
5) vf0 up and enabled directly as it use loadbalance mode
6) vf0 start to reply packages, while eth0 and the switch port are still in
lacp negotiation. So the switch port is not ready to forward packets. The
packets send from vf0 are dropped.
>
> >
> >To avoid vf0 active before eth0 finishing LACP negotiation. The
> >customer use 'teamnl -p eth0 team0 getoption enabled' to detect
> >eth0's state and setup vf0 manually. This method works. But I'm not
>
> Setup how? I don't understand the scenario, sorry :/ Could you attach
> some configs or something?
I dont have the script files. From the info they supply, I guess it looks
like:
```
#/bin/bash
while true; do
teamnl -p eth0 team0 getoption enabled && break || continue
done
teamnl team1 setoption activeport 4
```
If you need, I can ask for the script they use.
Thanks
Hangbin
5 years, 2 months
Re: A question about team port enable/disable
by Jiri Pirko
Sat, Dec 15, 2018 at 11:45:25AM CET, yanmiaobest(a)gmail.com wrote:
>Hi Jiri
>
>This is Miao, I am currently working on a project to implement a new team policy
>based on the kernel team support. Basically, I have lots of 'ports',
>each of them
>has a ID. I'd like to use the ID to calculate the hash and distribute
>the traffic to
>different team slave ports, similar to round-robin.
>
>And when looking into team code, I see the team kernel driver has
>the following comments:
>
>/*
> * Enable/disable port by adding to enabled port hashlist and setting
> * port->index (Might be racy so reader could see incorrect ifindex when
> * processing a flying packet, but that is not a problem). Write guarded
> * by team->lock.
> */
>
>The team datapath runs lockless, and libteam can enable/disble port
>one the fly when traffic happens. For exmaple, I have 4 port and I
>want, port0->slave0,
>port1->slave1, port2->slave2 and port3->slave3, when slave1 device is link
>down state, it becomes port0->slave0, port1->slave2, port2->slave3,
>port3->slave0,
>using the ID%num as the hash function.
>
>But it seems if datapath runs lockless, it could see the wrong ifindex during
>configuration as stated in the comment because the team drvier needs to
>reconstruct the active ports list. So my question is why this is not a problem
>when datapathsees a wrong ifindex, wouldn't that break the defined team
>policy ? Thank you very much for your help.
Yes, it would. For a packet from time to time. Is it a problem?
>
>Regards,
>Miao
5 years, 2 months