[libteam] Add fallback support for single-member-port LAG

[hzheng5]

- What I did

• Allow the port to be selected if the LAG is configured
  with fallback and port is in defaulted state due to missing
  LACP PDUs from remote end
• Only enable port if LAG is admin up and the member port
  is link up
  PLEASE note this pull request will only support LACP Fallback for single-member-port LAG as now. Since with this pull request, all member ports within LAG with fallback configured will be selected and active, which may not work for multiple member port LAG in some cases.

- How I did it

• Add is_lacp_fallback_eligible check to lacp_port_selectable_state to make the port selectable if fallback is configured and port is in defaulted state

- How to verify it

• Connect the LAG to a server without LACP running. Check the LAG is still up on the switch side with fallback enabled.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[lguohan]

build failure

[lguohan]

@liatgrozovik , to review and approve

[hzheng5]

Hi @lguohan, I have fixed this issue. how to restart the build?

Thanks,

[marian-pritsak]

In design document you have a fallback timer to switch from defaulted state to fallback, but I don't see it in the code. Is it no more required?

[marian-pritsak]

Why is it required to check admin state? If link is up, then we definitely know that admin state is also up.

[hzheng5]

Thanks Marian @marian-pritsak for the code review.

Yes, I removed the lacp fallback timer, which is the transition time from defaulted state to fallback state. Since we reached an agreement to not modify existing LACP receive state machine, I removed the fallback state and the fallback timer is no longer needed. Also I have updated the design document

https://github.com/Azure/SONiC/blob/gh-pages/doc/LACP%20Fallback%20Feature%20for%20SONiC_v0.5.md

We need to check the admin state because if we create a Lag PortChannel01 with member ports Ethernet1. The PortChannel will be by default admin Shut, but the member port will be in admin UP state. If we don't check the admin status of the LAG, the member will be moved into defaulted state since it's link up, and then be moved to fallback state if lacp fallback is enabled.

Please let me know if you still have questions.

Thanks,
Haiyang

[hzheng5]

@marian-pritsak
Hi Marian,

"If link is up, then we definitely know that admin state is also up."
This is not always true in libteam, here the admin state is actually the admin state of the LAG . So it's possible that the PortChannel is admin down, but the member port is admin up and link up. In this case, the member port will be moved into defaulted state (or fallback state if enabled), which is not desired as the PortChannel is admin shutdown.

Please let me know if you still have concerns regarding this admin check. If so, we can setup a skype call to discuss further.

FYI. Some log I captured after shutting down the PortChannel11, the admin_state is "0" (down), but the member port Ethernet11 is still link up

admin@ASW3.ET2:~$ teamshow
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available, S - selected, D - deselected
  No.  Team Dev       Protocol     Ports
-----  -------------  -----------  -------------
   11  PortChannel11  LACP(A)(Up)  Ethernet11(S)

admin@ASW3.ET2:~$ sudo ifconfig PortChannel11 down

admin@ASW3.ET2:~$ teamshow
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available, S - selected, D - deselected
  No.  Team Dev       Protocol     Ports
-----  -------------  -----------  -------------
   11  PortChannel11  LACP(A)(Dw)  Ethernet11(D)

admin@ASW3.ET2:~$ sudo grep admin_state /var/log/teamd.log  | grep PortChannel11
Nov 27 16:08:49.718118 ASW3.et2 DEBUG teamd_PortChannel11[63]: Ethernet11 **admin_state  "0" link_up.**
admin@ASW3.ET2:~$
admin@ASW3.ET2:~$ teamdctl PortChannel11 state
setup:
  runner: lacp
ports:
  Ethernet11
    link watches:
      link summary: up
      instance[link_watch_0]:
        name: ethtool
        link: up
        down count: 0
    runner:
      aggregator ID: 0
      selected: no
      state: disabled
runner:
  active: yes
  fast rate: no

admin@ASW3.ET2:~$ ip link show Ethernet11
22: Ethernet11: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast master PortChannel11 state UP mode DEFAULT group default qlen 1000
    link/ether 00:05:64:2f:2b:d5 brd ff:ff:ff:ff:ff:ff

admin@ASW3.ET2:~$ ip link show PortChannel11
5: PortChannel11: <BROADCAST,MULTICAST> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default
    link/ether 00:05:64:2f:2b:d5 brd ff:ff:ff:ff:ff:ff

admin@ASW3.ET2:~$ bcmcmd "ps xe11"
ps xe11
                 ena/        speed/ link auto    STP                  lrn  inter   max   cut   loop
           port  link  Lns   duplex scan neg?   state   pause  discrd ops   face frame  thru?  back
      xe11( 12)  up     1   25G  FD   SW  No   Forward  TX RX   None   FA     KR  1522    No

drivshell>
admin@ASW3.ET2:~$

Thanks,
Haiyang

[lguohan]

@hzheng5 , this is actually separate issue. whether or not member port admin status depends on the lag admin status is a separate issue. i tested on arista switch, when the lag is down, the member port is errdisabled. is this issue related to lacp fallback mechanism? can you explain?

[marian-pritsak]

retest this please

[liatgrozovik]

