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

Cleanup docker tmp dir on start #31741

Merged
merged 1 commit into from Mar 30, 2017

Conversation

@mlaventure
Copy link
Contributor

commented Mar 10, 2017

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

--

This should free up some space by removing old build contexts.

Copy link
Member

left a comment

LGTM 🐸

var tmpDir string
if tmpDir = os.Getenv("DOCKER_TMPDIR"); tmpDir == "" {
tmpDir = filepath.Join(rootDir, "tmp")
os.RemoveAll(tmpDir)

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 10, 2017

Member

We should have the Docker for Mac team have a look as well; IIRC they store other data in this directory, and I don't know if that can be removed; IIRC, we use specific prefixes for partial pulls, and build contexts, so we could be more specific and just remove those directories

In addition, we recently merged a PR for scenarios where docker when running dind (see #31265); which also removed the content instead of the directory itself would that make sense?

This comment has been minimized.

Copy link
@mlaventure

mlaventure Mar 10, 2017

Author Contributor

👌 for waiting on D4M (ping @justincormack ?) to have a look. Although, it's a temp directory, nothing is there should be assumed to survive be valid after a restart ;-)

Regarding the second point, I'm not sure I understand. If you remove the directory, you also remove its content.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 10, 2017

Member

second point is for a case where you'd bind mount /var/run/docker/tmp in that case, removing the directory itself would fail

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 10, 2017

Member

oh, I see I typed an also sorry, I see the confusion

This comment has been minimized.

Copy link
@mlaventure

mlaventure Mar 10, 2017

Author Contributor

@thaJeztah I only delete if the path is /var/lib/docker/tmp though. If the user specified a different path, I let it be.

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

I'm not sure how large the tmpdir would be 😅 , but if it can be large, we could call rename(2) first and then remove it in background for better performance.

@mlaventure

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2017

@AkihiroSuda mine was 2/3 GB, I'll update the PR with your suggestion :)

@mlaventure mlaventure force-pushed the mlaventure:cleanup-tmp-on-start branch from f7f47e2 to 84bb85d Mar 13, 2017
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure mlaventure force-pushed the mlaventure:cleanup-tmp-on-start branch from 84bb85d to 9c451ad Mar 16, 2017
@vdemeester

This comment has been minimized.

Copy link
Member

commented Mar 26, 2017

var tmpDir string
if tmpDir = os.Getenv("DOCKER_TMPDIR"); tmpDir == "" {
tmpDir = filepath.Join(rootDir, "tmp")
newName := tmpDir + "-old"

This comment has been minimized.

Copy link
@AkihiroSuda

AkihiroSuda Mar 26, 2017

Member

If a daemon crashes before completely removing tmpDir + "-old", the next daemon execution fails to execute os.Rename(tmpDir, tmpDir+"-old"), and hence the old tmpDir + "-old" will be left alone.

So I think we need os.RemoveAll(tmpDir + "-old") here.

This comment has been minimized.

Copy link
@mlaventure

mlaventure Mar 29, 2017

Author Contributor

and what if that one fails? ;-)

Copy link
Member

left a comment

LGTM

@thaJeztah thaJeztah merged commit f16d1a4 into moby:master Mar 30, 2017
6 checks passed
6 checks passed
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31804 has succeeded
Details
janky Jenkins build Docker-PRs 40714 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 515 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11505 has succeeded
Details
z Jenkins build Docker-PRs-s390x 406 has succeeded
Details
@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Mar 30, 2017
@mlaventure mlaventure deleted the mlaventure:cleanup-tmp-on-start branch Mar 30, 2017
liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017
(backport from moby#31741)

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
Signed-off-by: Wentao Zhang <zhangwentao234@huawei.com>
(cherry-pick from 9c451ad)

Conflicts:
	daemon/daemon.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.