fn: tests for hung and bad docker repo during docker-pull (#1298)

* fn: tests for hung and bad docker repo during docker-pull

api/agent/agent.go

@@ -227,6 +227,7 @@ func NewDockerDriver(cfg *Config) (drivers.Driver, error) {
 		MaxTmpFsInodes:       cfg.MaxTmpFsInodes,
 		EnableReadOnlyRootFs: !cfg.DisableReadOnlyRootFs,
 		EnableTini:           !cfg.DisableTini,
+		MaxRetries:           cfg.MaxDockerRetries,
 	})
 }
 

api/agent/agent_test.go

@@ -436,6 +436,110 @@ type dummyReader struct {
 	io.Reader
 }
 
+func TestDockerPullHungRepo(t *testing.T) {
+	hung, cancel := context.WithCancel(context.Background())
+	garbageServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// version check seem to have a sane timeout in docker, let's serve this, then stop
+		if r.URL.String() == "/v2/" {
+			w.WriteHeader(200)
+			return
+		}
+		<-hung.Done()
+	}))
+	defer garbageServer.Close()
+	defer cancel()
+
+	dest := strings.TrimPrefix(garbageServer.URL, "http://")
+
+	app := &models.App{ID: "app_id"}
+	fn := &models.Fn{
+		ID:    "fn_id",
+		Image: dest + "/fnproject/fn-test-utils",
+		ResourceConfig: models.ResourceConfig{
+			Timeout:     5,
+			IdleTimeout: 10,
+			Memory:      128,
+		},
+	}
+
+	url := "http://127.0.0.1:8080/invoke/" + fn.ID
+
+	ls := logs.NewMock()
+	cfg, err := NewConfig()
+	cfg.MaxDockerRetries = 1
+	cfg.HotStartTimeout = time.Duration(5) * time.Second
+	a := New(NewDirectCallDataAccess(ls, new(mqs.Mock)), WithConfig(cfg))
+	defer checkClose(t, a)
+
+	req, err := http.NewRequest("GET", url, &dummyReader{Reader: strings.NewReader(`{}`)})
+	if err != nil {
+		t.Fatal("unexpected error building request", err)
+	}
+
+	var out bytes.Buffer
+	callI, err := a.GetCall(FromHTTPFnRequest(app, fn, req), WithWriter(&out))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = a.Submit(callI)
+	if err == nil {
+		t.Fatal("submit should error!")
+	}
+	errS := err.Error()
+	if !strings.HasPrefix(errS, "Failed to pull image ") || !strings.Contains(errS, "context deadline exceeded") {
+		t.Fatalf("unexpected error %v", err)
+	}
+}
+
+func TestDockerPullBadRepo(t *testing.T) {
+
+	garbageServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprintln(w, "enjoy this lovely garbage")
+	}))
+	defer garbageServer.Close()
+
+	dest := strings.TrimPrefix(garbageServer.URL, "http://")
+
+	app := &models.App{ID: "app_id"}
+	fn := &models.Fn{
+		ID:    "fn_id",
+		Image: dest + "/fnproject/fn-test-utils",
+		ResourceConfig: models.ResourceConfig{
+			Timeout:     5,
+			IdleTimeout: 10,
+			Memory:      128,
+		},
+	}
+
+	url := "http://127.0.0.1:8080/invoke/" + fn.ID
+
+	ls := logs.NewMock()
+	cfg, err := NewConfig()
+	cfg.MaxDockerRetries = 1
+	a := New(NewDirectCallDataAccess(ls, new(mqs.Mock)), WithConfig(cfg))
+	defer checkClose(t, a)
+
+	req, err := http.NewRequest("GET", url, &dummyReader{Reader: strings.NewReader(`{}`)})
+	if err != nil {
+		t.Fatal("unexpected error building request", err)
+	}
+
+	var out bytes.Buffer
+	callI, err := a.GetCall(FromHTTPFnRequest(app, fn, req), WithWriter(&out))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = a.Submit(callI)
+	if err == nil {
+		t.Fatal("submit should error!")
+	}
+	if !strings.HasPrefix(err.Error(), "Failed to pull image ") {
+		t.Fatalf("unexpected error %v", err)
+	}
+}
+
 func TestHTTPWithoutContentLengthWorks(t *testing.T) {
 	// TODO it may be a good idea to mock out the http server and use a real
 	// response writer with sync, and also test that this works with async + log

api/agent/config.go

@@ -37,6 +37,7 @@ type Config struct {
 	IOFSAgentPath           string        `json:"iofs_path"`
 	IOFSMountRoot           string        `json:"iofs_mount_root"`
 	IOFSOpts                string        `json:"iofs_opts"`
+	MaxDockerRetries        uint64        `json:"max_docker_retries"`
 }
 
 const (

api/agent/drivers/docker/cookie.go

@@ -24,6 +24,9 @@ type cookie struct {
 	task drivers.ContainerTask
 	// pointer to docker driver
 	drv *DockerDriver
+
+	// do we need to remove container at exit?
+	isCreated bool
 }
 
 func (c *cookie) configureLogger(log logrus.FieldLogger) {
@@ -199,7 +202,10 @@ func (c *cookie) configureEnv(log logrus.FieldLogger) {
 
 // implements Cookie
 func (c *cookie) Close(ctx context.Context) error {
-	err := c.drv.removeContainer(ctx, c.task.Id())
+	var err error
+	if c.isCreated {
+		err = c.drv.removeContainer(ctx, c.task.Id())
+	}
 	c.drv.unpickPool(c)
 	c.drv.unpickNetwork(c)
 	return err

api/agent/drivers/docker/docker.go

@@ -80,7 +80,7 @@ func NewDocker(conf drivers.Config) *DockerDriver {
 	driver := &DockerDriver{
 		cancel:   cancel,
 		conf:     conf,
-		docker:   newClient(ctx),
+		docker:   newClient(ctx, conf.MaxRetries),
 		hostname: hostname,
 		auths:    auths,
 	}
@@ -275,6 +275,11 @@ func (drv *DockerDriver) PrepareCookie(ctx context.Context, c drivers.Cookie) er
 		return err
 	}
 
+	// here let's assume we have created container, logically this should be after 'CreateContainer', but we
+	// are not 100% sure that *any* failure to CreateContainer does not ever leave a container around especially
+	// going through fsouza+docker-api.
+	cookie.isCreated = true
+
 	cookie.opts.Context = ctx
 	_, err = drv.docker.CreateContainer(cookie.opts)
 	if err != nil {

api/agent/drivers/docker/docker_client.go

@@ -47,7 +47,7 @@ type dockerClient interface {
 }
 
 // TODO: switch to github.com/docker/engine-api
-func newClient(ctx context.Context) dockerClient {
+func newClient(ctx context.Context, maxRetries uint64) dockerClient {
 	// TODO this was much easier, don't need special settings at the moment
 	// docker, err := docker.NewClient(conf.Docker)
 	client, err := docker.NewClientFromEnv()
@@ -59,12 +59,18 @@ func newClient(ctx context.Context) dockerClient {
 		logrus.WithError(err).Fatal("couldn't connect to docker daemon")
 	}
 
+	// punch in default if not set
+	if maxRetries == 0 {
+		maxRetries = 10
+	}
+
 	go listenEventLoop(ctx, client)
-	return &dockerWrap{client}
+	return &dockerWrap{docker: client, maxRetries: maxRetries}
 }
 
 type dockerWrap struct {
 	docker     *docker.Client
+	maxRetries uint64
 }
 
 var (
@@ -210,13 +216,13 @@ func RegisterViews(tagKeys []string, latencyDist []float64) {
 }
 
 func (d *dockerWrap) retry(ctx context.Context, logger logrus.FieldLogger, f func() error) error {
-	var i int
+	var i uint64
 	var err error
 	defer func() { stats.Record(ctx, dockerRetriesMeasure.M(int64(i))) }()
 
 	var b common.Backoff
 	// 10 retries w/o change to backoff is ~13s if ops take ~0 time
-	for ; i < 10; i++ {
+	for ; i < d.maxRetries; i++ {
 		select {
 		case <-ctx.Done():
 			stats.Record(ctx, dockerTimeoutMeasure.M(0))

api/agent/drivers/driver.go

@@ -252,6 +252,7 @@ type Config struct {
 	MaxTmpFsInodes       uint64 `json:"max_tmpfs_inodes"`
 	EnableReadOnlyRootFs bool   `json:"enable_readonly_rootfs"`
 	EnableTini           bool   `json:"enable_tini"`
+	MaxRetries           uint64 `json:"max_retries"`
 }
 
 func average(samples []Stat) (Stat, bool) {