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.

Sign up for GitHub

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

Jump to bottom

[sonic-breakout_cfg.yang]: Remove pattern from sonic-breakout_cfg.yang. #6801

Merged
merged 2 commits into from
Nov 1, 2021
Merged

[sonic-breakout_cfg.yang]: Remove pattern from sonic-breakout_cfg.yang. #6801

merged 2 commits into from
Nov 1, 2021

Conversation

praveen-li
Copy link
Member

@praveen-li praveen-li commented Feb 17, 2021 •
edited
Loading 

[sonic-breakout_cfg.yang]: Remove pattern from sonic-breakout_cfg.yang.

Changes:
-- Remove pattern from sonic-breakout_cfg.yang, it is redundant.
-- test changes.

Signed-off-by: Praveen Chaudhary pchaudhary@linkedin.com

Why I did it

-- Remove pattern from sonic-breakout_cfg.yang, it is redundant.
Breakout Mode is verified in Sonic-utilities using platform.json

How I did it

Remove pattern from sonic-breakout_cfg.yang
.

How to verify it

Built the packages after changing the tests.

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)

@praveen-li praveen-li requested a review from lguohan as a code owner February 17, 2021 02:25
@ben-gale ben-gale added the YANG YANG model related changes label Feb 23, 2021
@kwangsuk
Copy link
Contributor

There is a reference to BREAKOUT_CFG table and yang.
https://github.com/Azure/SONiC/blob/master/doc/dynamic-port-breakout/sonic-dynamic-port-breakout-HLD.md

Don't you need to update the HLD accordingly?

@praveen-li
Copy link
Member Author

praveen-li commented Feb 23, 2021 via email

@kwangsuk
Copy link
Contributor

@praveen-li
Oh, ok. So, the BREAKOUT_CFG table will still exist. Then any reason to remove YANG file?

@praveen-li praveen-li force-pushed the remove_sonic_brkcfg branch from 8497f32 to cdea210 Compare March 4, 2021 01:28
@praveen-li
Copy link
Member Author

@praveen-li
Oh, ok. So, the BREAKOUT_CFG table will still exist. Then any reason to remove YANG file?

Possible breakout modes are controlled by platform.json and are thoroughly checked by the Python code. So we do not need extra layer of checking using Yang model because then we have to maintain it at 2 places.

Also breakout CFG table doesn't impact backend.

@praveen-li
Copy link
Member Author

/Azure Pipelines/Azure.sonic-buildimage run

@anshuv-mfst anshuv-mfst requested a review from zhenggen-xu March 11, 2021 18:11
@anshuv-mfst
Copy link

3/11: PR reviewed in YANG subgroup.
@zhenggen-xu - could you please review as well.

@lguohan
Copy link
Collaborator

lguohan commented Mar 28, 2021

@praveen-li, who to sign off?

@praveen-li
Copy link
Member Author

@praveen-li, who to sign off?

@lguohan I think Xu can review this one, once I resolve the conflicts.
@zhenggen-xu

Prasanth-KV
Prasanth-KV previously approved these changes Apr 6, 2021
View reviewed changes
@zhenggen-xu
Copy link
Collaborator

After some more thought, I think we don't want to treat this table BREAKOUT_CFG very differently. The main problem for this table with Yang models is that the breakout mode is not known for all platforms and hard to maintain. Maybe we can relax that in the Yang model file, and we validate that by the sonic-utilities or other plugins (which was the case already). The yang model here is still good to have at least to validate the sanity of field name etc. This also makes the table to be consistent with other tables.

zhenggen-xu
zhenggen-xu suggested changes May 12, 2021
View reviewed changes
Copy link
Collaborator

@zhenggen-xu zhenggen-xu left a comment

Choose a reason for hiding this comment

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

As requested

@zhenggen-xu zhenggen-xu mentioned this pull request May 12, 2021
Open
@praveen-li
Copy link
Member Author

After some more thought, I think we don't want to treat this table BREAKOUT_CFG very differently. The main problem for this table with Yang models is that the breakout mode is not known for all platforms and hard to maintain. Maybe we can relax that in the Yang model file, and we validate that by the sonic-utilities or other plugins (which was the case already). The yang model here is still good to have at least to validate the sanity of field name etc. This also makes the table to be consistent with other tables.

