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

rcmgr: warn if limits conflict with connmgr #1707

Closed
MarcoPolo opened this issue Jul 4, 2022 · 3 comments · Fixed by #2527
Closed

rcmgr: warn if limits conflict with connmgr #1707

MarcoPolo opened this issue Jul 4, 2022 · 3 comments · Fixed by #2527
Labels
exp/intermediate Prior experience is likely helpful good first issue Good issue for new contributors help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up

Comments

@MarcoPolo
Copy link
Collaborator

MarcoPolo commented Jul 4, 2022

We should emit some warning if the current limits conflict with the current settings for the connection manager.

Roughly:

  • Look at the limits set by the resource manager if the resource manager is the default implementation provided by go-libp2p (the rcmgr interface doesn't allow us to see what the limits are)
  • Look at the limits configured in the conn manager.
  • The rcmgr conn limit should be higher than the high watermark of the conn manager. Log a warning if this isn't the case.

I think this makes sense to put in the connmgr code.

@MarcoPolo MarcoPolo mentioned this issue Jul 4, 2022
39 tasks
@MarcoPolo MarcoPolo added good first issue Good issue for new contributors help wanted Seeking public contribution on this issue labels Jul 7, 2022
@marten-seemann marten-seemann mentioned this issue Aug 19, 2022
21 tasks
@marten-seemann marten-seemann transferred this issue from libp2p/go-libp2p-resource-manager Aug 19, 2022
@marten-seemann marten-seemann changed the title Warn if limits conflict with connmgr rcmgr: warn if limits conflict with connmgr Aug 19, 2022
@marten-seemann marten-seemann mentioned this issue Sep 20, 2022
34 tasks
@p-shahi p-shahi added this to the Best Effort Track milestone Nov 7, 2022
@p-shahi p-shahi added P1 High: Likely tackled by core team if no one steps up P2 Medium: Good to have, but can wait until someone steps up and removed P1 High: Likely tackled by core team if no one steps up labels Nov 7, 2022
@MarcoPolo MarcoPolo added the exp/intermediate Prior experience is likely helpful label Jun 19, 2023
@piersy
Copy link
Contributor

piersy commented Aug 25, 2023

Hi @MarcoPolo, id be interested in picking this up, but I would need a bit of clarification from you about how you think this should work.

It looks to me like a good place to insert the check would be just after this block, since everything should have been configured by that point:

go-libp2p/config/config.go

Lines 297 to 302 in 4caa4e5

func (cfg *Config) NewNode() (host.Host, error) {
eventBus := eventbus.NewBus(eventbus.WithMetricsTracer(eventbus.NewMetricsTracer(eventbus.WithRegisterer(cfg.PrometheusRegisterer))))
swrm, err := cfg.makeSwarm(eventBus, !cfg.DisableMetrics)
if err != nil {
return nil, err
}

However the resource manager in rcmgr is not exported, were you planning on exporting it in order to get at the conn limit? The alternative I see would be to add a method to network.ResourceManager to expose the conn limit. Similarly were you planning on adding a method to connmgr.ConnManager to check the limit extracted from the resource manger?

Thanks!

@MarcoPolo
Copy link
Collaborator Author

Thanks @piersy!

Here's what I'm thinking:

  1. We can do this at the start of the NewNode function since everything we need is in the Config.
  2. The ConnManager can have a new method called CheckLimits(limits GetterConnLimits).
  3. GetConnLimits is an interface with a single method GetConnLimits.
  4. The private resourceManager type implements this interface and returns the total system limits.

Conn manager defines a new interface for the thing it checks its limits against. And the resourceManager implements this interface. The glue code in NewNode checks to see if the configured resource manager implements the interface and calls the CheckLimits method on the conn manager.

There's probably some subtleties here, but hopefully this is enough to get you started :)

@piersy
Copy link
Contributor

piersy commented Aug 26, 2023

Thanks @MarcoPolo, that sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/intermediate Prior experience is likely helpful good first issue Good issue for new contributors help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up
Projects
Status: 🎉 Done
Development

Successfully merging a pull request may close this issue.

3 participants