From 8041d0ae78ca86f3e21b18a4f74dace5e26b85e8 Mon Sep 17 00:00:00 2001 From: Juha Ruotsalainen Date: Fri, 15 Nov 2019 13:36:30 +0200 Subject: [PATCH 1/6] Build quietly and capture the image hash At least on macOS combo Catalina + Docker engine 19.03.4 + Docker desktop 2.1.0.4 + vagrant 2.2.6 the original `matches = result.scan(/Successfully built (.+)$/i)` -line fails to generate a match. With this change I can `vagrant up --provider=docker` successfully. --- plugins/providers/docker/driver.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/providers/docker/driver.rb b/plugins/providers/docker/driver.rb index acad830cf..9a98d3a2d 100644 --- a/plugins/providers/docker/driver.rb +++ b/plugins/providers/docker/driver.rb @@ -18,8 +18,8 @@ module VagrantPlugins def build(dir, **opts, &block) args = Array(opts[:extra_args]) args << dir - result = execute('docker', 'build', *args, &block) - matches = result.scan(/Successfully built (.+)$/i) + result = execute('docker', 'build', '-q', *args, &block) + matches = result.scan(/^sha256:([0-9a-f]+)$/i) if matches.empty? # This will cause a stack trace in Vagrant, but it is a bug # if this happens anyways. From f3629ebd092a6dde1d7c9e0ebe4337bacf2bfc21 Mon Sep 17 00:00:00 2001 From: Rumpu-Jussi Date: Sat, 16 Nov 2019 16:38:20 +0200 Subject: [PATCH 2/6] Buildkit-based output processed a bit differently. --- plugins/providers/docker/driver.rb | 14 +++++++++----- plugins/providers/docker/executor/local.rb | 7 ++++++- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/plugins/providers/docker/driver.rb b/plugins/providers/docker/driver.rb index 9a98d3a2d..959be42ed 100644 --- a/plugins/providers/docker/driver.rb +++ b/plugins/providers/docker/driver.rb @@ -18,12 +18,16 @@ module VagrantPlugins def build(dir, **opts, &block) args = Array(opts[:extra_args]) args << dir - result = execute('docker', 'build', '-q', *args, &block) - matches = result.scan(/^sha256:([0-9a-f]+)$/i) + result = execute('docker', 'build', '--progress', 'plain', *args, &block) + matches = result.scan(/Successfully built (.+)$/i) if matches.empty? - # This will cause a stack trace in Vagrant, but it is a bug - # if this happens anyways. - raise "UNKNOWN OUTPUT: #{result}" + # Check for the new output format 'writing image sha256...' + matches = result.scan(/writing image sha256:([0-9a-z]+) done$/i) + if matches.empty? + # This will cause a stack trace in Vagrant, but it is a bug + # if this happens anyways. + raise "UNKNOWN OUTPUT: #{result}" + end end # Return the last match, and the capture of it diff --git a/plugins/providers/docker/executor/local.rb b/plugins/providers/docker/executor/local.rb index c78e8c90e..6a71d52d5 100644 --- a/plugins/providers/docker/executor/local.rb +++ b/plugins/providers/docker/executor/local.rb @@ -27,7 +27,12 @@ module VagrantPlugins stdout: result.stdout end - result.stdout + # If the new buildkit-based docker build is used, stdout is empty, and the output is in stderr + if result.stdout.to_s.strip.length == 0 + result.stderr + else + result.stdout + end end def windows? From 4fc8b07974ed5bc1154ca18f430627846ea828b3 Mon Sep 17 00:00:00 2001 From: Juha Ruotsalainen Date: Mon, 18 Nov 2019 08:53:33 +0200 Subject: [PATCH 3/6] Removed the word 'done'. There are cases, when 'done' is prefix with a duration, like `... 0.1s done`. --- plugins/providers/docker/driver.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/providers/docker/driver.rb b/plugins/providers/docker/driver.rb index 959be42ed..6dfd7a438 100644 --- a/plugins/providers/docker/driver.rb +++ b/plugins/providers/docker/driver.rb @@ -22,7 +22,7 @@ module VagrantPlugins matches = result.scan(/Successfully built (.+)$/i) if matches.empty? # Check for the new output format 'writing image sha256...' - matches = result.scan(/writing image sha256:([0-9a-z]+) done$/i) + matches = result.scan(/writing image sha256:([0-9a-z]+) +$/i) if matches.empty? # This will cause a stack trace in Vagrant, but it is a bug # if this happens anyways. From 4d70856b8a8a9ab6fab1636823d36ddc814fd427 Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Tue, 19 Nov 2019 10:59:28 -0800 Subject: [PATCH 4/6] Enhance docker build matching for determining built container ID Prior to this commit, docker would look for a container ID based on "Successfully built" string. This output does not exist if a user has enabled the experimental feature buildkit. This commit updates the build behavior to match against both kinds of outputs, and instead of using `scan`, it uses MatchData and groups the container id with match group name `:id` instead of making hard assumptions with the matches being contained inside arrays from scan. --- plugins/providers/docker/driver.rb | 23 ++++++++++++------- plugins/providers/docker/errors.rb | 4 ++++ plugins/providers/docker/executor/local.rb | 1 - templates/locales/providers_docker.yml | 2 ++ .../plugins/providers/docker/driver_test.rb | 21 +++++++++++++++++ 5 files changed, 42 insertions(+), 9 deletions(-) diff --git a/plugins/providers/docker/driver.rb b/plugins/providers/docker/driver.rb index 6dfd7a438..f2b382fc3 100644 --- a/plugins/providers/docker/driver.rb +++ b/plugins/providers/docker/driver.rb @@ -15,23 +15,30 @@ module VagrantPlugins @executor = Executor::Local.new end + # Returns the id for a new container built from `docker build`. Raises + # an exception if the id was unable to be captured from the output + # + # @return [String] id - ID matched from the docker build output. def build(dir, **opts, &block) args = Array(opts[:extra_args]) args << dir - result = execute('docker', 'build', '--progress', 'plain', *args, &block) - matches = result.scan(/Successfully built (.+)$/i) - if matches.empty? + result = execute('docker', 'build', *args, &block) + matches = result.match(/Successfully built (?.+)$/i) + if !matches # Check for the new output format 'writing image sha256...' - matches = result.scan(/writing image sha256:([0-9a-z]+) +$/i) - if matches.empty? + # In this case, docker builtkit is enabled. Its format is different + # from standard docker + @logger.warn("Could not determine docker container ID. Scanning for buildkit output instead") + matches = result.match(/writing image .+:(?[0-9a-z]+) done/i) + if !matches # This will cause a stack trace in Vagrant, but it is a bug # if this happens anyways. - raise "UNKNOWN OUTPUT: #{result}" + raise Errors::BuildError, result: result end end - # Return the last match, and the capture of it - matches[-1][0] + # Return the matched group `id` + matches[:id] end def create(params, **opts, &block) diff --git a/plugins/providers/docker/errors.rb b/plugins/providers/docker/errors.rb index 11ae6fd68..6569f6a3a 100644 --- a/plugins/providers/docker/errors.rb +++ b/plugins/providers/docker/errors.rb @@ -5,6 +5,10 @@ module VagrantPlugins error_namespace("docker_provider.errors") end + class BuildError < DockerError + error_key(:build_error) + end + class CommunicatorNonDocker < DockerError error_key(:communicator_non_docker) end diff --git a/plugins/providers/docker/executor/local.rb b/plugins/providers/docker/executor/local.rb index 6a71d52d5..f99c4f929 100644 --- a/plugins/providers/docker/executor/local.rb +++ b/plugins/providers/docker/executor/local.rb @@ -27,7 +27,6 @@ module VagrantPlugins stdout: result.stdout end - # If the new buildkit-based docker build is used, stdout is empty, and the output is in stderr if result.stdout.to_s.strip.length == 0 result.stderr else diff --git a/templates/locales/providers_docker.yml b/templates/locales/providers_docker.yml index 3c23b905c..4b6795c4c 100644 --- a/templates/locales/providers_docker.yml +++ b/templates/locales/providers_docker.yml @@ -159,6 +159,8 @@ en: run exits and doesn't keep running. errors: + build_error: |- + Vagrant received unknown output from docker: %{result} compose_lock_timeout: |- Vagrant encountered a timeout waiting for the docker compose driver to become available. Please try to run your command again. If you diff --git a/test/unit/plugins/providers/docker/driver_test.rb b/test/unit/plugins/providers/docker/driver_test.rb index 6504876cb..7ae6524ef 100644 --- a/test/unit/plugins/providers/docker/driver_test.rb +++ b/test/unit/plugins/providers/docker/driver_test.rb @@ -152,6 +152,27 @@ describe VagrantPlugins::DockerProvider::Driver do ].to_json } + describe '#build' do + let(:result) { "Successfully built 1a2b3c4d" } + let(:buildkit_result) { "writing image sha256:1a2b3c4d done" } + let(:cid) { "1a2b3c4d" } + + it "builds a container with standard docker" do + allow(subject).to receive(:execute).and_return(result) + + container_id = subject.build("/tmp/fakedir") + + expect(container_id).to eq(cid) + end + + it "builds a container with buildkit docker" do + allow(subject).to receive(:execute).and_return(buildkit_result) + + container_id = subject.build("/tmp/fakedir") + + expect(container_id).to eq(cid) + end + end describe '#create' do let(:params) { { From 16998215718eaa2ef799e6d269b667e1a6547b05 Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Tue, 19 Nov 2019 11:14:46 -0800 Subject: [PATCH 5/6] More specific language around docker build matching errors --- templates/locales/providers_docker.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/locales/providers_docker.yml b/templates/locales/providers_docker.yml index 4b6795c4c..0760b5567 100644 --- a/templates/locales/providers_docker.yml +++ b/templates/locales/providers_docker.yml @@ -160,7 +160,7 @@ en: errors: build_error: |- - Vagrant received unknown output from docker: %{result} + Vagrant received unknown output from `docker build` while building a container: %{result} compose_lock_timeout: |- Vagrant encountered a timeout waiting for the docker compose driver to become available. Please try to run your command again. If you From 2901dae9484e5311e68517016ddfe57f257734df Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Fri, 22 Nov 2019 12:04:09 -0800 Subject: [PATCH 6/6] Add option for docker executor to handle stderr from results Instead of always joining stdout and stderr, only join the two if the caller explicitly asks for it. Otherwise, only return stdout. --- plugins/providers/docker/driver.rb | 7 ++++--- plugins/providers/docker/executor/local.rb | 10 ++++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/plugins/providers/docker/driver.rb b/plugins/providers/docker/driver.rb index f2b382fc3..8932c74a2 100644 --- a/plugins/providers/docker/driver.rb +++ b/plugins/providers/docker/driver.rb @@ -20,9 +20,10 @@ module VagrantPlugins # # @return [String] id - ID matched from the docker build output. def build(dir, **opts, &block) - args = Array(opts[:extra_args]) - args << dir - result = execute('docker', 'build', *args, &block) + args = Array(opts[:extra_args]) + args << dir + opts = {with_stderr: true} + result = execute('docker', 'build', *args, opts, &block) matches = result.match(/Successfully built (?.+)$/i) if !matches # Check for the new output format 'writing image sha256...' diff --git a/plugins/providers/docker/executor/local.rb b/plugins/providers/docker/executor/local.rb index f99c4f929..7825a0292 100644 --- a/plugins/providers/docker/executor/local.rb +++ b/plugins/providers/docker/executor/local.rb @@ -27,10 +27,12 @@ module VagrantPlugins stdout: result.stdout end - if result.stdout.to_s.strip.length == 0 - result.stderr - else - result.stdout + if opts + if opts[:with_stderr] + return result.stdout + " " + result.stderr + else + return result.stdout + end end end