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

tests, zebra: rename nexthop-group depends dependends #16251

Closed
wants to merge 6 commits into from

Conversation

louis-6wind
Copy link
Contributor

The hierarchy between next-hop groups in the "show nexthop-group rib"
output can be challenging to understand. The terms "depends" and
"dependents" refer to child and parent next-hop groups, respectively.

r1# show nexthop-group rib 181818168
ID: 181818168 (sharp)
RefCnt: 1
Uptime: 00:06:42
VRF: default
Depends: (96) (97)
via 1.1.1.1 (vrf default) inactive, weight 1
via 1.1.1.2 (vrf default) inactive, weight 1
r1# show nexthop-group rib 96
ID: 96 (sharp)
RefCnt: 2
Uptime: 00:06:59
VRF: default
via 1.1.1.1 (vrf default) inactive, weight 1
Dependents: (181818168)

Rename "Depends" and "Dependents" to child and parent.

r1# show nexthop-group rib 181818168
ID: 181818168 (sharp)
RefCnt: 1
Uptime: 00:06:42
VRF: default
Child groups: (96) (97)
via 1.1.1.1 (vrf default) inactive, weight 1
via 1.1.1.2 (vrf default) inactive, weight 1
r1# show nexthop-group rib 96
ID: 96 (sharp)
RefCnt: 2
Uptime: 00:06:59
VRF: default
via 1.1.1.1 (vrf default) inactive, weight 1
Parent groups: (181818168)

Also rename the code

The hierarchy between next-hop groups in the "show nexthop-group rib"
output can be challenging to understand. The terms "depends" and
"dependents" refer to child and parent next-hop groups, respectively.

> r1# show nexthop-group  rib 181818168
> ID: 181818168 (sharp)
>      RefCnt: 1
>      Uptime: 00:06:42
>      VRF: default
>      Depends: (96) (97)
>            via 1.1.1.1 (vrf default) inactive, weight 1
>            via 1.1.1.2 (vrf default) inactive, weight 1
> r1# show nexthop-group  rib 96
> ID: 96 (sharp)
>      RefCnt: 2
>      Uptime: 00:06:59
>      VRF: default
>            via 1.1.1.1 (vrf default) inactive, weight 1
>      Dependents: (181818168)

Rename "Depends" and "Dependents" to child and parent.

> r1# show nexthop-group  rib 181818168
> ID: 181818168 (sharp)
>      RefCnt: 1
>      Uptime: 00:06:42
>      VRF: default
>      Child groups: (96) (97)
>            via 1.1.1.1 (vrf default) inactive, weight 1
>            via 1.1.1.2 (vrf default) inactive, weight 1
> r1# show nexthop-group  rib 96
> ID: 96 (sharp)
>      RefCnt: 2
>      Uptime: 00:06:59
>      VRF: default
>            via 1.1.1.1 (vrf default) inactive, weight 1
>      Parent groups: (181818168)

Signed-off-by: Louis Scalbert <[email protected]>
Clarify naming using:

> git grep nhg_dependents | cut -f1 -d: |xargs -L1 sed -e 's|nhg_dependents|nhg_parent|g' -i

Signed-off-by: Louis Scalbert <[email protected]>
Clarify naming using:

> it grep nhg_depends | cut -f1 -d: |xargs -L1 sed -e 's|nhg_depends|nhg_child|g' -i

Signed-off-by: Louis Scalbert <[email protected]>
> sed -e 's|dependent|parent|g' -i zebra/zebra_nhg.h
> sed -e 's|dependent|parent|g' -i zebra/zebra_nhg.c

Signed-off-by: Louis Scalbert <[email protected]>
Apply:

> sed -e 's|depends_|child_|g;s|\<depend\>|child|g' -i zebra/zebra_nhg.c

And rename depends to child or children in zebra_nhg

Signed-off-by: Louis Scalbert <[email protected]>
> sed -e 's|depends|chidren|g;s|depend|child|g' -i tests/topotests/all_protocol_startup/test_all_protocol_startup.py

Signed-off-by: Louis Scalbert <[email protected]>
Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

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

umm - instead of hundreds of lines of change, maybe just offer a revision to the cli output strings?

@louis-6wind
Copy link
Contributor Author

umm - instead of hundreds of lines of change, maybe just offer a revision to the cli output strings?

IMO, the code is also confusing

@louis-6wind
Copy link
Contributor Author

ci:rerun

@mjstapp
Copy link
Contributor

mjstapp commented Jun 21, 2024

No, this just doesn't make sense to me. it's too bad that you find the code confusing - it is complicated. but renaming some things isn't going to make it any easier to understand. if we diverge from the existing released branches, we'll have ongoing trouble backporting.
and frankly - "parent" and "child" aren't correct terms. you've decided that the "parent" is the thing that is composed of several "children" - but in fact the "children" form the "parent" - the parent only exists as a container for the "child" singleton nhgs, and consists of nothing other than a list of "children". the singleton "children" come first - then the parent is formed from them. that just ... doesn't help.

@Jafaral Jafaral closed this Jul 9, 2024
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.

3 participants