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: add support for ospf instances with unified configs #17331

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

Jafaral
Copy link
Member

@Jafaral Jafaral commented Nov 1, 2024

No description provided.

@frrbot frrbot bot added the tests Topotests, make check, etc label Nov 1, 2024
Copy link

github-actions bot commented Nov 1, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added size/L and removed size/M labels Nov 1, 2024
@github-actions github-actions bot removed the conflicts label Nov 1, 2024
@Jafaral Jafaral force-pushed the ospf-instance branch 2 times, most recently from 5b71132 to 78a716b Compare November 1, 2024 16:35
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.

hmm - no description, and nothing in the commit comment, about what this is trying to do.
so ... was it not working to have the test script say "I want to run an ospf with this command-line arg, pointing at the unified config file?"
if we think we're going to drive daemon start and daemon command-lines from the config, then I think we need an explicit "daemon x" command for that purpose: there are too many things that may need to be expressed on the daemon command-line to try to do that implicitly.

@aceelindem
Copy link
Collaborator

I tried the tests/topotests/lib changes with 2 instances and this PR worked fine for me.

@Jafaral
Copy link
Member Author

Jafaral commented Nov 1, 2024

@mjstapp , sorry, this was a"quick" pr following a discussion on slack after @aceelindem raised the issue the he coudn't get ospf instances to work. This PR doesn't do anyting more/less than what is stated in the title.

1- This PR does NOT add support for unified config in the tests, that capability exists already
2- This PR does NOT remove the ability to configure or start individual daemons/instances
3- This PR doesn't mean multi instance isn't supported when using separate configs files.
4- This PR was added to configure ospf instances in topotests as an alternative method to using separate configs files, after some tests showed that the current support possibly does not work with more than one instance.

I'm mostly interested in using unified frr.conf, so:
Current behavior : If you choose to use frr.conf, and also configure ospf instances, it doesn't work
What this PR adds: support for ospf instances when using frr.conf unified configs

@Jafaral
Copy link
Member Author

Jafaral commented Nov 1, 2024

Also, I do agree that it is good to come up with a way to be able to pass commands arguments to a daemon when using unified configs. I like to solve that problem, but it is out of scope for this PR.

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

@riw777
Copy link
Member

riw777 commented Nov 12, 2024

need to look at new core ...

@riw777
Copy link
Member

riw777 commented Nov 12, 2024

rerunning failed test

@riw777 riw777 merged commit 5456bc5 into FRRouting:master Nov 12, 2024
11 checks passed
@ton31337
Copy link
Member

ton31337 commented Dec 4, 2024

@Mergifyio backport stable/10.2

Copy link

mergify bot commented Dec 4, 2024

backport stable/10.2

✅ Backports have been created

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

Successfully merging this pull request may close these issues.

5 participants