Don't call ImageInspectWithRaw() twice if we don't have to

executor/mock/standalone/standalone_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/Netflix/titus-executor/executor/runner"
 	"github.com/Netflix/titus-executor/executor/runtime/docker"
 	runtimeTypes "github.com/Netflix/titus-executor/executor/runtime/types"
+	dockerTypes "github.com/docker/docker/api/types"
 	protobuf "github.com/golang/protobuf/proto"
 	"github.com/mesos/mesos-go/mesosproto"
 	"github.com/pborman/uuid"
@@ -182,7 +183,7 @@ func dockerImageRemove(t *testing.T, imgName string) {
 	require.NoErrorf(t, err, "No error removing docker image %s: +%v", imgName, err)
 }
 
-func dockerPull(t *testing.T, imgName string, imgDigest string) (bool, error) {
+func dockerPull(t *testing.T, imgName string, imgDigest string) (*dockerTypes.ImageInspect, error) {
 	cfg, dockerCfg := mock.GenerateConfigs()
 
 	ctx, cancel := context.WithCancel(context.Background())
@@ -923,14 +924,18 @@ func testTtyNegative(t *testing.T, jobID string) {
 }
 
 func testCachedDockerPull(t *testing.T, jobID string) {
+	// The no entrypoint image should never be in use by any running
+	// containers, so it should be safe to delete
 	dockerImageRemove(t, noEntrypoint.name+"@"+noEntrypoint.digest)
 	res, err := dockerPull(t, noEntrypoint.name, noEntrypoint.digest)
 	require.NoError(t, err, "No error from first docker pull")
 
-	assert.False(t, res, "image shouldn't be cached")
+	assert.Nil(t, res, "image shouldn't be cached")
 
 	res, err = dockerPull(t, noEntrypoint.name, noEntrypoint.digest)
 	require.NoError(t, err, "No error from second docker pull")
 
-	assert.True(t, res, "image should now be cached")
+	assert.NotNil(t, res, "image should now be cached")
+	assert.Len(t, res.RepoDigests, 1, "digest should be present")
+	assert.EqualValues(t, noEntrypoint.name+"@"+noEntrypoint.digest, res.RepoDigests[0], "Correct digest should be returned")
 }

executor/runtime/docker/docker.go

@@ -558,18 +558,18 @@ func sleepWithCtx(parentCtx context.Context, d time.Duration) error {
 	}
 }
 
-func imageExists(ctx context.Context, client *docker.Client, ref string) (bool, error) {
+func imageExists(ctx context.Context, client *docker.Client, ref string) (*types.ImageInspect, error) {
 	resp, _, err := client.ImageInspectWithRaw(ctx, ref)
 	if err != nil {
 		if docker.IsErrImageNotFound(err) {
-			return false, nil
+			return nil, nil
 		}
 
-		return false, err
+		return nil, err
 	}
 
 	log.WithField("imageName", ref).Debugf("Image exists. Response: %+v", resp)
-	return true, nil
+	return &resp, nil
 }
 
 // DockerImageRemove removes an image from the docker host
@@ -583,28 +583,28 @@ func (r *DockerRuntime) DockerImageRemove(ctx context.Context, imgName string) e
 }
 
 // DockerPull returns true if the image was cached, and we didn't need to pull
-func (r *DockerRuntime) DockerPull(ctx context.Context, c *runtimeTypes.Container) (bool, error) {
+func (r *DockerRuntime) DockerPull(ctx context.Context, c *runtimeTypes.Container) (*types.ImageInspect, error) {
 	imgName := c.QualifiedImageName()
 	logger := log.WithField("imageName", imgName)
 
 	if c.ImageHasDigest() {
 		// Only check for a cached image if a digest was specified: image tags are mutable
-		imgExists, err := imageExists(ctx, r.client, imgName)
+		imgInfo, err := imageExists(ctx, r.client, imgName)
 		if err != nil {
 			logger.WithError(err).Errorf("DockerPull: error inspecting image")
-			return false, err
+			return nil, err
 		}
 
-		if imgExists {
-			log.Info("DockerPull: not pulling image")
-			r.metrics.Counter("titus.executor.imageCachedPulls", 1, nil)
-			return true, nil
+		if imgInfo != nil {
+			log.Info("DockerPull: image exists: not pulling image")
+			r.metrics.Counter("titus.executor.dockerImageCachedPulls", 1, nil)
+			return imgInfo, nil
 		}
 	}
 
-	r.metrics.Counter("titus.executor.imagePulls", 1, nil)
+	r.metrics.Counter("titus.executor.dockerImagePulls", 1, nil)
 	log.Infof("DockerPull: pulling image")
-	return false, pullWithRetries(ctx, r.metrics, r.client, c, doDockerPull)
+	return nil, pullWithRetries(ctx, r.metrics, r.client, c, doDockerPull)
 }
 
 type dockerPuller func(context.Context, metrics.Reporter, *docker.Client, string) error
@@ -834,22 +834,26 @@ func (r *DockerRuntime) Prepare(parentCtx context.Context, c *runtimeTypes.Conta
 	}
 
 	group.Go(func() error {
-		if _, pullErr := r.DockerPull(errGroupCtx, c); pullErr != nil {
+		imageInfo, pullErr := r.DockerPull(errGroupCtx, c)
+		if pullErr != nil {
 			return pullErr
 		}
 
-		imageInfo, _, inspectErr := r.client.ImageInspectWithRaw(ctx, c.QualifiedImageName())
-		if inspectErr != nil {
-			l.WithField("imageName", c.QualifiedImageName()).WithError(inspectErr).Errorf("Error inspecting docker image")
-			return inspectErr
+		if imageInfo == nil {
+			inspected, _, inspectErr := r.client.ImageInspectWithRaw(ctx, c.QualifiedImageName())
+			if inspectErr != nil {
+				l.WithField("imageName", c.QualifiedImageName()).WithError(inspectErr).Errorf("Error inspecting docker image")
+				return inspectErr
+			}
+			imageInfo = &inspected
 		}
 
 		size = r.reportDockerImageSizeMetric(c, imageInfo)
 		if !r.hasEntrypointOrCmd(imageInfo, c) {
 			return NoEntrypointError
 		}
 
-		myImageInfo = &imageInfo
+		myImageInfo = imageInfo
 		return nil
 	})
 
@@ -1901,14 +1905,14 @@ func (r *DockerRuntime) Cleanup(c *runtimeTypes.Container) error {
 }
 
 // reportDockerImageSizeMetric reports a metric that represents the container image's size
-func (r *DockerRuntime) reportDockerImageSizeMetric(c *runtimeTypes.Container, imageInfo types.ImageInspect) int64 {
+func (r *DockerRuntime) reportDockerImageSizeMetric(c *runtimeTypes.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
 }
 
 // hasEntrypointOrCmd checks if the image has a an entrypoint, or if we were passed one
-func (r *DockerRuntime) hasEntrypointOrCmd(imageInfo types.ImageInspect, c *runtimeTypes.Container) bool {
+func (r *DockerRuntime) hasEntrypointOrCmd(imageInfo *types.ImageInspect, c *runtimeTypes.Container) bool {
 	entrypoint, cmd, err := c.Process()
 	if err != nil {
 		// If this happens, we return true, because this error will bubble up elsewhere