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

graphdriver: custom build-time priority list #35522

Merged
merged 1 commit into from Nov 17, 2017

Conversation

Projects
None yet
6 participants
@kolyshkin
Contributor

kolyshkin commented Nov 16, 2017

Add a way to specify a custom graphdriver priority list during build. This can be done with something like

go build -ldflags "-X github.com/docker/docker/daemon/graphdriver.priorityOverride=overlay2,devicemapper"

As ldflags are already used by the engine build process, and it seems that only one (last) -ldflags argument is taken into account by go, an envoronment variable DOCKER_LDFLAGS is introduced in order to be able to append some text to -ldflags. With this in place, using the feature becomes

make DOCKER_LDFLAGS="-X github.com/docker/docker/daemon/graphdriver.priorityOverride=overlay2,devicemapper" dynbinary

If this variable is either unset during compile time or empty, the default list (priority) is used.

The idea behind this is, the priority list might be different for different distros, so vendors are now able to change it without patching the source code.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Nov 16, 2017

Member

I think we should have a document about these overridable variables

Member

AkihiroSuda commented Nov 16, 2017

I think we should have a document about these overridable variables

Show outdated Hide outdated daemon/graphdriver/driver.go Outdated
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 16, 2017

Contributor

I reckon that the ideal way of doing this would be to have priority be a (const) string, with the same format, and then we override it as you're doing here. That way feels much more consistent (to me at least). Also I feel like this information should be present in docker info but that might just be me.

Contributor

cyphar commented Nov 16, 2017

I reckon that the ideal way of doing this would be to have priority be a (const) string, with the same format, and then we override it as you're doing here. That way feels much more consistent (to me at least). Also I feel like this information should be present in docker info but that might just be me.

@dnephin

LGTM!

Related #35353

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Nov 16, 2017

Member

I feel like this information should be present in docker info

I don't think info needs to report the priority. It already reports the active graphdriver which is really all that is important for someone querying info.

Member

dnephin commented Nov 16, 2017

I feel like this information should be present in docker info

I don't think info needs to report the priority. It already reports the active graphdriver which is really all that is important for someone querying info.

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 16, 2017

Contributor

I reckon that the ideal way of doing this would be to have priority be a (const) string,
with the same format, and then we override it as you're doing here.

@cyphar Makes sense (except it can't be const). Please see the updated commit

Contributor

kolyshkin commented Nov 16, 2017

I reckon that the ideal way of doing this would be to have priority be a (const) string,
with the same format, and then we override it as you're doing here.

@cyphar Makes sense (except it can't be const). Please see the updated commit

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Nov 16, 2017

Member

@kolyshkin

Please add a document about this?
Probably, just adding comments to Makefile should be ok now.

then LGTM

Member

AkihiroSuda commented Nov 16, 2017

@kolyshkin

Please add a document about this?
Probably, just adding comments to Makefile should be ok now.

then LGTM

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 16, 2017

Contributor

@kolyshkin 👍, plus the comment about documenting it in a Makefile.

Contributor

cyphar commented Nov 16, 2017

@kolyshkin 👍, plus the comment about documenting it in a Makefile.

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 17, 2017

Contributor

@AkihiroSuda @cyphar you mean the top-level Makefile? Would something like this be sufficient?

diff --git a/Makefile b/Makefile
index 547230fb1..cb5980d2f 100644
--- a/Makefile
+++ b/Makefile
@@ -16,6 +16,13 @@ export DOCKER_GITCOMMIT
 # env vars passed through directly to Docker's build scripts
 # to allow things like `make KEEPBUNDLE=1 binary` easily
 # `project/PACKAGERS.md` have some limited documentation of some of these
+#
+# DOCKER_LDFLAGS can be used to pass additional parameters to -ldflags
+# option of "go build". For example, a built-in graphdriver priority list
+# can be changed during build time like this:
+#
+# make DOCKER_LDFLAGS="-X github.com/docker/docker/daemon/graphdriver.priority=overlay2,devicemapper" dynbinary
+# 
 DOCKER_ENVS := \
        -e DOCKER_CROSSPLATFORMS \
        -e BUILD_APT_MIRROR \
Contributor

kolyshkin commented Nov 17, 2017

@AkihiroSuda @cyphar you mean the top-level Makefile? Would something like this be sufficient?

diff --git a/Makefile b/Makefile
index 547230fb1..cb5980d2f 100644
--- a/Makefile
+++ b/Makefile
@@ -16,6 +16,13 @@ export DOCKER_GITCOMMIT
 # env vars passed through directly to Docker's build scripts
 # to allow things like `make KEEPBUNDLE=1 binary` easily
 # `project/PACKAGERS.md` have some limited documentation of some of these
+#
+# DOCKER_LDFLAGS can be used to pass additional parameters to -ldflags
+# option of "go build". For example, a built-in graphdriver priority list
+# can be changed during build time like this:
+#
+# make DOCKER_LDFLAGS="-X github.com/docker/docker/daemon/graphdriver.priority=overlay2,devicemapper" dynbinary
+# 
 DOCKER_ENVS := \
        -e DOCKER_CROSSPLATFORMS \
        -e BUILD_APT_MIRROR \
graphdriver: custom build-time priority list
Add a way to specify a custom graphdriver priority list
during build. This can be done with something like

  go build -ldflags "-X github.com/docker/docker/daemon/graphdriver.priority=overlay2,devicemapper"

As ldflags are already used by the engine build process, and it seems
that only one (last) `-ldflags` argument is taken into account by go,
an envoronment variable `DOCKER_LDFLAGS` is introduced in order to
be able to append some text to `-ldflags`. With this in place,
using the feature becomes

  make DOCKER_LDFLAGS="-X github.com/docker/docker/daemon/graphdriver.priority=overlay2,devicemapper" dynbinary

The idea behind this is, the priority list might be different
for different distros, so vendors are now able to change it
without patching the source code.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Nov 17, 2017

Collaborator

LGTM

Collaborator

vieux commented Nov 17, 2017

LGTM

@vieux vieux merged commit edc204b into moby:master Nov 17, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37902 has succeeded
Details
janky Jenkins build Docker-PRs 46616 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7027 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18172 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6832 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment