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

[rfc1213] Interface MIB add l3 vlan interfaces & aggregate rif counters #133

Merged
merged 18 commits into from
Jul 6, 2020

Conversation

mykolaf
Copy link
Contributor

@mykolaf mykolaf commented Jun 1, 2020

- What I did

- How I did it

- How to verify it

- Description for the changelog

  • Add vlan interface to Interface MIB
  • Aggregate rif counters with l2 counters if rif counters are available.

According to HLD

Implies #4655
Relies on #78

Mykola Faryma added 9 commits May 15, 2020 12:32
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
@qiluo-msft
Copy link
Contributor

Could you resolve the conflicts?

@qiluo-msft qiluo-msft self-requested a review June 2, 2020 23:31
@@ -264,6 +298,29 @@ def _get_counter(self, oid, table_name):
mibs.logger.warning("SyncD 'COUNTERS_DB' missing attribute '{}'.".format(e))
return None

def aggregate_counters(self):
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 3, 2020

Choose a reason for hiding this comment

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

aggregate_counters [](start = 8, length = 18)

Is it only for Mellanox SKUs? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will only be executed if COUNTERS DB contains the rif counters. If RIF counters is not supported/disabled we will have unchanged behavior.

@@ -283,7 +340,14 @@ def get_counter(self, sub_id, table_name):
counter_value = 0
for lag_member in self.lag_name_if_name_map[self.oid_lag_name_map[oid]]:
counter_value += self._get_counter(mibs.get_index(lag_member), table_name)

# import pdb; pdb.set_trace()
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 3, 2020

Choose a reason for hiding this comment

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

remove dead code #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

sai_lag_id = self.lag_sai_map[self.oid_lag_name_map[oid]]
sai_lag_rif_id = self.port_rif_map[sai_lag_id]
if sai_lag_rif_id in self.rif_port_map:
_table_name = bytes(getattr(table_name, 'name', table_name), 'utf-8')
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 3, 2020

Choose a reason for hiding this comment

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

_table_name [](start = 16, length = 11)

leading underscore is for private class member. Could you use other name convention? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor

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

As comments

mykolaf and others added 6 commits June 6, 2020 17:41
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
@qiluo-msft
Copy link
Contributor

Retest this please

@mykolaf
Copy link
Contributor Author

mykolaf commented Jun 23, 2020

Test can not pass until sonic-net/sonic-py-swsssdk#78 is merged.

@mykolaf mykolaf marked this pull request as ready for review June 23, 2020 07:56
@qiluo-msft
Copy link
Contributor

Retest this please

@qiluo-msft
Copy link
Contributor

Test did not pass even after sonic-net/sonic-py-swsssdk#78 merged.

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
@mykolaf
Copy link
Contributor Author

mykolaf commented Jul 6, 2020

Hi @qiluo-msft, I added the mock data for the test to pass. Can we go forward and merge this?

@qiluo-msft qiluo-msft merged commit 253f58e into sonic-net:master Jul 6, 2020
SuvarnaMeenakshi added a commit to SuvarnaMeenakshi/sonic-snmpagent that referenced this pull request Jul 16, 2020
…f counters (sonic-net#133)"

This reverts commit 253f58e.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>
jleveque pushed a commit that referenced this pull request Jul 16, 2020
…f counters (#133)" (#151)

This reverts commit 253f58e.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>
qiluo-msft pushed a commit that referenced this pull request Dec 8, 2020
…rs (#169)

**- What I did**
Rework based on #133 and feedback #148.
**- How I did it**
- merge #133 with upstream master.
- Fix UnavailableDataError when RIF counters are not enabled.
- Make RIF keys in maps unique in multi asic env.
**- How to verify it**
- Disable rif counters
- restart swss
- snmpwalk
- Verify no UnavailableDataError in logs
- Verify port counters are returned
- Enable rif counters
- snmpwalk
- Verify no error in logs
- Verify error in/out counters are aggregated for rif ports and portchannels
- Verify VLAN RIF counters are present in the MIB.
**- Description for the changelog**
Added support for aggregated router interface counters and L3 VLAN counters to RFC1213 MIB.
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.

None yet

3 participants