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

[minigraph][dualtor] add DualToR type in DEVICE_METADATA #11689

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vdahiya12
Copy link
Contributor

@vdahiya12 vdahiya12 commented Aug 10, 2022

Signed-off-by: vaibhav-dahiya vdahiya@microsoft.com

Why I did it

This PR adds a DualToRType field to DEVICE_METADATA table in config DB.
the subtypes can be active-active or active-standby
Using this field we can distinguish between active-active or active-standby topology.

How I did it

Changed the minigraph parser for including the DualToRType field in DEVICE_METADATA

How to verify it

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

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

Description for the changelog

Link to config_db schema for YANG module changes

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

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@prsunny
Copy link
Contributor

prsunny commented Aug 11, 2022

please provide some details in the description as required by the template. It will be useful for referencing the new attribute and how it looks like with the change

@vdahiya12 vdahiya12 marked this pull request as draft August 11, 2022 00:28
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12 vdahiya12 marked this pull request as ready for review August 25, 2022 18:20
@@ -151,6 +151,12 @@ module sonic-device_metadata {
}
}

leaf DualToRtype {
Copy link
Collaborator

Choose a reason for hiding this comment

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

DualToRtype

dualtor_type? Is it following the cases from another place?

@@ -151,6 +151,12 @@ module sonic-device_metadata {
}
}

leaf DualToRtype {
type string {
pattern "active-active|active-standby";
Copy link
Collaborator

Choose a reason for hiding this comment

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

pattern

Need yang unit test.

@qiluo-msft qiluo-msft requested a review from ganglyu August 25, 2022 23:25
@qiluo-msft qiluo-msft added the YANG YANG model related changes label Aug 25, 2022
@@ -151,6 +151,12 @@ module sonic-device_metadata {
}
}

leaf DualToRtype {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add description

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not dual tor device, what's the expected value? active-active?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dualtortype is expected to be only active-active or active-standby

@ganglyu
Copy link
Contributor

ganglyu commented Aug 26, 2022

Please update sample_config_db.json

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Copy link
Collaborator

@dgsudharsan dgsudharsan left a comment

Choose a reason for hiding this comment

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

Please add UT for yang. Please update sample_config_db.json and also Configuration.md

@zhangyanzhao
Copy link
Collaborator

Please add UT for yang. Please update sample_config_db.json and also Configuration.md

@vdahiya12 can you please help to address this comments? Thanks.

@zhangyanzhao
Copy link
Collaborator

@vdahiya12 can you please help to take a look at this issue? Thanks.

@vdahiya12 vdahiya12 marked this pull request as draft March 18, 2023 22:36
@vdahiya12
Copy link
Contributor Author

lower priority right now

@qiluo-msft
Copy link
Collaborator

@ganglyu Why this could pass load_minigraph UT if this one is not merged?

@qiluo-msft
Copy link
Collaborator

@vdahiya12 Please prioritize the UT, and make PR ready for review.

@ganglyu
Copy link
Contributor

ganglyu commented Jan 26, 2024

@ganglyu Why this could pass load_minigraph UT if this one is not merged?

In load_minigraph UT, we run yang validation for generated config_db.
This PR has changed output config_db and yang models together, so it could pass ut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
YANG YANG model related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants