Skip to content

libcontainerd: split client and daemon supervision #37149

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Aug 16, 2018
Merged

libcontainerd: split client and daemon supervision #37149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 44 additions & 24 deletions cmd/dockerd/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import (
"github.com/docker/docker/daemon/config"
"github.com/docker/docker/daemon/listeners"
"github.com/docker/docker/dockerversion"
"github.com/docker/docker/libcontainerd"
"github.com/docker/docker/libcontainerd/supervisor"
dopts "github.com/docker/docker/opts"
"github.com/docker/docker/pkg/authorization"
"github.com/docker/docker/pkg/jsonmessage"
Expand All @@ -45,7 +45,6 @@ import (
"github.com/docker/docker/pkg/signal"
"github.com/docker/docker/pkg/system"
"github.com/docker/docker/plugin"
"github.com/docker/docker/registry"
"github.com/docker/docker/runconfig"
"github.com/docker/go-connections/tlsconfig"
swarmapi "github.com/docker/swarmkit/api"
Expand Down Expand Up @@ -112,6 +111,10 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
return err
}

if err := system.MkdirAll(cli.Config.ExecRoot, 0700, ""); err != nil {
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jul 10, 2018

Choose a reason for hiding this comment

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

This looks new; where/when was this directory created in the past?

I also see some validations steps below that may fail; is it possible to move this further down, so that we don't end up with this directory if the configuration happened to be invalid?

Finally; not sure if this directory needs special treatment (as the "daemon" root gets); also taking into account that this code is both Linux and Windows (and Windows doesn't handle permissions similar to Linux)

moby/daemon/daemon_unix.go

Lines 1136 to 1187 in 8e2f920

func setupDaemonRoot(config *config.Config, rootDir string, rootIDs idtools.IDPair) error {
config.Root = rootDir
// the docker root metadata directory needs to have execute permissions for all users (g+x,o+x)
// so that syscalls executing as non-root, operating on subdirectories of the graph root
// (e.g. mounted layers of a container) can traverse this path.
// The user namespace support will create subdirectories for the remapped root host uid:gid
// pair owned by that same uid:gid pair for proper write access to those needed metadata and
// layer content subtrees.
if _, err := os.Stat(rootDir); err == nil {
// root current exists; verify the access bits are correct by setting them
if err = os.Chmod(rootDir, 0711); err != nil {
return err
}
} else if os.IsNotExist(err) {
// no root exists yet, create it 0711 with root:root ownership
if err := os.MkdirAll(rootDir, 0711); err != nil {
return err
}
}
// if user namespaces are enabled we will create a subtree underneath the specified root
// with any/all specified remapped root uid/gid options on the daemon creating
// a new subdirectory with ownership set to the remapped uid/gid (so as to allow
// `chdir()` to work for containers namespaced to that uid/gid)
if config.RemappedRoot != "" {
config.Root = filepath.Join(rootDir, fmt.Sprintf("%d.%d", rootIDs.UID, rootIDs.GID))
logrus.Debugf("Creating user namespaced daemon root: %s", config.Root)
// Create the root directory if it doesn't exist
if err := idtools.MkdirAllAndChown(config.Root, 0700, rootIDs); err != nil {
return fmt.Errorf("Cannot create daemon root: %s: %v", config.Root, err)
}
// we also need to verify that any pre-existing directories in the path to
// the graphroot won't block access to remapped root--if any pre-existing directory
// has strict permissions that don't allow "x", container start will fail, so
// better to warn and fail now
dirPath := config.Root
for {
dirPath = filepath.Dir(dirPath)
if dirPath == "/" {
break
}
if !idtools.CanAccess(dirPath, rootIDs) {
return fmt.Errorf("a subdirectory in your graphroot path (%s) restricts access to the remapped root uid/gid; please fix by allowing 'o+x' permissions on existing directories", config.Root)
}
}
}
if err := setupDaemonRootPropagation(config); err != nil {
logrus.WithError(err).WithField("dir", config.Root).Warn("Error while setting daemon root propagation, this is not generally critical but may cause some functionality to not work or fallback to less desirable behavior")
}
return nil
}

moby/daemon/daemon_windows.go

Lines 472 to 479 in 6cd806a

func setupDaemonRoot(config *config.Config, rootDir string, rootIDs idtools.IDPair) error {
config.Root = rootDir
// Create the root directory if it doesn't exists
if err := system.MkdirAllWithACL(config.Root, 0, system.SddlAdministratorsLocalSystem); err != nil {
return err
}
return nil
}

Copy link
Copy Markdown
Member Author

@dmcgowan dmcgowan Jul 18, 2018

Choose a reason for hiding this comment

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

Previously this directory was getting created as a side effect of getting a containerd client. This seemed to be an inappropriate way to create a shared directory so I just moved it here. It is still getting created with same permissions at roughly the same time though.

return err
}

if cli.Pidfile != "" {
pf, err := pidfile.New(cli.Pidfile)
if err != nil {
Expand All @@ -135,19 +138,27 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
return fmt.Errorf("Failed to load listeners: %v", err)
}

registryService, err := registry.NewService(cli.Config.ServiceOptions)
if err != nil {
return err
}
ctx, cancel := context.WithCancel(context.Background())
if cli.Config.ContainerdAddr == "" && runtime.GOOS != "windows" {
opts, err := cli.getContainerdDaemonOpts()
if err != nil {
cancel()
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin Aug 16, 2018

Choose a reason for hiding this comment

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

It looks like if you move defer cancel() to right after context.WithCancel you won't have to call it explicitly, i.e.

	ctx, cancel := context.WithCancel(context.Background())
+ 	defer cancel()
 	if cli.Config.ContainerdAddr == "" && runtime.GOOS != "windows" {
 		opts, err := cli.getContainerdDaemonOpts()
 		if err != nil {
- 			cancel()
 			return fmt.Errorf("Failed to generate containerd options: %v", err)
 		}

Copy link
Copy Markdown
Member Author

@dmcgowan dmcgowan Aug 16, 2018

Choose a reason for hiding this comment

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

This is actually like this on purpose, the defer ordering is important because the cancel() should run before r.WaitTimeout(10 * time.Second). I should probably note this in a comment

return fmt.Errorf("Failed to generate containerd options: %v", err)
}

rOpts, err := cli.getRemoteOptions()
if err != nil {
return fmt.Errorf("Failed to generate containerd options: %v", err)
}
containerdRemote, err := libcontainerd.New(filepath.Join(cli.Config.Root, "containerd"), filepath.Join(cli.Config.ExecRoot, "containerd"), rOpts...)
if err != nil {
return err
r, err := supervisor.Start(ctx, filepath.Join(cli.Config.Root, "containerd"), filepath.Join(cli.Config.ExecRoot, "containerd"), opts...)
if err != nil {
cancel()
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin Aug 16, 2018

Choose a reason for hiding this comment

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

ditto

return fmt.Errorf("Failed to start containerd: %v", err)
}

cli.Config.ContainerdAddr = r.Address()

// Try to wait for containerd to shutdown
defer r.WaitTimeout(10 * time.Second)
}
defer cancel()

signal.Trap(func() {
cli.stop()
<-stopc // wait for daemonCli.start() to return
Expand All @@ -162,7 +173,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
logrus.Fatalf("Error creating middlewares: %v", err)
}

d, err := daemon.NewDaemon(cli.Config, registryService, containerdRemote, pluginStore)
d, err := daemon.NewDaemon(ctx, cli.Config, pluginStore)
if err != nil {
return fmt.Errorf("Error starting daemon: %v", err)
}
Expand Down Expand Up @@ -207,10 +218,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {

initRouter(routerOptions)

// process cluster change notifications
watchCtx, cancel := context.WithCancel(context.Background())
defer cancel()
go d.ProcessClusterNotifications(watchCtx, c.GetWatchStream())
go d.ProcessClusterNotifications(ctx, c.GetWatchStream())

cli.setupConfigReloadTrap()

Expand All @@ -227,8 +235,12 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
// Wait for serve API to complete
errAPI := <-serveAPIWait
c.Cleanup()

shutdownDaemon(d)
containerdRemote.Cleanup()

// Stop notification processing and any background processes
cancel()

if errAPI != nil {
return fmt.Errorf("Shutting down due to ServeAPI error: %v", errAPI)
}
Expand Down Expand Up @@ -511,14 +523,22 @@ func (cli *DaemonCli) initMiddlewares(s *apiserver.Server, cfg *apiserver.Config
return nil
}

func (cli *DaemonCli) getRemoteOptions() ([]libcontainerd.RemoteOption, error) {
opts := []libcontainerd.RemoteOption{}

pOpts, err := cli.getPlatformRemoteOptions()
func (cli *DaemonCli) getContainerdDaemonOpts() ([]supervisor.DaemonOpt, error) {
opts, err := cli.getPlatformContainerdDaemonOpts()
if err != nil {
return nil, err
}
opts = append(opts, pOpts...)

if cli.Config.Debug {
opts = append(opts, supervisor.WithLogLevel("debug"))
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jul 10, 2018

Choose a reason for hiding this comment

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

Should we use cli.Config.LogLevel for this? (That's set to debug if debug is enabled, but otherwise uses what's configured by the user) see #37419

Copy link
Copy Markdown
Member Author

@dmcgowan dmcgowan Jul 20, 2018

Choose a reason for hiding this comment

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

Looks like you addressed this based on the last rebase

} else if cli.Config.LogLevel != "" {
opts = append(opts, supervisor.WithLogLevel(cli.Config.LogLevel))
}

if !cli.Config.CriContainerd {
opts = append(opts, supervisor.WithPlugin("cri", nil))
}

return opts, nil
}

Expand Down
23 changes: 5 additions & 18 deletions cmd/dockerd/daemon_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
"github.com/containerd/containerd/runtime/linux"
"github.com/docker/docker/cmd/dockerd/hack"
"github.com/docker/docker/daemon"
"github.com/docker/docker/libcontainerd"
"github.com/docker/docker/libcontainerd/supervisor"
"github.com/docker/libnetwork/portallocator"
"golang.org/x/sys/unix"
)
Expand All @@ -36,29 +36,16 @@ func getDaemonConfDir(_ string) string {
return "/etc/docker"
}

func (cli *DaemonCli) getPlatformRemoteOptions() ([]libcontainerd.RemoteOption, error) {
opts := []libcontainerd.RemoteOption{
libcontainerd.WithOOMScore(cli.Config.OOMScoreAdjust),
libcontainerd.WithPlugin("linux", &linux.Config{
func (cli *DaemonCli) getPlatformContainerdDaemonOpts() ([]supervisor.DaemonOpt, error) {
opts := []supervisor.DaemonOpt{
supervisor.WithOOMScore(cli.Config.OOMScoreAdjust),
supervisor.WithPlugin("linux", &linux.Config{
Shim: daemon.DefaultShimBinary,
Runtime: daemon.DefaultRuntimeBinary,
RuntimeRoot: filepath.Join(cli.Config.Root, "runc"),
ShimDebug: cli.Config.Debug,
}),
}
if cli.Config.Debug {
opts = append(opts, libcontainerd.WithLogLevel("debug"))
} else if cli.Config.LogLevel != "" {
opts = append(opts, libcontainerd.WithLogLevel(cli.Config.LogLevel))
}
if cli.Config.ContainerdAddr != "" {
opts = append(opts, libcontainerd.WithRemoteAddr(cli.Config.ContainerdAddr))
} else {
opts = append(opts, libcontainerd.WithStartDaemon(true))
}
if !cli.Config.CriContainerd {
opts = append(opts, libcontainerd.WithPlugin("cri", nil))
}

return opts, nil
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/dockerd/daemon_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"os"
"path/filepath"

"github.com/docker/docker/libcontainerd"
"github.com/docker/docker/libcontainerd/supervisor"
"github.com/sirupsen/logrus"
"golang.org/x/sys/windows"
)
Expand Down Expand Up @@ -48,7 +48,7 @@ func notifyShutdown(err error) {
}
}

func (cli *DaemonCli) getPlatformRemoteOptions() ([]libcontainerd.RemoteOption, error) {
func (cli *DaemonCli) getPlatformContainerdDaemonOpts() ([]supervisor.DaemonOpt, error) {
return nil, nil
}

Expand Down
48 changes: 45 additions & 3 deletions daemon/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ import (
"sync"
"time"

"google.golang.org/grpc"

"github.com/containerd/containerd"
"github.com/containerd/containerd/defaults"
"github.com/containerd/containerd/pkg/dialer"
"github.com/docker/docker/api/types"
containertypes "github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/swarm"
Expand Down Expand Up @@ -94,6 +99,7 @@ type Daemon struct {
PluginStore *plugin.Store // todo: remove
pluginManager *plugin.Manager
linkIndex *linkIndex
containerdCli *containerd.Client
containerd libcontainerd.Client
defaultIsolation containertypes.Isolation // Default isolation mode on Windows
clusterProvider cluster.Provider
Expand Down Expand Up @@ -565,9 +571,14 @@ func (daemon *Daemon) IsSwarmCompatible() error {

// NewDaemon sets up everything for the daemon to be able to service
// requests from the webserver.
func NewDaemon(config *config.Config, registryService registry.Service, containerdRemote libcontainerd.Remote, pluginStore *plugin.Store) (daemon *Daemon, err error) {
func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.Store) (daemon *Daemon, err error) {
setDefaultMtu(config)

registryService, err := registry.NewService(config.ServiceOptions)
if err != nil {
return nil, err
}

// Ensure that we have a correct root key limit for launching containers.
if err := ModifyRootKeyLimit(); err != nil {
logrus.Warnf("unable to modify root key limit, number of containers could be limited by this quota: %v", err)
Expand Down Expand Up @@ -720,8 +731,35 @@ func NewDaemon(config *config.Config, registryService registry.Service, containe
}
registerMetricsPluginCallback(d.PluginStore, metricsSockPath)

gopts := []grpc.DialOption{
grpc.WithInsecure(),
grpc.WithBackoffMaxDelay(3 * time.Second),
grpc.WithDialer(dialer.Dialer),

// TODO(stevvooe): We may need to allow configuration of this on the client.
grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(defaults.DefaultMaxRecvMsgSize)),
grpc.WithDefaultCallOptions(grpc.MaxCallSendMsgSize(defaults.DefaultMaxSendMsgSize)),
}
if config.ContainerdAddr != "" {
d.containerdCli, err = containerd.New(config.ContainerdAddr, containerd.WithDefaultNamespace(ContainersNamespace), containerd.WithDialOpts(gopts))
if err != nil {
return nil, errors.Wrapf(err, "failed to dial %q", config.ContainerdAddr)
}
}

createPluginExec := func(m *plugin.Manager) (plugin.Executor, error) {
return pluginexec.New(getPluginExecRoot(config.Root), containerdRemote, m)
var pluginCli *containerd.Client

// Windows is not currently using containerd, keep the
// client as nil
if config.ContainerdAddr != "" {
pluginCli, err = containerd.New(config.ContainerdAddr, containerd.WithDefaultNamespace(pluginexec.PluginNamespace), containerd.WithDialOpts(gopts))
if err != nil {
return nil, errors.Wrapf(err, "failed to dial %q", config.ContainerdAddr)
}
}

return pluginexec.New(ctx, getPluginExecRoot(config.Root), pluginCli, m)
}

// Plugin system initialization should happen before restore. Do not change order.
Expand Down Expand Up @@ -880,7 +918,7 @@ func NewDaemon(config *config.Config, registryService registry.Service, containe

go d.execCommandGC()

d.containerd, err = containerdRemote.NewClient(ContainersNamespace, d)
d.containerd, err = libcontainerd.NewClient(ctx, d.containerdCli, filepath.Join(config.ExecRoot, "containerd"), ContainersNamespace, d)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1037,6 +1075,10 @@ func (daemon *Daemon) Shutdown() error {
daemon.netController.Stop()
}

if daemon.containerdCli != nil {
daemon.containerdCli.Close()
}

return daemon.cleanupMounts()
}

Expand Down
Loading