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

fix: curio: gui listen conflict #11965

Conversation

strahe
Copy link
Contributor

@strahe strahe commented May 6, 2024

Related Issues

#11941
Previous PR: #11944

Proposed Changes

  1. Change the default value of Subsystems.GuiAddress from :4701 to 127.0.0.1:4701.
  2. Rename the --listen parameter of curio web to --gui-listen, making it common and clear in both curio web and curio run.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@snadrus
Copy link
Contributor

snadrus commented May 6, 2024

The most common use is (we presume) to stand up a server that is likely to be used for long-term monitoring.
You can set config.Subsystems.GuiAddress by editing this on base, then curio run --layers=gui. This edit is even possible on the GUI.

@strahe
Copy link
Contributor Author

strahe commented May 7, 2024

So perhaps we can merge curio web and curio run? But in any case, it's clear that the --listen parameter of curio web is occupied by curio rpc. Do you have any suggestions to improve this PR?

lotus/cmd/curio/run.go

Lines 182 to 187 in a110986

s := `[Susbystems]
EnableWebGui = true
`
if err = setConfig(db, "web", s); err != nil {
return err
}

@strahe
Copy link
Contributor Author

strahe commented May 7, 2024

I am more inclined to write all configurations into the database, except for the listening address. This is because the listening address is a one-time parameter when running the service, and it cannot be modified while the service is running. Therefore, writing it into the database or modifying it in the database will not take effect immediately. A better approach would be to specify it as a parameter when running the service, as is currently done.

@strahe strahe changed the base branch from release/curio-beta to master May 7, 2024 07:32
@strahe strahe changed the base branch from master to release/curio-beta May 7, 2024 07:33
@strahe strahe mentioned this pull request May 7, 2024
8 tasks
@LexLuthr
Copy link
Contributor

LexLuthr commented May 7, 2024

I was thinking about this from user perspective and @strahe is correct. This is confusing at best. Our current system allows for a weird setup where depending upon the layer selected, we might not get a full picture of the cluster in GUI. Perhaps, we should rethink this command a little. I would suggest the following:

  1. Remove GUI from the run command. (ignore even if the layer is provided)
  2. web subcommand should ignore all the other subsystems and then run the GUI server(not use run action). We can add a listen-gui flag here and allow users to override the port supplied from the DB. This allows users to not care about the port number resulting from the different stacking possibilities.
  3. Since, UI doesn't really require any resources, running more than one Curio on the same node should not present a challenge. The only exception is the market-connect module in Curio. We can move that inside the startTask to allow user flexibility about starting market module only wherever they desire allowing dedicated piecePark nodes for large clusters.

This approach is less sophisticated but does allow for a more user friendly op in my opinion. WDYT?

@snadrus
Copy link
Contributor

snadrus commented May 17, 2024

Thanks. I've adapted it into #12013

@strahe strahe closed this May 20, 2024
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

Successfully merging this pull request may close these issues.

None yet

3 participants