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

[SNMP] management VRF SNMP support #2608

Merged
merged 11 commits into from
Sep 19, 2019

Conversation

vharish02
Copy link
Contributor

This commit adds SNMP support for Management VRF using l3mdev.
The patch included provides VRF support, there is no single
"listendevice" configuration, rather multiple agentaddress
config options can each have their own "interface" to bind to
using "ip%interface". The snmpd.conf file is accordingly
generated using the snmp.yml file and redis database info.

Adding below the comments of SNMP patch 1376

Since the Linux kernel added support for Virtual Routing
and Forwarding (VRF) in version 4.3
(Note: these won't compile on non-linux platforms)

https://www.kernel.org/doc/Documentation/networking/vrf.txt

Linux users could not use snmpd in its current form to
bind specific listening IP addresses to specific VRF
devices. A simplified description of a VRF inteface
is an interface that is a master (a container of sorts)
that collects a set of physicalinterfaces to form a
routing table.

This set of two patches (one for V5-7-patches and one
for V5-8-patches branches) is almost identical to patch
single "listendevice" configuration. Rather, multiple
agentAddress config options can each have their own
"interface" to bind to using the %
syntax.

Signed-off-by: Harish Venkatraman [email protected]

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

@jleveque
Copy link
Contributor

Retest this please

@lguohan lguohan requested a review from pavel-shirshov July 2, 2019 22:25
@pavel-shirshov
Copy link
Contributor

Hi,

Can you please resolve the conflict?

Thanks

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.

I reviewed changes, but I can't understand all changes in the snmpd patch

@kannankvs
Copy link
Collaborator

kannankvs commented Jul 11, 2019

I reviewed changes, but I can't understand all changes in the snmpd patch

SNMP does not have the support to differentiate the management VRF. When SNMP tries to send the trap to the SNMP manager, it uses "setsocketopt" to set the properties for the socket.
netsnmp_udp_sockopt_set function has been enhanced to do "SO_BINDTODEVICE" to bind to the management interface. This "iface" is being passed as parameter based on the user configuration, where users can configure the snmp_trap_manager_ip_address along with the VRF through which the IP is reachable. For example, if trap_mgr_ip1 is configured through management VRF, this SO_BINDTODEVICE enhancement will take care of binding to the management VRF interface. Without this enhancement, SNMP does not have a facility to bind sockets to the VRF.
If you need more information, we shall schedule for a conference call to go through the changes and the reasons.

@pavel-shirshov
Copy link
Contributor

@kannankvs Thanks for the explanations
Can you resolve the conflict in this PR please?

@kannankvs
Copy link
Collaborator

@pavel-shirshov : Thank you. I will resolve the conflict and checkin.

This commit adds SNMP support for Management VRF using l3mdev.
The patch included provides VRF support, there is no single
"listendevice" configuration, rather multiple agentaddress
config options can each have their own "interface" to bind to
using "ip%interface". The snmpd.conf file is accordingly
generated using the snmp.yml file and redis database info.

Adding below the comments of SNMP patch 1376
--------------------------------------------
Since the Linux kernel added support for Virtual Routing
and Forwarding (VRF) in version 4.3
(Note: these won't compile on non-linux platforms)

https://www.kernel.org/doc/Documentation/networking/vrf.txt

Linux users could not use snmpd in its current form to
bind specific listening IP addresses to specific VRF
devices. A simplified description of a VRF inteface
is an interface that is a master (a container of sorts)
that collects a set of physicalinterfaces to form a
routing table.

This set of two patches (one for V5-7-patches and one
for V5-8-patches branches) is almost identical to patch
single "listendevice" configuration. Rather, multiple
agentAddress config options can each have their own
"interface" to bind to using the <ip>%<interface>
syntax.</interface></ip>
-------------------------------------------

Signed-off-by: Harish Venkatraman <[email protected]>
@vharish02 vharish02 force-pushed the management_vrf_snmp_support branch from 032da8c to 2ad2a4b Compare July 15, 2019 20:58
@vharish02
Copy link
Contributor Author

@kannankvs Thanks for the explanations
Can you resolve the conflict in this PR please?

@pavel-shirshov I have resolved the conflicts

@pavel-shirshov
Copy link
Contributor

retest vs please

@pavel-shirshov
Copy link
Contributor

@vharish02 The vs test is failing for this PR. We tested two times.

@vharish02
Copy link
Contributor Author

@vharish02 The vs test is failing for this PR. We tested two times.

@pavel-shirshov I don't think the failures are related to the changes of the PR. Looks like some environment issue.

@lguohan
Copy link
Collaborator

lguohan commented Jul 18, 2019

are snmpd vrf related changes from upstream?

can we use a simpler approach to just run snmpd in cgexec so that snmpd is bind to the mgmt vrf?

also this pr should be splitted into two PR. one for enable snmp trap feature, another for mgmt vrf.

@vharish02
Copy link
Contributor Author

are snmpd vrf related changes from upstream?
snmp VRF Patch was picked from the following link where snmp patches are upstreamed
https://sourceforge.net/p/net-snmp/patches/1376/

can we use a simpler approach to just run snmpd in cgexec so that snmpd is bind to the mgmt vrf?
Will look into this approach and get back to you. You want snmpd to be started using cgexec on mgmt-vrf and stop snmpd in default VRF.

also this pr should be splitted into two PR. one for enable snmp trap feature, another for mgmt vrf.
Will do

@vharish02
Copy link
Contributor Author

can we use a simpler approach to just run snmpd in cgexec so that snmpd is bind to the mgmt vrf?

@lguohan When snmpd is run using cgexec snmpwalk will work through the mgmt port (eth0), snmpwalk through FPP will not work. Is this solution acceptable? where snmp via FPP is not feasible.

Note: We did a quick prototype to run snmpd using cgexec (without docker) and confirmed that snmpwalk works only through mgmt vrf (eth0) and not through Front panel ports (FPP).

But when snmp patch is used snmpwalk works with both mgmt vrf (eth0) and FPP interfaces.

@vharish02
Copy link
Contributor Author

can we use a simpler approach to just run snmpd in cgexec so that snmpd is bind to the mgmt vrf?

@lguohan When snmpd is run using cgexec snmpwalk will work through the mgmt port (eth0), snmpwalk through FPP will not work. Is this solution acceptable? where snmp via FPP is not feasible.

Note: We did a quick prototype to run snmpd using cgexec (without docker) and confirmed that snmpwalk works only through mgmt vrf (eth0) and not through Front panel ports (FPP).

But when snmp patch is used snmpwalk works with both mgmt vrf (eth0) and FPP interfaces.

Update on cgexec:

Without using snmp patch and using cgexec for snmp to bind to mgmt-vrf has complications.

  1. We can make SNMP to work via mgmt interface only (not through Front panel ports) but BGP FRR will have issue. BGP runs an SNMP subagent and it tries to connect to master agent using localhost.(127.0.0.1) When master agent SNMP docker is run using cgexec, the FRR BGP subagent will not be able to connect to the master agent. Loopback interface cannot be moved to management VRF. Loopback interface is very much required in default VRF for all docker and internal communication.

  2. Tried to create a dedicated loopback interface "lo1" for management VRF with IP 100.100.100,100/32, the subagent related to ax_implementation(ifMIB and other MIB's) can be made to work using this lo1, but still BGP docker won't be able to reach to this lo1 without having inter-vrf routes.

With the patch as per the PR SNMP for all MIB's including BGP MIBs works through both management VRF (eth0) and default VRF (FPP). We will have additional configuration in snmpd.conf
ex. agentaddress ipaddress%vrf where we can have both mgmt vrf and default vrf entries.
With this configuration master agent does setsockeropt with SO_BINDTODEVICE and binds it to the VRF and hence replies back to SNMP requests coming from external SNMP managers.

@lguohan
Copy link
Collaborator

lguohan commented Sep 11, 2019

understand the concern, fine with the original approach to patch snmpd to support vrf. Can you add original commit id in you snmpd patch and also resolve the conflict?

Copy link
Collaborator

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

please add original snmpd commit id in your snmpd patch and also resolve the conflict.

@kannankvs
Copy link
Collaborator

@lguohan : resolved the conflict. Regarding the "original snmpd commit id", I am not clear on how to get it. The patch was generated long time back and we are unable to track and find the commit ID related to it. Can this be ignored?

@lguohan
Copy link
Collaborator

lguohan commented Sep 13, 2019

@kannankvs , all upstream patch needs to have upstream commit id for tracking purpose so that when we later upgrade the code we will know if such patch can be remove or not. I wish i could find this issue earlier.

@lguohan
Copy link
Collaborator

lguohan commented Sep 15, 2019

can you also resolve conflict?

@kannankvs
Copy link
Collaborator

@lguohan : This patch is from https://sourceforge.net/p/net-snmp/patches/1376/. We are exploring on how to get the upstream commit ID for this patch. After finding it, we shall add it in the "Subject" inside the patch file 0008-Linux-VRF-5.7.3-Support.patch. We shall resolve the conflict then.

@lguohan
Copy link
Collaborator

lguohan commented Sep 16, 2019

there is still conflict.

@lguohan
Copy link
Collaborator

lguohan commented Sep 16, 2019

also, snmp has git repo, you can use git blame to find the commit id.

@kannankvs
Copy link
Collaborator

@lguohan : I was working in parallel to resolve the conflict. Its resolved now.
Regarding the commit ID, I am unable to find it in sourceforge.net as well as in the net-snmp github (https://github.com/net-snmp/net-snmp). I searched using "git blame" & "git log" in the net-snmp github in both master and V5-8, V5-7 branches as well, but no success. We will explore more to find the commit ID. Now that we have given the reference in the patch file, if that is enough, you can merge this PR.

@kannankvs
Copy link
Collaborator

@lguohan : Found the commit IDs related to this patch and added those details inside the patch.

@lguohan lguohan merged commit 9d2d617 into sonic-net:master Sep 19, 2019
@kannankvs kannankvs deleted the management_vrf_snmp_support branch September 19, 2019 05:48
vadymhlushko-mlnx added a commit to vadymhlushko-mlnx/sonic-buildimage that referenced this pull request Jan 24, 2023
Update sonic-utilities submodule pointer to include the following:
* fba87f4 Revert ([sonic-net#2599](sonic-net/sonic-utilities#2599))
* d6d7ab3 [warm-reboot] Use kexec_file_load instead of kexec_load when available ([sonic-net#2608](sonic-net/sonic-utilities#2608))
* db4683d fix show techsupport error ([sonic-net#2597](sonic-net/sonic-utilities#2597))
* 3d8e9c6 [GCU] Prohibit removal of PFC_WD POLL_INTERVAL field ([sonic-net#2545](sonic-net/sonic-utilities#2545))
* 163e766 [techsupport] include APPL_STATE_DB dump ([sonic-net#2607](sonic-net/sonic-utilities#2607))
* 8703773 YANG Validation for ConfigDB Updates: RADIUS_SERVER ([sonic-net#2604](sonic-net/sonic-utilities#2604))
* c2d746d Remove TODO comment which is no longer relevant ([sonic-net#2600](sonic-net/sonic-utilities#2600))
* f09da99 [show] Add bgpraw to show run all ([sonic-net#2537](sonic-net/sonic-utilities#2537))
* 39ac564 Extend fast-reboot STATE_DB entry timer ([sonic-net#2577](sonic-net/sonic-utilities#2577))

Signed-off-by: vadymhlushko-mlnx <[email protected]>
vadymhlushko-mlnx added a commit to vadymhlushko-mlnx/sonic-buildimage that referenced this pull request Jan 25, 2023
Update sonic-utilities submodule pointer to include the following:
* f4f857e [GCU] Ignore bgpraw in GCU applier ([sonic-net#2623](sonic-net/sonic-utilities#2623))
* b5ac600 [muxcable][config] Add support to enable/disable ceasing to be an advertisement interface when  service is stopped ([sonic-net#2622](sonic-net/sonic-utilities#2622))
* 981f953 [chassis][voq] Add show fabric reachability command. ([sonic-net#2528](sonic-net/sonic-utilities#2528))
* fba87f4 Revert ([sonic-net#2599](sonic-net/sonic-utilities#2599))
* d6d7ab3 [warm-reboot] Use kexec_file_load instead of kexec_load when available ([sonic-net#2608](sonic-net/sonic-utilities#2608))
* db4683d fix show techsupport error ([sonic-net#2597](sonic-net/sonic-utilities#2597))
* 3d8e9c6 [GCU] Prohibit removal of PFC_WD POLL_INTERVAL field ([sonic-net#2545](sonic-net/sonic-utilities#2545))
* 163e766 [techsupport] include APPL_STATE_DB dump ([sonic-net#2607](sonic-net/sonic-utilities#2607))
* 8703773 YANG Validation for ConfigDB Updates: RADIUS_SERVER ([sonic-net#2604](sonic-net/sonic-utilities#2604))
* c2d746d Remove TODO comment which is no longer relevant ([sonic-net#2600](sonic-net/sonic-utilities#2600))
* f09da99 [show] Add bgpraw to show run all ([sonic-net#2537](sonic-net/sonic-utilities#2537))
* 39ac564 Extend fast-reboot STATE_DB entry timer ([sonic-net#2577](sonic-net/sonic-utilities#2577))

Signed-off-by: vadymhlushko-mlnx <[email protected]>
qiluo-msft pushed a commit that referenced this pull request Jan 27, 2023
Includes below commits
```
0d5e68f5a [GCU] Ignore bgpraw table in GCU operation (#2628)
22757b1f3 Add interface link-training command into the CLI doc (#2257)
f4f857e10 [GCU] Ignore bgpraw in GCU applier (#2623)
b5ac60036 [muxcable][config] Add support to enable/disable ceasing to be an advertisement interface when `radv` service is stopped (#2622)
981f9531e [chassis][voq] Add "show fabric reachability" command. (#2528)
fba87f43f Revert (#2599)
d6d7ab37f [warm-reboot] Use kexec_file_load instead of kexec_load when available (#2608)
db4683d40 fix show techsupport error (#2597)
3d8e9c62d [GCU] Prohibit removal of PFC_WD POLL_INTERVAL field (#2545)
163e766cc [techsupport] include APPL_STATE_DB dump (#2607)
8703773eb YANG Validation for ConfigDB Updates: RADIUS_SERVER (#2604)
c2d746d4f Remove TODO comment which is no longer relevant (#2600)
f09da9983 [show] Add bgpraw to show run all (#2537)
39ac5641b Extend fast-reboot STATE_DB entry timer (#2577)
```
yxieca pushed a commit that referenced this pull request Apr 4, 2023
Why I did it
src/linkmgrd

* 373544f - (HEAD -> 202205, origin/202205) [active-active] Add mux server state correction mechanism (#191) (3 days ago) [Longxiang Lyu]
src/sonic-platform-daemons

* 0260695 - (HEAD -> 202205, origin/202205) Fix issue: Exception occured at SfpStateUpdateTask thread due to KeyError('status') (#346) (3 days ago) [Junchao-Mellanox]
src/sonic-swss

* af46930 - (HEAD -> 202205, origin/202205) Custom monitoring based priority tunnels (3 days ago) [siqbal1986]
src/sonic-utilities

* fe224f09 - (HEAD -> 202205, origin/202205) Revert "Convert IPv6 addresses to lowercase in apply-patch (#2299)" (#2758) (3 days ago) [jingwenxie]
* cf12bb5e - [warm-reboot] Use kexec_file_load instead of kexec_load when available (#2608) (10 days ago) [Saikrishna Arcot]
* 93f1d740 - [warm-reboot] fix warm-reboot when /tmp/cache is missing (#2367) (10 days ago) [Stepan Blyshchak]
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.

5 participants