Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fpmsyncd]: Linux netlink runtime option #1765

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qbdwlr
Copy link
Contributor

@qbdwlr qbdwlr commented Jun 3, 2021

What I did
Added runtime option for fpmsyncd "-l net" to allow fpmsyncd to listen directly to netlink messages from Linux kernel rather than listening to FRR FPM TCP socket.

Why I did it
FRR currently does not transmit any MPLS related information on its FPM TCP socket to fpmsyncd, not even MPLS attributes on IP routes. MPLS functionality verification had to be using alternate routing stacks (Juniper cRPD and MetaSwitch) but these routing stacks are not open-source and are not available to the community in general.
Some unit-testing during MPLS development was performed by switching fpmsyncd to listen directly to Linux kernel messages, so it was proposed to upstream this capability for general use.

How I verified it
Unit-tests in sonic-swss/tests test_route.py and test_mpls.py were run with "fpmsyncd -l net".

Details if related
Main difference between default fpmsyncd and "fpmsyncd -l net" behavior is related to VRF support. SONiC modifies actual netlink message from FRR to send VRF ifindex value in RTA_RT_TABLE_ID attribute, while Linux kernel netlink message is unmodified and contains the route table ID in RTA_RT_TABLE_ID attribute. This prevents "-l net" option being used in scenarios that test routes with VRF.
Refer to HLD: sonic-net/SONiC#706

@@ -581,7 +581,8 @@ void RouteSync::onMsg(int nlmsg_type, struct nl_object *obj)
char master_name[IFNAMSIZ] = {0};

/* if the table_id is not set in the route obj then route is for default vrf. */
if (master_index)
if (master_index &&
(master_index != RT_TABLE_MAIN) && (master_index != RT_TABLE_DEFAULT))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is trigger of this change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main difference between default fpmsyncd and "fpmsyncd -l net" behavior is related to VRF support. SONiC modifies actual netlink message from FRR to send VRF ifindex value in RTA_RT_TABLE_ID attribute, while Linux kernel netlink message is unmodified and contains the route table ID in RTA_RT_TABLE_ID attribute. This prevents "-l net" option being used in scenarios that test routes with VRF.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abdosi I can change this logic to just check a boolean to see if Linux Netlink connection is used and skip VRF if it is.

@prsunny
Copy link
Collaborator

prsunny commented Jun 11, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qbdwlr qbdwlr force-pushed the netlink branch 3 times, most recently from 6a0c877 to 28ae53e Compare June 14, 2021 15:49
@qbdwlr qbdwlr requested a review from prsunny as a code owner July 1, 2021 16:56
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…le (sonic-net#1765)

#### What I did

This PR fixed issue for [show interface breakout](sonic-net/sonic-buildimage#7957) issue

**Before this fix**
removed `Ethernet60` from breakout_cfg table for unit testing and got the issue
```
 
admin@lnos-x1-a-asw04:~$ sudo redis-cli -n 4 del "BREAKOUT_CFG|Ethernet60"
(integer) 1
admin@lnos-x1-a-asw04:~$ sudo redis-cli -n 4 hgetall "BREAKOUT_CFG|Ethernet60"
(empty array)
admin@lnos-x1-a-asw04:~$ show int breakout
Traceback (most recent call last):
  File "/usr/local/bin/show", line 8, in <module>
    sys.exit(cli())
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 1114, in invoke
    return Command.invoke(self, ctx)
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.7/dist-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/local/lib/python3.7/dist-packages/show/interfaces/__init__.py", line 181, in breakout
    cur_brkout_mode = cur_brkout_tbl[port_name]["brkout_mode"]
KeyError: 'Ethernet60'

```

**After the fix**
```

admin@lnos-x1-a-asw04:~$ show int breakout current-mode Ethernet60
+-------------+-------------------------+
| Interface   | Current Breakout Mode   |
+=============+=========================+
| Ethernet60  | Not Available           |
+-------------+-------------------------+
```
#### How I did it
Gracefully handle the error when port/interface is not present in `BREAKOUT_CFG` table.

#### How to verify it
```
show interface breakout 
show int interface current-mode

```
**_Added unit test case for negetive Test case_**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants