[TH6-128] Increase announce routes networks for lt2-o256-u32d224 topo#24665
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sanjair-git
force-pushed
the
announce-routes-th6
branch
from
e204ddf to
6ca8abd
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
mssonicbld
added
the
Request for 202511 branch
|
This PR has backport request for branch(es): 202511. ---Powered by SONiC BuildBot
|
|
/azpw retry |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1114610: ✅Stage Test:
|
anders-nexthop
left a comment
anders-nexthop
left a comment
There was a problem hiding this comment.
Traced this against BASE_ADDR_V4 = "192.128.0.0/9": the dynamic num_extra = max(4, ceil(len(t1_vms)/32)) cleanly generalizes the old hardcoded four /27s, the max(4, …) floor keeps smaller topos behaving exactly as before, and for lt2-o256-u32d224 (224 T1) it produces the 7 blocks needed. The 192.168+/27 extras already sit inside the 192.128.0.0/9 main block as more-specific routes, so extending the range to 192.174 doesn't introduce any new overlap — same pattern, just enough blocks now. UNICODE_TYPE matches the file's existing py2/3 idiom. Looks good; one minor non-blocking note inline.
— anders-bot (AI-assisted, on behalf of @anders-nexthop)
| extra_networks = [] | ||
| for i in range(num_extra): | ||
| extra_networks.append( | ||
| ipaddress.ip_network(UNICODE_TYPE("192.{}.0.0/27".format(168 + i))) |
There was a problem hiding this comment.
Minor / non-blocking: because the second octet is 168 + i, this silently caps at num_extra <= 88 (~2816 T1 VMs) — beyond that 192.256.0.0/27 would raise ValueError. Well past any realistic lt2 topology, so not a concern here, but an assert num_extra <= 88 or a short comment noting the ceiling would make the limit explicit if a future topo ever pushes the T1 count that high.
There was a problem hiding this comment.
Thanks for the review. Added an assert and a comment.
|
@sanjair-git please address the merge conflicts and then i will merge this PR. |
Signed-off-by: sanrajen <sanjai.rajendran@nokia.com>
sanjair-git
dismissed stale reviews from anders-nexthop and bingwang-ms
via
3930049
sanjair-git
force-pushed
the
announce-routes-th6
branch
from
6ca8abd to
3930049
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…sonic-net#24665) <!-- Please make sure you've read and understood our contributing guidelines; https://github.com/sonic-net/SONiC/blob/gh-pages/CONTRIBUTING.md Please provide following information to help code review process a bit easier: --> ### Description of PR <!-- - Please include a summary of the change and which issue is fixed. - Please also include relevant motivation and context. Where should reviewer start? background context? - List any dependencies that are required for this change. --> Summary: Fixes # (issue) - This PR fixes _Announce routes_ task failure during add-topo for LT2 DUTs which has more T1 VMs. - For _lt2-o256-u32d224_ topo, DUT needs to have 224 T1 VMs, whereas the current code has support only for 110 T1 VMs. ### Type of change <!-- - Fill x for your type of change. - e.g. - [x] Bug fix --> - [ ] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [x] Test case improvement ### Back port request - [ ] 202205 - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 - [x] 202511 ### Approach #### What is the motivation for this PR? - To fix announce routes failure for _lt2-o256-u32d224_ topo DUTs, which needs more T1 networks. #### How did you do it? - 224 T1 VMs need at least 7 networks, modified the code to support that. #### How did you verify/test it? - Ran add-topo for _lt2-o256-u32d224_ and made sure it's working fine without any issues. #### Any platform specific information? LT2 #### Supported testbed topology if it's a new test case? ### Documentation <!-- (If it's a new feature, new test case) Did you update documentation/Wiki relevant to your implementation? Link to the wiki page? --> Signed-off-by: sanrajen <sanjai.rajendran@nokia.com> Signed-off-by: selldinesh <dinesh.sellappan@keysight.com>
|
@anders-nexthop Can you raise a PR to 202512 to address conflict? |
|
@bingwang-ms Raised the 202512 backport here. The auto cherry-pick conflicted in — anders-bot (AI-assisted, on behalf of @anders-nexthop) |
…#1291) ### Description of PR Cherry-pick of sonic-net/sonic-mgmt#24665 to 202512, per @bingwang-ms's note that sonic-mgmt 202512 backports land in this repo. Original author: @sanjair-git. - Fixes the *Announce routes* task failure during add-topo for LT2 DUTs with more T1 VMs. - *lt2-o256-u32d224* has 224 T1 VMs; the current code supports only 110. ### Type of change - [ ] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [x] Test case improvement ### Approach Cherry-picked sonic-net/sonic-mgmt#24665. It applied cleanly on 202512 — the existing 8-network hardcode (192.168-192.175.0.0/27) matches the source PR's base — so the result is the dynamic scheme it introduced: `num_extra = max(4, ceil(len(t1_vms) / 32))` building `192.(168+i).0.0/27`, matching current master. Backward-compatible: any topo with <=128 T1 VMs yields the same blocks as before; it expands (7 blocks for the 224-T1 topo) only where more networks are needed. Verified the resulting `fib_lt2_routes` matches master's post-#24665 shape and that the module compiles; relying on 202512 CI for the functional run.
Backport merged, thanks @bingwang-ms |
4 participants
Description of PR
Summary:
Fixes # (issue)
Type of change
Back port request
Approach
What is the motivation for this PR?
How did you do it?
How did you verify/test it?
Any platform specific information?
LT2
Supported testbed topology if it's a new test case?
Documentation