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

[Namespace]: Fix interface counters in RFC 1213 #145

Merged
merged 6 commits into from
Jul 22, 2020

Conversation

SuvarnaMeenakshi
Copy link
Contributor

Signed-off-by: SuvarnaMeenakshi [email protected]

- What I did
In multi-asic platform, SAI OID is not unique for the whole device. It is unique for an asic and within a single namespace.
This PR is to fix InterfacesMIB rfc1213 implementation to make sure that interfaces counters is keyed based on interface index and not SAI OID.

- How I did it

  • Fix library functions to use namespace:sai-oid as key, instead of using sai-oid.
  • Fix RFC 1213 to use the new key format.

- How to verify it
Test InterfacesMIB on single and multi-asic platform.
single-asic platform should see no change in output.
multi-asic platform should show right interface counter values.

- Description for the changelog

multi-asic platforms. In multi-asic platform, SAI OID is not
unique for the whole device. Fix implementation to make sure
that interfaces counters is keyed based on interface index.

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

qiluo-msft commented Jul 14, 2020

    return  namespace.encode() + b':' + sai_id

One extra blank #Closed


Refers to: src/sonic_ax_impl/mibs/init.py:173 in 8ed0978. [](commit_id = 8ed0978, deletion_comment = False)

@SuvarnaMeenakshi
Copy link
Contributor Author

    return  namespace.encode() + b':' + sai_id

One extra blank

Refers to: src/sonic_ax_impl/mibs/init.py:173 in 8ed0978. [](commit_id = 8ed0978, deletion_comment = False)

Removed extra space.

@@ -355,7 +367,7 @@ def init_sync_d_lag_tables(db_conn):
return lag_name_if_name_map, if_name_lag_name_map, oid_lag_name_map, lag_sai_map

lag_sai_map = db_conn.get_all(COUNTERS_DB, b"COUNTERS_LAG_NAME_MAP")
lag_sai_map = {name: sai_id.lstrip(b"oid:0x") for name, sai_id in lag_sai_map.items()}
lag_sai_map = {name: get_sai_id_key(db_conn.namespace, sai_id.lstrip(b"oid:0x")) for name, sai_id in lag_sai_map.items()}
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 14, 2020

Choose a reason for hiding this comment

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

lag_sai_map [](start = 4, length = 11)

This line is too long to read. Could you rewrite by a loop? #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.

updated as per comment.

for sai_id in self.if_id_map}
for sai_id_key in self.if_id_map:
namespace, sai_id = mibs.split_sai_id_key(sai_id_key)
if_idx = get_index_from_str(self.if_id_map[sai_id_key].decode())
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 14, 2020

Choose a reason for hiding this comment

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

decode [](start = 67, length = 6)

Why previous we don't need decode() ? #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.

previously we never used the if_name from if_id_map, now we use this to get the interface index.
if_id_map previously also had name in bytes.

Copy link
Contributor

Choose a reason for hiding this comment

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

If previously never used, can you decode at the beginning of init_sync_d_interface_tables? So multiple decode could be simplified.


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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correction to what I mentioned before, it was not used in this file, but rfc4363 is using this.
mibs.get_index(self.if_id_map[port_id]) . mibs.get_index() - this takes a binary string and decodes internally.
Modifying it in the init(), will impact some other users of that library function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for providing an exception. I checked that function, and it is also a great opportunity to simplify this way.


In reply to: 454665605 [](ancestors = 454665605,454632465)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we update in init_sync_d_interface_tables(), we will end up doing a decode during reinit_data(); here decode is done during update_data(), so performance will not change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as per suggestion.

Signed-off-by: SuvarnaMeenakshi <[email protected]>
in different functions.

Signed-off-by: SuvarnaMeenakshi <[email protected]>
@@ -246,7 +246,7 @@ def init_sync_d_interface_tables(db_conn):
# namespace to get a unique key. Assuming that ':' is not present in namespace
# string or in sai id.
# sai_id_key = namespace : sai_id
if_id_map = {get_sai_id_key(db_conn.namespace, sai_id): if_name for sai_id, if_name in if_id_map.items() if \
if_id_map = {get_sai_id_key(db_conn.namespace, sai_id): if_name.decode() for sai_id, if_name in if_id_map.items() if \
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 15, 2020

Choose a reason for hiding this comment

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

if_name.decode() [](start = 60, length = 16)

Many duplicated calls inside this function. #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.

Have updated the code to reduce the decode().
I kept the if_name_map to use bytes if_name, as the user functions use it as bytes string.
But I had to add new dictionaries to fix this.

qiluo-msft
qiluo-msft previously approved these changes Jul 15, 2020
@SuvarnaMeenakshi SuvarnaMeenakshi merged commit 166c221 into sonic-net:master Jul 22, 2020
abdosi pushed a commit that referenced this pull request Sep 2, 2020
* [Namespace]: Fix interface counters in RFC 1213 for
multi-asic platforms. In multi-asic platform, SAI OID is not
unique for the whole device. Fix implementation to make sure
that interfaces counters is keyed based on interface index.

Signed-off-by: SuvarnaMeenakshi <[email protected]>
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