Add initial containerd client package by crosbymichael · Pull Request #904 · containerd/containerd

crosbymichael · 2017-05-24T23:07:31Z

This adds the initial containerd client package that consumers will use when working with containerd. It also adds a start to the integration tests for the daemon based on the client.

You can see a full, working example of the client here:

package main

import (
	"context"
	"fmt"
	"log"
	"syscall"
	"time"

	"github.com/containerd/containerd"
)

const (
	address = "/run/containerd/containerd.sock"
	redis   = "docker.io/library/redis:alpine"
)

func main() {
	if err := runRedis(); err != nil {
		log.Fatal(err)
	}
}

func runRedis() error {
	ctx := context.Background()

        // WithNamespace is a placeholder while we get namespace support in the daemon
	client, err := containerd.New(address, containerd.WithNamespace("scripts"))
	if err != nil {
		return err
	}
	defer client.Close()

	log.Printf("pulling image %s...\n", redis)
	// make sure we unpack the image for use after the pull
	image, err := client.Pull(ctx, redis, containerd.WithPullUnpack)
	if err != nil {
		return err
	}
	log.Println("finished pulling image...")

	spec, err := containerd.GenerateSpec(containerd.WithImage(ctx, image))
	if err != nil {
		return err
	}
	log.Println("generated spec from image...")

	container, err := client.NewContainer(ctx, "redis-master", spec, containerd.WithNewRootFS("redis-master", image))
	if err != nil {
		return err
	}
	defer container.Delete(ctx)
	log.Println("created new container...")

	// lets get the container's stdio in our current process
	task, err := container.NewTask(ctx, containerd.Stdio)
	if err != nil {
		return err
	}
	defer task.Delete(ctx)
	fmt.Printf("container pid %d\n", task.Pid())

	// you can do network namespace setup here or whatever you want without hooks

	if err := task.Start(ctx); err != nil {
		return err
	}

	// let it run for 5 seconds and then kill it
	go func() {
		time.Sleep(5 * time.Second)
		log.Println("killing task...")
		if err := task.Kill(ctx, syscall.SIGTERM); err != nil {
			log.Println(err)
		}
	}()

	status, err := task.Wait(ctx)
	if err != nil {
		return err
	}
	fmt.Printf("task exited with status %d\n", status)

	// all done
	return nil
}

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

dmcgowan · 2017-05-24T23:24:39Z

client.go

+	conn *grpc.ClientConn
+
+	Runtime   string
+	Namespace string


We should consider not having these exported. The With* pattern allows setting them and they should not be mutated by callers. Creating a new client to change these values seems reasonable.

codecov-io · 2017-05-24T23:24:57Z

Codecov Report

Merging #904 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #904   +/-   ##
=======================================
  Coverage   59.55%   59.55%           
=======================================
  Files           5        5           
  Lines         759      759           
=======================================
  Hits          452      452           
  Misses        199      199           
  Partials      108      108

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26183b3...cebe099. Read the comment docs.

dmcgowan · 2017-05-24T23:25:22Z

client.go

+type NewContainerOpts func(ctx context.Context, client *Client, c *containers.Container) error
+
+// WithContainerLables adds the provided labels to the container
+func WithContainerLables(labels map[string]string) NewContainerOpts {


Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

dmcgowan · 2017-05-24T23:28:09Z

client.go

+type PullOpts func(*Client, *PullContext) error
+
+type PullContext struct {
+	Resolver remotes.Resolver


Missing WithPullResolver

crosbymichael · 2017-05-24T23:28:30Z

FYI: Not all of the functionality is implemented yet. This is opened to get early feedback on the overall design for the client and then to have it merged in where we all can work on it together.

I also have the integration tests turned off at the moment while we figure out how to seed the content and run the daemon inside travis. I'll try to get integration tests running on travis by the end of this week.

Random-Liu · 2017-05-24T23:35:31Z

client.go

+		return nil, err
+	}
+	is := c.images()
+	if err := is.Put(ctx, name, desc); err != nil {


@stevvooe Should we Put image metadata before or after pulling the image or doesn't matter? I'm a bit confused now.

This should be before.

Random-Liu · 2017-05-24T23:36:05Z

client.go

+	}
+	if err := images.Dispatch(ctx, images.Handlers(handlers...), desc); err != nil {
+		return nil, err
+	}


Add TODO to wait for image pulling?

Yep, we will also be adding ways to hook into progress on the pull/push

Dispatch waits until all handlers are executed, so this effectively waits until the image is pulled.

Does CRI report status?

@stevvooe Nope. We only log it in dockershim now.

If there are 2 concurrent Dispatch pulling the same resource (say pulling 2 images sharing layers), one will return error immediately. So we may still want to wait and ignore the error.

See #897.

Alright, let me know what error is returned.

This error https://github.com/containerd/containerd/blob/8ae905b92b3246b27580b:19affb070ea165a59bf/content/locks.go#L31.

And previous discussion:
#885 (comment)

Ok, we should probably have an inprogress error and then detect that in the handler.

Random-Liu · 2017-05-24T23:51:31Z

client.go

+	return nil
+}
+
+type Unpacker interface {


Why only Unpacker is an interface?

It would help us write unit test if we could make all clients an interface. :)

You can always depend on a Client interface in your code. I think we talked about this some and @stevvooe may have some more context on this subject

@crosbymichael The problem is that, we expect Image and Container also to be an interface. It will be harder to wrap them all on our side, e.g. https://play.golang.org/p/JvlZhLONen will fail.

Random-Liu · 2017-05-24T23:53:25Z

spec_unix.go

+			parts := strings.Split(config.User, ":")
+			switch len(parts) {
+			case 1:
+				v, err := strconv.ParseUint(parts[0], 0, 10)


How do we handle user name and group name? We don't have a rootfs before creating the container.

I'll have to add some helpers for this because its complex when you have to parse a username

Random-Liu · 2017-05-24T23:55:21Z

image.go

+	client *Client
+
+	i images.Image
+}


It would be very helpful, if we could add some frequently used function here, GetManifest, GetConfig, GetSize etc.

Yep, will be adding those soon.

No getters in Go. Manifest, Config, Size.

Although, we should be very careful what we consider size. This will be the packed size.

@stevvooe Based on the discussion, would it help to introduce a Ready function? Or else Config or Manifest may return weird error during the image is being downloaded or unpacked.

AkihiroSuda · 2017-05-25T00:32:10Z

.gitignore

 /bin/
 **/coverage.txt
 **/coverage.out
+containerd.test


This output from compiling a test executable. go test -c, I believe.

AkihiroSuda · 2017-05-25T00:33:22Z

client_test.go

