Skip to content

Initial Config Management Design#87

Closed
danehans wants to merge 5 commits intoenvoyproxy:mainfrom
danehans:issue_43
Closed

Initial Config Management Design#87
danehans wants to merge 5 commits intoenvoyproxy:mainfrom
danehans:issue_43

Conversation

@danehans
Copy link
Copy Markdown
Contributor

@danehans danehans commented Jun 2, 2022

Adds a doc that details the initial config management design.

xref #43

@danehans danehans marked this pull request as draft June 2, 2022 20:20
Comment thread docs/design/CONFIG_MANAGEMENT.md Outdated
Comment on lines 169 to 171
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@youngnick PTAL at this. I see this as an issue for using Runnable since we need to support non-k8s environments in the future. cc: @arkodg

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Recording some notes from the discussion in the community meeting:

  • @danehans is going to look into another framework, and make sure it has Context support.
  • the rest of the PR is reviewable already.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From the call:

So it sounds like we're thinking that we'll need to have two goroutine managers: the controller-runtime one and a non-kube one, possibly nested. IMO, https://pkg.go.dev/github.com/datawire/dlib/dgroup is attractive for this because it takes a func(ctx context.Context) error function pointer as the argument, which happens to be the same signature as controller-runtime's manager.Runnable's Start method; instead of saying manager.Add(myRunnable), you'd say group.Go("name", myRunnable.Start).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer using https://github.com/tetratelabs/run which also provides the ability to use context for life cycle management of go routines. It provides a superset of functionality compared to dgroup and specifically also provides a notion of readiness through different stages of a routine lifecycle, which dgroup omits. This due to it being more than just go routine management but even more so a reusable component manager. It allows components to have their own configuration management based on https://github.com/spf13/pflag and provides a dedicated deterministically ordered pre-run wiring and set-up phase next to routine start/stop.

Based on it we (at Tetrate) have a bunch of components we can easily reuse and have configured in a consistent manner (e.g. gRPC server. HTTP server, Signal handler, etc.) Due to strong guarantees on lifecycle management it makes things as registering add-on gRPC services to a gRPC server easy and straight forward. Given we want EG to be highly extensible and easy to wrap/embed by other projects or products I think it would be the right choice.

Copy link
Copy Markdown
Contributor Author

@danehans danehans Jun 3, 2022

Choose a reason for hiding this comment

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

@basvanbeek thank you for providing additional context for https://github.com/tetratelabs/run. From the tetratelabs/run docs:

... It uses the https://github.com/oklog/run/ package as its basis and enhances it with configuration registration and validation as well as pre-run phase logic.

Have you considered adding config registration, validation, and pre-run logic to oklog/run?

Since we're at an impasse on whether to use https://github.com/tetratelabs/run or https://pkg.go.dev/github.com/datawire/dlib/dgroup, should we proceed with the upstream (oklog/run) that both projects are based on?

@arkodg @LukeShu @alexgervais @youngnick @skriss please provide input so I can proceed with iterating on the design.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would prefer picking one of our two more complete options than going with the upstream oklog/run and having to re-implement parts of functionalities we'll require anyway. I'm familiar with datawire/dlib/dgroup so it would be my go-to choice, but that's only because I haven't worked with tetratelabs/run.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To clarify, dgroup is not built on oklog/run. It is built on a forked/vendored version of golang.org/x/sync/errgroup.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@danehans I have very briefly considered to upstream the logic of tetratelabs/run when I started building it.

To be honest, oklog/run is a finished library. It serves the scope that @peterbourgon envisioned (goroutine lifecycle management prior to context.Context cancellation being available as standard lib) and is stable (not receiving any updates, doesn't have issues, etc.). The library is very small by itself so it carries more an idea than code.

tetratelabs/run's scope is much bigger. It has native support for context cancellation as well as services that have start/stop semantics. It provides more a framework for reusable components that allow for easy composition and self contained configuration management, bootstrap logic and allows ordered initialization and pre-run phases.

I agree with @alexgervais that choosing native oklog/run is a bad idea. However, as I expressed above, I would prefer tetratelabs/run over dgroup specifically for its inclusion of deterministically ordered configuration and pre goroutine run stages (things specifically mentioned to not be dealt with by dgroup.

The reason I request for these additional features is that it allows the component authors to have consistent handling and management of their components and isolated from others, while keeping a binary's wiring small and clean. This allows projects and Vendor products to reuse as much as possible, having an easy way to embed/wrap EG in their code without having to deal with untangling EG bootstrap or having to duplicate lots of code that will need constant sync with upstream.

Comment thread docs/design/CONFIG_MANAGEMENT.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 for a subcommand that spawns and manages all components.

Comment thread docs/design/CONFIG_MANAGEMENT.md Outdated
danehans added 3 commits June 6, 2022 11:39
Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans danehans marked this pull request as ready for review June 6, 2022 18:40
Comment thread docs/design/CONFIG_MANAGEMENT.md Outdated
Signed-off-by: danehans <daneyonhansen@gmail.com>
Comment thread docs/design/CONFIG_MANAGEMENT.md Outdated
Comment thread docs/design/CONFIG_MANAGEMENT.md Outdated
Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans
Copy link
Copy Markdown
Contributor Author

Closing since #111 and #95 cover config management.

@danehans danehans closed this Jun 27, 2022
@danehans danehans deleted the issue_43 branch July 12, 2022 19:21
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.

6 participants