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

changes to handle snmp configuration as per the modified CLI #3586

Merged
merged 1 commit into from
Oct 10, 2019
Merged

changes to handle snmp configuration as per the modified CLI #3586

merged 1 commit into from
Oct 10, 2019

Conversation

kannankvs
Copy link
Collaborator

@kannankvs kannankvs commented Oct 9, 2019

While doing CLI changes for SNMP configuration, few changes are made in backend to handle the modified CLI.

** Changes**

  1. "community" for "snmp trap" is also made as "configurable". snmpd_conf.j2 is modified to handle the same.
  2. Changed the snmp.yml file generation from postStartAction to preStartAction in docker_image_ctl.j2 specific to SNMP docker, to ensure that the snmp.yml is generated before sonic-cfggen generates the snmpd.conf.
  3. Changed to make the code common for management vrf and default vrf. Users can configure snmp trap and snmp listening IP for both management vrf and default vrf.

Testing Done
Tested by configuring the SNMP listening IP as well as the trap server IP.

admin@sonic:$ sudo config vrf add mgmt
admin@sonic:
$ sudo config snmpagentaddress add 100.104.45.9 -v mgmt
admin@sonic:$ sudo config snmptrap modify 3 100.94.212.7 -v mgmt
admin@sonic:
$ sudo cat /etc/sonic/snmp.yml
snmp_rocommunity: public
snmp_location: public
v1_trap_dest: NotConfigured
v2_trap_dest: NotConfigured
v3_trap_dest: 100.94.212.7:162%mgmt public
snmp_agent_address_1: 100.104.45.9%mgmt

root@sonic:~# docker exec -it snmp bash
The file /etc/snmp/snmpd.conf inside snmp docker shows the following configuration which will be used by snmpd daemon.
Following %mgmt will be used by snmpd to listen to the IP 100.104.45.9 in "mgmt" vrf.
agentAddress 100.104.45.9%mgmt

Following %mgmt will be used by snmpd to send traps to the server 100.94.212.7 in "mgmt" vrf.
informsink 100.94.212.7:162%mgmt public

@lguohan lguohan merged commit 150ed36 into sonic-net:master Oct 10, 2019
@kannankvs kannankvs deleted the management_vrf_snmp_support2 branch October 11, 2019 06:12
@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Jan 16, 2020

Hi @kannankvs,
By design, snmp.yml provides some static configurations are not supported in ConfigDB (redis).

In your PR, for example, v1Comm is read from ConfigDB and written to snmp.yml, in order to render snmpd.conf.j2 inside snmp docker container. This is actually not needed, because sonic-cfggen could directly use redis-cli content to render in below command

sonic-cfggen -d -y /etc/sonic/snmp.yml -t /usr/share/sonic/templates/snmpd.conf.j2 > /etc/snmp/snmpd.conf

root@str-msn2700-03:/# sonic-cfggen -h | grep configdb
  -d, --from-db         read config from configdb

Could you optimize code and submit another PR? #Closed

fi

echo -n "" > /tmp/snmpagentaddr.yml
keys=`/usr/bin/redis-cli -n 4 keys "SNMP_AGENT_ADDRESS_CONFIG|*"`
Copy link
Collaborator

@qiluo-msft qiluo-msft Jan 16, 2020

Choose a reason for hiding this comment

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

/usr/bin/redis-cli -n 4 keys "SNMP_AGENT_ADDRESS_CONFIG|*" [](start = 10, length = 58)

Try prevent usage redis-cli keys * because if will join keys by blanks without escaping. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@qiluo-msft : These lines are not needed with the new PR4057 and hence the comment is ignored.

@kannankvs
Copy link
Collaborator Author

Hi @kannankvs,
By design, snmp.yml provides some static configurations are not supported in ConfigDB (redis).

In your PR, for example, v1Comm is read from ConfigDB and written to snmp.yml, in order to render snmpd.conf.j2 inside snmp docker container. This is actually not needed, because sonic-cfggen could directly use redis-cli content to render in below command

sonic-cfggen -d -y /etc/sonic/snmp.yml -t /usr/share/sonic/templates/snmpd.conf.j2 > /etc/snmp/snmpd.conf

root@str-msn2700-03:/# sonic-cfggen -h | grep configdb
  -d, --from-db         read config from configdb

Could you optimize code and submit another PR?

@qiluo-msft : A new PR4057 has been raised to address the same. Request you to kindly review it and provide the comments.

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