builder: remove legacy build's session handling #39983

tiborvass · 2019-09-24T22:11:31Z

This feature was used by docker build --stream and it was kept experimental.

Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit.

Signed-off-by: Tibor Vass [email protected]

$ dockerd --experimental &> logs &
$ DOCKER_CLI_EXPERIMENTAL=enabled DOCKER_BUILDKIT=0 docker build --stream .
Error response from daemon: experimental session with v1 builder is no longer supported, use builder version v2 (BuildKit) instead

(for easier discovery of this change); this removes the experimental (non-buildkit) daemon changes related to #31829, #32677, #33786, #33859, docker/cli#231, docker/cli#678

AkihiroSuda · 2019-09-25T02:46:36Z

needs to error out if an old client requested --stream?

tiborvass · 2019-09-27T00:26:28Z

@AkihiroSuda Updated with a message that can be improved. I'm waiting on CLI to be merged first before marking this one ready to review.

thaJeztah

left a comment inline, but there's probably more to remove; I'll leave a comment below

thaJeztah · 2019-10-02T08:49:34Z

builder/dockerfile/builder.go

-		return nil, err
-	} else if src != nil {
-		source = src
+	if config.Options.SessionID != "" {


Could we move this check to the API, and either remove or deprecate the SessionID field;

moby/api/types/client.go

Line 182 in 827cb09

SessionID string

Looks like this field is set in a couple of places in the API, so perhaps we can catch it there;

moby/api/server/router/build/build_routes.go

Line 143 in ad1b781

options.SessionID = r.FormValue("session")

It's debatable where in the code to put the error. If I put it in the API and remove it from the builder package, then whoever uses the builder package will not have an error when they should. I don't mind that the HTTP handler code doesn't error out and leaves it to the builder package to error out. Also, as you realized later, the SessionID field is still needed for BuildKit, so I'm not planning to remove or deprecate that field.

thaJeztah · 2019-10-02T10:37:32Z

So, after some looking, I think he only changes needed would be my suggestion above (removing/deprecating the SessionID field), as BuildKit uses the same /session endpoint, so just the experimental use of that endpoint is removed.

@tiborvass I assume that the client/session code (e.g. changes made in #33859) is still used by BuildKit?

(I did notice the docs needed some updating, so opened a PR for that; #40028)

tiborvass · 2019-10-02T18:22:13Z

@thaJeztah the client/session package from #33859 is now in the buildkit repo. Thanks for the docs PR!

thaJeztah

LGTM

thaJeztah · 2019-10-02T18:35:59Z

CI failed on docker-py, which is broken, but will be fixed by #40030

We'll probably need to rebase this once that's merged, to get CI green

This feature was used by docker build --stream and it was kept experimental. Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit. Signed-off-by: Tibor Vass <[email protected]>

tiborvass · 2019-10-02T20:31:36Z

Rebased

thaJeztah · 2019-10-02T21:05:37Z

Let's wait for @tonistiigi to have a look; moved back to code-review,

thaJeztah · 2019-10-03T20:33:09Z

Looks like this broke master (vendor check), see #40032 (comment), and #40037

I was wondering how, but I think I understand;

[2019-10-02T20:48:17.877Z] + docker run --rm -t --privileged -v /home/ubuntu/workspace/moby_PR-39983/.git:/go/src/github.com/docker/docker/.git --name docker-pr4 -e DOCKER_EXPERIMENTAL -e DOCKER_GITCOMMIT=6ca3ec88ae9e1435abbed665ec598c00058659da -e DOCKER_GRAPHDRIVER docker:6ca3ec88ae9e1435abbed665ec598c00058659da hack/validate/vendor
[2019-10-02T20:48:18.810Z] No vendor changes in diff.

The vendor check only runs if vendor.conf is modified, but in this case the import was removed, therefore also affecting what has to be vendored

tiborvass · 2019-10-03T20:36:53Z

@thaJeztah thanks, opening up a fix.

tiborvass · 2019-10-03T20:37:33Z

Nevermind, you're faster #40037

full diff: moby/moby@b6684a4...a30990b relevant changes: - moby/moby#39995 Update containerd binary to v1.2.10 - moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884) - moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276) - moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596) - moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix. This is to ensure that users of the homedir package cannot compile statically (`CGO_ENABLED=0`) without also setting the `osusergo` build tag. - moby/moby#39983 builder: remove legacy build's session handling This feature was used by docker build --stream and it was kept experimental. Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit. - Related: docker#2105 build: remove --stream (was experimental) - moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty, golang/gddo, gorilla/mux - moby/moby#39713 bump containerd and dependencies to v1.3.0 - moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver - moby/moby#40070 Use ocischema package instead of custom handler - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image - relates to docker/hub-feedback#1871 - relates to distribution/distribution#3024 - moby/moby#39231 Add support for sending down service Running and Desired task counts - moby/moby#39822 daemon: Use short libnetwork ID in exec-root Signed-off-by: Sebastiaan van Stijn <[email protected]>

full diff: moby/moby@b6684a4...a09e6e3 relevant changes: - moby/moby#39995 Update containerd binary to v1.2.10 - moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884) - moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276) - moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596) - moby/moby#40134 Revert "homedir: add cgo or osusergo buildtag constraints for unix" - reverts moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix, in favor of documenting when to set the `osusergo` build tag. The `osusergo` build-flag must be used when compiling a static binary with `cgo` enabled, and linking against `glibc`. - moby/moby#39983 builder: remove legacy build's session handling This feature was used by docker build --stream and it was kept experimental. Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit. - Related: docker#2105 build: remove --stream (was experimental) - moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty, golang/gddo, gorilla/mux - moby/moby#39713 bump containerd and dependencies to v1.3.0 - moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver - moby/moby#40070 Use ocischema package instead of custom handler - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image - relates to docker/hub-feedback#1871 - relates to distribution/distribution#3024 - moby/moby#39231 Add support for sending down service Running and Desired task counts - moby/moby#39822 daemon: Use short libnetwork ID in exec-root - moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion() - updates/requires Microsoft/hscshim@2226e083fc390003ae5aa8325c3c92789afa0e7a Signed-off-by: Sebastiaan van Stijn <[email protected]>

full diff: moby/moby@b6684a4...a09e6e3 relevant changes: - moby/moby#39995 Update containerd binary to v1.2.10 - moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884) - moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276) - moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596) - moby/moby#40134 Revert "homedir: add cgo or osusergo buildtag constraints for unix" - reverts moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix, in favor of documenting when to set the `osusergo` build tag. The `osusergo` build-flag must be used when compiling a static binary with `cgo` enabled, and linking against `glibc`. - moby/moby#39983 builder: remove legacy build's session handling This feature was used by docker build --stream and it was kept experimental. Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit. - Related: #2105 build: remove --stream (was experimental) - moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty, golang/gddo, gorilla/mux - moby/moby#39713 bump containerd and dependencies to v1.3.0 - moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver - moby/moby#40070 Use ocischema package instead of custom handler - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image - relates to docker/hub-feedback#1871 - relates to distribution/distribution#3024 - moby/moby#39231 Add support for sending down service Running and Desired task counts - moby/moby#39822 daemon: Use short libnetwork ID in exec-root - moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion() - updates/requires Microsoft/hscshim@2226e083fc390003ae5aa8325c3c92789afa0e7a Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 7f6cd64335dc631efaa8204c01f92aa40939073a Component: cli

api/server/backend/build/backend.go

@@ -9,11 +9,9 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/builder"
 	buildkit "github.com/docker/docker/builder/builder-next"
-	"github.com/docker/docker/builder/fscache"


api/server/backend/build/backend.go

 	"github.com/docker/docker/image"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/pkg/errors"
-	"golang.org/x/sync/errgroup"


tiborvass mentioned this pull request Sep 24, 2019

build: remove --stream (was experimental) docker/cli#2105

Merged

tiborvass force-pushed the rm-legacy-build-session branch from 1e17ec9 to 0821ea8 Compare September 27, 2019 00:24

AkihiroSuda approved these changes Sep 27, 2019

View reviewed changes

andrewhsu mentioned this pull request Oct 1, 2019

Remove legacy build session dmcgowan/docker#52

Closed

tiborvass marked this pull request as ready for review October 1, 2019 20:58

tiborvass requested a review from tonistiigi as a code owner October 1, 2019 20:58

tiborvass force-pushed the rm-legacy-build-session branch from 0821ea8 to 30e356f Compare October 2, 2019 01:55

thaJeztah added area/builder impact/changelog kind/experimental status/2-code-review labels Oct 2, 2019

thaJeztah requested changes Oct 2, 2019

View reviewed changes

thaJeztah approved these changes Oct 2, 2019

View reviewed changes

builder: remove legacy build's session handling

6ca3ec8

This feature was used by docker build --stream and it was kept experimental. Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit. Signed-off-by: Tibor Vass <[email protected]>

tiborvass force-pushed the rm-legacy-build-session branch from 30e356f to 6ca3ec8 Compare October 2, 2019 20:29

thaJeztah added status/4-merge status/2-code-review and removed status/2-code-review status/4-merge labels Oct 2, 2019

thaJeztah assigned tonistiigi Oct 2, 2019

tonistiigi approved these changes Oct 2, 2019

View reviewed changes

tiborvass merged commit 3c54825 into moby:master Oct 3, 2019

thaJeztah mentioned this pull request Oct 3, 2019

Re-run vndr to fix master #40037

Merged

thaJeztah mentioned this pull request Oct 25, 2019

bump docker/docker to a09e6e323e55e1a9b21df9c2c555f5668df3ac9b docker/cli#2158

Merged

3 tasks

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

thaJeztah added the impact/deprecation label Oct 23, 2020

This was referenced Oct 23, 2020

Bump version to 20.10.0-beta1 docker-archive/docker-ce#660

Merged

Deprecation: change --stream error to a warning, and update deprecated.md docker/cli#2809

Merged

This was referenced Nov 6, 2020

[20.10] Regression: detection of experimental --stream is incorrect #41647

Closed

builder: fix detection of experimental --stream option (deprecated) #41648

Merged

This was referenced May 25, 2023

builder/remotecontext: deprecate CachableSource, NewCachableSource #45626

Merged

builder/remotecontext: remove CachableSource, NewCachableSource #45627

Merged

This was referenced Oct 10, 2024

integration/build: remove TestBuildWithSession, and fsutil direct dependency #48628

Merged

[27.x backport] integration/build: remove TestBuildWithSession, and fsutil direct dependency #48630

Merged

Magdeee reviewed Oct 10, 2024

View reviewed changes

moby locked as spam and limited conversation to collaborators Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builder: remove legacy build's session handling #39983

builder: remove legacy build's session handling #39983

tiborvass commented Sep 24, 2019 •

edited by thaJeztah

Loading

AkihiroSuda commented Sep 25, 2019

tiborvass commented Sep 27, 2019

thaJeztah left a comment

thaJeztah Oct 2, 2019

tiborvass Oct 2, 2019

thaJeztah commented Oct 2, 2019

tiborvass commented Oct 2, 2019 •

edited

Loading

thaJeztah left a comment

thaJeztah commented Oct 2, 2019

tiborvass commented Oct 2, 2019

thaJeztah commented Oct 2, 2019

thaJeztah commented Oct 3, 2019

tiborvass commented Oct 3, 2019

tiborvass commented Oct 3, 2019

This comment was marked as spam.

This comment was marked as spam.

builder: remove legacy build's session handling #39983

builder: remove legacy build's session handling #39983

Conversation

tiborvass commented Sep 24, 2019 • edited by thaJeztah Loading

AkihiroSuda commented Sep 25, 2019

tiborvass commented Sep 27, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Oct 2, 2019

Choose a reason for hiding this comment

tiborvass Oct 2, 2019

Choose a reason for hiding this comment

thaJeztah commented Oct 2, 2019

tiborvass commented Oct 2, 2019 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 2, 2019

tiborvass commented Oct 2, 2019

thaJeztah commented Oct 2, 2019

thaJeztah commented Oct 3, 2019

tiborvass commented Oct 3, 2019

tiborvass commented Oct 3, 2019

This comment was marked as spam.

This comment was marked as spam.

tiborvass commented Sep 24, 2019 •

edited by thaJeztah

Loading

tiborvass commented Oct 2, 2019 •

edited

Loading