Skip to content

Fix content store bug when already exists#2642

Merged
estesp merged 2 commits intocontainerd:masterfrom
dmcgowan:fix-commit-already-exists
Sep 14, 2018
Merged

Fix content store bug when already exists#2642
estesp merged 2 commits intocontainerd:masterfrom
dmcgowan:fix-commit-already-exists

Conversation

@dmcgowan
Copy link
Copy Markdown
Member

I found a bug in the content store which was causing a race condition in some cases in setting labels. The main issue is that the Commit was not consistently returning an already exists error. The local store implementation had an incorrect error check since os.Rename does not return an exists error. The metadata implementation was only checking this condition when the local store returned the error, allowing the new Commit to overwrite data for an existing blob. This was leading to labels getting stripped allowing a garbage collection while a blob was getting read due to another object attempting to create the same blob. This problem showed up in the case where the expected value of the blob is not known beforehand, not allowing us to either skip the Write+Commit or lease the existing object.

Also fixed the content service to properly translate the error to GRPC.

This needs to be backported.

@dmcgowan dmcgowan force-pushed the fix-commit-already-exists branch from e956d53 to 2d30907 Compare September 14, 2018 00:13
@dmcgowan dmcgowan force-pushed the fix-commit-already-exists branch from 2d30907 to 15b3d0a Compare September 14, 2018 00:25
Signed-off-by: Derek McGowan <derek@mcgstyle.net>
@dmcgowan dmcgowan force-pushed the fix-commit-already-exists branch 2 times, most recently from d919dd8 to e303e4d Compare September 14, 2018 08:05
Signed-off-by: Derek McGowan <derek@mcgstyle.net>
@dmcgowan dmcgowan force-pushed the fix-commit-already-exists branch from e303e4d to 6875d3d Compare September 14, 2018 08:26
@crosbymichael
Copy link
Copy Markdown
Member

LGTM

1 similar comment
@ehazlett
Copy link
Copy Markdown
Member

LGTM

Copy link
Copy Markdown
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

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.

4 participants