[sonic_ax_impl]: Add support of port channels. #21

oleksandrivantsiv · 2017-04-25T10:29:43Z

Add port channels support to RFC1213 (1.3.6.1.2.1.2) and
RFC2863 (1.3.6.1.2.1.31.1) MIBs.
Read oper state, admin state and MTU value from DB.

- Add port channels support to RFC1213 (1.3.6.1.2.1.2) and RFC2863 (1.3.6.1.2.1.31.1) MIBs. - Read oper state, admin state and MTU value from DB.

pavel-shirshov · 2017-04-25T17:42:03Z

src/sonic_ax_impl/mibs/ietf/rfc1213.py

+        if sub_id in self.oid_lag_name_map:
+            counter_value = 0
+            for lag_member in self.lag_name_if_name_map[self.oid_lag_name_map[sub_id]]:
+                counter_value += self.get_counter(mibs.get_index(lag_member), table_name)


What if the sum of the interface counters will be greater then 2^32?

It will be truncated to 32 bit in upper layer. Changed code to truncate it explicitly.

pavel-shirshov · 2017-04-25T17:46:16Z

src/sonic_ax_impl/mibs/__init__.py

    """
    match = re.match(SONIC_ETHERNET_RE_PATTERN, if_name)
    if match:
        n = match.group(1)
        return int(n) + 1

+    match = re.match(SONIC_PORTCHANNEL_RE_PATTERN, if_name)


Probably it's better to make a loop over regexps with if_name.
regexps = {
SONIC_ETHERNET_RE_PATTERN: 1
SONIC_PORTCHANNEL_RE_PATTERN: 1000
}
If we want to add vlan or other interfaces later, we could just add more entry to table.

lguohan · 2017-04-27T17:46:44Z

@qiluo-msft, canyou review?

qiluo-msft · 2017-04-27T18:50:16Z

src/sonic_ax_impl/mibs/__init__.py

-        return int(n) + 1
+    patterns = {
+        SONIC_ETHERNET_RE_PATTERN: 1,
+        SONIC_PORTCHANNEL_RE_PATTERN: 1000


1000 [](start = 38, length = 4)

Magic number. Give a name and some comment? May be '1' above is in the same context. #Closed

There is comment in function description

qiluo-msft · 2017-04-27T19:00:53Z

src/sonic_ax_impl/mibs/__init__.py

+
+    for lag_entry in lag_entries:
+        lag_name = lag_entry[len(b"LAG_TABLE:"):]
+        lag_members = db_conn.keys(APPL_DB, b"LAG_MEMBER_TABLE:%s:*" % lag_name)


b"LAG_MEMBER_TABLE:%s:*" % lag_name [](start = 44, length = 35)

regex replace LAG_TABLE->LAG_MEMBER_TABLE
Is it more readable?

It will look the same. Isn't it?

qiluo-msft · 2017-04-27T19:01:12Z

src/sonic_ax_impl/mibs/__init__.py

+    if_name_lag_name_map = {}
+
+    for lag_entry in lag_entries:
+        lag_name = lag_entry[len(b"LAG_TABLE:"):]


lag_entry[len(b"LAG_TABLE:"):] [](start = 19, length = 30)

Split by ':' and get the second part?

what is the difference?

More readable.

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

qiluo-msft · 2017-04-27T19:04:07Z

src/sonic_ax_impl/mibs/__init__.py

+
+    # { OID -> lag_name (SONiC) }
+    oid_lag_name_map = {get_index(if_name): if_name for if_name in lag_name_if_name_map.keys()
+                        if get_index(if_name)}


get_index [](start = 27, length = 9)

get_index() twice?

Yes, to verify that get_index return value is not None

Could you optimize it to once?

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

qiluo-msft · 2017-04-27T20:13:19Z

src/sonic_ax_impl/mibs/__init__.py

+        def member_name_str(val, lag_name):
+            return val[len(b"LAG_MEMBER_TABLE:%s:" % lag_name):]
+
+        lag_name_if_name_map[lag_name] = [member_name_str(m, lag_name) for m in lag_members]


m [](start = 75, length = 1)

Check m is valid. And it will make sure recursive get_counter() is safe.

"m" is always valid we get it from DB

Recursion here is safe. Maximum depth is always 1.

There is a trust boundary between DB and this component. If anything bad happens in DB, this component should not be trapped in a non-stopping recursion and crash.

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

SNMP in SONiC works in read only mode. It takes information from DB converts it in special format and returns to user. Without working DB it is meaningless. If something bad happens with DB it will crash anyway because all code is implemented taking into consideration that DB can't fail. This is actual for any existing component.

qiluo-msft · 2017-04-27T20:28:45Z

        return counter_value

We can cache this return value, so if recursion happens, it can be fetch back immediately.

Refers to: src/sonic_ax_impl/mibs/ietf/rfc1213.py:152 in 61c87fb. [](commit_id = 61c87fb, deletion_comment = False)

qiluo-msft · 2017-04-27T20:52:10Z

def interface_name(self, sub_id):

I notice lots functions are the same as rfc1213. Suggest to refactor.

Refers to: src/sonic_ax_impl/mibs/ietf/rfc2863.py:73 in 61c87fb. [](commit_id = 61c87fb, deletion_comment = False)

qiluo-msft

As comments

oleksandrivantsiv · 2017-04-28T07:08:18Z

Refactor is not in scope of this commit. Suggest to create an enhancement

lguohan · 2017-05-01T16:26:44Z

src/sonic_ax_impl/mibs/__init__.py

+    """
+    db_conn.connect(APPL_DB)
+
+    lag_entries = db_conn.keys(APPL_DB, b"LAG_TABLE:*")


this function seems to be only executed at the init time. this may not be complete at certain point of time. For example, the system has 4 port channels, and at the time when you tries to get the LAG_TABLE, you only see 3.

Will update this PR soon

lguohan

init_sync_d_lag_tables is only executed at the init time, lag_table may not be complete at that point of time. need periodically scan the lag_table.

oleksandrivantsiv · 2017-05-05T19:09:25Z

@qiluo-msft Created an issue to refactor rfc1213.py and rfc2863.py files #23. Will clarify with Liat if someone from MLNX team can contribute to this.

There are still comments not fully resolved. However, they are not blocking.

* [sonic_ax_impl]: Add support of port channels. - Add port channels support to RFC1213 (1.3.6.1.2.1.2) and RFC2863 (1.3.6.1.2.1.31.1) MIBs. - Read oper state, admin state and MTU value from DB.

qiluo-msft · 2017-05-19T22:19:40Z

This merging breaks unit test. @oleksandrivantsiv Could you help check?

pytest -s -k tests/test_interfaces.py

[sonic_ax_impl]: Add support of port channels.

61c87fb

- Add port channels support to RFC1213 (1.3.6.1.2.1.2) and RFC2863 (1.3.6.1.2.1.31.1) MIBs. - Read oper state, admin state and MTU value from DB.

pavel-shirshov suggested changes Apr 25, 2017

View reviewed changes

stcheng added the Enhancement label Apr 25, 2017

Fix review comments

2297acb

qiluo-msft reviewed Apr 27, 2017

View reviewed changes

qiluo-msft previously requested changes Apr 27, 2017

View reviewed changes

pavel-shirshov approved these changes Apr 28, 2017

View reviewed changes

lguohan reviewed May 1, 2017

View reviewed changes

lguohan requested changes May 1, 2017

View reviewed changes

Update LAG table periodicaly.

795281b

lguohan approved these changes May 8, 2017

View reviewed changes

Fix review comments

35c2802

lguohan merged commit 74e57b8 into sonic-net:master May 8, 2017

qiluo-msft mentioned this pull request May 19, 2017

Unit test fails #25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sonic_ax_impl]: Add support of port channels. #21

[sonic_ax_impl]: Add support of port channels. #21

oleksandrivantsiv commented Apr 25, 2017

pavel-shirshov Apr 25, 2017

oleksandrivantsiv Apr 27, 2017

pavel-shirshov Apr 25, 2017

lguohan commented Apr 27, 2017

qiluo-msft Apr 27, 2017 •

edited

Loading

oleksandrivantsiv Apr 28, 2017

qiluo-msft Apr 27, 2017

oleksandrivantsiv Apr 28, 2017

qiluo-msft Apr 27, 2017

oleksandrivantsiv Apr 28, 2017

qiluo-msft Apr 28, 2017

qiluo-msft Apr 27, 2017

oleksandrivantsiv Apr 28, 2017

qiluo-msft Apr 28, 2017

qiluo-msft Apr 27, 2017

oleksandrivantsiv Apr 28, 2017

oleksandrivantsiv Apr 28, 2017

qiluo-msft Apr 28, 2017 •

edited

Loading

oleksandrivantsiv Apr 28, 2017

qiluo-msft commented Apr 27, 2017 •

edited

Loading

qiluo-msft commented Apr 27, 2017

qiluo-msft left a comment

oleksandrivantsiv commented Apr 28, 2017

lguohan May 1, 2017

oleksandrivantsiv May 4, 2017

oleksandrivantsiv May 5, 2017

lguohan left a comment

oleksandrivantsiv commented May 5, 2017

qiluo-msft commented May 19, 2017

[sonic_ax_impl]: Add support of port channels. #21

[sonic_ax_impl]: Add support of port channels. #21

Conversation

oleksandrivantsiv commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan commented Apr 27, 2017

qiluo-msft Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Apr 27, 2017 • edited Loading

qiluo-msft commented Apr 27, 2017

qiluo-msft left a comment

Choose a reason for hiding this comment

oleksandrivantsiv commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan left a comment

Choose a reason for hiding this comment

oleksandrivantsiv commented May 5, 2017

qiluo-msft commented May 19, 2017

qiluo-msft Apr 27, 2017 •

edited

Loading

qiluo-msft Apr 28, 2017 •

edited

Loading

qiluo-msft commented Apr 27, 2017 •

edited

Loading