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 image pull after a failure #1916

Merged
merged 1 commit into from Dec 14, 2017

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Dec 13, 2017

Fixes #1893

When resuming from a failed pull the local content store Writer.Truncate() was not seeking to the proper position in the file. This caused writes to happen after the previously written content, instead of at the start of the file. Ref https://golang.org/pkg/os/#File.Truncate ( It does not change the I/O offset)

Truncate() is called from https://github.com/containerd/containerd/blob/v1.0.0/services/content/service.go#L395-L399.

When resuming from a failed pull writer.Truncate() was not
seeking to the proper position in the file. This caused writes to
happen after the previously written content, instead of at the start
of the file.

Signed-off-by: Daniel Nephin <dnephin@gmail.com>
@dnephin
Copy link
Contributor Author

dnephin commented Dec 14, 2017

appveyor timed out, doesn't seem related to the changes in this PR

@mlaventure
Copy link
Contributor

Wow, some of the tests are taking a minute to ran, hence the timeout. Might be due to a recent change

@mlaventure
Copy link
Contributor

Restart the build, hopefully it was just an issue on appveyor side

@codecov-io
Copy link

Codecov Report

Merging #1916 into master will increase coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1916      +/-   ##
==========================================
+ Coverage   47.29%   47.32%   +0.02%     
==========================================
  Files          89       89              
  Lines        8830     8833       +3     
==========================================
+ Hits         4176     4180       +4     
+ Misses       3968     3965       -3     
- Partials      686      688       +2
Flag Coverage Δ
#linux 50.7% <0%> (+0.04%) ⬆️
#windows 42.28% <0%> (+0.03%) ⬆️
Impacted Files Coverage Δ
content/local/writer.go 52.63% <0%> (+2.63%) ⬆️
content/helpers.go 32.87% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03bc5e9...9184908. Read the comment docs.

@Random-Liu
Copy link
Member

Random-Liu commented Dec 14, 2017

Should we cherry pick this? This seems not good to me.
@crosbymichael @stevvooe

@crosbymichael
Copy link
Member

LGTM

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estesp estesp merged commit 5971d36 into containerd:master Dec 14, 2017
@dnephin dnephin deleted the fix-pull-after-failure branch December 14, 2017 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants