Thank you Jamie.
I was incorrectly focused on the Collecting & Distributing bits; do
with those what you will. The Synchronization bit must be cleared when
a port is a member of a standby aggregator.
Patrick
On Mon, Apr 17, 2017 at 5:52 AM, Xin Long <lucien.xin(a)gmail.com> wrote:
> On Sat, Apr 15, 2017 at 8:03 AM, Jamie Bainbridge
> <jamie.bainbridge(a)gmail.com> wrote:
>> On 14 April 2017 at 17:24, Xin Long <lucien.xin(a)gmail.com> wrote:
>>> Patrick reported an issue about COLLECTING, DISTRIBUTING and
>>> SYNCHRONIZATION flags when the ports with standby agg. :
>>>
>>> The ports associated with the non-selected (standby) aggregation
>>> should not sent the Synchronising, Collecting, and Distributing flags
>>> in their LACPDUs. So a port state of 7 for fast rate or 5 for slow
>>> rate.
>>>
>>> It also causes team work different from bonding.
>>>
>>> I've noticed this thread:
>>>
https://lists.fedorahosted.org/archives/list/libteam@lists.fedorahosted.o...
>>>
>>> But here we will talk more about 'the ports with standby agg'.
>>>
>>> --------------
>>> He thinks:
>>>
>>> 1. Collecting & Distributing bits indicate to the partner that the
>>> port can actively be used to both receive and transmit normal network
>>> traffic. If a port is part of a *standby* aggregator, it just seems
>>> wrong to set these bits. With the bits set, the switch is being told
>>> that this port can be used to receive regular network traffic which is
>>> not true.
>>>
>>> From The IEEE 802.1AX-2008 standard section 5.4.2.2 description of the
>>> Collecting & Distributing bits:
>>>
>>> Collecting is encoded in bit 4. TRUE (encoded as a 1) means collection
>>> of incoming frames on
>>> this link is definitely enabled; i.e., collection is currently enabled
>>> and is not expected to be
>>> disabled in the absence of administrative changes or changes in
>>> received protocol information.
>>> Its value is otherwise FALSE (encoded as a 0).
>>>
>>> Distributing is encoded in bit 5. FALSE (encoded as a 0) means
>>> distribution of outgoing frames
>>> on this link is definitely disabled; i.e., distribution is currently
>>> disabled and is not expected to be enabled in the absence of
>>> administrative changes or changes in received protocol
>>> information. Its value is otherwise TRUE (encoded as a 1).
>>>
>>>
>>> 2. The bonding driver clearly agrees with me :p as it does not set
>>> these bits on ports of an standby aggregator. It only sets bits 1 & 3
>>> (& 2 depending on lacp rate) for ports in a standby aggregator.
>>>
>>>
>>> 3. In 5.4.12.2 Selection Logic Requirements:
>>>
>>> If there are further constraints on the attachment of ports that have
>>> selected an Aggregator, those
>>> ports may be selected as standby in accordance with the rules
>>> specified in 5.6.1. Selection or
>>> deselection of that Aggregator can cause the Selection Logic to
>>> re-evaluate the ports to be selected
>>> as standby.
>>>
>>> .... and in 5.6.1 Use of system and port priorities:
>>>
>>> For each link that a given System cannot include in the aggregation,
>>> the Selection Logic identifies
>>> the Selection state of the corresponding port as STANDBY, preventing
>>> the link from becoming
>>> active. The synchronization state signaled in transmitted LACPDUs for
>>> such links will be
>>> OUT_OF_SYNC.
>>>
>>>
>>> (out of sync means the Synchornization bit should not be set).
>>>
>>>
>>>
>>> Basically, there should be some logic where each port should check
>>> whether it is a member of a 'selected' aggregator and if not, then
it
>>> should set its Synchronization, Collecting, and Distributing bits to
>>> 0.
>>
>> From 5.6.1 the correct behaviour seems to be:
>>
>> * Standby ports MAY set Collecting and Distributing bits
>> * Standby ports MUST NOT set Synchronization bit
>>
>> The lack of Synchronization bit should prevent the standby port being
>> included in the selection logic.
> Thanks Jamie.
>
> I think we are on same page about that standby port shouldn't set
> Synchronization bit.
>
> As for Collecting and Distributing, if no other special reasons, to keep
> consistent with bonding, I agree with Patrick to clear them as well.
>
> --- a/teamd/teamd_runner_lacp.c
> +++ b/teamd/teamd_runner_lacp.c
> @@ -913,9 +913,11 @@ static void lacp_port_actor_update(struct
> lacp_port *lacp_port)
> state |= INFO_STATE_LACP_ACTIVITY;
> if (lacp_port->lacp->cfg.fast_rate)
> state |= INFO_STATE_LACP_TIMEOUT;
> - if (lacp_port_selected(lacp_port))
> - state |= INFO_STATE_SYNCHRONIZATION;
> - state |= INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;
> + if (lacp_port_selected(lacp_port) &&
> + lacp_port_agg_selected(lacp_port)) {
> + state |= INFO_STATE_SYNCHRONIZATION |
> + INFO_STATE_COLLECTING | INFO_STATE_DISTRIBUTING;
> + }
>
> Jiri, what do you think ?