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

[POC] Break subsystem deps #34144

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions api/server/router/plugin/plugin.go
@@ -1,6 +1,11 @@
package plugin

import "github.com/docker/docker/api/server/router"
import (
"context"

"github.com/docker/docker/api/server/router"
"github.com/docker/docker/component/plugincontroller"
)

// pluginRouter is a router to talk with the plugin controller
type pluginRouter struct {
Expand All @@ -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())
Copy link
Member

Choose a reason for hiding this comment

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

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.

r := &pluginRouter{
backend: b,
}
Expand All @@ -28,7 +34,7 @@ func (r *pluginRouter) initRoutes() {
router.NewGetRoute("/plugins/{name:.*}/json", r.inspectPlugin),
router.NewGetRoute("/plugins/privileges", r.getPrivileges),
router.NewDeleteRoute("/plugins/{name:.*}", r.removePlugin),
router.NewPostRoute("/plugins/{name:.*}/enable", r.enablePlugin), // PATCH?
router.NewPostRoute("/plugins/{name:.*}/enable", r.enablePlugin),
router.NewPostRoute("/plugins/{name:.*}/disable", r.disablePlugin),
router.NewPostRoute("/plugins/pull", r.pullPlugin, router.WithCancel),
router.NewPostRoute("/plugins/{name:.*}/push", r.pushPlugin, router.WithCancel),
Expand Down
3 changes: 1 addition & 2 deletions cmd/dockerd/daemon.go
Expand Up @@ -253,7 +253,6 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
Root: cli.Config.Root,
Name: name,
Backend: d,
PluginBackend: d.PluginManager(),
NetworkSubnetsProvider: d,
DefaultAdvertiseAddr: cli.Config.SwarmDefaultAdvertiseAddr,
RuntimeRoot: cli.getSwarmRunRoot(),
Expand Down Expand Up @@ -518,7 +517,7 @@ func initRouter(opts routerOptions) {
build.NewRouter(opts.buildBackend, opts.daemon),
sessionrouter.NewRouter(opts.sessionManager),
swarmrouter.NewRouter(opts.cluster),
pluginrouter.NewRouter(opts.daemon.PluginManager()),
pluginrouter.NewRouter(),
distributionrouter.NewRouter(opts.daemon),
}

Expand Down
24 changes: 24 additions & 0 deletions component/containerstore/component.go
@@ -0,0 +1,24 @@
package containerstore

import (
"github.com/docker/docker/component"
"github.com/docker/docker/container"
"golang.org/x/net/context"
)

const name = "containerstore"

func Set(s container.Store) (cancel func(), err error) {
return component.Register(name, s)
}

func Get(ctx context.Context) (container.Store, error) {
c := component.Wait(ctx, name)
if c == nil {
return nil, ctx.Err()
}

// This could panic... but I think this is ok.
// This should never be anything else
return c.(container.Store), nil
}
34 changes: 34 additions & 0 deletions component/plugincontroller/component.go
@@ -0,0 +1,34 @@
package plugincontroller

import (
"github.com/docker/docker/component"
"github.com/docker/docker/plugin"
"golang.org/x/net/context"
)

const name = "plugincontroller"

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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{}.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

"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.

Copy link
Member

Choose a reason for hiding this comment

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

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


// Wait waits for the plugin controller component to be available
func Wait(ctx context.Context) *plugin.Manager {
c := component.Wait(ctx, name)
if c == nil {
return nil
}

// This could panic... but I think this is ok.
// This should never be anything else
return c.(*plugin.Manager)
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@dnephin dnephin Jul 26, 2017

Choose a reason for hiding this comment

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

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.

if c == nil {
return nil
}
return c.(*plugin.Manager)
}
137 changes: 137 additions & 0 deletions component/store.go
@@ -0,0 +1,137 @@
package component

import (
"sync"

"github.com/pkg/errors"
"golang.org/x/net/context"
)

var (
existsErr = errors.New("component exists")
nilComponentErr = errors.New("cannot store nil component")

defaultStore = NewStore()
)

// Component is the type stored in the component store
type Component interface{}

// NewStore creates a new
func NewStore() *Store {
return &Store{
components: make(map[string]Component),
waiters: make(map[string]map[chan Component]struct{}),
}
}

// Store stores components.
type Store struct {
mu sync.Mutex
components map[string]Component
waiters map[string]map[chan Component]struct{}
}

// Register registers a component with the default store
func Register(name string, c Component) (cancel func(), err error) {
return defaultStore.Register(name, c)
}

// Get looks up the passed in component name in the default store.
// Get returns nil if the component does not exist.
func Get(name string) Component {
return defaultStore.Get(name)
}

// Wait waits for a component with the given name in the default store
func Wait(ctx context.Context, name string) Component {
return defaultStore.Wait(ctx, name)
}

// Register registers a component with the given name in the store.
func (s Store) Register(name string, c Component) (cancel func(), err error) {
if c == nil {
return nil, errors.Wrap(nilComponentErr, name)
}

s.mu.Lock()
defer s.mu.Unlock()
if _, exists := s.components[name]; exists {
return nil, errors.Wrap(existsErr, name)
}

s.components[name] = c
s.notifyWaiters(name, c)

return func() {
s.mu.Lock()
delete(s.components, name)
s.mu.Unlock()
}, nil
}

func (s Store) notifyWaiters(name string, c Component) {
for waiter := range s.waiters[name] {
waiter <- c
}
}

func (s Store) unregister(name string) {
s.mu.Lock()
defer s.mu.Unlock()
delete(s.components, name)
}

// Get gets a component from the store.
// If the component does not exist it returns nil.
func (s Store) Get(name string) Component {
s.mu.Lock()
defer s.mu.Unlock()
return s.components[name]
}

func (s Store) addWaiter(name string, ch chan Component) {
idx, exists := s.waiters[name]
if !exists {
idx = make(map[chan Component]struct{})
s.waiters[name] = idx

}
idx[ch] = struct{}{}
}

func (s Store) removeWaiter(name string, ch chan Component) {
delete(s.waiters[name], ch)
if len(s.waiters) == 0 {
delete(s.waiters, name)
}
}

// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

Why's this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

This allows components to be able to be started async.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

s.mu.Lock()
if c := s.components[name]; c != nil {
s.mu.Unlock()
return c
}

wait := make(chan Component, 1)

s.addWaiter(name, wait)
defer func() {
s.mu.Lock()
s.removeWaiter(name, wait)
s.mu.Unlock()
}()

s.mu.Unlock()

select {
case <-ctx.Done():
return nil
case c := <-wait:
return c
}
}
127 changes: 127 additions & 0 deletions component/store_test.go
@@ -0,0 +1,127 @@
package component

