Merge branch '10-5-stable-prepare-rc2' into 10-5-stable

.codeclimate.yml

@@ -36,3 +36,4 @@ plugins:
     enabled: false
 exclude_patterns:
 - vendor/
+- .gopath/

CHANGELOG.md

+v 10.5.0-rc2 (2018-02-15)
+- Always prefer creating new containers when running with Docker Executor !818
+- Improve output of /debug/jobs/list !826
+- Fix panic running docker package tests !828
+
 v 10.5.0-rc1 (2018-02-08)
 - Fix git 1.7.1 compatibility in executors/shell package tests !791
 - Always load OS certificate pool when evaluating TLS connections !804

commands/builds_helper.go

@@ -3,7 +3,6 @@ package commands
 import (
 	"fmt"
 	"net/http"
-	"strings"
 	"sync"
 
 	"gitlab.com/gitlab-org/gitlab-runner/common"
@@ -209,15 +208,12 @@ func (b *buildsHelper) Collect(ch chan<- prometheus.Metric) {
 func (b *buildsHelper) ListJobsHandler(w http.ResponseWriter, r *http.Request) {
 	w.Header().Add("Content-Type", "text/plain")
 
-	var jobs []string
 	for _, job := range b.builds {
-		jobDescription := fmt.Sprintf(
-			"id=%d url=%s state=%s stage=%s executor_stage=%s",
+		fmt.Fprintf(
+			w,
+			"id=%d url=%s state=%s stage=%s executor_stage=%s\n",
 			job.ID, job.RepoCleanURL(),
 			job.CurrentState, job.CurrentStage, job.CurrentExecutorStage(),
 		)
-		jobs = append(jobs, jobDescription)
 	}
-
-	w.Write([]byte(strings.Join(jobs, "\n")))
 }

common/build.go

@@ -353,7 +353,10 @@ func (b *Build) Run(globalConfig *Config, trace JobTrace) (err error) {
 	var executor Executor
 
 	b.logger = NewBuildLogger(trace, b.Log())
-	b.logger.Println(fmt.Sprintf("Running with %s\n  on %s (%s)", AppVersion.Line(), b.Runner.Name, b.Runner.ShortDescription()))
+	b.logger.Println("Running with", AppVersion.Line())
+	if b.Runner != nil && b.Runner.ShortDescription() != "" {
+		b.logger.Println("  on", b.Runner.Name, b.Runner.ShortDescription())
+	}
 
 	b.CurrentState = BuildRunStatePending
 

common/support.go

@@ -42,6 +42,18 @@ func GetRemoteSuccessfulBuild() (JobResponse, error) {
 	return getRemoteBuildResponse("echo Hello World")
 }
 
+func GetRemoteSuccessfulBuildWithAfterScript() (JobResponse, error) {
+	jobResponse, err := getRemoteBuildResponse("echo Hello World")
+	jobResponse.Steps = append(jobResponse.Steps,
+		Step{
+			Name:   StepNameAfterScript,
+			Script: []string{"echo Hello World"},
+			When:   StepWhenAlways,
+		},
+	)
+	return jobResponse, err
+}
+
 func GetRemoteSuccessfulBuildWithDumpedVariables() (response JobResponse, err error) {
 	variableName := "test_dump"
 	variableValue := "test"

executors/docker/executor_docker.go

@@ -2,6 +2,7 @@ package docker
 
 import (
 	"bytes"
+	"context"
 	"crypto/md5"
 	"errors"
 	"fmt"
@@ -25,8 +26,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab-runner/executors"
 	"gitlab.com/gitlab-org/gitlab-runner/helpers"
 	docker_helpers "gitlab.com/gitlab-org/gitlab-runner/helpers/docker"
-
-	"golang.org/x/net/context"
 )
 
 const (
@@ -44,16 +43,22 @@ var neverRestartPolicy = container.RestartPolicy{Name: "no"}
 
 type executor struct {
 	executors.AbstractExecutor
-	client      docker_helpers.Client
-	failures    []string // IDs of containers that have failed in some way
-	builds      []string // IDs of successfully created build containers
-	services    []*types.Container
-	caches      []string // IDs of cache containers
-	info        types.Info
-	binds       []string
-	volumesFrom []string
-	devices     []container.DeviceMapping
-	links       []string
+	client docker_helpers.Client
+	info   types.Info
+
+	temporary []string // IDs of containers that should be removed
+
+	builds   []string // IDs of successfully created build containers
+	services []*types.Container
+	caches   []string // IDs of cache containers
+
+	binds []string
+	links []string
+
+	devices []container.DeviceMapping
+
+	usedImages     map[string]string
+	usedImagesLock sync.RWMutex
 }
 
 func (s *executor) getServiceVariables() []string {
@@ -154,7 +159,7 @@ func (s *executor) pullDockerImage(imageName string, ac *types.AuthConfig) (*typ
 	return &image, err
 }
 
-func (s *executor) getDockerImage(imageName string) (*types.ImageInspect, error) {
+func (s *executor) getDockerImage(imageName string) (image *types.ImageInspect, err error) {
 	pullPolicy, err := s.Config.Docker.PullPolicy.Get()
 	if err != nil {
 		return nil, err
@@ -163,31 +168,52 @@ func (s *executor) getDockerImage(imageName string) (*types.ImageInspect, error)
 	authConfig := s.getAuthConfig(imageName)
 
 	s.Debugln("Looking for image", imageName, "...")
-	image, _, err := s.client.ImageInspectWithRaw(s.Context, imageName)
+	existingImage, _, err := s.client.ImageInspectWithRaw(s.Context, imageName)
+
+	// Return early if we already used that image
+	if err == nil && s.wasImageUsed(imageName, existingImage.ID) {
+		return &existingImage, nil
+	}
+
+	defer func() {
+		if err == nil {
+			s.markImageAsUsed(imageName, image.ID)
+		}
+	}()
 
 	// If never is specified then we return what inspect did return
 	if pullPolicy == common.PullPolicyNever {
-		return &image, err
+		return &existingImage, err
 	}
 
 	if err == nil {
 		// Don't pull image that is passed by ID
-		if image.ID == imageName {
-			return &image, nil
+		if existingImage.ID == imageName {
+			return &existingImage, nil
 		}
 
 		// If not-present is specified
 		if pullPolicy == common.PullPolicyIfNotPresent {
 			s.Println("Using locally found image version due to if-not-present pull policy")
-			return &image, err
+			return &existingImage, err
 		}
 	}
 
-	newImage, err := s.pullDockerImage(imageName, authConfig)
+	return s.pullDockerImage(imageName, authConfig)
+}
+
+func (s *executor) expandAndGetDockerImage(imageName string, allowedImages []string) (*types.ImageInspect, error) {
+	imageName, err := s.expandImageName(imageName, allowedImages)
+	if err != nil {
+		return nil, err
+	}
+
+	image, err := s.getDockerImage(imageName)
 	if err != nil {
 		return nil, err
 	}
-	return newImage, nil
+
+	return image, nil
 }
 
 func (s *executor) getArchitecture() string {
@@ -258,6 +284,21 @@ func (s *executor) getPrebuiltImage() (*types.ImageInspect, error) {
 	return &image, err
 }
 
+func (s *executor) getBuildImage() (*types.ImageInspect, error) {
+	imageName, err := s.expandImageName(s.Build.Image.Name, []string{})
+	if err != nil {
+		return nil, err
+	}
+
+	// Fetch image
+	image, err := s.getDockerImage(imageName)
+	if err != nil {
+		return nil, err
+	}
+
+	return image, nil
+}
+
 func (s *executor) getAbsoluteContainerPath(dir string) string {
 	if path.IsAbs(dir) {
 		return dir
@@ -319,7 +360,7 @@ func (s *executor) createCacheVolume(containerName, containerPath string) (strin
 	resp, err := s.client.ContainerCreate(s.Context, config, hostConfig, nil, containerName)
 	if err != nil {
 		if resp.ID != "" {
-			s.failures = append(s.failures, resp.ID)
+			s.temporary = append(s.temporary, resp.ID)
 		}
 		return "", err
 	}
@@ -327,14 +368,14 @@ func (s *executor) createCacheVolume(containerName, containerPath string) (strin
 	s.Debugln("Starting cache container", resp.ID, "...")
 	err = s.client.ContainerStart(s.Context, resp.ID, types.ContainerStartOptions{})
 	if err != nil {
-		s.failures = append(s.failures, resp.ID)
+		s.temporary = append(s.temporary, resp.ID)
 		return "", err
 	}
 
 	s.Debugln("Waiting for cache container", resp.ID, "...")
 	err = s.waitForContainer(resp.ID)
 	if err != nil {
-		s.failures = append(s.failures, resp.ID)
+		s.temporary = append(s.temporary, resp.ID)
 		return "", err
 	}
 
@@ -387,7 +428,7 @@ func (s *executor) addCacheVolume(containerPath string) error {
 	}
 
 	s.Debugln("Using container", containerID, "as cache", containerPath, "...")
-	s.volumesFrom = append(s.volumesFrom, containerID)
+	s.caches = append(s.caches, containerID)
 	return nil
 }
 
@@ -439,7 +480,7 @@ func (s *executor) createBuildVolume() error {
 	}
 
 	s.caches = append(s.caches, id)
-	s.volumesFrom = append(s.volumesFrom, id)
+	s.temporary = append(s.temporary, id)
 
 	return nil
 }
@@ -521,14 +562,28 @@ func (s *executor) bindDevices() (err error) {
 	return nil
 }
 
-func (s *executor) printUsedDockerImageID(imageName, imageID, containerType, containerTypeName string) {
-	var line string
-	if imageName == imageID {
-		line = fmt.Sprintf("Using docker image %s for %s %s...", imageName, containerTypeName, containerType)
-	} else {
-		line = fmt.Sprintf("Using docker image %s ID=%s for %s %s...", imageName, imageID, containerTypeName, containerType)
+func (s *executor) wasImageUsed(imageName, imageID string) bool {
+	s.usedImagesLock.RLock()
+	defer s.usedImagesLock.RUnlock()
+
+	if s.usedImages[imageName] == imageID {
+		return true
+	}
+	return false
+}
+
+func (s *executor) markImageAsUsed(imageName, imageID string) {
+	s.usedImagesLock.Lock()
+	defer s.usedImagesLock.Unlock()
+
+	if s.usedImages == nil {
+		s.usedImages = make(map[string]string)
+	}
+	s.usedImages[imageName] = imageID
+
+	if imageName != imageID {
+		s.Println("Using docker image", imageID, "for", imageName, "...")
 	}
-	s.Println(line)
 }
 
 func (s *executor) splitServiceAndVersion(serviceDescription string) (service, version, imageName string, linkNames []string) {
@@ -571,8 +626,6 @@ func (s *executor) createService(serviceIndex int, service, version, image strin
 		return nil, err
 	}
 
-	s.printUsedDockerImageID(image, serviceImage.ID, "service", service)
-
 	serviceSlug := strings.Replace(service, "/", "__", -1)
 	containerName := fmt.Sprintf("%s-%s-%d", s.Build.ProjectUniqueName(), serviceSlug, serviceIndex)
 
@@ -598,7 +651,7 @@ func (s *executor) createService(serviceIndex int, service, version, image strin
 		NetworkMode:   container.NetworkMode(s.Config.Docker.NetworkMode),
 		Binds:         s.binds,
 		ShmSize:       s.Config.Docker.ShmSize,
-		VolumesFrom:   s.volumesFrom,
+		VolumesFrom:   s.caches,
 		Tmpfs:         s.Config.Docker.ServicesTmpfs,
 		LogConfig: container.LogConfig{
 			Type: "json-file",
@@ -614,7 +667,7 @@ func (s *executor) createService(serviceIndex int, service, version, image strin
 	s.Debugln("Starting service container", resp.ID, "...")
 	err = s.client.ContainerStart(s.Context, resp.ID, types.ContainerStartOptions{})
 	if err != nil {
-		s.failures = append(s.failures, resp.ID)
+		s.temporary = append(s.temporary, resp.ID)
 		return nil, err
 	}
 
@@ -698,6 +751,7 @@ func (s *executor) createFromServiceDefinition(serviceIndex int, serviceDefiniti
 			}
 			s.Debugln("Created service", serviceDefinition.Name, "as", container.ID)
 			s.services = append(s.services, container)
+			s.temporary = append(s.temporary, container.ID)
 		}
 		linksMap[linkName] = container
 	}
@@ -725,26 +779,34 @@ func (s *executor) createServices() (err error) {
 	return
 }
 
-func (s *executor) createContainer(containerType string, imageDefinition common.Image, cmd []string, allowedInternalImages []string) (*types.ContainerJSON, error) {
-	imageName, err := s.expandImageName(imageDefinition.Name, allowedInternalImages)
-	if err != nil {
-		return nil, err
+func (s *executor) getValidContainers(containers []string) []string {
+	var newContainers []string
+
+	for _, container := range containers {
+		if _, err := s.client.ContainerInspect(s.Context, container); err == nil {
+			newContainers = append(newContainers, container)
+		}
 	}
 
-	// Fetch image
-	image, err := s.getDockerImage(imageName)
+	return newContainers
+}
+
+func (s *executor) createContainer(containerType string, imageDefinition common.Image, cmd []string, allowedInternalImages []string) (*types.ContainerJSON, error) {
+	image, err := s.expandAndGetDockerImage(imageDefinition.Name, allowedInternalImages)
 	if err != nil {
 		return nil, err
 	}
 
-	s.printUsedDockerImageID(imageName, image.ID, "container", containerType)
-
 	hostname := s.Config.Docker.Hostname
 	if hostname == "" {
 		hostname = s.Build.ProjectUniqueName()
 	}
 
-	containerName := s.Build.ProjectUniqueName() + "-" + containerType
+	// Always create unique, but sequential name
+	containerIndex := len(s.builds)
+	containerName := s.Build.ProjectUniqueName() + "-" +
+		containerType + "-" + strconv.Itoa(containerIndex)
+
 	config := &container.Config{
 		Image:        image.ID,
 		Hostname:     hostname,
@@ -768,6 +830,15 @@ func (s *executor) createContainer(containerType string, imageDefinition common.
 		return nil, err
 	}
 
+	// By default we use caches container,
+	// but in later phases we hook to previous build container
+	volumesFrom := s.caches
+	if len(s.builds) > 0 {
+		volumesFrom = []string{
+			s.builds[len(s.builds)-1],
+		}
+	}
+
 	hostConfig := &container.HostConfig{
 		Resources: container.Resources{
 			CpusetCpus: s.Config.Docker.CPUSetCPUs,
@@ -789,7 +860,7 @@ func (s *executor) createContainer(containerType string, imageDefinition common.
 		Binds:         s.binds,
 		ShmSize:       s.Config.Docker.ShmSize,
 		VolumeDriver:  s.Config.Docker.VolumeDriver,
-		VolumesFrom:   append(s.Config.Docker.VolumesFrom, s.volumesFrom...),
+		VolumesFrom:   append(s.Config.Docker.VolumesFrom, volumesFrom...),
 		LogConfig: container.LogConfig{
 			Type: "json-file",
 		},
@@ -804,18 +875,19 @@ func (s *executor) createContainer(containerType string, imageDefinition common.
 	resp, err := s.client.ContainerCreate(s.Context, config, hostConfig, nil, containerName)
 	if err != nil {
 		if resp.ID != "" {
-			s.failures = append(s.failures, resp.ID)
+			s.temporary = append(s.temporary, resp.ID)
 		}
 		return nil, err
 	}
 
 	inspect, err := s.client.ContainerInspect(s.Context, resp.ID)
 	if err != nil {
-		s.failures = append(s.failures, resp.ID)
+		s.temporary = append(s.temporary, resp.ID)
 		return nil, err
 	}
 
 	s.builds = append(s.builds, resp.ID)
+	s.temporary = append(s.temporary, resp.ID)
 	return &inspect, nil
 }
 
@@ -1130,20 +1202,8 @@ func (s *executor) Cleanup() {
 		}()
 	}
 
-	for _, failureID := range s.failures {
-		remove(failureID)
-	}
-
-	for _, service := range s.services {
-		remove(service.ID)
-	}
-
-	for _, cacheID := range s.caches {
-		remove(cacheID)
-	}
-
-	for _, buildID := range s.builds {
-		remove(buildID)
+	for _, temporaryID := range s.temporary {
+		remove(temporaryID)
 	}
 
 	wg.Wait()

executors/docker/executor_docker_command.go

@@ -11,8 +11,7 @@ import (
 
 type commandExecutor struct {
 	executor
-	predefinedContainer *types.ContainerJSON
-	buildContainer      *types.ContainerJSON
+	buildContainer *types.ContainerJSON
 }
 
 func (s *commandExecutor) Prepare(options common.ExecutorPrepareOptions) error {
@@ -27,41 +26,66 @@ func (s *commandExecutor) Prepare(options common.ExecutorPrepareOptions) error {
 		return errors.New("Script is not compatible with Docker")
 	}
 
-	prebuildImage, err := s.getPrebuiltImage()
+	_, err = s.getPrebuiltImage()
 	if err != nil {
 		return err
 	}
 
+	_, err = s.getBuildImage()
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+func (s *commandExecutor) requestNewPredefinedContainer() (*types.ContainerJSON, error) {
+	prebuildImage, err := s.getPrebuiltImage()
+	if err != nil {
+		return nil, err
+	}
+
 	buildImage := common.Image{
 		Name: prebuildImage.ID,
 	}
 
-	// Start pre-build container which will git clone changes
-	s.predefinedContainer, err = s.createContainer("predefined", buildImage, common.ContainerCommandBuild, []string{prebuildImage.ID})
+	containerJSON, err := s.createContainer("predefined", buildImage, common.ContainerCommandBuild, []string{prebuildImage.ID})
 	if err != nil {
-		return err
+		return nil, err
 	}
 
-	// Start build container which will run actual build
-	s.buildContainer, err = s.createContainer("build", s.Build.Image, s.BuildShell.DockerCommand, []string{})
-	if err != nil {
-		return err
+	return containerJSON, err
+}
+
+func (s *commandExecutor) requestBuildContainer() (*types.ContainerJSON, error) {
+	if s.buildContainer == nil {
+		var err error
+
+		// Start build container which will run actual build
+		s.buildContainer, err = s.createContainer("build", s.Build.Image, s.BuildShell.DockerCommand, []string{})
+		if err != nil {
+			return nil, err
+		}
 	}
-	return nil
+
+	return s.buildContainer, nil
 }
 
 func (s *commandExecutor) Run(cmd common.ExecutorCommand) error {
-	s.SetCurrentStage(DockerExecutorStageRun)
-
 	var runOn *types.ContainerJSON
+	var err error
 	if cmd.Predefined {
-		runOn = s.predefinedContainer
+		runOn, err = s.requestNewPredefinedContainer()
 	} else {
-		runOn = s.buildContainer
+		runOn, err = s.requestBuildContainer()
+	}
+	if err != nil {
+		return err
 	}
 
 	s.Debugln("Executing on", runOn.Name, "the", cmd.Script)
 
+	s.SetCurrentStage(DockerExecutorStageRun)
+
 	return s.watchContainer(cmd.Context, runOn.ID, bytes.NewBufferString(cmd.Script))
 }
 

executors/docker/executor_docker_command_test.go

@@ -2,6 +2,7 @@ package docker_test
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 	"net/url"
 	"os"
@@ -19,8 +20,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab-runner/executors/docker"
 	"gitlab.com/gitlab-org/gitlab-runner/helpers"
 	"gitlab.com/gitlab-org/gitlab-runner/helpers/docker"
-
-	"golang.org/x/net/context"
 )
 
 func TestDockerCommandSuccessRun(t *testing.T) {
@@ -129,14 +128,14 @@ func TestDockerCommandWithAllowedImagesRun(t *testing.T) {
 }
 
 func isDockerOlderThan17_07(t *testing.T) bool {
-	cmd := exec.Command("docker", "version")
-	output, err := cmd.Output()
-	require.NoError(t, err, "docker version should return output")
+	client, err := docker_helpers.New(
+		docker_helpers.DockerCredentials{}, docker.DockerAPIVersion)
+	require.NoError(t, err, "should be able to connect to docker")
 
-	r := regexp.MustCompile(`(?ms)Server:\s*\n\s+Version:\s+([^\n]+)$`)
-	v := r.FindStringSubmatch(string(output))[1]
+	types, err := client.Info(context.Background())
+	require.NoError(t, err, "should be able to get docker info")
 
-	localVersion, err := version.NewVersion(v)
+	localVersion, err := version.NewVersion(types.ServerVersion)
 	require.NoError(t, err)
 
 	checkedVersion, err := version.NewVersion("17.07.0-ce")
@@ -652,7 +651,7 @@ func waitForDocker(credentials docker_helpers.DockerCredentials) error {
 	}
 
 	for i := 0; i < 20; i++ {
-		_, err = client.Info(context.TODO())
+		_, err = client.Info(context.Background())
 		if err == nil {
 			break
 		}
@@ -870,5 +869,44 @@ func TestDockerCommandWithHelperImageConfig(t *testing.T) {
 	assert.NoError(t, err)
 	out := buffer.String()
 	assert.Contains(t, out, "Pulling docker image "+helperImageConfig)
-	assert.Contains(t, out, "Using docker image sha256:bbd86c6ba107ae2feb8dbf9024df4b48597c44e1b584a3d901bba91f7fc500e3 for predefined container...")
+	assert.Contains(t, out, "Using docker image sha256:bbd86c6ba107ae2feb8dbf9024df4b48597c44e1b584a3d901bba91f7fc500e3 for gitlab/gitlab-runner-helper:x86_64-64eea86c ...")
+}
+
+func TestDockerCommandWithDoingPruneAndAfterScript(t *testing.T) {
+	if helpers.SkipIntegrationTests(t, "docker", "info") {
+		return
+	}
+
+	successfulBuild, err := common.GetRemoteSuccessfulBuildWithAfterScript()
+
+	// This scripts removes self-created containers that do exit
+	// It will fail if: cannot be removed, or no containers is found
+	// It is assuming that name of each runner created container starts
+	// with `runner-doprune-`
+	successfulBuild.Steps[0].Script = common.StepScript{
+		"docker ps -a -f status=exited | grep runner-doprune-",
+		"docker rm $(docker ps -a -f status=exited | grep runner-doprune- | awk '{print $1}')",
+	}
+
+	assert.NoError(t, err)
+	build := &common.Build{
+		JobResponse: successfulBuild,
+		Runner: &common.RunnerConfig{
+			RunnerCredentials: common.RunnerCredentials{
+				Token: "doprune",
+			},
+			RunnerSettings: common.RunnerSettings{
+				Executor: "docker",
+				Docker: &common.DockerConfig{