ingest/ledgerbackend: Parse captive core toml to implement better merging #3558

tamirms · 2021-04-21T12:58:55Z

PR Checklist

PR Structure

This PR has reasonably narrow scope (if not, break it down into smaller PRs).
This PR avoids mixing refactoring changes with feature changes (split into two PRs
otherwise).
This PR's title starts with name of package that is most changed in the PR, ex.
services/friendbot, or all or doc if the changes are broad or impact many
packages.

Thoroughness

This PR adds tests for the most critical parts of the new functionality or fixes.
I've updated any docs (developer docs, .md
files, etc... affected by this change). Take a look in the docs folder for a given service,
like this one.

Release planning

I've updated the relevant CHANGELOG (here for Horizon) if
needed with deprecations, added features, breaking changes, and DB schema changes.
I've decided if this PR requires a new major/minor version according to
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.

What

This PR fixes #3486 and lays the groundwork for implementing #3442 .

This PR changes how we interpret captive core append file (configured via --captive-core-config-append-path). Previously, we automatically generated the stellar core toml file for the captive core subprocess in ingest/ledgerbackend/stellar_core_runner.go by presetting some defaults and then concatenating the generated configuration with the user supplied append file.

This strategy did not work out so well because sometimes horizon operators included configuration in the append file which was already configured in the presets generated by ingest/ledgerbackend/stellar_core_runner.go. An example of this type of error can be found in #3486 .

Now, in this PR, instead of generating the stellar core toml file via simple string concatenation we actually parse the user supplied append file and try to do more robust merging between the user supplied configuration and the captive core preset defaults. The merging code is implemented in ingest/ledgerbackend/toml.go .

The merging strategy is unfortunately complicated by the fact that captive core configuration can come from 3 places:

User supplied append file
horizon command line flag (e.g. --captive-core-peer-port, --captive-core-http-port, etc)
default values which we preset (e.g. DISABLE_XDR_FSYNC)

In theory the user supplied append file should only contain the quorum set but in practice we have never validated the append file to ensure that it configures a quorum set and nothing else. We could simplify the merging logic in this PR by choosing to validate the append file in this way. I decided not to take that approach because I think that it would be a breaking change for many horizon operators (including SDF) who have configuration in the append file which is not related to quorum sets. If you disagree with this design decision please let me know.

The merging algorithm generally works as follows:

If there is a configuration field which is set both in source (1) and source (2) we check that both sources provide equivalent values, otherwise we return an error.

The exception to this rule is the history archive configuration. We take the history archive configuration from source (1) and completely disregard what is provided in source (2) unless there is no history archive configuration in source (1). The reason for this is that it's difficult to compare the history archive values from source (1) and source (2).

If a configuration field is defined in source (1) but not source (2), we will use the field which is explicitly defined. The same logic applies if the field is defined in source (2) but not source (1).

Finally, if there is no configuration defined in either source (1) or source (2), we use the value from source (3).

In the next PR, my plan is to fully implement #3442 by introducing a new horizon command line flag --captive-core-config-file which deprecates --captive-core-config-append-path. The difference between --captive-core-config-file and --captive-core-config-append-path is that the validation is stricter.

--captive-core-config-file will not allow any unknown fields to be present in the toml file.

Furthermore, if --captive-core-config-file is provided then you will not be allowed to use any horizon command line flags to configure captive core. In other words, --captive-core-config-file deprecates source (2).