+
+func TestNewClient(t *testing.T) {
+	if testing.Short() {
+		return


AkihiroSuda · 2017-05-25T00:35:20Z

client_test.go

+)
+
+const defaultAddress = "/run/containerd/containerd.sock"
+


Does tester need to launch the daemon manually?

AkihiroSuda · 2017-05-25T00:38:47Z

I'd expect containerd.New to instantiate a daemon. How about containerd/client.New?

samuelkarp · 2017-05-25T00:31:25Z

task.go

+func (t *Task) Wait(ctx context.Context) (uint32, error) {
+	events, err := t.client.tasks().Events(ctx, &execution.EventsRequest{})
+	if err != nil {
+		return 255, err


I understand why this is 255, but it still looks like a magic number. Can you put this in a constant so it has a name? Something like failureExitStatus?

I'm more for UnknownExitStatus, but yes, probably better to have it defined (and exported)

Just put a comment:

// this number is magic return 255, err

mlaventure

I really like the principles, but I had a few comments obviously 😉

mlaventure · 2017-05-25T15:51:42Z

client_windows.go

+)
+
+func dialer(address string, timeout time.Duration) (net.Conn, error) {
+	return winio.DialPipe(bindAddress, &timeout)


s/bindAddress/address/

mlaventure · 2017-05-25T15:52:09Z

client_unix.go

+)
+
+func dialer(address string, timeout time.Duration) (net.Conn, error) {
+	address = strings.TrimPrefix(address, "unix://")


Validate that the prefix is actually present?

Why validate first? Just trim it. same reason you don't stat a file before opening, just open it

Fair enough

mlaventure · 2017-05-25T15:54:54Z

client.go

+
+// Client is the client to interact with containerd and its various services
+// using a uniform interface
+type Client struct {


I think I would prefer for Client to be an interface and have client internally as the type. Since the struct doesn't have any exported data, I don't think it would be an issue and it will definitely help when writing unit tests.

mlaventure · 2017-05-25T15:57:18Z

client.go

+
+// New returns a new containerd client that is connected to the containerd
+// instance provided by address
+func New(address string, opts ...NewClientOpts) (*Client, error) {


Take a context here and use grc.DialContext()

mlaventure · 2017-05-25T15:57:54Z

client.go

+func New(address string, opts ...NewClientOpts) (*Client, error) {
+	gopts := []grpc.DialOption{
+		grpc.WithInsecure(),
+		grpc.WithTimeout(100 * time.Second),


Add a WithTimeout NewClientOpts

mlaventure · 2017-05-25T16:22:47Z

container.go

+		Stderr:      i.Stderr,
+	}
+	// get the rootfs from the snapshotter and add it to the request
+	mounts, err := c.client.snapshotter().Mounts(ctx, c.c.RootFS)


Use of the snapshotter should be optional,. If c.c.RootFS is not set, this should just be skipped

mlaventure · 2017-05-25T16:24:42Z

spec.go

+	}
+}
+
+func WithArgs(args ...string) SpecOpts {


WithProcessArgs ?

mlaventure · 2017-05-25T16:28:57Z

task.go

+	return nil
+}
+
+type Task struct {


idem, make it an interface.

Easier to mock it in test that way

mlaventure · 2017-05-25T16:29:35Z

task.go

+func (t *Task) Wait(ctx context.Context) (uint32, error) {
+	events, err := t.client.tasks().Events(ctx, &execution.EventsRequest{})
+	if err != nil {
+		return 255, err


I'm more for UnknownExitStatus, but yes, probably better to have it defined (and exported)

mlaventure · 2017-05-25T16:30:29Z

spec_unix.go

@@ -0,0 +1,252 @@
+package containerd


cough, cough

windows? 😇

spec_linux.go?

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

stevvooe · 2017-05-25T18:24:18Z

client_test.go

+	"testing"
+)
+
+const defaultAddress = "/run/containerd/containerd.sock"


Maybe a test flag here?

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

crosbymichael · 2017-05-25T20:10:49Z

This should be good for another round of review. We can get it merged in and continue filling out the missing functionality and getting integration tests running in a separate PR for travis.

mlaventure · 2017-05-25T20:46:03Z

client.go

+
+// NewContainer will create a new container in container with the provided id
+// the id must be unique within the namespace
+func (c *Client) NewContainer(ctx context.Context, id string, spec *specs.Spec, opts ...NewContainerOpts) (Container, error) {


To fully support runtimes that need more than the OCI spec (e.g. windows atm), we're either going to have to introduce an interface for the spec, or have the spec specified as an option (i.e. WithSpec)

@mlaventure We could wrap it in an interface and then type assert?

I actually think the WithSpec would do just fine and nicely fit into the current definition, wdyt?

We really need a way to test windows stuff because its just guessing for most of us

AkihiroSuda · 2017-05-25T21:25:42Z

client_test.go

+func TestNewClient(t *testing.T) {
+	if testing.Short() {
+		t.Skip()
+		return


No need to return, t.Skip calls os.Exit

AkihiroSuda · 2017-05-25T21:26:04Z

client_test.go

+	_, err = client.Pull(context.Background(), ref)
+	if err != nil {
+		t.Error(err)
+		return


No, I want the defers to run

AkihiroSuda · 2017-05-25T21:27:06Z

spec_unix.go

@@ -0,0 +1,252 @@
+package containerd


spec_linux.go?

stevvooe · 2017-05-25T21:57:37Z

container.go

+	Delete(context.Context) error
+	NewTask(context.Context, IOCreation) (Task, error)
+	Spec() (*specs.Spec, error)
+	Task() Task


How can we add labels to this container?

NewContainer(WithLabels)

What if I want to change the labels on an existing container?

I don't have an update method yet cuz this is the initial client PR

stevvooe · 2017-05-25T22:02:45Z

task.go

+	Resume(context.Context) error
+	Pid() uint32
+	Start(context.Context) error
+	Status(context.Context) (string, error)


Would it be possible to have a type for status? This is hard to discover in docker today because it is just a string field.

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

dmcgowan · 2017-05-25T22:49:42Z

LGTM

stevvooe · 2017-05-25T23:09:03Z

LGTM

crosbymichael added 7 commits May 24, 2017 14:08

Add initial containerd *Client

d0e5732

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Add NewContainer and Delete container support

31a3bda

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Add NewTask support to client

3ba06e4

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Add pull support to client

9232360

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Fix spec generation for task execution

bf9ad0c

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Add WithNamespace for client

01c4c86

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Add test -short for non-integration tests

8cd882c

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

crosbymichael force-pushed the client branch from 591fbee to 8cd882c Compare May 24, 2017 23:14

dmcgowan reviewed May 24, 2017

View reviewed changes

Unexport client fields

e2db289

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

dmcgowan reviewed May 24, 2017

View reviewed changes

Random-Liu reviewed May 24, 2017

View reviewed changes

AkihiroSuda reviewed May 25, 2017

View reviewed changes

samuelkarp reviewed May 25, 2017

View reviewed changes

This was referenced May 25, 2017

Use containerd client. containerd/cri#49

Closed

We need fake containerd services. containerd/cri#20

Closed

mlaventure reviewed May 25, 2017

View reviewed changes

crosbymichael added 4 commits May 25, 2017 10:15

Add pause resume to task

1de25c0

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Make Task, Container, Image interface types

608e6da

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Export GRPC services from client

a2b0824

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Don't require rootfs if not set on container

d0b2229

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

stevvooe reviewed May 25, 2017

View reviewed changes

client_test.go Outdated

"testing"

)

const defaultAddress = "/run/containerd/containerd.sock"

Copy link
Copy Markdown

Member

stevvooe May 25, 2017

Choose a reason for hiding this comment

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

Maybe a test flag here?

Add windows spec generation

8903756

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

stevvooe mentioned this pull request May 25, 2017

implement direct containerd Executor. moby/swarmkit#1965

Merged

mlaventure reviewed May 25, 2017

View reviewed changes

AkihiroSuda reviewed May 25, 2017

View reviewed changes

stevvooe reviewed May 25, 2017

View reviewed changes

crosbymichael force-pushed the client branch from 627099a to 5e637f7 Compare May 25, 2017 22:39

Add test flag for setting containerd address

cebe099

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

crosbymichael force-pushed the client branch from 5e637f7 to cebe099 Compare May 25, 2017 22:41

dmcgowan merged commit 199544e into containerd:master May 25, 2017

crosbymichael deleted the client branch May 25, 2017 23:22

Conversation

crosbymichael commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

crosbymichael commented May 24, 2017 •

edited

Loading

codecov-io commented May 24, 2017 •

edited

Loading

dmcgowan May 24, 2017 •

edited

Loading

crosbymichael commented May 24, 2017 •

edited

Loading

Random-Liu May 25, 2017 •

edited

Loading

Random-Liu May 25, 2017 •

edited

Loading

Random-Liu May 25, 2017 •

edited

Loading

Random-Liu May 24, 2017 •

edited

Loading

Random-Liu May 25, 2017 •

edited

Loading