@zhenggen-xu : I guess then , it makes sense to go ahead by just removing the pattern in this yang model and we can still keep the yang model file.

                leaf brkout_mode {
                    type string {
                        length 1..64;
                        /*
                         * Below allowed patterns are based on most used Platforms,
                         * Add any other breakout-mode to allow Dynamic Port
                         * Breakout to that breakout-mode.
                         */
                        pattern '1x100G\[40G\]|2x50G|4x25G\[10G\]|2x25G\(2\)\+1x50G\(2\)|1x50G\(2\)\+2x25G\(2\)|1x400G|2x200G|4x100G|8x50G|1x200G\[100G,50G,40G,25G,10G,1G\]|2x100G\[50G,40G,25G,10G,1G\]|4x50G\[40G,25G,10G,1G\]|1x25G\[10G\]|1x100G\[50G,40G,25G,10G\]|2x50G\[40G,25G,10G\]|4x25G\[10G\]|1x25G\[10G,1G\]|1x100G\[50G,40G,25G,10G,1G\]|2x50G\[40G,25G,10G,1G\]|4x25G\[10G,1G\]|1x400G\[200G,100G,50G,40G,25G,10G,1G\]|2x200G\[100G,50G,40G,25G,10G,1G\]|4x100G\[50G,40G,25G,10G,1G\]';
                    }
                }

@zhenggen-xu
Copy link
Collaborator

After some more thought, I think we don't want to treat this table BREAKOUT_CFG very differently. The main problem for this table with Yang models is that the breakout mode is not known for all platforms and hard to maintain. Maybe we can relax that in the Yang model file, and we validate that by the sonic-utilities or other plugins (which was the case already). The yang model here is still good to have at least to validate the sanity of field name etc. This also makes the table to be consistent with other tables.

@zhenggen-xu : I guess then , it makes sense to go ahead by just removing the pattern in this yang model and we can still keep the yang model file.

                leaf brkout_mode {
                    type string {
                        length 1..64;
                        /*
                         * Below allowed patterns are based on most used Platforms,
                         * Add any other breakout-mode to allow Dynamic Port
                         * Breakout to that breakout-mode.
                         */
                        pattern '1x100G\[40G\]|2x50G|4x25G\[10G\]|2x25G\(2\)\+1x50G\(2\)|1x50G\(2\)\+2x25G\(2\)|1x400G|2x200G|4x100G|8x50G|1x200G\[100G,50G,40G,25G,10G,1G\]|2x100G\[50G,40G,25G,10G,1G\]|4x50G\[40G,25G,10G,1G\]|1x25G\[10G\]|1x100G\[50G,40G,25G,10G\]|2x50G\[40G,25G,10G\]|4x25G\[10G\]|1x25G\[10G,1G\]|1x100G\[50G,40G,25G,10G,1G\]|2x50G\[40G,25G,10G,1G\]|4x25G\[10G,1G\]|1x400G\[200G,100G,50G,40G,25G,10G,1G\]|2x200G\[100G,50G,40G,25G,10G,1G\]|4x100G\[50G,40G,25G,10G,1G\]';
                    }
                }

Yes, let's keep the file, but not maintain the pattern here, the pattern validation is done by platform specific platform.json and library.

@anshuv-mfst
Copy link

@praveen-li - can you please address review comments and close on open items.

[sonic-breakout_cfg.yang]: Remove pattern from sonic-breakout_cfg.yang.
da7db2d 
Changes:
-- Remove pattern from sonic-breakout_cfg.yang, it is redundant.
-- test changes.

Signed-off-by: Praveen Chaudhary <pchaudhary@linkedin.com>
@praveen-li praveen-li force-pushed the remove_sonic_brkcfg branch from cdea210 to da7db2d Compare June 23, 2021 01:10
@praveen-li praveen-li changed the title [sonic-breakout_cfg.yang]: Remove yang model for BREAKOUT_CFG table. [sonic-breakout_cfg.yang]: Remove pattern from sonic-breakout_cfg.yang. Jun 23, 2021
@praveen-li
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

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

[sonic-breakout_cfg.yang]: description spelling.
b3b1ac1 
Signed-off-by: Praveen Chaudhary <pchaudhary@linkedin.com>
@praveen-li praveen-li force-pushed the remove_sonic_brkcfg branch from a1d4ce4 to b3b1ac1 Compare August 3, 2021 00:10
@zhangyanzhao
Copy link
Collaborator

will merge after the test passed

@praveen-li
Copy link
Member Author

/apzw run

@alexrallen alexrallen mentioned this pull request Oct 4, 2021
Closed
5 tasks
@zhangyanzhao
Copy link
Collaborator

@praveen-li will check the build failure.

@praveen-li
Copy link
Member Author

praveen-li commented Oct 7, 2021 via email

@zhangyanzhao
Copy link
Collaborator

/azpw run

@zhangyanzhao
Copy link
Collaborator

azpw /run

@praveen-li
Copy link
Member Author

/apzw run

@praveen-li
Copy link
Member Author

Below failures is seen for many other PRs, so I am waiting for it. @wangxin If you know that this failure is known. Thx

        test_report = {}
        check_services(duthost)
        check_interfaces_and_transceivers(duthost, request)
        check_neighbors(duthost, tbinfo)
        verify_no_coredumps(duthost, pre_existing_cores)
        check_all = all([check == True for check in list(test_report.values())])
        pytest_assert(check_all, "Health check failed after reboot: {}"
>           .format(test_report))
E       Failed: Health check failed after reboot: {'check_interfaces_and_transceivers': True, 'check_services': True, 'verify_no_coredumps': 'Core dumps found. Expected: 0 Found: 1', 'check_neighbors': True}

check      = True
check_all  = False
duthost    = <MultiAsicSonicHost> vlab-01
duthosts   = <tests.common.devices.duthosts.DutHosts object at 0x7fb593efb310>
pre_existing_cores = u'0'
rand_one_dut_hostname = 'vlab-01'
request    = <SubRequest 'verify_dut_health' for <Function test_warm_reboot>>
tbinfo     = {'auto_recover': 'False', 'comment': 'Tests virtual switch vm', 'conf-name': 'vms-kvm-t0', 'duts': ['vlab-01'], ...}

@praveen-li
Copy link
Member Author

/apzw run

@praveen-li
Copy link
Member Author

/apzw run

@qiluo-msft, Kindly help to rerun tests, seems "/apzw run" not working. Thx a lot in advance for it.

@praveen-li praveen-li mentioned this pull request Oct 25, 2021
Closed
5 tasks
@vadymhlushko-mlnx
Copy link
Contributor

vadymhlushko-mlnx commented Oct 26, 2021 •
edited
Loading

@praveen-li do you plan to cherry-pick this PR to 202106 branch, if yes please add the label to this PR?

@liat-grozovik
Copy link
Collaborator

@praveen-li could you please help to have this PR approved and merged? it is needed for 202106 as well.

@qiluo-msft
Copy link
Collaborator

@praveen-li It's /azpw run

@qiluo-msft
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@praveen-li
Copy link
Member Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@volodymyrsamotiy
Copy link
Collaborator

volodymyrsamotiy commented Oct 29, 2021 •
edited
Loading

@zhangyanzhao, there is a label you added recently to this PR - Included in 202106 branch.
I am not sure whether these changes are already in 202106, maybe label supposed to be Request for 202106 branch.
Could you please clarify?

@lguohan lguohan merged commit 021b7dc into sonic-net:master Nov 1, 2021
judyjoseph pushed a commit that referenced this pull request Nov 6, 2021
@judyjoseph
[sonic-breakout_cfg.yang]: Remove pattern from sonic-breakout_cfg.yan…
d627587 
…g. (#6801)

Changes:
-- Remove pattern from sonic-breakout_cfg.yang, it is redundant.
-- test changes.

Signed-off-by: Praveen Chaudhary <pchaudhary@linkedin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@venkatmahalingam venkatmahalingam venkatmahalingam left review comments

@lguohan lguohan lguohan approved these changes

@dgsudharsan dgsudharsan dgsudharsan approved these changes

@zhenggen-xu zhenggen-xu zhenggen-xu approved these changes

@Prasanth-KV Prasanth-KV Prasanth-KV left review comments

Assignees
No one assigned
Labels
Included in 202106 Branch Request for 202106 Branch YANG YANG model related changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.