Along with defining --captive-core-config-file, the next PR will also add documentation for the captive core configuration file in an captive-core_example.cfg file (similar to how stellar core does it https://github.com/stellar/stellar-core/blob/master/docs/stellar-core_example.cfg ). The example file will document the full range of configuration options available to captive core instances.

Unfortunately there will be some slight differences between the stellar core example cfg and the captive core example cfg because captive core has different defaults compared to stellar core. For example, captive core sets DISABLE_XDR_FSYNC to true by default whereas stellar core sets DISABLE_XDR_FSYNC to false by default. We should think carefully about each of the cases where captive core diverges from stellar core.

Known limitations

The golang toml library doesn't handle nested tables in a way which is consistent with stellar core. As a workaround I implemented some hacky code to flatten nested tables using string manipulation.

Shaptic

I know this is in-progress, but just a few thoughts

ingest/ledgerbackend/stellar_core_runner.go

ingest/ledgerbackend/stellar_core_runner_test.go

tamirms · 2021-04-24T12:32:24Z

ingest/doc_test.go

@@ -71,7 +71,6 @@ func Example_changes() {
 	backend, err := ledgerbackend.NewCaptive(
 		ledgerbackend.CaptiveCoreConfig{
 			BinaryPath:         "/bin/stellar-core",
-			ConfigAppendPath:   "/opt/stellar-core.cfg",


need to update this example

bartekn

It's looking good! I don't have any major comments except NewCaptiveCoreTomlFromFile.

ingest/ledgerbackend/stellar_core_runner_test.go

ingest/ledgerbackend/toml.go

jacekn · 2021-04-26T15:00:25Z

@tamirms this is a great improvement! It feels fine for the most part but the history archive handling could be a bit confusing IMO.

The exception to this rule is the history archive configuration. We take the history archive configuration from source (1) and completely disregard what is provided in source (2) unless there is no history archive configuration in source (1). The reason for this is that it's difficult to compare the history archive values from source (1) and source (2).

It's not a very strong opinion but I think it would be best to error if it's set in both places, even if there is a chance both are the same.
Downside of throwing an error is that we might do that if values are equivalent. But on the plus side we remove ambiguity.
I feel that erroring with a clear message is less risky than possibility or using archive other than what the operator intended.

tamirms · 2021-05-02T15:24:36Z

@stellar/horizon-committers this ready for a final review now, PTAL!

bartekn · 2021-05-05T12:08:25Z

exp/services/captivecore/main.go

@@ -14,16 +14,17 @@ import (
 	"github.com/stellar/go/ingest/ledgerbackend"
 	"github.com/stellar/go/network"
 	"github.com/stellar/go/support/config"
+	support "github.com/stellar/go/support/config"


Isn't it imported line above?

Suggested change

support "github.com/stellar/go/support/config"

paulbellamy · 2021-05-05T12:29:52Z

exp/services/captivecore/main.go

+			OptType:        types.Uint,
+			CustomSetValue: support.SetOptionalUint,
+			Required:       false,
+			FlagDefault:    uint(0),


Should this be 11626?

paulbellamy · 2021-05-05T12:33:57Z

ingest/ledgerbackend/stellar_core_runner.go


-	return result, nil
+	text, err := captiveCoreToml.Marshall()


Typo?

Suggested change

text, err := captiveCoreToml.Marshall()

text, err := captiveCoreToml.Marshal()

paulbellamy · 2021-05-05T12:41:14Z

ingest/ledgerbackend/toml.go

+
+	return re.ReplaceAllStringFunc(text, func(match string) string {
+		insideBrackets := match[1 : len(match)-1]
+		original := tablePlaceHolders.get(insideBrackets)


Super nit:

Suggested change

original := tablePlaceHolders.get(insideBrackets)

original, ok := tablePlaceHolders.get(insideBrackets)

paulbellamy · 2021-05-05T12:41:42Z

ingest/ledgerbackend/toml.go

+}
+
+// Marshall serializes the CaptiveCoreToml into a toml document.
+func (c *CaptiveCoreToml) Marshall() ([]byte, error) {


Typo?

Suggested change

func (c *CaptiveCoreToml) Marshall() ([]byte, error) {

func (c *CaptiveCoreToml) Marshal() ([]byte, error) {

paulbellamy · 2021-05-05T12:42:42Z

ingest/ledgerbackend/toml.go

+	historyEntries := map[string]History{}
+	// The toml library has trouble with nested tables so we need to flatten all nested
+	// QUORUM_SET and HISTORY tables as a workaround.
+	// In Marshall() we apply the inverse process to unflatten the nested tables.


Suggested change

// In Marshall() we apply the inverse process to unflatten the nested tables.

// In Marshal() we apply the inverse process to unflatten the nested tables.

paulbellamy · 2021-05-05T12:43:55Z

ingest/ledgerbackend/toml.go

+// the catchup command on captive core.
+func (c *CaptiveCoreToml) CatchupToml() (*CaptiveCoreToml, error) {
+	// clone the existing toml file
+	data, err := c.Marshall()


Suggested change

data, err := c.Marshall()

data, err := c.Marshal()

paulbellamy · 2021-05-05T12:45:19Z

ingest/ledgerbackend/toml.go

+	var offline CaptiveCoreToml
+	if err = offline.unmarshal(data); err != nil {
+		return nil, errors.Wrap(err, "could not unmarshall captive core toml")
+	}


Should we have one of these, just to make this more clear what we're doing here?

func (c *CaptiveCoreToml) clone() (*CaptiveCoreToml, error) {

tamirms force-pushed the captive-core-toml branch 3 times, most recently from 708ed1c to 57080a0 Compare April 22, 2021 08:28

Shaptic reviewed Apr 22, 2021

View reviewed changes

tamirms force-pushed the captive-core-toml branch 2 times, most recently from ceea647 to 18bef97 Compare April 24, 2021 12:29

tamirms commented Apr 24, 2021

View reviewed changes

Parse captive core toml to implement better merging

e142246

tamirms force-pushed the captive-core-toml branch from 18bef97 to e142246 Compare April 24, 2021 13:16

tamirms requested a review from a team April 26, 2021 09:12

bartekn reviewed Apr 26, 2021

View reviewed changes

ingest/ledgerbackend/stellar_core_runner_test.go Outdated Show resolved Hide resolved

ingest/ledgerbackend/toml.go Outdated Show resolved Hide resolved

Add github.com/pelletier/go-toml dependency

e57e187

tamirms force-pushed the captive-core-toml branch from 1ddff54 to 2ce5690 Compare April 30, 2021 12:21

Implement work around for parsing nested tables

60c50ab

tamirms force-pushed the captive-core-toml branch from 2ce5690 to 60c50ab Compare April 30, 2021 12:24

Fix separator logic

4436d00

tamirms force-pushed the captive-core-toml branch from 3ffa3cf to 4436d00 Compare April 30, 2021 15:38

tamirms and others added 2 commits April 30, 2021 17:51

Merge branch 'master' into captive-core-toml

0c925eb

Use placeholders instead of replacing separators

81f1e28

tamirms force-pushed the captive-core-toml branch from 8b5a5b0 to 81f1e28 Compare April 30, 2021 17:32

tamirms added 8 commits April 30, 2021 19:53

Add more comments

61fdeb7

Simplify flatten and unflatten

da563d9

Add validation tests

0377a23

Log cases of multiple history archive configurations

3884fc7

Add tests for optional flags

580abf8

Update example code

aaa4c8b

Update changelog

47fa616

Use raw string

d7f72d8

tamirms marked this pull request as ready for review May 2, 2021 15:23

tamirms requested a review from a team May 2, 2021 15:24

bartekn approved these changes May 5, 2021

View reviewed changes

paulbellamy approved these changes May 5, 2021

View reviewed changes

Code review fixes

a8878ca

tamirms changed the base branch from master to release-horizon-v2.3.0 May 5, 2021 13:17

Merge branch 'release-horizon-v2.3.0' into captive-core-toml

ba12102

tamirms merged commit 4c98380 into stellar:release-horizon-v2.3.0 May 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ingest/ledgerbackend: Parse captive core toml to implement better merging #3558

ingest/ledgerbackend: Parse captive core toml to implement better merging #3558

tamirms commented Apr 21, 2021 •

edited

Loading

Shaptic left a comment

tamirms Apr 24, 2021

bartekn left a comment

jacekn commented Apr 26, 2021

tamirms commented May 2, 2021

bartekn May 5, 2021

paulbellamy May 5, 2021

paulbellamy May 5, 2021

paulbellamy May 5, 2021

paulbellamy May 5, 2021

paulbellamy May 5, 2021

paulbellamy May 5, 2021

paulbellamy May 5, 2021

	text, err := captiveCoreToml.Marshall()
	text, err := captiveCoreToml.Marshal()

	original := tablePlaceHolders.get(insideBrackets)
	original, ok := tablePlaceHolders.get(insideBrackets)

	func (c *CaptiveCoreToml) Marshall() ([]byte, error) {
	func (c *CaptiveCoreToml) Marshal() ([]byte, error) {

	// In Marshall() we apply the inverse process to unflatten the nested tables.
	// In Marshal() we apply the inverse process to unflatten the nested tables.

ingest/ledgerbackend: Parse captive core toml to implement better merging #3558

ingest/ledgerbackend: Parse captive core toml to implement better merging #3558

Conversation

tamirms commented Apr 21, 2021 • edited Loading

PR Structure

Thoroughness

Release planning

What

Known limitations

Shaptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekn left a comment

Choose a reason for hiding this comment

jacekn commented Apr 26, 2021

tamirms commented May 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirms commented Apr 21, 2021 •

edited

Loading