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

[POC] Break subsystem deps #34144

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@cpuguy83
Copy link
Contributor

cpuguy83 commented Jul 17, 2017

Right now the Daemon object is a mix of a few different things, it provides service discovery to various subsystems (b/c it caches it on the struct and provides accessors to them), it's a container manager, it's a volume manager, it is an API controller, etc...
The Daemon object is required to have lots of deep knowledge about various subystems, deals with large interfaces and is generally difficult to find what pieces are needed for different subsystems, what touches what.

This is a POC to introduce a component store which breaks away the need of storing a bunch of objects on the daemon in favor of a common registry for components.
I've provided an example of this decoupling with the plugin manager (even more to do here, really).
It would be nice to even break the plugin manager up into even smaller components, but I didn't want to begin doing that here.

Posting for comments on design before going any further with it.

cpuguy83 added some commits Jul 17, 2017

Add component store
The goal of the component store is to break various pieces currently
glued together, most often, by the `Daemon` object into discrete
components that can be called on as needed.

With this model, one component can wait for another to become ready
(e.g. listed in the component store) rather than having a strict
heirarchy of objects with an all-powerful object expected to have deep
knowledge of multiple subsystems.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Add plugin component
Adds a plugin component and changes around code to request the component
rather than requesting from `Daemon`.
Stops storing the plugin component on the daemon object.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jul 17, 2017

@dnephin
Copy link
Member

dnephin left a comment

Reminds me of #26609

We really need something like this.

Some questions about the design.

@@ -9,7 +14,8 @@ type pluginRouter struct {
}

// NewRouter initializes a new plugin router
func NewRouter(b Backend) router.Router {
func NewRouter() router.Router {
b := plugincontroller.Wait(context.Background())

This comment has been minimized.

@dnephin

dnephin Jul 17, 2017

Member

It would be a lot nicer if this could continue to receive it's dependency instead of having to wait for it to be available. I don't think the API routers should be aware of the components.

We should try to avoid the component framework/registry leaking into every package. I think only a few central packages should be aware of components, and everything else should just receive references to their dependencies.

// Set sets the plugin controller component
func Set(s *plugin.Manager) (cancel func(), err error) {
return component.Register(name, s)
}

This comment has been minimized.

@dnephin

dnephin Jul 17, 2017

Member

This seems like a strange/unnecessary pattern. Why not just register it directly from where it's created?

registry.Register(name, pluginManager)

What's the purpose of the return values?

I guess cancel() is for removing a component. Why would that be necessary?

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 17, 2017

Contributor

It's neccessary otherwise we end up interacting with just interface{} which is undesirable.
I would even like to figure out a way to make the Component type not be just an alias to interface{}.

This comment has been minimized.

@dnephin

dnephin Jul 17, 2017

Member

I had the same problem in #26609. Separating the component interface and implementation? That way you can cast to the interface, but we can still have different implementations?

If we're never going to have different implementations, and we have to reference the implementation directly, why even have a registry? We could just split things out of the daemon struct and pass them around as references.

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 17, 2017

Contributor

The main point is to not pass around references which will have to have some object managing this, which currently the Daemon does.

This comment has been minimized.

@dnephin

dnephin Jul 17, 2017

Member

Isn't the problem right now that the daemon is responsible for way too may things. A small class which handles coordination between components would be fine, but a large class that handles coordination plus lots of "core application logic" is a nightmare.

All the application logic would move to components which leaves us with a very small coordinator.

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 21, 2017

Contributor

"Very small coordinator", except it needs to import packages for every single subsystem (and often some of the dependencies of those subsystems).

I don't know about you, but I can barely work in the daemon package just simply because it imports too much and brings my editor to a screeching halt, this is nothing to do with the amount of logic it holds but rather the number of things it's initializing, interacting with, etc.

This comment has been minimized.

@dnephin

dnephin Jul 26, 2017

Member

Yes, I agree that the coordinator should not import the subsystem packages. They should register themselves with the coordinator.

}

func Get() *plugin.Manager {
c := component.Get(name)

This comment has been minimized.

@dnephin

dnephin Jul 17, 2017

Member

Why this abstraction instead of getting the component from a registry directly?

It seems like the value of a component registry is that you don't need to be aware of where that component is coming from. If you are getting the component directly from a package we've lost that value.

So either we could remove the component registry, and just pass around references, or we should drop these helpers.

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 17, 2017

Contributor

Same as above. I don't want callers to deal with interface{} values directly.

This comment has been minimized.

@dnephin

dnephin Jul 26, 2017

Member

I agree, we should establish some interface type for a component. Like https://github.com/dnephin/docker/blob/41a6e8db0c0907da5bb45f9ac2988dbf11267f35/component/component.go#L12-L38

But I don't think the "get and cast to desired component interface" should be in the same package as the implementation. Otherwise we've just recoupled the dependency to the component implementation.

// Wait waits for a component with the given name to be added to the store.
// If the component already exists it is returned immediately.
// Wait only returns nil if the passed in context is cancelled.
func (s Store) Wait(ctx context.Context, name string) Component {

This comment has been minimized.

@dnephin

dnephin Jul 17, 2017

Member

Why's this necessary?

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 17, 2017

Contributor

This allows components to be able to be started async.

This comment has been minimized.

@dnephin

dnephin Jul 17, 2017

Member

Because one component might depend on another?

What do you see as the "start" process for a component? Is it just initializing some structures?

Could we handle this with a predefined interface for components with a call to start?

Like this: https://github.com/moby/moby/pull/26609/files#diff-fdbfb41b6307b71e2384f3530c199a81R12

That way we can still start components async, from a central place without everything having to deal with the async behaviour.

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 21, 2017

Contributor

@dnephin It seems a bit awkward to expect the subsystem to initialize other components in this way. The goal is to decouple the components, and hiding the coupling behind an interface is not the same thing.

E.g. "I need access to the volume store service" vs "Let me fire up a volume store service".
The latter is tied to an implementation.

This comment has been minimized.

@dnephin

dnephin Jul 26, 2017

Member

I agree that one component should not initialize other component, that is not what I was suggesting.

It should be up to the centralized coordinator to iterate over all the registered components and initialize them all. It should be able to do that asynchronously without any other code being aware of it.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Jul 17, 2017

Another question: How would you define a component?

I notice the V1 REST API is not part of the component here. Would it ever be part of a component, or are components just the parts that are currently in daemon?

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jul 17, 2017

I'm not sure that the rest API would need to be here unless it was provided by some external interface since nothing explicitly depends on the HTTP endpoints.

@AkihiroSuda

This comment has been minimized.

Copy link
Member

AkihiroSuda commented Jul 17, 2017

Is this PR related to splitting the monolith?

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jul 17, 2017

@AkihiroSuda not explicitly, no.

@stevvooe

This comment has been minimized.

Copy link
Contributor

stevvooe commented Jul 21, 2017

@cpuguy83 There might be some background reading on DI frameworks in Go. Almost every project I've worked on with non-trivial dependencies seems to get here and the daemon has gone very far over the edge. Dare I say, "I miss guice?". It might help to peruse the guice or springboot documentation and squint really hard.

From DI perspective, it looks like we are using packages as a global module registry. This is a good trade off in Go but I worry about how far it can go. Note that we might want to parameterize module access based on other attributes, so we'd have to consider that access pattern, as well.

The other consideration that we might need to make is how can we avoid having singleton code. Let's take this example:

func doWork(id string) error {
  ws := worksubsystem.Get()
  return ws.FooThis(id)
}

This might be fairly pathological in practice and these kinds of things will start cropping up in such a large code base, making it impossible to tell what is required to implement a specific thing. Mind you, this is a great pattern for cross cutting concerns, like metrics, tracing and logging:

func doWork(ctx context.Context, id string, ws WorkSubsystem) error {
  log.G(ctx).WithField("id", id).Info("do some work, here!")
  metrics.G(ctx).Inc("work requested", {"id": id})
  defer metrics.G(ctx).Inc("work completed", {"id": id})
  return ws.FooThis(id)
}

But the thing to notice is that context provides some control over the global state. In practice, the context approach works well when you have globals that need to change based on where the execution takes place.

That said, I think we need to tightly control where the injection happens. Accessing these packages from the wrong place can introduce all manner of bugs, as hinted at above. In guice, this happens in components called modules, which are effectively where config is joined with concrete types. In Go, this might look like this:

var daemon Daemon
if err := modules.Inject(daemon); err != nil { ... }

Such an approach might match the POC here, but hiding the place where these are accessed in the modules (or components, as we call it). This POC clearly gets the registration aspect up and running, but I think we can balance access in this manner. We may even be able to use struct tags to populate these modules.

/ramble

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Sep 19, 2017

I'm going to go ahead and close this since this isn't the direction we really want to go.
Stevvooe's comment outlines quite well why this is not quite the right direction.

@cpuguy83 cpuguy83 closed this Sep 19, 2017

@cpuguy83 cpuguy83 deleted the cpuguy83:break_subsystem_deps branch Sep 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment