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: Component framework - daemon architecture #26609

Closed
wants to merge 19 commits into
base: master
from

Conversation

@dnephin
Copy link
Member

dnephin commented Sep 15, 2016

This PR implements a "Component framework" (component/) and moves the volume logic from the scattered locations around the codebase into a volume component (components/volume).

The goal of this framework is to improve the architecture of the docker daemon by organising functional units by their functionality. Currently:

  • the core logic of the daemon is primarily in a single large daemon package
  • there are no clear interfaces between functional units
  • adding new functionality requires expanding the already large daemon package , and adding files all over the code base (daemon/, api/types, api/server/router, cli/command, cli/command/formatter, client)

With the component framework:

  • the "core logic" of a component is implemented behind an interface so the boundaries between components are explicit
  • all the code for a functional unit should be under a single directory tree in component/<name>
  • the daemon doesn't need to be aware of individual components. It can manage each component through generic lifecycle hooks (Init(), Start(), Reload(), Shutdown()).

Index:

Still TODO:

  • split docker/docker/volume into two packages. The mountpoint package would eventually move into the container component, and the remaining parts of the volume package will move into the volume component
  • move client/volume_* into the component
  • move some integration tests from integration_cli to be "component integration tests
  • only expose API types from the VolumeComponent interface

If accepted, the plan would be to migrate the rest of the daemon code (over time) to components:

  • builder
  • network
  • plugin
  • swarm
  • container
  • image

Includes #26537

@dnephin dnephin force-pushed the dnephin:components_draft branch from 7634016 to a8f55f7 Sep 15, 2016

@dnephin dnephin force-pushed the dnephin:components_draft branch 2 times, most recently from 7d64217 to b4b9295 Sep 15, 2016

Routes() []router.Route
// CommandLine returns the CLI commands provided by the Component
CommandLine(*command.DockerCli) []*cobra.Command
// Interface returns the interfaced used by other Components to access the

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Sep 16, 2016

Member

nit: interfaced -> interface ?

This comment has been minimized.

@dnephin

dnephin Sep 16, 2016

Member

Ya, I should rework this comment

GID int
}

// Events interface used by components to log events

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Sep 16, 2016

Member

nit: Events -> EventLogger or something else? (Events looks like as if it is a slice of Event, IMO 😃 )

This comment has been minimized.

@dnephin

dnephin Sep 16, 2016

Member

Ya, I can change this

}

// ForEach runs a function on each component.Component
func (r *Registry) ForEach(f func(component.Component) error) error {

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Sep 16, 2016

Member

IMO one can use for _, component := range r.All(){..} instead of this ForEach()

@@ -16,6 +16,13 @@ var (
errNameConflict = errors.New("conflict: volume name must be unique")
)

// OperationErr is an error used by the store when operations fail

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Sep 16, 2016

Member

why not use OpErr struct directly?

This comment has been minimized.

@dnephin

dnephin Sep 16, 2016

Member

I tried that, but I ended up with a bunch of test failures on if err != nil. I think it has something to do with go interfaces.

@dnephin dnephin force-pushed the dnephin:components_draft branch 4 times, most recently from 99307d1 to 360c461 Sep 16, 2016

dnephin added some commits Aug 15, 2016

Add initial interface and registry for Components.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Add component lifecycle hooks to the daemon.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Create the volume component.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Move daemon volume functions to volume component.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Move volume component initialization to the component
Also move Events into component.Config

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Remove remaining references to volume store in the daemon package.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Update consumers to accept context in init.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Move volume cli into component.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Move volume api types into the volume component.
Also move volume/client to volume/command

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Move volume formatter to the volume component.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Move the volume component interface into types.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Move volume/driver package to the volume component.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Move volume/local to components/volume/local
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Move volume/store to component/volume/store
Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin added some commits Sep 15, 2016

Move pkg/component to component to get past the validate-pkg check.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Register the volume component.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Fix some names, comments and tests.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Fix volume.Create logging an event when no volume was created.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Fix cross.
Signed-off-by: Daniel Nephin <dnephin@docker.com>

@dnephin dnephin force-pushed the dnephin:components_draft branch from 360c461 to 41a6e8d Sep 20, 2016

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Sep 23, 2016

ping @docker/core-engine-maintainers PTAL

@@ -0,0 +1,63 @@
package registry

This comment has been minimized.

@thaJeztah

thaJeztah Sep 23, 2016

Member

Wonder if naming this "registry" will lead to confusion with the "docker registry" (for images)

This comment has been minimized.

@mlaventure

mlaventure Sep 23, 2016

Contributor

I agree, I got confused a bit initially. Maybe using store like we do elsewhere may be better?

This comment has been minimized.

@dnephin

dnephin Sep 23, 2016

Member

I believe we generally use the term store for things we're storing on disk. I don't know if that fits any better here.

I think it's kind of unfortunate that we can't re-use the term registry. This really is a registry and calling it anything else seems confusing.

This comment has been minimized.

@stevvooe

stevvooe Sep 23, 2016

Contributor

registry has a specific meaning in docker. I would recommend having a components package, with registration tools.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Sep 23, 2016

I like this concept, but wondering; we did a lot of refactoring to make the API a standalone package, and now API types are "spread all over the place"; will this complicate other projects to consume the API code?

@mlaventure

This comment has been minimized.

Copy link
Contributor

mlaventure commented Sep 23, 2016

Maybe we should keep the API Types as used when communicating with the daemon in the API package, and have an internal API's for the component.

It'd require converting from one to the other, but at least we'd be able to make changes to the internal component type without it automatically bleeding out into the public API types.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Sep 23, 2016

we did a lot of refactoring to make the API a standalone package, and now API types are "spread all over the place";

I think it's important we remember why we moved the types around. The goal was to ensure that there was a clear distinction between internal types and public interface types. I think we can continue to accomplish this goal without putting them all in the same package. We can actually do an even better job of this by generating the types from a spec, which is something I'm working on as part of docker/engine-api#77. Moving the types closer to the packages where they are used shouldn't interfere with the original goal.

will this complicate other projects to consume the API code?

It shouldn't, the types are still in a separate package from the application logic. Other projects will primarily interact with the api client. I think the client also needs to move to components/volume/client, but that's more involved so I haven't completed that work yet. Working with these packages shouldn't be any more difficult, they are just imported from a different path. The dependencies of the packages don't change.

@mlaventure

This comment has been minimized.

Copy link
Contributor

mlaventure commented Sep 23, 2016

So a client making use of the API would have to import all components types in the end? (i.e.: "github.com/docker/docker/components/*/types"

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Sep 23, 2016

Thanks for explaining @dnephin, sounds good

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Sep 23, 2016

Maybe we should keep the API Types as used when communicating with the daemon in the API package, and have an internal API's for the component.

I was thinking about this as well. I'm not sure if makes sense to have two different API types. The goal of this project is to attempt to structure the code in such a way that the components could eventually be run out-of-process. Given that goal I think that we should considering everything outside of the component as "external". So we're left with only two types: internal types, and external types (current the api types).

The component interface would then only expose methods which used the external types. We wouldn't need two api types, because other components and the client would both use the "external types". The internal types would only be used within the component.

If we were to keep api types in the current api/types package I think we don't really accomplish the goal of this architecture. It should be possible to add, remove, or swap a component very easily. If we have types in a separate package, outside of the component hierarchy, that task becomes more difficult.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Sep 23, 2016

So a client making use of the API would have to import all components types in the end?

It depends on what the client is doing. Right now we assume a docker client just needs access to everything, but I think as that "everything" grows to include more and more things, the reality is that any given client might only interactive with one or two components at a time.

So a client might be concerned with images and containers, and import those two components, or it might be concerned with swarm nodes and services and import the clustering component client.

@mlaventure

This comment has been minimized.

Copy link
Contributor

mlaventure commented Sep 23, 2016

@dnephin thanks for the explanation. SGTM

}

// Context is the context used by Componenets to run
type Context struct {

This comment has been minimized.

@stevvooe

stevvooe Sep 23, 2016

Contributor

Could we avoid having another thing called a context that doesn't do context things? If we are to pass around cross-cutting things, let's consider using context.

This comment has been minimized.

@dnephin

dnephin Sep 26, 2016

Member

I don't understand. This is a context, it's passing around cross-cutting things.

This comment has been minimized.

@thaJeztah

thaJeztah Sep 26, 2016

Member

I think @stevvooe means possible confusion with https://blog.golang.org/context

Filesystem FilesystemConfig
}

// FilesystemConfig is the configuration for the root filesystem used by

This comment has been minimized.

@stevvooe

stevvooe Sep 23, 2016

Contributor

What is the "root filesystem"?

type Component interface {
CompType
// Routes returns the API routes provided by the Component
Routes() []router.Route

This comment has been minimized.

@stevvooe

stevvooe Sep 23, 2016

Contributor

I'm not sure this is a well structured abstraction. We have a controller (start, stop), metadata, a routable thingy, a cli thingy and a thingy thingy. These should all be different things.

This comment has been minimized.

@dnephin

dnephin Sep 26, 2016

Member

I'll move the controller methods to a Controller type

// OperationErr is an error used by the store when operations fail
type OperationErr interface {
error
IsInUse() bool

This comment has been minimized.

@stevvooe

stevvooe Sep 23, 2016

Contributor

These should all be separate interfaces.

I see what is being rafactored here, so it might not be easy to make these changes.

errNameConflict = errors.New("conflict: volume name must be unique")
// errVolumeExists is a typed error returned on create when a volume exists
// with the give name and the same driver.
errVolumExists = errors.New("duplicate: volume already exists")

This comment has been minimized.

@stevvooe

stevvooe Sep 23, 2016

Contributor

Sp.

Remove(volume.Volume) error

// Drivers
DriverList() []string

This comment has been minimized.

@stevvooe

stevvooe Sep 23, 2016

Contributor

Seems orthogonal.


const (
// ComponentType is the name identifying this type of component
ComponentType = "volumes"

This comment has been minimized.

@stevvooe

stevvooe Sep 23, 2016

Contributor

What we are calling components somewhat sounds like drivers. Can there be multiple volume components?

This comment has been minimized.

@dnephin

dnephin Sep 26, 2016

Member

No, there is only a single volume component.

This comment has been minimized.

@stevvooe

stevvooe Sep 26, 2016

Contributor

So what is the point of a component type? Why have components if they are just singletons?

@stevvooe

This comment has been minimized.

Copy link
Contributor

stevvooe commented Sep 23, 2016

@dnephin I don't think types packages make sense in the context of most components. This only makes sense in the context of controlled APIs, and even then, the types should be local to the API with which they are used. For example, the API is a component, and the types used with the API should be used together. Where we went wrong initially was trying to share these types between components.

There is better separation today, but we need to be careful not to reintroduce this coupling unless it is a design goal.

My argument may different if we define a component as an APIController, which might make a lot more sense. For example, if a component exports a handler method, we could bind it to a URL path.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Sep 26, 2016

[types] only makes sense in the context of controlled APIs, and even then, the types should be local to the API with which they are used

Yes, that's exactly what is being done here.

the API is a component

I disagree. Each component has an API, the API is not a component in itself.

My argument may different if we define a component as an APIController, which might make a lot more sense. For example, if a component exports a handler method, we could bind it to a URL path

Instead of exposing handlers, the components expose Routes(), but the idea is basically the same. The routes are just handlers + metadata instructing the engine on when to call them.

@stevvooe

This comment has been minimized.

Copy link
Contributor

stevvooe commented Sep 26, 2016

I disagree. Each component has an API, the API is not a component in itself.

The docker API is a unified, interconnected HTTP API. If we vertically couple API and implementation, we'll be back in the same place we started.

Instead of exposing handlers, the components expose Routes(), but the idea is basically the same. The routes are just handlers + metadata instructing the engine on when to call them.

If the idea is the same, why depart from net/http? We can decouple the metadata and leave that as part of the registration.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Sep 26, 2016

why depart from net/http?

As I understand it, net/http does not provide any way to specify routes. It is a very-low level interface. We are already using http://www.gorillatoolkit.org/pkg/mux, which is what we are continuing to use in this PR.

@LK4D4

This comment has been minimized.

Copy link
Contributor

LK4D4 commented Nov 10, 2016

design LGTM

@MHBauer
Copy link
Contributor

MHBauer left a comment

Indifferent about this.

With additional work it will be structured more by complete functional unit.

The final components/volume directory does look very neat and tidy.

// Register adds a new component to the registry. If there is already a
// component with the same ComponentType an error is returned.
func (r *Registry) Register(c component.Component) {
r.components[c.Provides()] = c

This comment has been minimized.

@MHBauer

MHBauer Nov 17, 2016

Contributor

I know this is core stuff, but do we think there will ever be a time we'll want more than one of each of these components?

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Nov 22, 2016

I think a good indication of the right abstraction here is that we can have a unified way to configure and create containers (as in something just above the raw oci config for runc/containerd) and we have another layer that's for "Containers" (capital "C") that users interact with.

Right now we have 2 container abstractions in the daemon (plugins and "Containers"), not to mention the namespace dancing that libnetwork does and how we'd like to stuff even more work into containers.

So, likewise with volumes. If volumes are a singleton, then other components can't really use them because they are an abstraction for the user only.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Dec 27, 2016

This PR sat long enough that it's probably easier to start from scratch than it is to rebase, so I'm going to close it.

@dnephin dnephin closed this Dec 27, 2016

@xproax
Copy link

xproax left a comment

🥇

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