Make sure when there is a failure on pulling the build image it only executes once

We should add a test to ensure that when there is a failure on pulling, executor.Prepare is not retried (by retryCreateExecutor).

The following discussion from !2623 (merged) should be addressed:

@steveazz started a discussion:

issue: There is a behavior change here between master and this branch. I'm guessing this is because we are wrapping the error, and not unwrapping it.

Master:

=== RUN   TestDockerCommandMissingImage
Running with gitlab-runner development version (HEAD)
  on  76d86af5
Preparing the "docker" executor
Using Docker executor with image some/non-existing/image ...
Pulling docker image some/non-existing/image ...
ERROR: Job failed: Error response from daemon: pull access denied for some/non-existing/image, repository does not exist or may require 'docker login': denied: requested access to the resource is denied (docker.go:147:2s)
2021/01/05 10:10:56 err: &common.BuildError{Inner:(*fmt.wrapError)(0xc000110a60), FailureReason:"", ExitCode:0}
--- PASS: TestDockerCommandMissingImage (3.28s)
PASS

Process finished with exit code 0

This branch:

=== RUN   TestDockerCommandMissingImage
Running with gitlab-runner development version (HEAD)
  on  76d86af5
Preparing the "docker" executor
Using Docker executor with image some/non-existing/image ...
Pulling docker image some/non-existing/image ...
WARNING: Failed to pull image with policy "if-not-present": Error response from daemon: pull access denied for some/non-existing/image, repository does not exist or may require 'docker login': denied: requested access to the resource is denied (docker.go:147:2s)
ERROR: Preparation failed: failed to pull image "some/non-existing/image" with specified policies [if-not-present]: Error response from daemon: pull access denied for some/non-existing/image, repository does not exist or may require 'docker login': denied: requested access to the resource is denied (docker.go:147:2s)
Will be retried in 3s ...
Using Docker executor with image some/non-existing/image ...
Pulling docker image some/non-existing/image ...
WARNING: Failed to pull image with policy "if-not-present": Error response from daemon: pull access denied for some/non-existing/image, repository does not exist or may require 'docker login': denied: requested access to the resource is denied (docker.go:147:2s)
ERROR: Preparation failed: failed to pull image "some/non-existing/image" with specified policies [if-not-present]: Error response from daemon: pull access denied for some/non-existing/image, repository does not exist or may require 'docker login': denied: requested access to the resource is denied (docker.go:147:2s)
Will be retried in 3s ...
Using Docker executor with image some/non-existing/image ...
Pulling docker image some/non-existing/image ...
WARNING: Failed to pull image with policy "if-not-present": Error response from daemon: pull access denied for some/non-existing/image, repository does not exist or may require 'docker login': denied: requested access to the resource is denied (docker.go:147:3s)
ERROR: Preparation failed: failed to pull image "some/non-existing/image" with specified policies [if-not-present]: Error response from daemon: pull access denied for some/non-existing/image, repository does not exist or may require 'docker login': denied: requested access to the resource is denied (docker.go:147:3s)
Will be retried in 3s ...
ERROR: Job failed (system failure): failed to pull image "some/non-existing/image" with specified policies [if-not-present]: Error response from daemon: pull access denied for some/non-existing/image, repository does not exist or may require 'docker login': denied: requested access to the resource is denied (docker.go:147:3s)
--- PASS: TestDockerCommandMissingImage (19.20s)
PASS

Process finished with exit code 0

I think the fix for this is to update our assertion of the error. We need to check if this breaks anything else somewhere but it should be fine if we use errors.As since it is equivalent to err.(*BuildError), we aren't using error.Is because we don't care about equality.

diff --git a/common/build.go b/common/build.go
index 3047c14f6..e1acaaa6d 100644
--- a/common/build.go
+++ b/common/build.go
@@ -714,7 +714,8 @@ func (b *Build) retryCreateExecutor(
                        return executor, nil
                }
                executor.Cleanup()
-               if _, ok := err.(*BuildError); ok {
+               var buildErr *BuildError
+               if errors.As(err, &buildErr) {
                        return nil, err
                } else if options.Context.Err() != nil {
                        return nil, b.handleError(options.Context.Err())

I wish our CI caught this because it would have been somewhat of performance degradation for invalid images we just keep retrying. Should we add a test to make sure when there is a failure on pulling the build image it only executes ones? This should probably be done in a follow-up merge request of course not make this merge request bigger.

Edited Jan 05, 2021 by Pedro Pombeiro