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

Resource Manager: ipfs swarm limit <scope> --reset is setting to zero all other scopes. #9559

Closed
3 tasks done
Tracked by #9442
ajnavarro opened this issue Jan 17, 2023 · 5 comments
Closed
3 tasks done
Tracked by #9442
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization topic/resource-manager Issues related to Swarm.ResourceMgr (resource manager)

Comments

@ajnavarro
Copy link
Member

ajnavarro commented Jan 17, 2023

Checklist

Installation method

ipfs-desktop

Version

master

Config

-

Description

When executing ipfs swarm limit <scope> --reset the expected output configuration should be:

"ResourceMgr": {
      "Limits": {
        "System": {
          "Conns": 1000000000,
          "ConnsInbound": 564,
          "ConnsOutbound": 1000000000,
          "FD": 524288,
          "Memory": 8400000000,
          "Streams": 1000000000,
          "StreamsInbound": 9034,
          "StreamsOutbound": 1000000000
        },
      }

Bit it is:

"ResourceMgr": {
      "Limits": {
        "AllowlistedSystem": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        },
        "AllowlistedTransient": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        },
        "Conn": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        },
        "PeerDefault": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        },
        "ProtocolDefault": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        },
        "ProtocolPeerDefault": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        },
        "ServiceDefault": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        },
        "ServicePeerDefault": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        },
        "Stream": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        },
        "System": {
          "Conns": 1000000000,
          "ConnsInbound": 564,
          "ConnsOutbound": 1000000000,
          "FD": 524288,
          "Memory": 8400000000,
          "Streams": 1000000000,
          "StreamsInbound": 9034,
          "StreamsOutbound": 1000000000
        },
        "Transient": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        }
      }

To fix this we have to use a new Limit struct that ignores zero values when encoding to JSON, this will avoid all unwanted zero values on our config file.

This can be done at libp2p level, adding the json:"omitempty" tag, or duplicating the struct on kubo side wrapping the actual one but adding these JSON tags.

@ajnavarro ajnavarro added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Jan 17, 2023
@ajnavarro
Copy link
Member Author

Tentative fix at go-libp2p side: libp2p/go-libp2p#1998

@MarcoPolo
Copy link
Contributor

Does this fix anything besides making the output less verbose? The missing values still get parsed as 0.

@ajnavarro
Copy link
Member Author

ajnavarro commented Jan 18, 2023

@MarcoPolo it is making it less verbose and less confusing for the final user. Now that we know zero values mean default values if the limit is obtained from the config, but real zero if you see that from the final parsed value sent to CM, that change makes even more sense.

Example:
After executing ipfs swarm limit system --reset the user sees from the configuration that Transient has the following values:

"Transient": {
          "Conns": 0,
          "ConnsInbound": 0,
          "ConnsOutbound": 0,
          "FD": 0,
          "Memory": 0,
          "Streams": 0,
          "StreamsInbound": 0,
          "StreamsOutbound": 0
        }

But when executing ipfs swarm limit transient they can see the following:

{
  "Conns": 1000000000,
  "ConnsInbound": 157,
  "ConnsOutbound": 1000000000,
  "FD": 131072,
  "Memory": 1083441152,
  "Streams": 1000000000,
  "StreamsInbound": 1129,
  "StreamsOutbound": 1000000000
}

Also if for any reason after executing ipfs swarm limit transient we have an output like:

{
  "Conns": 0,
  "ConnsInbound": 0,
  "ConnsOutbound": 0,
  "FD": 0,
  "Memory": 0,
  "Streams": 0,
  "StreamsInbound": 0,
  "StreamsOutbound": 0
}

That means a totally different thing than the values we can see from the configuration because in that case, they are real zeroes, not undefined.

@guseggert
Copy link
Contributor

Also consider that a use case that went into the design of those commands was to be able to copy-and-paste between the commands.

@BigLep
Copy link
Contributor

BigLep commented Feb 16, 2023

We're going to remove this command as part of #9621 . As a result closing as not planned.

@BigLep BigLep closed this as not planned Won't fix, can't repro, duplicate, stale Feb 16, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization topic/resource-manager Issues related to Swarm.ResourceMgr (resource manager)
Projects
No open projects
Archived in project
Development

No branches or pull requests

5 participants