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

[DPB][YANG] extended yang-model - added 'buffer_model' field #6464

Conversation

vadymhlushko-mlnx
Copy link
Contributor

@vadymhlushko-mlnx vadymhlushko-mlnx commented Jan 15, 2021

Signed-off-by: Vadym Hlushko [email protected]

- Why I did it

The fix for the issue [DPB][YANG] sonic-device_metadata.yang is not aligned with newest changes in CONFIG_DB

- How I did it

CONFIG_DB was extended with the field buffer_model - added representation of this field inside the sonic-device_metadata.yang

- How to verify it

Run the command config interface breakout <interface> <breakout_mode>

- 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)

@vadymhlushko-mlnx
Copy link
Contributor Author

Could you please review

@praveen-li, @zhenggen-xu, @stephenxs, @liat-grozovik

Copy link
Member

@praveen-li praveen-li left a comment

Choose a reason for hiding this comment

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

Kindly add a test case for pattern:

To add Test for sonic-yang-module:

1.) Add example Config at:
https://github.com/Azure/sonic-buildimage/blob/e52581e919c95e861cf630c9c7e1a840fc5e8ebd/src/sonic-yang-models/tests/yang_model_tests/yangTest.json#L2

Example config should be in sonic-yang format.

2.) Add matching detail and expected error string at:
https://github.com/Azure/sonic-buildimage/blob/e52581e919c95e861cf630c9c7e1a840fc5e8ebd/src/sonic-yang-models/tests/yang_model_tests/test_yang_model.py#L52

If You add a new leaf in YANG Models:
Then add the corresponding config in the same format as in configDB.json at:
https://github.com/Azure/sonic-buildimage/blob/4cf9316ec349f80deceb0d7665b0aaf85bfe2599/src/sonic-yang-models/tests/yang_model_tests/yangTest.json#L1038

And build both:
sonic_yang_mgmt-1.0-py3-none-any.whl
sonic_yang_models-1.0-py3-none-any.whl

Build tests will make sure, Translation works for new leaves in YANG models.

@vadymhlushko-mlnx
Copy link
Contributor Author

@praveen-li could you please review?

@@ -215,6 +215,26 @@ def initTest(self):
'value': 'disable'
}
},
'DEVICE_METADATA_CORRECT_BUFFER_MODEL_PATTERN': {
'desc': 'DEVICE_METADATA correct value for BUFFER_MODEL field',
'eStr': self.defaultYANGFailure['Verify'],
Copy link
Member

Choose a reason for hiding this comment

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

Just Say self.defaultYANGFailure['None']

And Remove:

'verify': {'xpath': '/sonic-device_metadata:sonic-device_metadata/DEVICE_METADATA/localhost/buffer_model',
                    'key': 'sonic-device_metadata:buffer_model',
                    'value': 'dynamic'

We usually use verify for default statements in YANG.

'DEVICE_METADATA_CORRECT_BUFFER_MODEL_PATTERN2': {
'desc': 'DEVICE_METADATA correct value for BUFFER_MODEL field',
'eStr': self.defaultYANGFailure['Verify'],
'verify': {'xpath': '/sonic-device_metadata:sonic-device_metadata/DEVICE_METADATA/localhost/buffer_model',
Copy link
Member

Choose a reason for hiding this comment

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

Same Here.

@vadymhlushko-mlnx
Copy link
Contributor Author

@praveen-li could you please review?

@vadymhlushko-mlnx
Copy link
Contributor Author

retest Azure.sonic-buildimage please

"platform": "x86_64-mlnx_msn4700-r0",
"hostname": "DUT-ASW",
"bgp_asn": "65000",
"buffer_model": "separated"
Copy link
Member

Choose a reason for hiding this comment

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

It will be great if the value itself can be "incorrect_pattern" instead of "separated".
ping me after that and I will approve, thx a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@praveen-li, done

praveen-li
praveen-li previously approved these changes Jan 28, 2021
liat-grozovik
liat-grozovik previously approved these changes Jan 31, 2021
@liat-grozovik
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@lguohan seems like KVM tests are stuck for more than a day. is this is known? how can we overcome it?

@liat-grozovik
Copy link
Collaborator

@lguohan we have no KVM test results. how can we merge?

@lguohan lguohan added the YANG YANG model related changes label Feb 4, 2021
@vadymhlushko-mlnx
Copy link
Contributor Author

/AzurePipelines run

@azure-pipelines
Copy link

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

@liat-grozovik
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

/AzurePipleines run

@liat-grozovik
Copy link
Collaborator

@lguohan Azure.sonic-buildimage is pending results for that last few days. Can you please help to merge?

@anshuv-mfst
Copy link

Hi @vadymhlushko-mlnx - Could you please join YANG subgroup meeting 2/25 (10-11am pst) to review with the YANG community, thanks.

@@ -1263,6 +1293,738 @@
}
},

"SAMPLE_CONFIG_DB_JSON": {
Copy link
Member

Choose a reason for hiding this comment

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

I think, this diff is not expected ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@praveen-li, I did git rebase, so this diff is expected because there was a merge conflicts, because "SAMPLE_CONFIG_DB_JSON" keys increased with new fields in yangTest.json file.

@praveen-li
Copy link
Member

@praveen-li , who to sign off this pr?

@lguohan : I have requested a changes on this PR, after that I can approve it.

@vadymhlushko-mlnx vadymhlushko-mlnx force-pushed the dpb_yang_device_metadata_extension branch 2 times, most recently from 19ae560 to a95a900 Compare March 30, 2021 09:34
@liat-grozovik
Copy link
Collaborator

@vadymhlushko-mlnx please handle conflicts.
@praveen-li could you please confirm your request changes were handled and PR can be approved?

@vadymhlushko-mlnx vadymhlushko-mlnx force-pushed the dpb_yang_device_metadata_extension branch from a95a900 to 90529e3 Compare April 5, 2021 12:27
@vadymhlushko-mlnx
Copy link
Contributor Author

@praveen-li, could you please review?

I have reworked this PR, according to the last changes in sonic-yang-models folder.

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@praveen-li could you please also approve?

1 similar comment
@vadymhlushko-mlnx
Copy link
Contributor Author

@praveen-li could you please also approve?

@anshuv-mfst anshuv-mfst requested a review from neethajohn April 15, 2021 22:32
@anshuv-mfst
Copy link

Hi @neethajohn - Could you please take a look, thanks.

@vadymhlushko-mlnx
Copy link
Contributor Author

@praveen-li could you please merge?

@liat-grozovik
Copy link
Collaborator

liat-grozovik commented Apr 22, 2021

removed the 202012 label. I don't think we should have it on 202012. DPB should be released as part of 202106.
since all tests pass and approval was provided, I merged it.

@liat-grozovik liat-grozovik merged commit 6bcea45 into sonic-net:master Apr 22, 2021
@longhuan-cisco
Copy link
Contributor

removed the 202012 label. I don't think we should have it on 202012. DPB should be released as part of 202106.

@liat-grozovik T0/T1 support is planned with 202012. Don't we need DPB supported for that? Just would like to confirm. Thanks.

raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-buildimage that referenced this pull request May 23, 2021
…est cases (sonic-net#6464)

- Why I did it
The fix for the issue [DPB][YANG] sonic-device_metadata.yang is not aligned with newest changes in CONFIG_DB

- How I did it
CONFIG_DB was extended with the field buffer_model - added representation of this field inside the sonic-device_metadata.yang

- How to verify it
Run the command config interface breakout <interface> <breakout_mode>
Signed-off-by: Vadym Hlushko <[email protected]>
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
…est cases (sonic-net#6464)

- Why I did it
The fix for the issue [DPB][YANG] sonic-device_metadata.yang is not aligned with newest changes in CONFIG_DB

- How I did it
CONFIG_DB was extended with the field buffer_model - added representation of this field inside the sonic-device_metadata.yang

- How to verify it
Run the command config interface breakout <interface> <breakout_mode>
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx vadymhlushko-mlnx deleted the dpb_yang_device_metadata_extension branch August 16, 2022 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Info ⌛ Bug 🐛 YANG YANG model related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants