Skip to content

Support L1 & L3 Config generation in SONiC #7637

Merged
judyjoseph merged 9 commits intosonic-net:masterfrom
VenkatCisco:VenkatCisco/master/PR12_L1_L2_cfg
Jul 8, 2021
Merged

Support L1 & L3 Config generation in SONiC #7637
judyjoseph merged 9 commits intosonic-net:masterfrom
VenkatCisco:VenkatCisco/master/PR12_L1_L2_cfg

Conversation

@VenkatCisco
Copy link
Contributor

Why I did it

This provides support for: PR #7074.

How I did it

Extend sonic-config-engine/config_samples.py to provide support for l1 & l3

How to verify it

Verify system bring up with L1 & L3 configuration

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)

@VenkatCisco VenkatCisco requested a review from lguohan as a code owner May 18, 2021 06:43
@VenkatCisco VenkatCisco changed the title Venkat cisco/master/pr12 l1 l2 cfg Support L1 & L3 Config in generation in SONiC May 18, 2021
@VenkatCisco VenkatCisco changed the title Support L1 & L3 Config in generation in SONiC Support L1 & L3 Config generation in SONiC May 18, 2021
'l2': generate_l2_config,
'empty': generate_empty_config
'empty': generate_empty_config,
'l1': generate_l1_config,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a unit test for this? You can refer as done for #7215

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comments. Added UT. Please review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsunny do you have any more comments or else would be able to approve this PR ?

@prsunny
Copy link
Contributor

prsunny commented May 19, 2021

How is this different from #7368. I see it being actively reviewed?

@VenkatCisco
Copy link
Contributor Author

How is this different from #7368. I see it being actively reviewed?

We discussed in todays scrum. The PR was raised agains #7368 but a new set of updates to the existing PR ends up been tracked as different PR (i raised this in todays mtg with Anshu/Guhon as well). I confirmed that #7368 will be closed and tracked via #7637

UNICODE_TYPE = unicode


# The new enhancements to this file provides capabilities to generate l1 & l3
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment looks more like PR description. I think this should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it will require the comment update again once someone add an additional template..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed the comments and uploaded the new change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests for both L1 & L3 passed as belows.

test_l1_ports_template (tests.test_j2files.TestJ2Files) ... ok
test_l3_ports_template (tests.test_j2files.TestJ2Files) ... ok

New failures are seen in unrleated area.

@judyjoseph
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@judyjoseph
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VenkatCisco
Copy link
Contributor Author

/azp run

thanks for triggering the run. would you able able to approve given all tests have passed now ?

@judyjoseph
Copy link
Contributor

@VenkatCisco, can you remove this file src/sonic-config-engine/.config_samples.py.swp from this PR ?

"Ethernet5": {},
"Ethernet6": {},
"Ethernet7": {},
"Ethernet8": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is an l3 template did you want to add IP addresses to these interfaces in your usecase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@VenkatCisco, can you remove this file src/sonic-config-engine/.config_samples.py.swp from this PR ?

Sure, removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is an l3 template did you want to add IP addresses to these interfaces in your usecase?

In the review mtg, there was an ask from Guhon and team to not hardcode the ip address etc. Even though the config_samples.py has other areas like generate_t1_sample_config catered to specific needs, this method is made more generic as per request.

@VenkatCisco
Copy link
Contributor Author

@VenkatCisco, can you remove this file src/sonic-config-engine/.config_samples.py.swp from this PR ?

Done. removed the .swp file.

@VenkatCisco VenkatCisco reopened this Jun 28, 2021
@judyjoseph
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 7637 in repo Azure/sonic-buildimage

Copy link
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

lgtm

@anshuv-mfst
Copy link

@abdosi - can you please review, approve asap.

@judyjoseph judyjoseph merged commit 487dd46 into sonic-net:master Jul 8, 2021
judyjoseph pushed a commit that referenced this pull request Jul 13, 2021
Why I did it
This provides support for: PR #7074.

How I did it
Extend sonic-config-engine/config_samples.py to provide support for l1 & l3
@anamehra
Copy link
Contributor

anamehra commented Aug 6, 2021

@judyjoseph Please help to add this to 202012 and 2012911 as well.

@abdosi
Copy link
Contributor

abdosi commented Aug 6, 2021

cc @qiluo-msft @yxieca

qiluo-msft pushed a commit that referenced this pull request Aug 7, 2021
Why I did it
This provides support for: PR #7074.

How I did it
Extend sonic-config-engine/config_samples.py to provide support for l1 & l3
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
Why I did it
This provides support for: PR sonic-net#7074.

How I did it
Extend sonic-config-engine/config_samples.py to provide support for l1 & l3
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.

8 participants