libcontainerd: split client and daemon supervision by dmcgowan · Pull Request #37149 · moby/moby

dmcgowan · 2018-05-25T00:22:43Z

Adds a supervisor package for starting and monitoring containerd.
Moves grpc connection into daemon for use by libcontainerd and any other process.

cpuguy83 · 2018-05-25T15:15:55Z

Build failing due to conflicts.

cpuguy83 · 2018-05-25T18:05:09Z

Is there some reason you have the containerd 1.1 update in this PR? I think this is blocked on other issues.

dmcgowan · 2018-05-25T20:35:37Z

I think this is blocked on other issues.

Which other issues? The containerd 1.1 is blocked by some issues updating grpc. I believe I also need an updated GRPC for this PR, haven't tested with older ones. I don't see a need to push for this without containerd 1.1 though.

codecov · 2018-06-06T20:35:05Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f57f260). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #37149   +/-   ##
=========================================
  Coverage          ?   35.84%           
=========================================
  Files             ?      606           
  Lines             ?    44699           
  Branches          ?        0           
=========================================
  Hits              ?    16021           
  Misses            ?    26469           
  Partials          ?     2209

thaJeztah · 2018-06-20T05:33:29Z

@dmcgowan this still WIP?

dmcgowan · 2018-06-20T20:10:46Z

Removing wip, janky is at least passing now

thaJeztah

left some questions

Also needs a rebase (probably I'm to blame with #37419)

thaJeztah · 2018-07-10T10:05:02Z

cmd/dockerd/daemon.go

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

}

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.

thaJeztah · 2018-07-10T10:16:59Z

cmd/dockerd/daemon.go

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

Looks like you addressed this based on the last rebase

thaJeztah

LGTM, thanks!

Adds a supervisor package for starting and monitoring containerd. Separates grpc connection allowing access from daemon. Signed-off-by: Derek McGowan <[email protected]>

kolyshkin · 2018-08-16T09:50:30Z

cmd/dockerd/daemon.go

+	if cli.Config.ContainerdAddr == "" && runtime.GOOS != "windows" {
+		opts, err := cli.getContainerdDaemonOpts()
+		if err != nil {
+			cancel()


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) }

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

kolyshkin · 2018-08-16T09:50:44Z

cmd/dockerd/daemon.go

-		return err
+		r, err := supervisor.Start(ctx, filepath.Join(cli.Config.Root, "containerd"), filepath.Join(cli.Config.ExecRoot, "containerd"), opts...)
+		if err != nil {
+			cancel()


kolyshkin · 2018-08-16T10:36:38Z

LGTM except for a tiny nitpick with cancel()

thaJeztah · 2018-08-16T17:00:02Z

let's merge; the defer is a nice to have; think we should be good with the code as is 👍

dmcgowan requested review from cpuguy83 and tianon as code owners May 25, 2018 00:22

GordonTheTurtle added the status/0-triage label May 25, 2018

dmcgowan force-pushed the split-libcontainerd branch 2 times, most recently from e7f5ed8 to afc7dd0 Compare May 25, 2018 00:50

vdemeester added status/2-code-review and removed status/0-triage labels May 25, 2018

vdemeester mentioned this pull request May 25, 2018

rfc: move client/types from api/types… #37000

Closed

5 tasks

dmcgowan force-pushed the split-libcontainerd branch 2 times, most recently from 955f0cc to ae24d4c Compare June 6, 2018 20:35

dmcgowan force-pushed the split-libcontainerd branch from ae24d4c to 0abacdb Compare June 6, 2018 20:57

dmcgowan added the rebuild/windowsRS1 label Jun 6, 2018

GordonTheTurtle removed the rebuild/windowsRS1 label Jun 6, 2018

GordonTheTurtle assigned dnephin Jun 9, 2018

dmcgowan force-pushed the split-libcontainerd branch 3 times, most recently from 7f07422 to 51dffbf Compare June 12, 2018 23:28

dmcgowan changed the title ~~[wip] libcontainerd: split client and daemon supervision~~ libcontainerd: split client and daemon supervision Jun 20, 2018

thaJeztah added rebuild/experimental labels Jun 21, 2018

GordonTheTurtle removed rebuild/experimental labels Jun 21, 2018

thaJeztah added area/runtime Runtime rebuild/z labels Jun 21, 2018

GordonTheTurtle removed the rebuild/z label Jun 21, 2018

thaJeztah requested changes Jul 10, 2018

View reviewed changes

dmcgowan force-pushed the split-libcontainerd branch from 51dffbf to 09b899a Compare July 18, 2018 18:45

thaJeztah added rebuild/janky labels Jul 23, 2018

GordonTheTurtle removed rebuild/janky labels Jul 23, 2018

thaJeztah approved these changes Jul 23, 2018

View reviewed changes

libcontainerd: split client and supervisor

dd2e19e

Adds a supervisor package for starting and monitoring containerd. Separates grpc connection allowing access from daemon. Signed-off-by: Derek McGowan <[email protected]>

dmcgowan force-pushed the split-libcontainerd branch from 09b899a to dd2e19e Compare August 6, 2018 17:23

kolyshkin reviewed Aug 16, 2018

View reviewed changes

thaJeztah merged commit 4d62192 into moby:master Aug 16, 2018

thaJeztah mentioned this pull request Aug 10, 2022

libcontainerd/supervisor: refactoring and cleanup in preparation of custom containerd.toml #43945

Merged

	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
	}

Conversation

dmcgowan commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented May 25, 2018

Uh oh!

cpuguy83 commented May 25, 2018

Uh oh!

dmcgowan commented May 25, 2018

Uh oh!

codecov bot commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Jun 20, 2018

Uh oh!

dmcgowan commented Jun 20, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jul 18, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jul 20, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin Aug 16, 2018

Choose a reason for hiding this comment

Uh oh!

dmcgowan Aug 16, 2018

Choose a reason for hiding this comment

Uh oh!

kolyshkin Aug 16, 2018

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Aug 16, 2018

Uh oh!

thaJeztah commented Aug 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dmcgowan commented May 25, 2018 •

edited

Loading

codecov bot commented Jun 6, 2018 •

edited

Loading