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

'--enable core --local' only runs core, no ancillary services #615

Merged
merged 5 commits into from
Jul 11, 2024

Conversation

sreuland
Copy link
Contributor

@sreuland sreuland commented Jul 5, 2024

What Changed

when using --local --enable core,, do not start the horizon/friendbot service in container, only core service will be running.

this removes prior default behavior, which started horizon and friendbot services automatically when --local was used.

Why

we are using quickstart for new end-to-end integration tests now, and noticed with --enable core,, if --local was used, quickstart would still start horizon/friendbot in container, here is reference of how the test uses these flags:

https://github.com/stellar/go/pull/5370/files#diff-1c787ef8e522ba695cb843ceef6c93e842a3cfc319085d6b78349a1c4ec201abR279

the testing environment only needs the local core validator/history archive server available which quickstart automates the provisioning of these services very conveniently(and reliably for latest versions) and allows the tests to avoid significant boilerplate code otherwise to spin up it's own local core/archive, however, with quickstart spinning up other services in the container, it's giving up some of the gains made in the test use case, since it is consuming more integration test host compute.

@sreuland sreuland changed the title '--enable core' only runs core, no ancillary services '--enable core --local' only runs core, no ancillary services Jul 5, 2024
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing behaviour is an intentional special case to run everything in local mode.

A local instance isn't very usable by a developer (think app/contract developer) without Friendbot. Friendbot needs Horizon so local mode also runs it. At that point the only thing not running is the rpc so we just run everything in local mode because that's an easier statement to make on behaviour.

@sreuland
Copy link
Contributor Author

sreuland commented Jul 8, 2024

The existing behaviour is an intentional special case to run everything in local mode.

A local instance isn't very usable by a developer (think app/contract developer) without Friendbot. Friendbot needs Horizon so local mode also runs it. At that point the only thing not running is the rpc so we just run everything in local mode because that's an easier statement to make on behaviour.

ok, yes, that default behavior makes sense given that context, this was driven by observation during new usage of quickstart for end-to-end integration test:

[edit] - I've updated the description on PR to explain the new use case further.

How about if I retain default behavior by changing this to just check for ENABLE="core,," and skip horizon in this case? As that setting is explicitly saying 'I only want core running' correct?

@leighmcculloch
Copy link
Member

leighmcculloch commented Jul 10, 2024

Thanks for the context, that helps to understand the use case we're enabling. And the resource usage while not outrageous is significant, the memory usage is about half (180mb 👉🏻 70mb) without Horizon, and CPU goes way down (18% 👉🏻 2%). 💯

This will be a breaking change. I don't think we can do this in a way that it wouldn't be a breaking change. But I think that's okay if we minimize the breakage.


How about if I retain default behavior by changing this to just check for ENABLE="core,,"

That feels a bit awkward. The input value is unintuitive as to the surprising behavior it'd result in.

I think we can compromise and move ahead with your original plan, we just need to minimise the breakage and I have a suggestion for an additional change to make below.


We don't have metrics to know how people are executing the quickstart image but in tutorials and commands I see folks having used we should preserve default behavior when no --enable option is passed, and make sure that when folks ask for the rpc with --enable rpc they still get a working friendbot. If we can make that happen then I think this is good to merge.

The default behavior is already that all run, so we don't need to do anything to change that.

A change needs to be made so that when running in local mode and rpc is selected then horizon also runs so that friendbot is also running and working. That change can be made at the lines below by making it so when local then also enable horizon when rpc is requested:

quickstart/start

Lines 204 to 212 in 611afc9

if [[ ",$ENABLE," = *",core,"* ]]; then
ENABLE_CORE=true
fi
if [[ ",$ENABLE," = *",horizon,"* ]]; then
ENABLE_HORIZON=true
fi
if [[ ",$ENABLE," = *",rpc,"* ]]; then
ENABLE_SOROBAN_RPC=true
fi

The result of your existing change here plus the change above should be:

  • --local - runs core, horizon, friendbot, rpc
  • --local --enable core - runs only core
  • --local --enable core,horizon - runs core, horizon, friendbot
  • --local --enable horizon - runs core, horizon, friendbot
  • --local --enable rpc - runs core, horizon, friendbot, rpc

That should be a small tweak.

Wdyt?

@sreuland
Copy link
Contributor Author

The result of your existing change here plus the change above should be:

--local - runs core, horizon, friendbot, rpc
--local --enable core - runs only core
--local --enable core,horizon - runs core, horizon, friendbot
--local --enable horizon - runs core, horizon, friendbot
--local --enable rpc - runs core, horizon, friendbot, rpc
That should be a small tweak.

Wdyt?

sounds good, applied that flow and updated the README to document this expectation - 5381df9

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@leighmcculloch leighmcculloch enabled auto-merge (squash) July 11, 2024 00:00
@leighmcculloch leighmcculloch merged commit 807b2b3 into stellar:master Jul 11, 2024
149 checks passed
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.

2 participants