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

libcontainerd: split client and daemon supervision #37149

Merged
merged 1 commit into from Aug 16, 2018

Conversation

Projects
None yet
7 participants
@dmcgowan
Copy link
Member

dmcgowan commented May 25, 2018

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

@dmcgowan dmcgowan requested review from cpuguy83 and tianon as code owners May 25, 2018

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

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 25, 2018

Build failing due to conflicts.

@vdemeester vdemeester referenced this pull request May 25, 2018

Closed

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

0 of 5 tasks complete
@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 25, 2018

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

@dmcgowan

This comment has been minimized.

Copy link
Member Author

dmcgowan commented May 25, 2018

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.

@dmcgowan dmcgowan force-pushed the dmcgowan:split-libcontainerd branch 2 times, most recently from 955f0cc to ae24d4c Jun 6, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jun 6, 2018

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

@dmcgowan dmcgowan force-pushed the dmcgowan:split-libcontainerd branch from ae24d4c to 0abacdb Jun 6, 2018

@dmcgowan dmcgowan force-pushed the dmcgowan:split-libcontainerd branch 3 times, most recently from 7f07422 to 51dffbf Jun 11, 2018

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jun 20, 2018

@dmcgowan this still WIP?

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

@dmcgowan

This comment has been minimized.

Copy link
Member Author

dmcgowan commented Jun 20, 2018

Removing wip, janky is at least passing now

@thaJeztah
Copy link
Member

thaJeztah left a comment

left some questions

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

@@ -111,6 +110,10 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
return err
}

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

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jul 10, 2018

Member

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
}

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
}

This comment has been minimized.

Copy link
@dmcgowan

dmcgowan Jul 18, 2018

Author Member

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.

opts = append(opts, pOpts...)

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

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jul 10, 2018

Member

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

This comment has been minimized.

Copy link
@dmcgowan

dmcgowan Jul 20, 2018

Author Member

Looks like you addressed this based on the last rebase

@thaJeztah
Copy link
Member

thaJeztah left a comment

LGTM, thanks!

libcontainerd: split client and supervisor
Adds a supervisor package for starting and monitoring containerd.
Separates grpc connection allowing access from daemon.

Signed-off-by: Derek McGowan <derek@mcgstyle.net>

@dmcgowan dmcgowan force-pushed the dmcgowan:split-libcontainerd branch from 09b899a to dd2e19e Aug 6, 2018

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

This comment has been minimized.

Copy link
@kolyshkin

kolyshkin Aug 16, 2018

Contributor

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 comment has been minimized.

Copy link
@dmcgowan

dmcgowan Aug 16, 2018

Author Member

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 err
r, err := supervisor.Start(ctx, filepath.Join(cli.Config.Root, "containerd"), filepath.Join(cli.Config.ExecRoot, "containerd"), opts...)
if err != nil {
cancel()

This comment has been minimized.

Copy link
@kolyshkin

kolyshkin Aug 16, 2018

Contributor

ditto

@kolyshkin

This comment has been minimized.

Copy link
Contributor

kolyshkin commented Aug 16, 2018

LGTM except for a tiny nitpick with cancel()

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Aug 16, 2018

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

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

7 of 8 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
codecov/project No report found to compare against
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 41540 has succeeded
Details
janky Jenkins build Docker-PRs 50309 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 10734 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 21624 has succeeded
Details
z Jenkins build Docker-PRs-s390x 10606 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.