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

Windows Daemon should respect DOCKER_TMPDIR #35077

Merged
merged 1 commit into from Oct 20, 2017

Conversation

@ryansimmen
Contributor

ryansimmen commented Oct 3, 2017

- What I did
Fixes #35076 by instructing the windows daemon to utilize the tmp directory under data-root and respect DOCKER_TMPDIR if it exists.

- How I did it
Leveraged Windows TMP and TEMP environment variables.

- How to verify it

  1. Install Docker for Windows
  2. Pull a large image
  3. Observe disk activity via Resource Monitor

- Description for the changelog
Bug fix: Windows Daemon will now utilize the tmp directory under data-root and respect DOCKER_TMPDIR if it exists instead of leveraging the Temp directory under C:\Windows.

@darstahl

This comment has been minimized.

Show comment
Hide comment
@darstahl

darstahl Oct 6, 2017

Contributor

@jhowardmsft @johnstep PTAL

hcsshim uses Golang's built in tmpdir utilities, which create a tmp dir at the env TMP location during layer extraction. This doesn't currently respect Moby's tmpdir settings.

This would also fix any other dependencies using Golang's built in tmpdir utilities.

Note that Golang will not attempt to create TMP if it does not exist (layer extraction will fail in this case). We might want to add a check here to either bail out if TMP doesn't exist, create it, or not set TMP if DOCKER_TMPDIR does not exist.

Contributor

darstahl commented Oct 6, 2017

@jhowardmsft @johnstep PTAL

hcsshim uses Golang's built in tmpdir utilities, which create a tmp dir at the env TMP location during layer extraction. This doesn't currently respect Moby's tmpdir settings.

This would also fix any other dependencies using Golang's built in tmpdir utilities.

Note that Golang will not attempt to create TMP if it does not exist (layer extraction will fail in this case). We might want to add a check here to either bail out if TMP doesn't exist, create it, or not set TMP if DOCKER_TMPDIR does not exist.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Oct 6, 2017

Contributor

LGTM. But agree with @darrenstahlmsft that it might be good to stat the directory/create it if it doesn't exist, or error out if it doesn't exist. I don't think it matters which though.

Contributor

jhowardmsft commented Oct 6, 2017

LGTM. But agree with @darrenstahlmsft that it might be good to stat the directory/create it if it doesn't exist, or error out if it doesn't exist. I don't think it matters which though.

@ryansimmen

This comment has been minimized.

Show comment
Hide comment
@ryansimmen

ryansimmen Oct 19, 2017

Contributor

@jhowardmsft the code will now create the directory if it does not exist and error out if it cannot create it.

Contributor

ryansimmen commented Oct 19, 2017

@jhowardmsft the code will now create the directory if it does not exist and error out if it cannot create it.

@ryansimmen ryansimmen closed this Oct 19, 2017

@ryansimmen ryansimmen reopened this Oct 19, 2017

Windows Daemon should respect DOCKER_TMPDIR
Signed-off-by: Ryan Simmen <ryan.simmen@gmail.com>
@yongtang

LGTM

@yongtang yongtang merged commit ab0eb8f into moby:master Oct 20, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37445 has succeeded
Details
janky Jenkins build Docker-PRs 46136 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6526 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17721 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6335 has succeeded
Details

@ryansimmen ryansimmen deleted the ryansimmen:35076-WindowsDaemonTmpDir branch Oct 24, 2017

@PatrickLang

This comment has been minimized.

Show comment
Hide comment
@PatrickLang

PatrickLang Jan 9, 2018

We need to get this into a new EE release. Azure VMs are deploying with a 30GB C:, but 100GB D: . Many customers are putting DOCKER_ROOT on d: because there is more space available, its faster & cheaper. d: is local SSD storage so its perfect for non-persistent container storage

PatrickLang commented Jan 9, 2018

We need to get this into a new EE release. Azure VMs are deploying with a 30GB C:, but 100GB D: . Many customers are putting DOCKER_ROOT on d: because there is more space available, its faster & cheaper. d: is local SSD storage so its perfect for non-persistent container storage

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 9, 2018

Member

@PatrickLang I can add an internal tracking issue for that, but catch up on slack internally to discuss prioritisation

Member

thaJeztah commented Jan 9, 2018

@PatrickLang I can add an internal tracking issue for that, but catch up on slack internally to discuss prioritisation

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