Skip to content

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

Merged
qiluo-msft merged 6 commits into
sonic-net:masterfrom
liorghub:zero_ports_2
Aug 23, 2021
Merged

qiluo-msft merged 6 commits into
sonic-net:masterfrom
liorghub:zero_ports_2

Conversation

@liorghub

Copy link
Copy Markdown
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
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:

@qiluo-msft qiluo-msft Jul 19, 2021 •

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

if not ports:
print('Failed to get port config', file=sys.stderr)
sys.exit(1)
deep_update(data, {'PORT': ports})

@qiluo-msft qiluo-msft Jul 19, 2021 •

Copy link
Copy Markdown
Collaborator

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

@liorghub liorghub Jul 26, 2021 •

Copy link
Copy Markdown
Contributor Author

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>
Signed-off-by: liora <liora@nvidia.com>
Signed-off-by: liora <liora@nvidia.com>
@liorghub

liorghub commented Aug 3, 2021

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

liorghub commented Aug 8, 2021

Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld

Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@liorghub

Copy link
Copy Markdown
Contributor Author

/AzurePipelines run

@azure-pipelines

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

@liorghub

Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld

Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@liorghub

Copy link
Copy Markdown
Contributor Author

@qiluo-msft can you please approve this commit?

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
Copy Markdown
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