import (
"testing"
"time"

"context"

"github.com/pkg/errors"
)

type testComponent struct{}

func TestStoreRegister(t *testing.T) {
s := NewStore()

c := testComponent{}
cancel, err := s.Register("test", c)
if err != nil {
t.Fatal(err)
}

_, err = s.Register("test", c)
if errors.Cause(err) != existsErr {
t.Fatal(err)
}

cancel()
cancel, err = s.Register("test", c)
if err != nil {
t.Fatal(err)
}
cancel()

if _, err := s.Register("niltest", nil); errors.Cause(err) != nilComponentErr {
t.Fatal(err)
}
}

func TestStoreGet(t *testing.T) {
s := NewStore()

var c testComponent
cancel, err := s.Register("test", c)
if err != nil {
t.Fatal(err)
}

service := s.Get("test")
if service == nil {
t.Fatal("expected non-nil service")
}

if service != c {
t.Fatal("got wrong service after get")
}

cancel()
service = s.Get("test")
if service != nil {
t.Fatal("expected nil service")
}
}

func TestStoreWait(t *testing.T) {
s := NewStore()

ch := make(chan interface{})
go func() {
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
ch <- s.Wait(ctx, "test")
}()

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()

// make sure the wait is in place
for {
select {
case <-ctx.Done():
t.Fatal(ctx.Err())
default:
}

s.mu.Lock()
ready := len(s.waiters["test"]) > 0
s.mu.Unlock()
if ready {
break
}
time.Sleep(10 * time.Millisecond)
}

// nothing added yet, so there shouldn't be anything in this channel
select {
case <-ch:
t.Fatal("wait returned unexpectedly")
default:
}

var c testComponent
_, err := s.Register("test", c)
if err != nil {
t.Fatal(err)
}

ctx, cancel = context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()

select {
case <-ctx.Done():
t.Fatal(ctx.Err())
case s := <-ch:
if s != c {
t.Fatalf("got unexpected service: %v", s)
}
}

if len(s.waiters["test"]) != 0 {
t.Fatalf("unpexected waiters: %d", len(s.waiters))
}
}

func TestComponentTransoform(t *testing.T) {

}