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: cleanup ospf6 ecmp inter area #16811

Closed

Conversation

louis-6wind
Copy link
Contributor

@louis-6wind louis-6wind commented Sep 12, 2024

see commit logs. Changes to help the topotest maintenance

expect_num_nexthops() errors are not understandable.

Use router_json_cmp.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
eth3 shutdown is not applied because it is ospf6d.conf.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
@frrbot frrbot bot added the tests Topotests, make check, etc label Sep 12, 2024
Copy link
Collaborator

@aceelindem aceelindem left a comment

Choose a reason for hiding this comment

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

I definitely like this approach better than the existing next-hop counting. If the test continues to fail intermittently, we'll have a much better idea of what is failing.

# tgen.mininet_cli()
# Check nexthops post link-down
expect_num_nexthops("r1", [1, 1, 1, 1, 1, 2, 2, 2, 2], 8)
json_file = "{}/{}/show_ipv6_routes_ospf6-2.json".format(CWD, router.name)
Copy link
Collaborator

@aceelindem aceelindem Sep 12, 2024

Choose a reason for hiding this comment

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

Also suggest putting in some "step()" invocations describing the testcase variation.

@louis-6wind
Copy link
Contributor Author

I definitely like this approach better than the existing next-hop counting. If the test continues to fail intermittently, we'll have a much better idea of what is failing.

The test is still failing. Do you have any idea of what is going wrong ?

@riw777
Copy link
Member

riw777 commented Sep 17, 2024

it looks like the test is related ... maybe we need to modify the test based on this fix?

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

looks good

@gromit1811
Copy link
Contributor

I definitely like this approach better than the existing next-hop counting. If the test continues to fail intermittently, we'll have a much better idea of what is failing.

The test is still failing. Do you have any idea of what is going wrong ?

Well, it's not the test that is broken (my original version's output just isn't as helpful as it could be for analyzing failures), but ospf6d. See #16197 for all the gory details. You probably want to skip the first comments and start at #16197 (comment)
So for that specific case, the additional output created by this PR wouldn't have helped either and I'm already a few steps further: I know exactly what's wrong, but I don't have the slightest idea about how to fix it.

@@ -13,9 +13,6 @@ interface r7-eth2
ipv6 ospf6 hello-interval 2
ipv6 ospf6 dead-interval 10
!
interface r7-eth3
shutdown
!
Copy link
Contributor

Choose a reason for hiding this comment

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

Not having the shutdown in ospf6d.conf makes sense, but instead of removing it, it probably should get moved to zebra.conf. Same for r8.

)
_, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
assertmsg = '"{}" JSON output mismatches'.format(router.name)
assert result is None, assertmsg
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the JSON comparison instead of my original nexthop counting, but may I suggest to move these comparisons to a separate function instead of having essentially 3 copies of the same code?

@riw777
Copy link
Member

riw777 commented Dec 17, 2024

is someone still working on this?

@gromit1811
Copy link
Contributor

is someone still working on this?

Doesn't look like it. I can take over if @louis-6wind doesn't complete this PR. After all, I'm to blame for the original test case 😉 But I fear I won't be able to git push to it?

@louis-6wind
Copy link
Contributor Author

is someone still working on this?

Doesn't look like it. I can take over if @louis-6wind doesn't complete this PR. After all, I'm to blame for the original test case 😉 But I fear I won't be able to git push to it?

@gromit1811 I have no time to complete the PR for the moment. Feel free to open a new PR

@gromit1811
Copy link
Contributor

gromit1811 commented Dec 22, 2024

Replaced/superseded by #17707

@ton31337 ton31337 closed this Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master size/L tests Topotests, make check, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants