New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make experimental a runtime flag #27223

Merged
merged 1 commit into from Oct 24, 2016

Conversation

@mlaventure
Contributor

mlaventure commented Oct 7, 2016

Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

Left to do:

  • Update the install script to automatically set "experimental": true if fetched from https://experimental.docker.com/
  • See what solution is adopted by #25498 and piggy back on it to check the daemon experimental flag
  • Update documentation

Closes #26713

Show outdated Hide outdated integration-cli/check_test.go
if experimentalDaemon {
args = append(args, "--experimental")
}
err := d.StartWithBusybox(args...) // avoid networking conflicts

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Oct 13, 2016

Member

nit: IMO the comment should be moved to L267

@AkihiroSuda

AkihiroSuda Oct 13, 2016

Member

nit: IMO the comment should be moved to L267

This comment has been minimized.

@mlaventure

mlaventure Oct 14, 2016

Contributor

Updated, thanks.

@mlaventure

mlaventure Oct 14, 2016

Contributor

Updated, thanks.

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Oct 17, 2016

Contributor

See what solution is adopted by #25498 and piggy back on it to check the daemon experimental flag

I think it's perfectly fine to rely on /info in the meantime (we're just talking about optimizations here). Nevermind, Swarm-mode makes this potentially slow.

Otherwise design LGTM 👍

Contributor

icecrime commented Oct 17, 2016

See what solution is adopted by #25498 and piggy back on it to check the daemon experimental flag

I think it's perfectly fine to rely on /info in the meantime (we're just talking about optimizations here). Nevermind, Swarm-mode makes this potentially slow.

Otherwise design LGTM 👍

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Oct 19, 2016

Contributor

See what solution is adopted by #25498 and piggy back on it to check the daemon experimental flag

This is a requirement, but it won't change much about this PR, so I'm moving it to code review so we can try and get it in for 1.13.0.

Contributor

icecrime commented Oct 19, 2016

See what solution is adopted by #25498 and piggy back on it to check the daemon experimental flag

This is a requirement, but it won't change much about this PR, so I'm moving it to code review so we can try and get it in for 1.13.0.

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Oct 20, 2016

Contributor

This seems to be working as expected, code looks fine.

LGTM

Contributor

justincormack commented Oct 20, 2016

This seems to be working as expected, code looks fine.

LGTM

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Oct 20, 2016

Contributor

LGTM
@mlaventure but need rebase

Contributor

LK4D4 commented Oct 20, 2016

LGTM
@mlaventure but need rebase

@@ -96,8 +96,7 @@ set -e
# Install runc and containerd
RUN ./hack/dockerfile/install-binaries.sh runc-dynamic containerd-dynamic
EOF
if [ "$DOCKER_EXPERIMENTAL" ]; then
if [[ "$VERSION" == *-dev ]] || [ -n "$(git status --porcelain)" ]; then
echo 'ENV DOCKER_EXPERIMENTAL 1' >> "$DEST/$version/Dockerfile.build"
fi

This comment has been minimized.

@tianon

tianon Oct 20, 2016

Member

Shouldn't this block just be removed completely, as in build-deb?

@tianon

tianon Oct 20, 2016

Member

Shouldn't this block just be removed completely, as in build-deb?

This comment has been minimized.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 20, 2016

Member

We should have a CI check or label to prevent future PR's from adding an experimental build-flag

/cc @ehazlett @mlaventure

Member

thaJeztah commented Oct 20, 2016

We should have a CI check or label to prevent future PR's from adding an experimental build-flag

/cc @ehazlett @mlaventure

@@ -1,4 +1,4 @@
// +build experimental
// +build !windows

This comment has been minimized.

@tianon

tianon Oct 20, 2016

Member

Were we just never running tests on windows with experimental enabled previously?

@tianon

tianon Oct 20, 2016

Member

Were we just never running tests on windows with experimental enabled previously?

This comment has been minimized.

@mlaventure

mlaventure Oct 20, 2016

Contributor

Correct, all the test have DaemonIsLinux as a prerequisite

@mlaventure

mlaventure Oct 20, 2016

Contributor

Correct, all the test have DaemonIsLinux as a prerequisite

if [ "$DOCKER_EXPERIMENTAL" ]; then
echo >&2 '# DOCKER_EXPERIMENTAL is set: starting daemon with experimental features enabled! '
extra_params="$extra_params --experimental"
fi

This comment has been minimized.

@tianon

tianon Oct 20, 2016

Member

Aren't the experimental features tested regardless of whether this is set on our integration daemon, or does this mean we'll still need to run the full test suite twice (once without and once with this set)?

@tianon

tianon Oct 20, 2016

Member

Aren't the experimental features tested regardless of whether this is set on our integration daemon, or does this mean we'll still need to run the full test suite twice (once without and once with this set)?

This comment has been minimized.

@mlaventure

mlaventure Oct 20, 2016

Contributor

Without it the experimental endpoint are not "exposed" to the clients.

However, if there''s code elsewhere that is supposed to be experimental and isn't guard by the proper check, testing both with and without the flag should help catch it.

@mlaventure

mlaventure Oct 20, 2016

Contributor

Without it the experimental endpoint are not "exposed" to the clients.

However, if there''s code elsewhere that is supposed to be experimental and isn't guard by the proper check, testing both with and without the flag should help catch it.

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Oct 20, 2016

Contributor
  • Added a new Docker-Experimental header to every request.
  • Using /_ping endpoint to check for the above value to determine if experimental is enabled on the daemon.
Contributor

mlaventure commented Oct 20, 2016

  • Added a new Docker-Experimental header to every request.
  • Using /_ping endpoint to check for the above value to determine if experimental is enabled on the daemon.
@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Oct 20, 2016

Contributor

@thaJeztah I couldn't find a place where to document the extra header, any pointers?

Contributor

mlaventure commented Oct 20, 2016

@thaJeztah I couldn't find a place where to document the extra header, any pointers?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 20, 2016

Member

@mlaventure I'd consider adding it to the API changes at least; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md#v125-api-changes

Perhaps it should be in the introduction, just like we mention error messages (i.e. explain which headers are returned by API calls) https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api_v1.25.md#1-brief-introduction

@mstanleyjones wdyt?

Member

thaJeztah commented Oct 20, 2016

@mlaventure I'd consider adding it to the API changes at least; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md#v125-api-changes

Perhaps it should be in the introduction, just like we mention error messages (i.e. explain which headers are returned by API calls) https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api_v1.25.md#1-brief-introduction

@mstanleyjones wdyt?

@mistyhacks

This comment has been minimized.

Show comment
Hide comment
@mistyhacks

mistyhacks Oct 21, 2016

Contributor

+1 to @thaJeztah's suggestions. Maybe we should have an explanation somewhere of what "experimental" really means, as far as stability / support, and what kinds of features are likely to be added as experimental ones first.

Contributor

mistyhacks commented Oct 21, 2016

+1 to @thaJeztah's suggestions. Maybe we should have an explanation somewhere of what "experimental" really means, as far as stability / support, and what kinds of features are likely to be added as experimental ones first.

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Oct 21, 2016

Contributor

@thaJeztah added it to the api changes.

For the rest, it can be done on a separate PR. I wouldn't even know where to start 😅

Contributor

mlaventure commented Oct 21, 2016

@thaJeztah added it to the api changes.

For the rest, it can be done on a separate PR. I wouldn't even know where to start 😅

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Oct 21, 2016

Contributor

ping @LK4D4 @justincormack one last review?

I'll do the script changes in a separate PR, the script has to be manually tested anyhow.

Contributor

mlaventure commented Oct 21, 2016

ping @LK4D4 @justincormack one last review?

I'll do the script changes in a separate PR, the script has to be manually tested anyhow.

@mlaventure mlaventure changed the title from [WIP] Make experimental a runtime flag to Make experimental a runtime flag Oct 24, 2016

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Oct 24, 2016

Contributor

Both experimental & janky are actually 💚

Contributor

mlaventure commented Oct 24, 2016

Both experimental & janky are actually 💚

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Oct 24, 2016

@vdemeester

Few comments, but looks good 👼

// ExperimentalMiddleware is a the middleware in charge of adding the
// 'Docker-Experimental' header to every outgoing request
type ExperimentalMiddleware struct {

This comment has been minimized.

@vdemeester

vdemeester Oct 24, 2016

Member

Doesn't golint errors out on this 😅 ? middleware.ExperimentalMiddleware seams a bit redondant 👼 😝

@vdemeester

vdemeester Oct 24, 2016

Member

Doesn't golint errors out on this 😅 ? middleware.ExperimentalMiddleware seams a bit redondant 👼 😝

This comment has been minimized.

@mlaventure

mlaventure Oct 24, 2016

Contributor

I just followed the same convention as the others one (e.g. here

@mlaventure

mlaventure Oct 24, 2016

Contributor

I just followed the same convention as the others one (e.g. here

This comment has been minimized.

@vdemeester

vdemeester Oct 24, 2016

Member

ah, ok 👼 nevermind then 😝 We could (should) rename those I think, but it's not the scope of the PR 👼

@vdemeester

vdemeester Oct 24, 2016

Member

ah, ok 👼 nevermind then 😝 We could (should) rename those I think, but it's not the scope of the PR 👼

Show outdated Hide outdated docs/reference/api/docker_remote_api.md
@@ -146,7 +146,7 @@ This section lists each version from latest to oldest. Each listing includes a
* `POST /containers/prune` prunes stopped containers.
* `POST /images/prune` prunes unused images.
* `POST /volumes/prune` prunes unused volumes.
* Every API request answer will now include a `Docker-Experimental` specifying if experimental feature are enabled (value can be `true` or `false`).

This comment has been minimized.

@vdemeester

vdemeester Oct 24, 2016

Member

It's not docs-review yet but shouldn't this be Every API response … ?

@vdemeester

vdemeester Oct 24, 2016

Member

It's not docs-review yet but shouldn't this be Every API response … ?

This comment has been minimized.

@mlaventure

mlaventure Oct 24, 2016

Contributor

Yep, sounds better, thanks!

@mlaventure

mlaventure Oct 24, 2016

Contributor

Yep, sounds better, thanks!

@vdemeester

LGTM 🐯

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Oct 24, 2016

Member

@thaJeztah @mlaventure should we move this to docs-review or should we do that in a follow up PR?

Member

vdemeester commented Oct 24, 2016

@thaJeztah @mlaventure should we move this to docs-review or should we do that in a follow up PR?

@vieux

vieux approved these changes Oct 24, 2016

LGTM

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett Oct 24, 2016

Contributor

@docker/core-machine-maintainers machine will need some updates to support the new experimental flag.

LGTM

Contributor

ehazlett commented Oct 24, 2016

@docker/core-machine-maintainers machine will need some updates to support the new experimental flag.

LGTM

Show outdated Hide outdated docs/reference/api/docker_remote_api.md
@@ -157,7 +157,7 @@ This section lists each version from latest to oldest. Each listing includes a
* `POST /containers/prune` prunes stopped containers.
* `POST /images/prune` prunes unused images.
* `POST /volumes/prune` prunes unused volumes.
* Every API response will now include a `Docker-Experimental` specifying if experimental feature are enabled (value can be `true` or `false`).

This comment has been minimized.

@thaJeztah

thaJeztah Oct 24, 2016

Member

Missing "header", I guess?

Every API response now includes a `Docker-Experimental` header specifying if 
experimental features are enabled (value can be `true` or `false`).
@thaJeztah

thaJeztah Oct 24, 2016

Member

Missing "header", I guess?

Every API response now includes a `Docker-Experimental` header specifying if 
experimental features are enabled (value can be `true` or `false`).
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 24, 2016

Member

Had one small nit on docs, but feel free to go ahead if you prefer

Member

thaJeztah commented Oct 24, 2016

Had one small nit on docs, but feel free to go ahead if you prefer

Make experimental a runtime flag
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Oct 24, 2016

Contributor

@thaJeztah made the doc change.

Contributor

mlaventure commented Oct 24, 2016

@thaJeztah made the doc change.

@thaJeztah

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 24, 2016

Member

all green!

Member

thaJeztah commented Oct 24, 2016

all green!

@thaJeztah thaJeztah merged commit ba41a5e into moby:master Oct 24, 2016

4 checks passed

docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 25332 has succeeded
Details
janky Jenkins build Docker-PRs 33933 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 4799 has succeeded
Details

@mlaventure mlaventure deleted the mlaventure:merge-experimental branch Oct 25, 2016

@thaJeztah thaJeztah referenced this pull request Nov 6, 2016

Closed

Changing the definition of experimental #26713

2 of 4 tasks complete

tianon added a commit to tianon/jenkins-groovy that referenced this pull request Nov 14, 2016

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Dec 14, 2016

Member

https://github.com/docker/docker/blob/master/experimental/README.md should probably be updated for this change as well, right?

Member

tianon commented Dec 14, 2016

https://github.com/docker/docker/blob/master/experimental/README.md should probably be updated for this change as well, right?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 14, 2016

Member

@tianon see #28160 ❤️

Member

thaJeztah commented Dec 14, 2016

@tianon see #28160 ❤️

@praving55

This comment has been minimized.

Show comment
Hide comment
@praving55

praving55 Jan 19, 2017

Can you point me to the reference please? I could not see it in docs

Can you point me to the reference please? I could not see it in docs

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 19, 2017

Member

docs for 1.13 are not yet published, but the flag itself is documented in the dockerd reference. You can either set a --experimental flag on the daemon (but depends on what init system (upstart, sysv-init, systemd) you're using, or use a daemon.json configuration file, as described here; https://github.com/docker/docker/blob/v1.13.0/docs/reference/commandline/dockerd.md#daemon-configuration-file

Member

thaJeztah commented Jan 19, 2017

docs for 1.13 are not yet published, but the flag itself is documented in the dockerd reference. You can either set a --experimental flag on the daemon (but depends on what init system (upstart, sysv-init, systemd) you're using, or use a daemon.json configuration file, as described here; https://github.com/docker/docker/blob/v1.13.0/docs/reference/commandline/dockerd.md#daemon-configuration-file

@praving55

This comment has been minimized.

Show comment
Hide comment
@praving55

praving55 Jan 19, 2017

Is there a reason docs are lagging? I never saw that happening before.

Is there a reason docs are lagging? I never saw that happening before.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 19, 2017

Member

Not all parts of the release are released yet, so the release will be officially announced after all projects have finished their release, and the docs will probably go out together with that.

Member

thaJeztah commented Jan 19, 2017

Not all parts of the release are released yet, so the release will be officially announced after all projects have finished their release, and the docs will probably go out together with that.

@tuxity tuxity referenced this pull request Jan 19, 2017

Merged

[WIP] Docker 1.13 #100

3 of 5 tasks complete
@praving55

This comment has been minimized.

Show comment
Hide comment

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment