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

[submodule] Update sonic-snmpagent submodule #7859

Merged
merged 2 commits into from
Jul 13, 2021

Conversation

raphaelt-nvidia
Copy link
Contributor

@raphaelt-nvidia raphaelt-nvidia commented Jun 13, 2021

Why I did it

Update sonic-snmpagent submodule to pick up new commits:

21d7d97 2021-07-12 Fix: SonicV2Connector behavior change: get_all will return empty dict if (#226)
0813b42 2021-07-12 Entries under .1.3.6.1.2.1.31.1.1.1.18 OID should return the "description" field of PORT_TABLE entries in APPL_DB or CONFIG_DB. (#224)
7a78703 2021-07-08 Install dotnet core to fix python gcov warning for code covery color bar showing (#215)
e0f36a5 2021-06-30 [multi-asic]: Udpate to use SonicDBConfig from swsscommon (#219)
266bd15 2021-06-10 Restored snmp vlan support per RFC1213 and added the missing support for RFC2863 (#218)

How I did it

Update the submodule pointer to including the new commits

How to verify it

Unit tests are run via make target/python-wheels/asyncsnmp-2.1.0-py3-none-any.whl.

Which release branch to backport (provide reason below if selected)

Description for the changelog

Update submodule pointer for sonic-snmpagent #218.

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

Signed-off-by: Raphael Tryster <[email protected]>
@raphaelt-nvidia raphaelt-nvidia requested a review from lguohan as a code owner June 13, 2021 10:49
@raphaelt-nvidia raphaelt-nvidia changed the title Update submodule pointer for sonic-snmpagent #218 [submodule] Update sonic-snmpagent submodule Jun 13, 2021
qiluo-msft
qiluo-msft previously approved these changes Jun 14, 2021
@raphaelt-nvidia
Copy link
Contributor Author

I believe that a failure in the automatic tests of this PR is due to interaction between the code I inserted (recall that PR #218 in sonic-snmpagent reverts #191, which was itself a revert of #169, and added code) and testing code in sonic-mgmt/tests/snmp/test_snmp_interfaces.py. The places where the test fails were added by SuvarnaMeenakshi in sonic-net/sonic-mgmt@35e1969. The sonic-mgmt code I tested with was before this update. I tried running the test with debugger, both with my code from this PR, which fails, and with a recent image from the master branch, which passes. In both instances of this code sequence:

result = verify_port_ifindex(snmp_facts, speed_snmp)
pytest_assert(not result, "Unexpected comparsion of SNMP: {}".format(result))

it appears that when testing with the master version, verify_port_ifindex is passed two objects with a large amount of data with no intersection, yielding empty result, whereas with the code from this PR, the objects are smaller but there is an intersection, yielding non-empty result and failing the test. In one of the places, the test has this comment:

TODO: Remove this check after operational status of mgmt interface # is implemented for multi-asic platform

Could the test author or someone familiar with what it does check if the test makes some assumption that the code in this PR violates?

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@SuvarnaMeenakshi can you please review and provide your feedback to the issue raised on the test?

@liat-grozovik
Copy link
Collaborator

@qiluo-msft and @SuvarnaMeenakshi can you please refer to the question above?

@SuvarnaMeenakshi
Copy link
Contributor

I believe that a failure in the automatic tests of this PR is due to interaction between the code I inserted (recall that PR #218 in sonic-snmpagent reverts #191, which was itself a revert of #169, and added code) and testing code in sonic-mgmt/tests/snmp/test_snmp_interfaces.py. The places where the test fails were added by SuvarnaMeenakshi in Azure/sonic-mgmt@35e1969. The sonic-mgmt code I tested with was before this update. I tried running the test with debugger, both with my code from this PR, which fails, and with a recent image from the master branch, which passes. In both instances of this code sequence:

result = verify_port_ifindex(snmp_facts, speed_snmp)
pytest_assert(not result, "Unexpected comparsion of SNMP: {}".format(result))

it appears that when testing with the master version, verify_port_ifindex is passed two objects with a large amount of data with no intersection, yielding empty result, whereas with the code from this PR, the objects are smaller but there is an intersection, yielding non-empty result and failing the test. In one of the places, the test has this comment:

TODO: Remove this check after operational status of mgmt interface # is implemented for multi-asic platform

Could the test author or someone familiar with what it does check if the test makes some assumption that the code in this PR violates?

This looks like a valid failure and is not due to the test case, but is due to the mismatch of "interface description" between config_db and SNMP facts.

Restored snmp vlan support per RFC1213 and added the missing support for RFC2863 by raphaelt-nvidia · Pull Request #218 · Azure/sonic-snmpagent (github.com) – This PR modified the ifAlias information in RFC2863, this changes the output of 1.3.6.1.2.1.31.1.1.1.18 OID.
sonic-mgmt/snmp_facts.py at master · Azure/sonic-mgmt (github.com) – This OID is used in snmp_facts to get interface description of Port channel and management interface.

The sonic-mgmt test_snmp_interfaces sonic-mgmt/test_snmp_interfaces.py at master · Azure/sonic-mgmt (github.com) compares the interface description from config_db to result from SNMP facts.

Prior to PR 218, the description of eth0 and port channels are empty string from SNMP walk and config_db, so test comparison passed.

But after PR 218, SNMP displays “alias” as description of Portchannel and management interface, config_db does not ‘description’ for port channel and management interface, this mismatch results in the failure.

Ideally, interface description output in snmp_facts should be :
(Pdb) snmp_facts[u'snmp_interfaces'][u'117'][u'description'] – Ethernet interface
u'ARISTA02T1:Ethernet1'

(Pdb) snmp_facts[u'snmp_interfaces'][u'10000'][ u'description'] - Management Interface
u''

We will not have description for port channel and management interface in config_db, so SNMP facts should be fixed to match this.

@SuvarnaMeenakshi
Copy link
Contributor

@raphaelt-nvidia Can you modify SNMP PR 218, so that interface description of Port Channel and management interface matches the value retrieved from config db, which will be empty string.
The below added code in "def interface_alias" in rfc2863.py; should be removed.

    if not result:
        #RFC2863 tables don't have descriptions for LAG, vlan & mgmt; take from RFC1213
        oid = self.get_oid(sub_id)
        if oid in self.oid_lag_name_map:
            result = self.oid_lag_name_map[oid]
        elif oid in self.mgmt_oid_name_map:
            result = self.mgmt_alias_map[self.mgmt_oid_name_map[oid]]
        elif oid in self.vlan_oid_name_map:
            result = self.vlan_oid_name_map[oid]

@raphaelt-nvidia
Copy link
Contributor Author

What is the rationale for providing values for 1.3.6.1.2.1.31.1.1.1.18 OID port interfaces, but not for portchannels, vlans or management interfaces? It was my understanding that one of the purposes of this work was to rectify the deficiency and provide these values from whatever source was available.

SuvarnaMeenakshi pushed a commit to sonic-net/sonic-snmpagent that referenced this pull request Jul 12, 2021
…tion" field of PORT_TABLE entries in APPL_DB or CONFIG_DB. (#224)

- What I did
This is a correction of #218, which is contained in sonic-net/sonic-buildimage#7859, after community decided that entries under .1.3.6.1.2.1.31.1.1.1.18 OID should return the "description" field of PORT_TABLE entries in APPL_DB or CONFIG_DB. For vlan, management and LAG, these are empty strings.
- How I did it
Deleted the lines of code quoted by Suvarna in the above PRs. This necessitated modifying 4 unit tests that had been written under the assumption that these OIDs would return non-empty data.
- How to verify it
Run unit tests in build and snmp tests in sonic-mgmt.
- Description for the changelog
Entries under .1.3.6.1.2.1.31.1.1.1.18 OID should return the "description" field of PORT_TABLE entries in APPL_DB or CONFIG_DB.

Signed-off-by: Raphael Tryster <[email protected]>
@SuvarnaMeenakshi
Copy link
Contributor

@raphaelt-nvidia Can you take latest SNMP changes to this PR.

Signed-off-by: Raphael Tryster <[email protected]>
@qiluo-msft
Copy link
Collaborator

Could you update your PR description with the commits information? Sample #8161

@qiluo-msft qiluo-msft merged commit c508a10 into sonic-net:master Jul 13, 2021
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
Update sonic-snmpagent submodule to pick up new commits:

21d7d97 2021-07-12 Fix: SonicV2Connector behavior change: get_all will return empty dict if (sonic-net#226)
0813b42 2021-07-12 Entries under .1.3.6.1.2.1.31.1.1.1.18 OID should return the "description" field of PORT_TABLE entries in APPL_DB or CONFIG_DB. (sonic-net#224)
7a78703 2021-07-08 Install dotnet core to fix python gcov warning for code covery color bar showing (sonic-net#215)
e0f36a5 2021-06-30 [multi-asic]: Udpate to use SonicDBConfig from swsscommon (sonic-net#219)
266bd15 2021-06-10 Restored snmp vlan support per RFC1213 and added the missing support for RFC2863 (sonic-net#218)
SuvarnaMeenakshi pushed a commit to SuvarnaMeenakshi/sonic-snmpagent that referenced this pull request Feb 24, 2023
…tion" field of PORT_TABLE entries in APPL_DB or CONFIG_DB. (sonic-net#224)

- What I did
This is a correction of sonic-net#218, which is contained in sonic-net/sonic-buildimage#7859, after community decided that entries under .1.3.6.1.2.1.31.1.1.1.18 OID should return the "description" field of PORT_TABLE entries in APPL_DB or CONFIG_DB. For vlan, management and LAG, these are empty strings.
- How I did it
Deleted the lines of code quoted by Suvarna in the above PRs. This necessitated modifying 4 unit tests that had been written under the assumption that these OIDs would return non-empty data.
- How to verify it
Run unit tests in build and snmp tests in sonic-mgmt.
- Description for the changelog
Entries under .1.3.6.1.2.1.31.1.1.1.18 OID should return the "description" field of PORT_TABLE entries in APPL_DB or CONFIG_DB.

Signed-off-by: Raphael Tryster <[email protected]>
(cherry picked from commit 0813b42)
SuvarnaMeenakshi pushed a commit to SuvarnaMeenakshi/sonic-snmpagent that referenced this pull request Feb 28, 2023
…tion" field of PORT_TABLE entries in APPL_DB or CONFIG_DB. (sonic-net#224)

- What I did
This is a correction of sonic-net#218, which is contained in sonic-net/sonic-buildimage#7859, after community decided that entries under .1.3.6.1.2.1.31.1.1.1.18 OID should return the "description" field of PORT_TABLE entries in APPL_DB or CONFIG_DB. For vlan, management and LAG, these are empty strings.
- How I did it
Deleted the lines of code quoted by Suvarna in the above PRs. This necessitated modifying 4 unit tests that had been written under the assumption that these OIDs would return non-empty data.
- How to verify it
Run unit tests in build and snmp tests in sonic-mgmt.
- Description for the changelog
Entries under .1.3.6.1.2.1.31.1.1.1.18 OID should return the "description" field of PORT_TABLE entries in APPL_DB or CONFIG_DB.

Signed-off-by: Raphael Tryster <[email protected]>
(cherry picked from commit 0813b42)
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.

4 participants