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

[RouteUpdater]: Fix multi_asic mock function implementation and multi_asic variable name #186

Merged
merged 2 commits into from
Dec 23, 2020

Conversation

SuvarnaMeenakshi
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi commented Dec 22, 2020

[RouteUpdater]: Fix multi_asic mock function implementation.
Update multi_asic mock function to return port_table from config_db
was returning empty dictionary due to which RouteUpdater class
was not completely unit-tested as one of the condition check was
never set to true.

Signed-off-by: Suvarna Meenakshi sumeenak@microsoft.com

- What I did

  1. Fix multi_asic mock function implementation to get port_table information. There was a mistake in mock_get_port_table function due to which port_table was always empty dictionary.
  2. Use multi_asic.get_port_table_for_asic() function as we require port_table only of a specific namespace and do not require the entire port_table.
  3. Fix the incorrect multi_asic variable name used in RouteUdpater.
    Provides fix for 'sonic_py_common.multi_asic' has no attribute 'ROLE'" error message #188

- How I did it

  1. Update multi_asic mock_get_port_table_for_asic implementation.
  2. Fix the incorrect multi_asic variable name used in RouteUdpater.

- How to verify it
Load updated docker and query 1.3.6.1.2.1.4.24 MIB. Verify the result.
"AttributeError: module 'sonic_py_common.multi_asic' has no attribute 'ROLE'" error message should not be seen in syslog .

- Description for the changelog

multi_asic mock function to return port_table from config_db
was returning empty dictionary due to which RouteUpdater class
was not completely unit-tested as one of the condition check was
never set to true.

Signed-off-by: Suvarna Meenakshi <sumeenak@microsoft.com>
@svc-acs
Copy link
Collaborator

svc-acs commented Dec 22, 2020

Can one of the admins verify this patch?

multi_asic.ROLE in port_table[ifn] and
port_table[ifn][multi_asic.ROLE] == multi_asic.INTERNAL_PORT):
multi_asic.PORT_ROLE in port_table[ifn] and
port_table[ifn][multi_asic.PORT_ROLE] == multi_asic.INTERNAL_PORT):
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 23, 2020

Choose a reason for hiding this comment

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

[](start = 86, length = 1)

Remove trailing blank char. #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.

multi_asic.ROLE in port_table[ifn] and
port_table[ifn][multi_asic.ROLE] == multi_asic.INTERNAL_PORT):
multi_asic.PORT_ROLE in port_table[ifn] and
port_table[ifn][multi_asic.PORT_ROLE] == multi_asic.INTERNAL_PORT):
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 23, 2020

Choose a reason for hiding this comment

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

If if statement is line breaked, you may add more indentation to them, so the branch code block is easier to find. There is a PEP. #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.

Signed-off-by: Suvarna Meenakshi <sumeenak@microsoft.com>
@abdosi
Copy link
Contributor

abdosi commented Dec 23, 2020

@SuvarnaMeenakshi INTERNAL_PORT is already present in mock db ?

@SuvarnaMeenakshi
Copy link
Contributor Author

@SuvarnaMeenakshi INTERNAL_PORT is already present in mock db ?

yes, for example: https://github.com/Azure/sonic-snmpagent/blob/381ae478a6cb9f99e150406ed68cb672207bbddc/tests/mock_tables/asic0/config_db.json#L21
The mock_table's Role is not used to determine if a port channel is internal or not, to make the mock functions simpler, I have added a list of internal PortChannels here: https://github.com/Azure/sonic-snmpagent/blob/master/tests/mock_tables/multi_asic.py#L9
Also, there is a unit-test case to check if have an route through internal interface:
https://github.com/Azure/sonic-snmpagent/blob/381ae478a6cb9f99e150406ed68cb672207bbddc/tests/namespace/test_forward.py#L134

@qiluo-msft qiluo-msft merged commit 025483a into sonic-net:master Dec 23, 2020
abdosi pushed a commit that referenced this pull request Dec 31, 2020
…_asic variable name (#186)

[RouteUpdater]: Fix multi_asic mock function implementation.
Update multi_asic mock function to return port_table from config_db
was returning empty dictionary due to which RouteUpdater class
was not completely unit-tested as one of the condition check was
never set to true.

**- What I did**
1. Fix multi_asic mock function implementation to get port_table information.  There was a mistake in mock_get_port_table function due to which port_table was always empty dictionary.
2. Use multi_asic.get_port_table_for_asic() function as we require port_table only of a specific namespace and do not require the entire port_table.
3. Fix the incorrect multi_asic variable name used in RouteUdpater.
Provides fix for #188

**- How I did it**
1. Update multi_asic mock_get_port_table_for_asic implementation.
3. Fix the incorrect multi_asic variable name used in RouteUdpater.

**- How to verify it**
Load updated docker and query 1.3.6.1.2.1.4.24 MIB. Verify the result.
"AttributeError: module 'sonic_py_common.multi_asic' has no attribute 'ROLE'" error message should not be seen in syslog .
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.

4 participants