Skip to content
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

Merge global storage options on create #34508

Merged
merged 1 commit into from Aug 15, 2017
Merged

Conversation

@lowenna
Copy link
Contributor

lowenna commented Aug 14, 2017

Signed-off-by: John Howard jhoward@microsoft.com

Fixes #34074 and docker/for-win#999.

Ensures the daemons global storage options are merged into the user requested options. Required to ensure that the sandbox size is passed through to the driver (this applies to both WCOW and LCOW). See screenshot in docker/for-win#999 (comment) for demonstration of this fixing the issue.

ping @thaJeztah @johnstep @darrenstahlmsft

cc @vikalyan & @RustyCZ

@@ -137,6 +137,23 @@ func (daemon *Daemon) create(params types.ContainerCreateConfig, managed bool) (

container.HostConfig.StorageOpt = params.HostConfig.StorageOpt

// Fixes: https://github.com/moby/moby/issues/34074 and
// https://github.com/docker/for-win/issues/999.
// Merge the daemons storage options if they aren't already present. We only

This comment has been minimized.

Copy link
@johnstep

johnstep Aug 14, 2017

Member

Nit: daemon's

This comment has been minimized.

Copy link
@lowenna

lowenna Aug 14, 2017

Author Contributor

Punctuation police ;) Yeah, fixed.

Copy link
Member

johnstep left a comment

LGTM

Signed-off-by: John Howard <jhoward@microsoft.com>
@lowenna lowenna force-pushed the microsoft:jjh/mergestorageopt branch from 631f633 to 932ae42 Aug 14, 2017
}
for _, v := range daemon.configStore.GraphOptions {
opt := strings.SplitN(v, "=", 2)
if _, ok := container.HostConfig.StorageOpt[opt[0]]; !ok {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 14, 2017

Member

Should we explicitly limit this to only the size option? --storage-opt on the daemon is also used to initialise the storage-driver, so may include options that cannot be used per-container, or is that not a concern?

This comment has been minimized.

Copy link
@lowenna

lowenna Aug 14, 2017

Author Contributor

We don't need to be concerned with this on either of the Windows graphdrivers (WCOW and LCOW).

Nice spelling of initialise though ;)

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 15, 2017

Member

Nice spelling of initialise though ;)

🇬🇧👍

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Aug 14, 2017

This needs an update to the daemon docs as well (in the docker/cli repository); https://github.com/docker/cli/blob/master/docs/reference/commandline/dockerd.md

@lowenna

This comment has been minimized.

Copy link
Contributor Author

lowenna commented Aug 14, 2017

This needs an update to the daemon docs as well (in the docker/cli repository); https://github.com/docker/cli/blob/master/docs/reference/commandline/dockerd.md

Yeah, that can be a follow up PR 😇

@vikalyan

This comment has been minimized.

Copy link

vikalyan commented Aug 15, 2017

@jhowardmsft Super! How do I get this build on Win2k16 server?

Copy link
Member

thaJeztah left a comment

LGTM

@lowenna

This comment has been minimized.

Copy link
Contributor Author

lowenna commented Aug 15, 2017

When merged it will be on master.dockerproject.org shortly after

@lowenna

This comment has been minimized.

Copy link
Contributor Author

lowenna commented Aug 15, 2017

Jenkins or Leeroy are playing up but started RS1 manually @ https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/16302/console

@vikalyan

This comment has been minimized.

Copy link

vikalyan commented Aug 15, 2017

@jhowardmsft Sounds good. Looking forward to trying this out tomorrow! Thanks for the quick turnaround!

@lowenna

This comment has been minimized.

Copy link
Contributor Author

lowenna commented Aug 15, 2017

Success. Just not showing here for some reason.

00:56:10.411 Set build name.
00:56:10.412 New build name is '#34508'
00:56:10.416 Variable with name 'BUILD_DISPLAY_NAME' already exists, current value: '#34508', new value: '#34508'
00:56:11.530 [PostBuildScript] - Execution post build scripts.
00:56:11.531 Notifying endpoint with url 'https://leeroy.dockerproject.org/notification/jenkins'
00:56:12.141 Finished: SUCCESS

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Aug 15, 2017

Works for me 👍

@thaJeztah thaJeztah merged commit cd90284 into moby:master Aug 15, 2017
5 of 6 checks passed
5 of 6 checks passed
windowsRS1 Jenkins build is being scheduled
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 36261 has succeeded
Details
janky Jenkins build Docker-PRs 44881 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 5269 has succeeded
Details
z Jenkins build Docker-PRs-s390x 5034 has succeeded
Details
@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Aug 15, 2017

@jhowardmsft can you do a follow-up for the documentation?

@lowenna lowenna deleted the microsoft:jjh/mergestorageopt branch Aug 15, 2017
@lowenna

This comment has been minimized.

Copy link
Contributor Author

lowenna commented Aug 15, 2017

@PatrickLang

This comment has been minimized.

Copy link

PatrickLang commented Jan 12, 2018

We need to get this into a Docker-EE release. Today, the default scratch space has a hard limit of 20GB, and if anyone needs to build containers that will exceed 20GB, they need to override it.

Problems we've heard:

Build Workers
If you try to install big apps like the Visual Studio tools along with the full Windows SDKs, then you will run out of space during the docker build process. This could be a blocker for customers wanting to build containers with the right tools for Jenkins, appveyor, VSTS, and so on.

@carlfischer1

This comment has been minimized.

Copy link

carlfischer1 commented Jan 13, 2018

@PatrickLang thanks - I've opened an internal tracking issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.