Thu, Feb 16, 2017 at 01:45:37PM CET, lucien.xin(a)gmail.com wrote:
On Thu, Feb 16, 2017 at 5:39 PM, Jiri Pirko <jiri(a)resnulli.us>
wrote:
> Tue, Feb 07, 2017 at 05:03:32PM CET, lucien.xin(a)gmail.com wrote:
>>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.
>
> So if the teamd_port_enabled fails, you would assume
> curr_enabled_state == false
> and continue on?
I'm thinking why ab runner doesn't need to check if 'enabled' option
exists
before setting it, ab's .port_link_changed just calls team_set_port_enabled
directly.
can't we just do it for lb and lacp runner ? is there any special reason
we should check if 'enabled' option exists first ?
The reason AB does this differently is that it does not have to check if
the port was enabled before or not. On enable it is clear that the port
is disabled and vice versa.
For lb/lacp we need to check. So my original suggestion still stands.
Thanks.
>
>>
>> I would add a teamd_log_dbg print if that happens, but I don't see any
>> problem in that.
>>
>>
>>
>>>
>>>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.