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

cicd/tcpsctpperf: Hosts ep2,3 are started but not used #449

Closed
luisgerhorst opened this issue Dec 4, 2023 · 7 comments
Closed

cicd/tcpsctpperf: Hosts ep2,3 are started but not used #449

luisgerhorst opened this issue Dec 4, 2023 · 7 comments

Comments

@luisgerhorst
Copy link
Contributor

config_docker_host --host1 l3ep2 --host2 llb1 --ptype phy --addr 32.32.32.1/24 --gw 32.32.32.254

I believe these are not actually used. Should they be removed or does it make sense to adapt the benchmark to actually use them?

@TrekkieCoder
Copy link
Collaborator

Your observation is correct. These are not needed and it would be best to remove them as these might consume resources which can in some.way adversely impact the perf test itself.

@TrekkieCoder
Copy link
Collaborator

Closing this as per this commit

@luisgerhorst
Copy link
Contributor Author

I'm sorry I think there was some confusion. The MR was not ready for merging and thus did not fix this issue, it was only a request for comments regarding the benchmarking approach. Please reopen, I will close it myself when appropriate. (I'm sorry if this was not clear, I will make it more explicit next time.)

@TrekkieCoder
Copy link
Collaborator

TrekkieCoder commented Dec 11, 2023

Ok got it. Noticed a few breaks in loxilb's cicd. I will temporarily fix this. Please double check and commit once all your changes are ready.

@TrekkieCoder TrekkieCoder reopened this Dec 11, 2023
@luisgerhorst
Copy link
Contributor Author

Yes, this is expected. I will create a PR with a hotfix, you either apply that or revert the whole series.

@luisgerhorst
Copy link
Contributor Author

With #460 this should be fixed. But to repeat: #460 was not tested. I appreciate the fast merge but I intended to look at the CI output first :D - We will see I guess.

@TrekkieCoder
Copy link
Collaborator

The hotfix worked. Will be looking forward for your full set of perf script changes. Looks great !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants