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

Fix Windows layer leak when write fails #36728

Merged
merged 1 commit into from Apr 5, 2018
Merged

Conversation

@darstahl
Copy link
Contributor

darstahl commented Mar 29, 2018

Signed-off-by: Darren Stahl darst@microsoft.com

fixes #31253

- What I did

Fixed a leaked handle that was causing layer deletion to fail when the write of the tar contents failed. This was preventing layer deletion, resulting in a lot of un-removable layers left on disk.

- How I did it

Fixed a leaked handle that occurred when writeLayerFromTar failed, as the hcsshim.LayerWriter was never closed in this case.

- How to verify it

Verified locally during stress tests that were causing leaked layers. I've not leaked a layer in days, where before this fix they were leaking regularily.

- Description for the changelog

Fixed a layer leak on Windows.

@johnstep @jhowardmsft

Copy link
Member

johnstep left a comment

LGTM

@lowenna

This comment has been minimized.

Copy link
Contributor

lowenna commented Mar 29, 2018

LGTM. Nice find. (Although nit, I'm not a fan of named return variables)

@lowenna

This comment has been minimized.

Copy link
Contributor

lowenna commented Mar 29, 2018

@carlfischer1 - for backport consideration

@darstahl

This comment has been minimized.

Copy link
Contributor Author

darstahl commented Mar 29, 2018

@jhowardmsft Agreed, but in cases like this, named return values are the only safe way to prevent variable shadowing causing err in the defer to be a different err than what is being returned. I think this pattern is the safest way to do this.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 30, 2018

Codecov Report

Merging #36728 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #36728      +/-   ##
==========================================
- Coverage   35.21%   35.19%   -0.02%     
==========================================
  Files         614      614              
  Lines       45645    45645              
==========================================
- Hits        16073    16067       -6     
- Misses      27441    27447       +6     
  Partials     2131     2131
func writeLayer(layerData io.Reader, home string, id string, parentLayerPaths ...string) (int64, error) {
err := winio.EnableProcessPrivileges([]string{winio.SeBackupPrivilege, winio.SeRestorePrivilege})
func writeLayer(layerData io.Reader, home string, id string, parentLayerPaths ...string) (size int64, err error) {
err = winio.EnableProcessPrivileges([]string{winio.SeBackupPrivilege, winio.SeRestorePrivilege})

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Apr 2, 2018

Member

@jhowardmsft would renaming the err output variable to retErr address your concerns?

I know we've had cases where the output variable was overlooked, and naming it retErr instead of plain err makes it a bit more apparent that it's an output variable

This comment has been minimized.

Copy link
@darstahl

darstahl Apr 2, 2018

Author Contributor

I like that change. Changed to use retErr to prevent confusion.

@darstahl darstahl force-pushed the darstahl:LayerLeak branch from 8493f5a to 4b140ec Apr 2, 2018
@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Apr 3, 2018

ping @jhowardmsft @johnstep PTAL if the latest version still LGTY

return 0, err
}
defer func() {
if err2 := w.Close(); err2 != nil {

This comment has been minimized.

Copy link
@johnstep

johnstep Apr 3, 2018

Member

Does it make sense to rename err2 to err since that no longer conflicts with the return name?

This comment has been minimized.

Copy link
@darstahl

darstahl Apr 4, 2018

Author Contributor

Yep. Done.

Signed-off-by: Darren Stahl <darst@microsoft.com>
@darstahl darstahl force-pushed the darstahl:LayerLeak branch from 4b140ec to 1f28844 Apr 4, 2018
Copy link
Member

thaJeztah left a comment

LGTM - looks like all comments are addressed (thanks!)

@anusha-ragunathan anusha-ragunathan merged commit a826005 into moby:master Apr 5, 2018
8 checks passed
8 checks passed
codecov/patch Coverage not affected when comparing b159da1...1f28844
Details
codecov/project 35.19% (-0.02%) compared to b159da1
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 40131 has succeeded
Details
janky Jenkins build Docker-PRs 48855 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 9303 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 20308 has succeeded
Details
z Jenkins build Docker-PRs-s390x 9250 has succeeded
Details
@darstahl darstahl deleted the darstahl:LayerLeak branch Apr 5, 2018
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.