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

[FRR] Enable SNMP support #2981

Merged
merged 2 commits into from
Jun 19, 2019
Merged

[FRR] Enable SNMP support #2981

merged 2 commits into from
Jun 19, 2019

Conversation

MichelMoriniaux
Copy link
Contributor

@MichelMoriniaux MichelMoriniaux commented Jun 7, 2019

This is a follow-up of sonic-net/sonic-snmpagent#92
Now that licensing issues have been solved FRR is distributed with SNMP
support compiled-in. This PR adds the last bits of configuration to get
the frr-snmp debian packages added to the docker container and the
config bits to enable the snmp module in FRR

This PR brings the functionality of being able to poll bgpd for routes
and peer status.

Signed-off-by: Michel Moriniaux m.moriniaux@criteo.com

yoda-small

@lguohan lguohan requested a review from pavel-shirshov June 7, 2019 23:47
@lguohan
Copy link
Collaborator

lguohan commented Jun 7, 2019

which oid tree does frr support? is there is conflict with snmp subagent?

@MichelMoriniaux
Copy link
Contributor Author

MichelMoriniaux commented Jun 8, 2019

This exposes though the standard sonic snmp subagent agentx.
you can walk the 1.3.6.1.2.1.15 tree (standard BGP mib)

We've had this in production for almost a year now, for licensing reasons only recently with the move to FRR 6.0 and 7.0 it was safe to upstream ( vis-a-vis the community binaries). Please see the discussion in sonic-snmpagent PR 92 for a full explanation of the issues that prevented us to upstream this sooner for FRR 4.0 ( TLDR: prior to 6.0 SNMP support had to be specifically compiled-in, FRR 6.0 provided the frr-snmp.deb package by default ) #Closed

@MichelMoriniaux
Copy link
Contributor Author

Note that this PR only enables SNMP for BGPd and zebra

@lguohan
Copy link
Collaborator

lguohan commented Jun 8, 2019

you need to fix the sonic-cfggen unit test

@MichelMoriniaux
Copy link
Contributor Author

@lguohan unit test corrected for frr.conf

@qiluo-msft qiluo-msft self-requested a review June 12, 2019 19:10
@@ -118,6 +118,7 @@ load 12 10 5
#
# Run as an AgentX master agent
master agentx
agentxsocket tcp:localhost:3161
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 12, 2019

Choose a reason for hiding this comment

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

3161 [](start = 30, length = 4)

How did you choose this port? I see it also in snmp.conf. Suggest prevent magic numbers, or add comment for reference each other so we will not forget if we want to modify it. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

standard port is 705, chose 3000+ snmp port to not run into issues for daemons that re not launched as root.
We can make this a configurable option in rules/config?

Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 12, 2019

Choose a reason for hiding this comment

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

rules/config is good! #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually talked to fast: this would require a nested template (dockers/docker-snmp-sv2/snmpd.conf.j2.j2 ) which would add complexity, the best thing I believe is to add it to our plan to integrate snmp config in config_db.json as per sonic-net/SONiC#231
we should start working on this in Q3 ( within a month ). what do you think?

Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 13, 2019

Choose a reason for hiding this comment

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

OK to me. Then please check the alternative recommendation

add comment for reference each other so we will not forget if we want to modify it. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added references to both files

@qiluo-msft
Copy link
Collaborator

Is there a document listing all the supported OIDs added by this integration?


In reply to: 500073690 [](ancestors = 500073690)

@@ -118,6 +118,7 @@ load 12 10 5
#
# Run as an AgentX master agent
master agentx
agentxsocket tcp:localhost:3161
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 12, 2019

Choose a reason for hiding this comment

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

agentxsocket [](start = 0, length = 12)

How to test this feature? Can you add a test case in sonic-mgmt? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you need an agentx client to be able to test this feature: we can provide an end to end test by using the bgp daemon as agentx client and doing an snmp request to a RFC1657 OID

Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 12, 2019

Choose a reason for hiding this comment

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

Not sure why we need an agentx client to test. I guess you can add some queries into existing sonic-mgmt snmp test. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I'm looking into it right now, I'm checking to see if the ansible snmp_facts module queries the right OIDs which would make the test trivial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/Azure/sonic-mgmt/blob/master/ansible/library/snmp_facts.py does not support RFC1657 at this time, we could patch it to add a subset of OIDs but its going to take some time. I suggest we add it to the issues backlog of sonic-mgmt and we can pick it up

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you could not provide a sonic-mgmt test case right now, please add some manual test instruction here (with steps and sample output). We could not accept feature without test.


In reply to: 293172354 [](ancestors = 293172354)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK will add a link to look at the docker-fpm-frr snmp.conf file and put the test protocol there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the info to both files

Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments

Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

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

Please see other reviewers requests

@MichelMoriniaux
Copy link
Contributor Author

Is there a document listing all the supported OIDs added by this integration?

In reply to: 500073690 [](ancestors = 500073690)

This is the domain of the FRR package, we have brought no modifications there, this patch exposes to sonic what FRR makes available
the best documentation I can find on the exposed OIDs is the code of the daemons itself:
https://github.com/FRRouting/frr/blob/master/bgpd/bgp_snmp.c#L45

This is a follow-up of sonic-snmpagent PR 92
Now that licensing issues have been solved FRR is distributed with SNMP
support compiled-in. This PR adds the last bits of configuration to get
the frr-snmp debian packages added to the docker container and the
config bits to enable the snmp module in FRR

This PR brings the functionality of being able to poll bgpd for routes
and peer status.

Signed-off-by: Michel Moriniaux <m.moriniaux@criteo.com>
@@ -118,6 +118,10 @@ load 12 10 5
#
# Run as an AgentX master agent
master agentx
# internal socket to allow extension to other docker containers
# Currently the othe container using this is docker-fpm-frr
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 13, 2019

Choose a reason for hiding this comment

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

othe [](start = 16, length = 4)

typo #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

@qiluo-msft qiluo-msft dismissed their stale review June 13, 2019 01:13

Unblock

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Jun 13, 2019

Please try keep commit history in a PR, so we could compare between them. Only force push on rebasing. #Closed

This is a follow-up of sonic-snmpagent PR 92
Now that licensing issues have been solved FRR is distributed with SNMP
support compiled-in. This PR adds the last bits of configuration to get
the frr-snmp debian packages added to the docker container and the
config bits to enable the snmp module in FRR

This PR brings the functionality of being able to poll bgpd for routes
and peer status.

Signed-off-by: Michel Moriniaux <m.moriniaux@criteo.com>
@lguohan lguohan merged commit 1854453 into sonic-net:master Jun 19, 2019
@batmancn
Copy link

batmancn commented Aug 6, 2019

Hi,

Is there some test command or how to use? As I want to have a try of this feature.

@MichelMoriniaux
Copy link
Contributor Author

MichelMoriniaux commented Aug 6, 2019 via email

@batmancn
Copy link

batmancn commented Aug 7, 2019

You can look at the comments in the code it gives test cases, if not enough I will amend it to make them clearer

On Mon, Aug 5, 2019, 21:13 Simon Jones @.***> wrote: Hi, Is there some test command or how to use? As I want to have a try of this feature. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2981?email_source=notifications&email_token=ADBS7WR3BTSR52Q2XEDTLGTQDD27RA5CNFSM4HV6TEKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3TYTUQ#issuecomment-518490578>, or mute the thread https://github.com/notifications/unsubscribe-auth/ADBS7WXKPJCBRSMQBNZIWSLQDD27RANCNFSM4HV6TEKA .

Hi, why my test fails... Is there some log or debug method?

I run FRR with snmp support

root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:~# docker exec -it bgp bash
root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:/# ps aux -w
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root         1  0.0  0.2  58624 20884 ?        Ss+  04:57   0:01 /usr/bin/python /usr/bin/supervisord
root        27  0.0  0.0 250136  3028 ?        Sl   04:57   0:00 /usr/sbin/rsyslogd -n
root        32  0.0  0.1  45560 14312 ?        S    04:57   0:00 python /usr/bin/bgpcfgd
frr         35  0.0  0.1 560432 11564 ?        Sl   04:57   0:01 /usr/lib/frr/zebra -A 127.0.0.1 -s 90000000 -M fpm -M snmp
frr         38  0.0  0.0  93528  5992 ?        S    04:57   0:00 /usr/lib/frr/staticd -A 127.0.0.1
frr         41  0.0  0.1 347672 14100 ?        Sl   04:57   0:00 /usr/lib/frr/bgpd -A 127.0.0.1 -M snmp
root        47  0.0  0.0 101488  4428 ?        Sl   04:57   0:00 fpmsyncd
root        49  0.0  0.0  18184  3428 ?        Ss   04:58   0:00 bash
root        54  0.0  0.0  36632  2888 ?        R+   05:32   0:00 ps aux -w

I run snmpwalk in snmp docker

root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:~# docker exec -it snmp bash
root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:/# snmpwalk -v 2c -c public 172.18.8.212 1.3.6.1.2.1.15
Timeout: No Response from 172.18.8.212
root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:/# snmpwalk -v 2c -c public 172.18.8.212 .1.3.6.1.2.1.15
Timeout: No Response from 172.18.8.212
root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:/# ping 172.18.8.212
PING 172.18.8.212 (172.18.8.212) 56(84) bytes of data.
64 bytes from 172.18.8.212: icmp_seq=1 ttl=64 time=0.077 ms
64 bytes from 172.18.8.212: icmp_seq=2 ttl=64 time=0.102 ms

@MichelMoriniaux
Copy link
Contributor Author

MichelMoriniaux commented Aug 7, 2019 via email

@batmancn
Copy link

batmancn commented Aug 7, 2019

vtysh -c "sh ip bgp summ" vtysh -c "sh run" | grep agentx cat /etc/sonic/snmp.yml docker exec -it bgp cat /etc/snmp/frr.conf docker exec -it snmp snmpwalk -v2c -c 210e9D65 127.0.0.1 .1.3.6.1.2.1.15.2.0

This is output:

root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:~# vtysh

Hello, this is FRRouting (version 7.0.1-sonic).
Copyright 1996-2005 Kunihiro Ishiguro, et al.

NX-ZWYC-M1F202-A16-HW6865-A-INT-212# show ip bgp summary
% BGP instance not found
NX-ZWYC-M1F202-A16-HW6865-A-INT-212#
root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:~# vtysh -c "sh run" | grep agentx
root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:~# cat /etc/sonic/snmp.yml
snmp_rocommunity: snmp@......
snmp_location: public
root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:~# docker exec -it bgp cat /etc/snmp/frr.conf
# This line allows the FRR docker to speak with the snmp container
# Make sure this line matches the one in the snmp docker
#   snmp:/etc/snmp/snmpd.conf
# To verify this works you need to have a valid bgp daemon running and configured
# Check that a snmpwalk to 1.3.6.1.2.1.15 gives an output
# Further verification: 1.3.6.1.2.1.15.2.0 = INTEGER: 65000 the returned value should be the confiugred ASN
agentXSocket    tcp:localhost:3161
root@NX-ZWYC-M1F202-A16-HW6865-A-INT-212:~# docker exec -it snmp snmpwalk -v2c -c 210e9D65 127.0.0.1 .1.3.6.1.2.1.15.2.0
Timeout: No Response from 127.0.0.1

@MichelMoriniaux
Copy link
Contributor Author

MichelMoriniaux commented Aug 7, 2019 via email

@MichelMoriniaux
Copy link
Contributor Author

MichelMoriniaux commented Aug 9, 2019 via email

@batmancn
Copy link

Hi,

I'm now test this feature.

As http://docs.frrouting.org/en/latest/snmp.html said, if there are BGP peer up/down event, I should got log like below in LOG

snmpd[13733]: Got trap from peer on fd 14

So I up/down BGP peer manually, but I could NOT found BGP trap log in /var/log/syslog. So what's this reason?

  1. snmpd has no log in /var/log/syslog?
  2. Or log level reason? But I use INFO log level in snmpd like this: /usr/sbin/snmpd -f -LS6d -u Debian-snmp -g Debian-snmp -I -smux mte...
  3. No snmp trap support now? If it is, how to support snmp trap?

Thank you~

@MichelMoriniaux
Copy link
Contributor Author

MichelMoriniaux commented Aug 14, 2019 via email

zhenggen-xu pushed a commit to zhenggen-xu/sonic-buildimage that referenced this pull request Oct 17, 2019
Merge to 201811 with FRR 4.0
@qiluo-msft
Copy link
Collaborator

@MichelMoriniaux Is it true this MIB is for IPv4? Are you aware of similar implementation for IPv6 BGP neighbors?

mssonicbld added a commit that referenced this pull request Dec 14, 2023
…lly (#17501)

#### Why I did it
src/sonic-swss
```
* ff524e6d - (HEAD -> master, origin/master, origin/HEAD) [dash] add a retry for an ACL rule creation if a tag is not created yet (#2972) (7 hours ago) [Yakiv Huryk]
* 620db3da - [ci] Allow partially success build artifact in PR checker pipeline. #2986 (3 days ago) [Liu Shilong]
* d357e6f1 - [copporch] Add safeguard during policer attribute update (#2977) (4 days ago) [Vivek]
* cb460394 - [fpmsyncd][WR] Relax the static schema constraint for ROUTE_TABLE (#2981) (5 days ago) [Vivek]
* a1ce21f6 - Change base directory referenced in coverage.xml (#2976) (6 days ago) [Lawrence Lee]
* 920959cf - [Dash] [UT] Add ZMQ test case for dash (#2967) (6 days ago) [Hua Liu]
```
#### How I did it
#### How to verify it
#### Description for the changelog
mssonicbld added a commit that referenced this pull request Apr 8, 2024
…lly (#18587)

#### Why I did it
src/sonic-swss
```
* f83a0baf - (HEAD -> 202311, origin/202311) [fpmsyncd][WR] Relax the static schema constraint for ROUTE_TABLE (#2981) (#3067) (2 days ago) [Vivek]
```
#### How I did it
#### How to verify it
#### Description for the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants