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

[LLDP]: Modify OID index of LLDPRemTableUpdater MIB #153

Closed
wants to merge 2 commits into from

Conversation

SuvarnaMeenakshi
Copy link
Contributor

Signed-off-by: SuvarnaMeenakshi sumeenak@microsoft.com

- What I did
LLDPRemTableUpdater index in OID is a tuple of (remote time mark, if index, remote device index).

For example: LLDP-MIB::lldpRemSysName.615.177.11 = STRING: remote_device_name
here , remote time mark is 615, interface index is 615, remote device index is 11.

In case of multi-asic platform, when querying this MIB it can happen that same if index result is seen in SNMP walk, with a different remote time mark.

For example: 
LLDP-MIB::lldpRemSysName.615.177.11 = STRING: remote_device_name
LLDP-MIB::lldpRemSysName.625.177.11 = STRING: remote_device_name

In the above walk, 177 interface index data appears twice.

- How I did it
To avoid showing same if index result in the result, set remote time mark to 0, as this value is not significant,

- How to verify it
Make sure the output of single asic platform shows the same result as before but with OID modified as: lldpRemSysName.0.if_index.remote_dev_id.
This will take effect in the entire LLDPRemTableUpdater table.

Make sure the output on mulit asic platform shows the result with unique interface index.

- Description for the changelog

(if index, remote time mark, remote device index).
In case of multi-asic platform, when querying this MIB it can happen
that same if index result is seen in SNMP walk, with a different
remote time mark. To avoid showing same if index result in the
result, set remote time mark to 0.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>
@qiluo-msft qiluo-msft requested a review from mykolaf July 23, 2020 22:54
@mykolaf
Copy link
Contributor

mykolaf commented Jul 27, 2020

Hi @SuvarnaMeenakshi ,
I would like to better understand the cause of the issue we are trying to solve with this PR.
My guess is that we for some reason have duplicate entries for a single interface in APP DB's LLDP_ENTRY table?

@SuvarnaMeenakshi
Copy link
Contributor Author

SuvarnaMeenakshi commented Jul 27, 2020

Hi @SuvarnaMeenakshi ,
I would like to better understand the cause of the issue we are trying to solve with this PR.
My guess is that we for some reason have duplicate entries for a single interface in APP DB's LLDP_ENTRY table?

Hi @mykolaf , It is not that there are duplicate entries for same single interface in LLDP_ENTRY table, but the additional data for an interface is only seen on SNMP walk output.
iso.0.8802.1.1.2.1.4.1.1.9
LLDP-MIB::lldpRemSysName.615.177.11 = STRING: remove_device_name
LLDP-MIB::lldpRemSysName.625.177.11 = STRING: remove_device_name

The OID last 3 digits are formed as: (remote time mark, if index, remote device index)

This is my understanding of the issue. when we issue SNMP walk –

  1. this triggers continuous get_next() – get_next reads the data structure to form the next OID.
  2. This data structure has the same tuple as the OID (remote time mark, if index, remote device index)
  3. At this point we have (615,177,11)
  4. In between, update_data() timer kicks in, it updates the same data structure, with a different remote time mark.
  5. Get_next continues, and gets the next data of previous OID, which returns (625,177, 11) – same data but new time mark.

So data structure in SNMP code has the correct data, DB has the correct data, it is only SNMP walk that has the issue.
This issue is very evident in mulit-asic platform. But this issue might be seen on single-asic platform as well, if there is some delay.

@SuvarnaMeenakshi SuvarnaMeenakshi marked this pull request as ready for review July 28, 2020 01:23
qiluo-msft
qiluo-msft previously approved these changes Jul 28, 2020
abdosi
abdosi previously approved these changes Jul 28, 2020
@@ -430,7 +430,7 @@ def update_data(self):
if not lldp_kvs:
continue
try:
time_mark = int(lldp_kvs[b'lldp_rem_time_mark'])
time_mark = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

please add comment for the motivation behind this change so that in future we can go-back and understand

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 comment.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>
@SuvarnaMeenakshi SuvarnaMeenakshi dismissed stale reviews from abdosi and qiluo-msft via 6e4e226 July 29, 2020 18:13
@SuvarnaMeenakshi
Copy link
Contributor Author

retest this please

@SuvarnaMeenakshi
Copy link
Contributor Author

retest license/cla please

@SuvarnaMeenakshi
Copy link
Contributor Author

The license/cla check continues to be in 'waiting for status'. Closing this PR, Will raise a new PR for the same diff to re-run license/cla check.

@SuvarnaMeenakshi
Copy link
Contributor Author

Raised PR: #155 for the same diff.

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