The commit message has duplicate information (at the beginning and after the 'What I did'
There is also need to describe that this feature can work only if there is a single port in LAG. this limitation should be very clear.

Not sure i see in this PR how the configuration is set and where it is added.

General comments:

1. Any function should have header function which describes the functionality, input.output parameters as well as return code.
2. Any if statement should have {} for single line as well.

[hzheng5]

@liatgrozovik

Really appreciate your input and suggestions.

Have removed the duplicate part in commit messages. Also add the single member port limitation in the message.

Regarding your two comments:

1. Will do this for the function I added is_lacp_fallback_eligible.
2. Agreed on this. But I didn't introduce those single line statement without {} into libteam. Here I just modified the if condition to satisfy my needs. There are already multiple existing places in libteam using single line if statement without {}. I didn't update those in this pull request because I wanted to keep my changes minimum. If needed, we can ask the libteam team to update the coding guidance and address those single line existing if statement using a separate pull request to make them consistent. Please let me know your suggestions.

Thanks,
Haiyang

[liatgrozovik]

@hzheng5 i dont think i saw your comment about "Not sure i see in this PR how the configuration is set and where it is added." can you please elaborate how does it suppose to work?

[hzheng5]

@liatgrozovik Sorry I missed this one.

By default, the fallback feature is disabled. To configure LACP fallback on a LAG, there are two options.

1. Add "fallback": true, to the libteam configure file. And use "teamd -g -d -f PortChannel10.conf" to create the LAG.

PortChannel10.conf :
{
"device":"PortChannel10",
"runner":
{
"name":"lacp",
"active": true,
"fast_rate": true,
"fallback": true,
"tx_hash": ["eth", "ipv4"]
},
"link_watch":{"name":"ethtool"},
"ports":
{
"Ethernet10":{}
}
}

2. Config via config_db.json, which will generate the above config file using teamd.j2 template and load it into libteam after startup.

   "PORTCHANNEL": {
   "PortChannel10": {
   "fallback": "true",
   "members": [
   "Ethernet10"
   ]
   }
   }

Thanks,
Haiyang

[rodnymolina]

I understand that "fallback" feature is currently not supporting multi-member portchannels, but what would be the behavior if a user (deliberately/accidentally) enables "fallback" for more than one member? Shouldn't we write some logic in minigraph/config_db parsers to prevent those scenarios? If the current "fallback" logic were to allow more than one port to come up, I'm concerned about the implications that may have as we would be keeping two L2 "doors" wide-opened to the same server (traffic-loops, broadcast-storms, etc).

[hzheng5]

Thanks @rodnymolina for the code review.

Ideally, we should have cfgmgr to check such misconfig and reject if fallback is configured on multiple member port. However, the infra is not ready yet. If we add logic in minigraph/config_db parsers to reject such case, there will be discrepancy between the config_db and the config we applied on the LAG unless the parser is able to update the config DB.

The fallback is disabled by default on all LAGs, and we will make it clear in the release note. Anyone using such feature should avoid such misconfig.

Please let me know if you think such check logic is needed. I can add it to the teamd.j2 template to disable the member port is more than 1. However the user may still manually add more member port after the LAG is created using teamd cli.
There is no way to complete block fallback on a multiple member LAG>

Thanks,
Haiyang

[rodnymolina]

I understand all that. In fact, you can indeed prevent users from enabling fallback on multiple members (that's what vendors do), but that requires extra logic on teamd to always determine a single "winner" port (based on mac, or port name, etc). But I'm not suggesting to do all that. What i'm saying is that given the potential consequences a bogus configuration may bring, i would suggest you to enforce some prevention logic somewhere (perhaps teamd.j2 is the right location while we wait for a proper configMgr). Personally, i don't know how a server may behave in early bootup-stages. For example, it could see both NICs as UP, and it may be tempted to send traffic through both of them. Now, what if it uses the same source IP? This may trigger mac-move/learning events across your entire broadcast-domain. In essence, this is not a problem i would like to troubleshoot in my network. I will leave the final decision to you, so i'll approve this review now.

[hzheng5]

Thanks @rodnymolina

As you suggested, I added some logic to teamd.j2 to only enable fallback if it is a single-member-port LAG.

+{% if PORTCHANNEL[pc]['fallback'] and ((PORTCHANNEL[pc]['members'] | length) == 1) %}
+{# Set fallback only if it has single member port #}

Thanks,
Haiyang

[lguohan]

"Only enable port if LAG is admin up and the member port
is link up". is the separate issue, which is more fundamental.

We need to address it in a separate PR.

[lguohan]

will merge once build finishes

[hzheng5]

Thanks a lot, Guohan Will submit another pull request for "Only enable port if LAG is admin up and the member port is link up" Thanks,Haiyang------------------------------------------------------------------From:lguohan <notifications@github.com>Time:2017 Dec 16 (Sat) 13:15To:Azure/sonic-buildimage <sonic-buildimage@noreply.github.com>Cc:Haiyang <haiyang.z@alibaba-inc.com>; Mention <mention@noreply.github.com>Subject:Re: [Azure/sonic-buildimage] [libteam] Add fallback support for single-member-port LAG (#1118) will merge once build finishes— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.