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

mvrf_avoid_snmp_yml_config: made changes to pass SNMP config from con… #4057

Merged
merged 2 commits into from
Jan 29, 2020
Merged

mvrf_avoid_snmp_yml_config: made changes to pass SNMP config from con… #4057

merged 2 commits into from
Jan 29, 2020

Conversation

kannankvs
Copy link
Collaborator

This PR is based on the new comments given for the already merged PR3586.
By design, snmp.yml provides some static configurations that are not supported in ConfigDB (redis).
Hence, the configurations that got added in snmp.yml as part of PR3586 is being removed.
Instead, the configurations are directly read from ConfigDB and rendered in snmpd.conf.j2 inside snmp docker container to generate the snmpd.conf directly without using snmp.yml.
These are tested by configuring the snmpagentaddress and snmptrap commands.
@qiluo-msft : Request you to kindly review, approve, merge & cherrypick to 201911.

@kannankvs
Copy link
Collaborator Author

retest this please

{% endif %}
{% if SNMP_AGENT_ADDRESS_CONFIG %}
{% for (agentip, port, vrf) in SNMP_AGENT_ADDRESS_CONFIG %}
agentAddress {{ agentip }}{% if port %}:{{ port }}{% endif %}{% if vrf %}%{{ vrf }}{% endif %}{{ "" }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

{{ "" }} [](start = 94, length = 8)

Why you need this?

Could you give some samples of input and render results?

Copy link
Collaborator Author

@kannankvs kannankvs Jan 27, 2020

Choose a reason for hiding this comment

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

@qiluo-msft : I assume you mean the last {{ "" }}.
I am using the following example configuration in config_db,

    "SNMP_AGENT_ADDRESS_CONFIG": {
        "1.1.1.1||": {},
        "2.2.2.2||": {}
    },

Without this {{ "" }}, if we configure more than one agentAddress, they all come in same line in snmpd.conf.
Example: In snmpd.conf without this {{ "" }} as follows.

agentAddress 1.1.1.1agentAddress 2.2.2.2

With this {{ "" }}, a new line is inserted between agentAddress.
Example: In snmpd.conf with this {{ "" }} as follows.

agentAddress 1.1.1.1
agentAddress 2.2.2.2

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not true. Could you please test again? I tested and got the expected multiple line output.


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

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 : My previous example was from my device only. I am not sure why you are not seeing same result as mine. I used the commands (given below) to configure. You can try once using the commands as well.

I redid the configuration and the result is given below.

root@sonic-s6100-07:~# config snmpagentaddress add 1.1.1.1
root@sonic-s6100-07:~# config snmpagentaddress add 2.2.2.2

root@sonic-s6100-07:~# docker exec -it snmp bash
root@sonic-s6100-07:/# cat /etc/snmp/snmpd.conf | grep agentAdd
agentAddress 1.1.1.1agentAddress 2.2.2.2
root@sonic-s6100-07:/# 

Above is the output from my device without that {{ "" }}. If you are 100% sure that {{ "" }} is not required, I can remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I test the image built in this PR https://sonic-jenkins.westus2.cloudapp.azure.com/job/mellanox/job/buildimage-mlnx-all-pr/2154/artifact/target/sonic-mellanox.bin. Please check if the test is valid and recheck yours?

admin@sonic:~$ docker exec -it snmp bash
root@sonic:/# redis-cli -n 4
127.0.0.1:6379[4]> hset "SNMP_AGENT_ADDRESS_CONFIG|1.1.1.1||" "" ""
(integer) 1
127.0.0.1:6379[4]> hset "SNMP_AGENT_ADDRESS_CONFIG|2.2.2.2||" "" ""
(integer) 1
127.0.0.1:6379[4]> exit
root@sonic:/# sonic-cfggen -d -y /etc/sonic/snmp.yml -t /usr/share/sonic/templates/snmpd.conf.j2
###############################################################################
# Managed by sonic-config-engine
###############################################################################
#
# EXAMPLE.conf:
#   An example configuration file for configuring the Net-SNMP agent ('snmpd')
#   See the 'snmpd.conf(5)' man page for details
#
#  Some entries are deliberately commented out, and will need to be explicitly activated
#
###############################################################################
#
#  AGENT BEHAVIOUR
#

#  Listen for connections on all ip addresses, including eth0, ipv4 lo
#
agentAddress 1.1.1.1
agentAddress 2.2.2.2

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 : Unfortunately I dont have any mellanox device to test the mellanox bin. I tested in Dell device that uses sonic-broadcom.bin. I am using Jan 22 image from Jenkins.
Doubt: If we have this {{ "" }}. will it create any issue? Since it always gives error in my setup, can we retain it and merge?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I double checked my test case and it is not true.

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.

Minor issues

@kannankvs
Copy link
Collaborator Author

Minor issues

@qiluo-msft : Is this a different comment? Can you please explain?

@qiluo-msft qiluo-msft merged commit 7cb6300 into sonic-net:master Jan 29, 2020
@kannankvs kannankvs deleted the mvrf_avoid_snmp_yml_config branch January 29, 2020 05:05
abdosi pushed a commit that referenced this pull request Feb 4, 2020
#4057)

* mvrf_avoid_snmp_yml_config: made changes to pass SNMP config from confiDB to snmpd.conf without using snmp.yml
* added a missing if condition
pphuchar pushed a commit to SONIC-DEV/sonic-buildimage that referenced this pull request Mar 9, 2020
sonic-net#4057)

* mvrf_avoid_snmp_yml_config: made changes to pass SNMP config from confiDB to snmpd.conf without using snmp.yml
* added a missing if condition
tiantianlv pushed a commit to SONIC-DEV/sonic-buildimage that referenced this pull request Apr 24, 2020
sonic-net#4057)

* mvrf_avoid_snmp_yml_config: made changes to pass SNMP config from confiDB to snmpd.conf without using snmp.yml
* added a missing if condition
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.

3 participants