Clean up inconsistent stores also in readers #1407
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Oct 21, 2022
> go mod edit -replace github.com/containers/storage=github.com/mtrmac/storage@cleanup-in-readers > make vendor Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Collaborator
Author
This was referenced Oct 24, 2022
mtrmac
force-pushed
the
cleanup-in-readers
branch
from
ce4fcd5 to
4682b8d
Compare
tyler92
reviewed
Oct 26, 2022
mtrmac
force-pushed
the
cleanup-in-readers
branch
from
4682b8d to
28cc4ec
Compare
Collaborator
Author
|
|
mtrmac
force-pushed
the
cleanup-in-readers
branch
from
5f161a7 to
c2a3d16
Compare
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Nov 1, 2022
> go mod edit -replace github.com/containers/storage=github.com/mtrmac/storage@cleanup-in-readers > make vendor Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Member
mtrmac
force-pushed
the
cleanup-in-readers
branch
from
d7eb381 to
9c473b9
Compare
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Nov 4, 2022
> go mod edit -replace github.com/containers/storage=github.com/mtrmac/storage@cleanup-in-readers > make vendor Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Collaborator
Author
|
So this was completely ineffective (calling Waiting for another round of downstream tests : containers/buildah#4368 containers/podman#16253 ; if they pass, I’m calling this ready for review. |
mtrmac
force-pushed
the
cleanup-in-readers
branch
from
9c473b9 to
2d1cbd1
Compare
Member
vrothberg
reviewed
Nov 9, 2022
Member
vrothberg
left a comment
vrothberg
left a comment
There was a problem hiding this comment.
Changes LGTM
Note that I am far from being familiar with the low-level details.
Collaborator
|
@mtrmac containers/buildah#4368 containers/podman#16253 did not run test the most recent push on this PR |
Collaborator
Author
|
@flouthoc There were no changes to this PR’s code, but, sure, let me rebase and re-test to be sure. |
mtrmac
force-pushed
the
cleanup-in-readers
branch
from
2d1cbd1 to
c5fb207
Compare
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... instead of combining control flow branches; we will change behavior on some of them. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... as an error value instead of just a boolean. That will allow extending the logic to more kinds of inconsistencies, and consolidates the specifics of the inconsistency knowledge into a single place. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
- In read-write stores, fail even readers if there are incomplete layers. Right now that would increase observed failures (OTOH with better consistency), but we'll fix that again soon - Decide whether to save read-write stores strictly based on the need to clean up, instead of cleaning up opportunistically (which is less predictable). - Correctly return the right error, depending on whether there are duplicate layers Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... instead of failing for duplicate names, and instead of ignoring the "incomplete" state of layers. Try up to 3 times if other writers are creating inconsistent state in the meantime. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
force-pushed
the
cleanup-in-readers
branch
from
c5fb207 to
7f6a670
Compare
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Nov 10, 2022
> go mod edit -replace github.com/containers/storage=github.com/mtrmac/storage@cleanup-in-readers > make vendor Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Nov 10, 2022
> go mod edit -replace github.com/containers/storage=github.com/mtrmac/storage@cleanup-in-readers > make vendor Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Nov 10, 2022
[NO NEW TESTS NEEDED] Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Member
|
LGTM |
Collaborator
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we reload the store in a reader, drop the lock, lock for writing, reload again to trigger cleanup, and then lock again for reading and proceed with the originally intended operation.
This should fix #1136 , although that’s not yet proven. (It only took 6 weeks of work to get this far…)
Built on top of #1399 and #1406 .