search

stellar / quickstart

Home of the stellar/quickstart docker image for development and testing
Apache License 2.0
186 stars 206 forks source link

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

Closed sreuland closed 2 months ago

sreuland commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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: https://github.com/stellar/quickstart/blob/611afc9b360f4549439bc6f3e78d59a5327edd71/start#L204-L212

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

That should be a small tweak.

Wdyt?

sreuland commented 2 months ago

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 - https://github.com/stellar/quickstart/pull/615/commits/5381df98b1d941bc7bd9f623f07148da0b072d5b