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] Fix init t1 config to align with YANG #21195

Merged
merged 8 commits into from
Jan 15, 2025

Conversation

wen587
Copy link
Contributor

@wen587 wen587 commented Dec 17, 2024

Why I did it

Improve the t1 config to align with YANG validation

Work item tracking
  • Microsoft ADO (number only): 31268169

How I did it

Add missing leafref and mandatory field to the config

How to verify it

YANG validation check on generated config

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

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

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wen587
Copy link
Contributor Author

wen587 commented Dec 17, 2024

/azpw ms_conflict

This reverts commit 32692ea.
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wen587
Copy link
Contributor Author

wen587 commented Dec 17, 2024

/azp run Azure.sonic-buildimage

Copy link

Commenter does not have sufficient privileges for PR 21195 in repo sonic-net/sonic-buildimage

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wen587 wen587 marked this pull request as ready for review December 18, 2024 08:34
@wen587 wen587 requested a review from qiluo-msft December 18, 2024 08:34
@lguohan lguohan added the YANG YANG model related changes label Dec 19, 2024
@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Dec 24, 2024

def generate_t1_sample_config(data):

How to prevent regression in future? Could you add a yang validation into unit test? #Closed


Refers to: src/sonic-config-engine/config_samples.py:49 in 051a6ee. [](commit_id = 051a6ee, deletion_comment = False)

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wen587
Copy link
Contributor Author

wen587 commented Jan 10, 2025

YANG validation check blocked by #21111

@@ -238,6 +244,7 @@ def generate_l2_config(data):
# Ports in use should be admin up, unused ports default to admin down
if port in downlinks or port in uplinks:
data['PORT'][port].setdefault('admin_status', 'up')
data['PORT'][port].setdefault('speed', '50000')
Copy link
Collaborator

@qiluo-msft qiluo-msft Jan 10, 2025

Choose a reason for hiding this comment

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

setdefault

the l2_config is used for long time, why it is working but not yang compliant? Is it possible a yang bug? #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, port_config.ini always contains the speed column.
But the Unit test was using a port_config.ini doesn't contain speed https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-config-engine/tests/t0-sample-port-config.ini
That's probably why we didn't hit yang issue for such long time.

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Jan 10, 2025

                                'Loopback2|3.3.3.3': {}

@theasianpianist to review #Closed


Refers to: src/sonic-config-engine/config_samples.py:192 in 051a6ee. [](commit_id = 051a6ee, deletion_comment = True)

@theasianpianist
Copy link
Contributor

                                'Loopback2|3.3.3.3': {}

@theasianpianist to review

Refers to: src/sonic-config-engine/config_samples.py:192 in 051a6ee. [](commit_id = 051a6ee, deletion_comment = True)

This change looks ok to me, did you have a specific concern?

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qiluo-msft qiluo-msft enabled auto-merge (squash) January 14, 2025 23:56
@qiluo-msft qiluo-msft merged commit 7e397b0 into sonic-net:master Jan 15, 2025
20 checks passed
VladimirKuk pushed a commit to Marvell-switching/sonic-buildimage that referenced this pull request Jan 21, 2025
Why I did it
Improve the t1 config to align with YANG validation

How I did it
Add missing leafref and mandatory field to the config

How to verify it
YANG validation check on generated config
@wen587
Copy link
Contributor Author

wen587 commented Feb 6, 2025

Re add 202411 to see if it generate PR automatically.

@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202411: #21641

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.

6 participants