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

(Alternative to 1140): Record layers as incomplete before trying to create them #1148

Merged
merged 6 commits into from May 2, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Feb 22, 2022

This implements the suggestion in #1139, along with EDIT partially fixing #1147 ; as an alternative to #1140 .

Conceptually I like this much better than #1140, but it’s quite a bit more invasive, and potentially risky by invoking the layer removal code in previously-unreachable situations.

Marking as draft because it’s completely untested so far.

@rhatdan
Copy link
Member

rhatdan commented Feb 22, 2022

Should we open a PR against this in Podman to get its test suite to run against it?


succeeded := false
cleanupFailureContext := ""
defer func() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that using defer means that we would try to delete the WIP layer even on a panic. That’s probably correct for some banal nil dereference, but might make things worse if the in-memory state were significantly corrupt.

@mtrmac mtrmac changed the title Record layers as incomplete before trying to create them (Alternative to 1140): Record layers as incomplete before trying to create them Feb 22, 2022
@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 22, 2022

Marking as draft because it’s completely untested so far.

Tested now, at least for success and observing the intermediate states. Note that any ambition to being crash-resilient requires a solution to #1136 .

mtrmac added a commit to mtrmac/libpod that referenced this pull request Feb 22, 2022
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 22, 2022

Should we open a PR against this in Podman to get its test suite to run against it?

Good idea, containers/podman#13316 . (And similarly for the other PR.)

@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 23, 2022

Marking as ready to review — please consider this only in tandem with #1140.

@mtrmac mtrmac marked this pull request as ready for review February 23, 2022 15:46
@mtrmac
Copy link
Collaborator Author

mtrmac commented Apr 13, 2022

@nalind @giuseppe PTAL. This might be very wrong, I’d just like to make sure something is done about these failure cases.

@nalind
Copy link
Member

nalind commented Apr 19, 2022

This looks like it has a decent chance of avoiding the types of cases that #1140 would have to clean up on behalf of binaries using a version of the storage library that doesn't include this change, so it looks like it's worth pursuing.

err must be nil at that point.

This also un-indents the success case, so that
it proceeds as a straight-line code.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
For now, this only causes two redundant saves for
non-tarball layers, which is not useful; but it will allow
us to build infrastructure for saving the incomplete record
much earlier.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that we also remove the layer from layerStore.layers
and the like.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We will need want to refer to "layer" in a defer
block, in order to delete that layer. That doesn't work
with "layer" being a named return value, because a
(return nil, -1, ...) sets "layer" to nil.

So, turn "layer" into a local variable, and use an unnamed
return value. And beause all return values must be named,
or unnamed, consistently, turn "size" and "err" also into
local variables.

Then decrease the scope of the "size" and "err" local variables
to simplify understanding the code a bit.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that we don't repeat it all over the place.

Introduce a pretty ugly cleanupFailureContext variable
for that purpose; still, it's better than copy&pasting everything.

This will be even more useful soon.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that it can be also automatically cleaned up.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented May 2, 2022

Merging both PRs with agreement from @nalind and @mtrmac

@rhatdan rhatdan merged commit 512d612 into containers:main May 2, 2022
@mtrmac mtrmac deleted the create-incomplete branch May 2, 2022 18:12
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

3 participants