From 93e2a88ce8d7481e9c5781764da58e173e595cc8 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Thu, 9 Nov 2017 18:29:34 -0800 Subject: [PATCH 1/2] Fail on no entrypoint, don't crash --- executor/mock/standalone/standalone_test.go | 16 +++++++++ executor/runtime/docker.go | 38 +++++++++++++++++---- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/executor/mock/standalone/standalone_test.go b/executor/mock/standalone/standalone_test.go index 6040af6e0..725dd3ef0 100644 --- a/executor/mock/standalone/standalone_test.go +++ b/executor/mock/standalone/standalone_test.go @@ -56,6 +56,10 @@ var ( name: "titusoss/big-image", tag: "20171025-1508900976", } + noEntrypoint = testImage{ + name: "titusoss/no-entrypoint-test", + tag: "20171109-1510275133", + } ) // This file still uses log as opposed to using the testing library's built-in logging framework. @@ -79,6 +83,7 @@ func TestStandalone(t *testing.T) { testImageNonExistingDigestFails, testImagePullError, testBadEntrypoint, + testNoEntrypoint, testCanWriteInLogsAndSubDirs, testShutdown, testCancelPullBigImage, @@ -478,6 +483,17 @@ func testBadEntrypoint(t *testing.T) { } } +func testNoEntrypoint(t *testing.T) { + ji := &mock.JobInput{ + ImageName: noEntrypoint.name, + Version: noEntrypoint.tag, + } + // We expect this to fail + if mock.RunJobExpectingSuccess(ji, false) { + t.Fail() + } +} + func testCanWriteInLogsAndSubDirs(t *testing.T) { cmd := `sh -c "mkdir -p /logs/prana && echo begining > /logs/prana/prana.log && ` + `mv /logs/prana/prana.log /logs/prana/prana-2016.log && echo ending >> /logs/out"` diff --git a/executor/runtime/docker.go b/executor/runtime/docker.go index 30929f6d8..59224f96f 100644 --- a/executor/runtime/docker.go +++ b/executor/runtime/docker.go @@ -97,6 +97,9 @@ var ( // and it's a temporary measure during protocol evolution. var ErrMissingIAMRole = errors.New("IAM Role Missing") +// NoEntrypointError indicates that the Titus job does not have an entrypoint, or command +var NoEntrypointError = &BadEntryPointError{reason: errors.New("Image, and job have no entrypoint, or command")} + // I'm sorry for using regex, it's a simple rule though // 1. The string must start with a-z, A-Z, or _ // 2. The string MAY contain more characters, in the set a-z, A-Z, 0-9, or _ @@ -629,6 +632,7 @@ func (r *DockerRuntime) Prepare(parentCtx context.Context, c *Container, binds [ var dockerCfg *container.Config var hostCfg *container.HostConfig var size int64 + var imageInfo types.ImageInspect err := r.validateEFSMounts(c) if err != nil { @@ -640,7 +644,18 @@ func (r *DockerRuntime) Prepare(parentCtx context.Context, c *Container, binds [ goto error } - size = r.reportDockerImageSizeMetric(c, c.QualifiedImageName()) + imageInfo, _, err = r.client.ImageInspectWithRaw(ctx, c.QualifiedImageName()) + if err != nil { + log.Errorf("Error in inspecting docker image %s: %v", c.QualifiedImageName(), err) + return err + } + size = r.reportDockerImageSizeMetric(c, imageInfo, c.QualifiedImageName()) + + // Check if this image (container) has a an entrypoint, or if we + // were passed one + if !r.hasEntrypoint(imageInfo, c) { + return NoEntrypointError + } dockerCfg, hostCfg, err = r.dockerConfig(c, binds, size) if err != nil { @@ -1471,17 +1486,26 @@ func (r *DockerRuntime) Cleanup(c *Container) error { } // reportDockerImageSizeMetric reports a metric that represents the container image's size -func (r *DockerRuntime) reportDockerImageSizeMetric(c *Container, image string) int64 { - imageInfo, _, err := r.client.ImageInspectWithRaw(context.TODO(), image) - if err != nil { - log.Errorf("Error in inspecting docker image %s : %v\n", image, err) - return 0 - } +func (r *DockerRuntime) reportDockerImageSizeMetric(c *Container, imageInfo types.ImageInspect, image string) int64 { // reporting image size in KB r.metrics.Gauge("titus.executor.dockerImageSize", int(imageInfo.Size/KB), c.ImageTagForMetrics()) return imageInfo.Size } +func (r *DockerRuntime) hasEntrypoint(imageInfo types.ImageInspect, c *Container) bool { + if entrypoint, err := GetEntrypointFromProto(c.TitusInfo); err != nil { + // If this happens, we return true, because this error will be bubbled up elsewhere + return true + } else if entrypoint != nil { + return true + } + + if imageInfo.Config.Entrypoint != nil || imageInfo.Config.Cmd != nil { + return true + } + return false +} + func shouldClose(c io.Closer) { if err := c.Close(); err != nil { log.Error("Could not close: ", err) From cf5109526473a0b494a0fe2100b45068629760c9 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Mon, 13 Nov 2017 09:30:32 -0800 Subject: [PATCH 2/2] Small refactor --- executor/runtime/docker.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/executor/runtime/docker.go b/executor/runtime/docker.go index 59224f96f..ffafa2e0f 100644 --- a/executor/runtime/docker.go +++ b/executor/runtime/docker.go @@ -649,7 +649,7 @@ func (r *DockerRuntime) Prepare(parentCtx context.Context, c *Container, binds [ log.Errorf("Error in inspecting docker image %s: %v", c.QualifiedImageName(), err) return err } - size = r.reportDockerImageSizeMetric(c, imageInfo, c.QualifiedImageName()) + size = r.reportDockerImageSizeMetric(c, imageInfo) // Check if this image (container) has a an entrypoint, or if we // were passed one @@ -1486,7 +1486,7 @@ func (r *DockerRuntime) Cleanup(c *Container) error { } // reportDockerImageSizeMetric reports a metric that represents the container image's size -func (r *DockerRuntime) reportDockerImageSizeMetric(c *Container, imageInfo types.ImageInspect, image string) int64 { +func (r *DockerRuntime) reportDockerImageSizeMetric(c *Container, imageInfo types.ImageInspect) int64 { // reporting image size in KB r.metrics.Gauge("titus.executor.dockerImageSize", int(imageInfo.Size/KB), c.ImageTagForMetrics()) return imageInfo.Size @@ -1500,10 +1500,7 @@ func (r *DockerRuntime) hasEntrypoint(imageInfo types.ImageInspect, c *Container return true } - if imageInfo.Config.Entrypoint != nil || imageInfo.Config.Cmd != nil { - return true - } - return false + return imageInfo.Config.Entrypoint != nil || imageInfo.Config.Cmd != nil } func shouldClose(c io.Closer) {