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

[sonic-cfggen] Allow cfggen to work on system without ports #7999

Merged
merged 6 commits into from
Aug 23, 2021

Conversation

liorghub
Copy link
Contributor

Signed-off-by: liora liora@nvidia.com

Why I did it

Allow cfggen to work on system without ports in platform.json or in port_config.ini

How I did it

Add json write of PORT section only if the dictionary that contains the ports is not empty.

How to verify it

sonic-cfggen -k ACS-MSN3700 -H -j /etc/sonic/init_cfg.json --print-data

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

@liorghub liorghub requested a review from lguohan as a code owner June 28, 2021 14:42
@liorghub liorghub force-pushed the zero_ports_2 branch 3 times, most recently from 237a6ec to 38e39ac Compare July 5, 2021 15:56
Signed-off-by: liora <liora@nvidia.com>
dgsudharsan
dgsudharsan previously approved these changes Jul 7, 2021
@@ -317,9 +317,6 @@ def main():
if args.port_config is None:
args.port_config = device_info.get_path_to_port_config_file(hwsku)
(ports, _, _) = get_port_config(hwsku, platform, args.port_config, asic_id)
if not ports:
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 19, 2021

Choose a reason for hiding this comment

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

ports

This is a new feature or significant fix. Could you add new testcase to protect it? #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still applicable. Could you help add a positive testcase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiluo-msft
While I implemented the unittest I noticed a bug.
I handled the flow of reading the ports from port_config.ini but not the flow of reading the ports from platrfom.json+hwsku.json. Now it is fixed and I added unittest as well.
Appreciate your review.

@@ -317,9 +317,6 @@ def main():
if args.port_config is None:
args.port_config = device_info.get_path_to_port_config_file(hwsku)
(ports, _, _) = get_port_config(hwsku, platform, args.port_config, asic_id)
if not ports:
print('Failed to get port config', file=sys.stderr)
sys.exit(1)
deep_update(data, {'PORT': ports})
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 19, 2021

Choose a reason for hiding this comment

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

ports

Does it work with:

  1. ports == None
  2. Or, ports == {}

If only one, let's use the explicit contant. #Closed

Copy link
Contributor Author

@liorghub liorghub Jul 26, 2021

Choose a reason for hiding this comment

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

@qiluo-msft
Thanks, I changed the code according to your comment, I will explain.

The code I deleted, verified both conditions:

  1. ports is not None.
  2. ports is not empty

Since now (in modular chassis without linecards), ports dictionary can be empty, we just need to verify it is not None.

qiluo-msft
qiluo-msft previously approved these changes Jul 26, 2021
Signed-off-by: liora <liora@nvidia.com>
@liorghub
Copy link
Contributor Author

liorghub commented Aug 3, 2021

@qiluo-msft
@dprital

The compilation is failing since test "test_buffers_multi_asic_template " is failing with the error below.
When I am running this test in my environment it is passing.
I am not sure how to proceed, appreciate your advice.

Failed test log:
2021-08-03T18:25:18.2099599Z FAIL: test_buffers_multi_asic_template (tests.test_multinpu_cfggen.TestMultiNpuCfgGen)
2021-08-03T18:25:18.2100081Z ----------------------------------------------------------------------
2021-08-03T18:25:18.2100384Z Traceback (most recent call last):
2021-08-03T18:25:18.2100898Z File "/sonic/src/sonic-config-engine/tests/test_multinpu_cfggen.py", line 417, in test_buffers_multi_asic_template
2021-08-03T18:25:18.2101348Z 'Ethernet12': '300m'
2021-08-03T18:25:18.2102110Z AssertionError: {u'AZURE': {u'Ethernet8': u'0m', u'Ethernet0': u'300m', u'Ethernet4': u'300m', u [truncated]... != {'AZURE': {'Ethernet-BP4': '5m', 'Ethernet8': '300m', 'Ethernet-BP0': '5m', 'Eth [truncated]...
2021-08-03T18:25:18.2102684Z Diff is 903 characters long. Set self.maxDiff to None to see it.
2021-08-03T18:25:18.2102835Z

Passing test log from my env:
liora@6874c9fe3fa6:/sonic/src/sonic-config-engine/tests$ pytest-3 test_multinpu_cfggen.py -vvv -k test_buffers_multi_asic_template
=========================================================================================================== test session starts ===========================================================================================================
platform linux -- Python 3.7.3, pytest-3.10.1, py-1.7.0, pluggy-0.8.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /sonic/src/sonic-config-engine, inifile:
plugins: cov-2.6.0
collected 36 items / 35 deselected

test_multinpu_cfggen.py::TestMultiNpuCfgGen::test_buffers_multi_asic_template PASSED [100%]

================================================================================================= 1 passed, 35 deselected in 0.18 seconds =================================================================================================
liora@6874c9fe3fa6:/sonic/src/sonic-config-engine/tests$

@saiarcot895
Copy link
Contributor

It might be worth noting that the pipeline build for #8191 is now failing with that error. The logs for that error is here.

@liorghub
Copy link
Contributor Author

liorghub commented Aug 8, 2021

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liorghub
Copy link
Contributor Author

/AzurePipelines run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 7999 in repo Azure/sonic-buildimage

@liorghub
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liorghub
Copy link
Contributor Author

@qiluo-msft can you please approve this commit?

@@ -85,3 +91,10 @@ def test_platform_json_all_ethernet_interfaces(self):
output_dict = ast.literal_eval(output.strip())
expected = ast.literal_eval(json.dumps(fh_data))
self.assertDictEqual(output_dict, expected)

@mock.patch('portconfig.readJson', mock.MagicMock(return_value={INTF_KEY:{}}))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure what this file is? Can you please elaborate? Additionally, can you confirm if the format readJson is correct?

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.

5 participants