We previously started "pulling up" images when we changed their names,
and started denying the presence of images in read-only stores which
shared their ID with an image in the read-write store, so that it would
be possible to "remove" names from an image in read-only storage. We
forgot about the Flags field, so start pulling that up, too.
Do all of the above when we're asked to create an image, since denying
the presence of images with the same ID in read-only stores would
prevent us from finding the image by any of the names that it "had" just
a moment before we created the new record.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
When updateNames() copies an image's record from a read-only store into
the read-write store, copy the accompanying data as well.
Add fields for setting data items at creation-time to LayerOptions,
ImageOptions, and ContainerOptions to make this easier for us and our
consumers.
Replace the store-specific Create() (and the one CreateWithFlags() and
Put()) with private create() and put() methods, since they're not
intended for consumption outside of this package, and add Flags to the
options structures we pass into those methods. In create() methods,
make copies of those passed-in options structures before modifying any
of their contents.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
In that case, we can just get read locks, confirm that nothing has changed,
and continue; no need for any serialization on exclusively holding
loadMut / inProcessLock.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead of basing this on exclusivity loading via loadMut (which was incorrect,
because contrary to the original design, the r.layerspathModified
check in r.Modified() could trigger during the lifetime of a read lock)
use a very traditional read-write lock to protect the fields of imageStore.
Also explicitly document how concurrent access to fields of imageStore
is managed.
Note that for the btrfs and zfs graph drivers, Diff() can trigger
Mount() and unmount() in a way that violates the locking design.
That's not fixed in this PR.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This should be fixed, it just seems too hard to do without
breaking API (and performance).
So, just be clear about that to warn future readers.
It's tracked in https://github.com/containers/storage/issues/1379 .
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We can't safely do that because the read-only callers don't allow us
to write to layerStore state.
Luckily, with the recent changes to Mounted, we don't really need to
reload in those places.
Also, fairly extensively document the locking design or implications
for users.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead of reading that value, releasing the mount lock,
and then unmounting, provide a "conditional" unmount mode.
And use that in the existing "loop unmounting" code.
That's at least safer against concurrent processes unmounting
the same layer. But the callers that try to "really unmount"
the layer in a loop are still possibly racing against other processes
trying to mount the layer in the meantime.
I'm not quite sure that we need the "conditional" parameter as an
explicit choice; it seems fairly likely that Umount() should just fail
with ErrLayerNotMounted for all !force callers. I chose to use the flag
to be conservative WRT possible unknown constraints.
Similarly, it's not very clear to me that the unmount loops need to exist;
maybe they should just be unmount(force=true, conditional=true).
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The lockfile we use propertly handles the case that we Touch() it. In
other words, a later Modified() call will return false.
However, we're also looking at the mtime, which was failing. This
uses the new AtomicWriteFileWithOpts() feature to also record the
mtime of the file we write on updates.
Signed-off-by: Alexander Larsson <alexl@redhat.com>
This was using the graphDriver field without locks, and the graph driver itself,
while the implementation assumed exclusivity.
Luckily all callers are actually holding the layer store lock for writing, so
use that for exclusion. (layerStore already seems to extensively assume
that locking the layer store for writing guarantees exclusive access to the graph driver,
and because we always recreate a layer store after recreating the graph driver,
that is true in practice.)
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Only update recorded LastWrite values _after_ we succesfully reload
container/image/layer stores; so, if the reload fails, the functions
will keep failing instead of using obsolete (and possibly partially loaded
and completely invalid) data.
Also, further improve mount change tracking, so that if layerStore.load()
loads it, we don't reload it afterwards.
This does not change the store.graphLock users; they will need to be cleaned up
more comprehensively, and separately, in the future.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
If the mounts file is changed, don't reload all of the layers
as well. This is more efficient, and it will allow us to better
track/update r.lastWrite and r.mountsLastWrite in the future.
Exiting code calling reloadMountsIfChanged() indicates that this
must already be safe.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
They are a shared state across all users of the *LockFile in the process,
and therefore incorrect to use for any single consumer for change tracking.
Direct users to user the new GetLastWrite, ModifiedSince, and RecordWrite,
instead, and convert all c/storage users.
In addition to just being correct, the new API is also more efficient:
- We now initialize stores with GetLastWrite before loading state;
so, we don't trigger an immediate reload on the _second_ use of a store,
due to the problematic semantics of .Modified().
- Unlike Modified() and Touch(), the new APi can be safely used without
locking *LockFile.stateMutex, for a tiny speedup.
The conversion is, for now, trivial, just immediately updating the lastWrite
value after the ModifiedSince call. That will get better.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
- Don't exit after saving only one of the locations
- Only touch the lock once if saving both of the locations
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This looks in the container store for existing data dirs with ids not in
the container files and removes them. It also adds an (optional) driver
method to list available layers, then uses this and compares it to the
layers json file and removes layers that are not references.
Losing track of containers and layers can potentially happen in the
case of some kind of unclean shutdown, but mainly it happens at reboot
when using transient storage mode. Such users are recommended to run
a garbage collect at boot.
Signed-off-by: Alexander Larsson <alexl@redhat.com>
We store information about the layers in two files, layers.json and
volatile-layers.json. This allows us to treat the two stores
differently, for example saving the volatile json with the NoSync
option, which is faster (but less robust).
In normal mode we store only layers for the containers that are marked
volatile (`--rm`), as these are not expected to persist anyway. This way
informantion about such containers are faster to save, and in case
of an unclean shutdown we only risk loss of information about other
such volatile containers.
In transient mode all container layers (but not image layers) are
stored in the volatile json, and additionally it is stored in tmpfs.
This improving performance as well as automatically making sure no
container layers are persisted across boots.
Signed-off-by: Alexander Larsson <alexl@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>
- 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>
... 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>
... 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>
.. to be closer to the lock / load set of methods.
Only moves unchanged code, should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
They are now only used in the constructors, so use a variant
of startReading/startWriting instead. This code path is not
performance-critical, so let's share as much code as possible
to ensure consistency in locking.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It is done implicitly by all writers already.
Also fix the documentation not to point at an explicit Touch(),
which is not actually necessary.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Exposing the internals of the lock is not necessary, and exposes
too many implementation details.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This integrates ReloadIfChanged, and makes it clearer that the responsibility
for maintaining locking details is with the layerStore; we can change it
in a single place.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This integrates ReloadIfChanged, and makes it clearer that the responsibility
for maintaining locking details is with the layerStore; we can change it
in a single place.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead of going
3 store methods
store.updateNames+enum
3 sub-store methods
subStore.updateNames+enum
applyNameOperation+enum,
simplify to
3 store methods
store.updateNames+enum
subStore.updateNames+enum
applyNameOperation+enum,
Should not change behavior. Looking purely at updateNameOperation,
invalid values would now be detected after doing more work,
but there is no way for an external caller to trigger use of
an invalid value anyway.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
There is no public way to obtain implementations of these interfaces; so
replace the public interfaces with private ones, to make it clear that we
_can_ modify them.
For formal API compatibility, preserve the old interface definitions
as copies.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... instead of doing it in callers.
This makes it clearer that the Touch() always happens,
and it adds error handling.
OTOH there might be some undocumented reason why the Touch()
should happen in callers even if the saveMounts() call is
not reached.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
AtomicWriteFile truly is atomic, it only changes the file
on success. So there's no point notifying other processes about
a changed file if we failed, they are going to see the same JSON data.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
There was a possibility to panic due to such behavior:
attempted to update last-writer in lockfile without the write lock
Fixes: https://github.com/containers/storage/issues/1324
Signed-off-by: Mikhail Khachayants <tyler92@inbox.ru>
When removing layers, try to remove layers before removing their
parents, as the lower-level driver may enforce the dependency beyond
what the higher-level logic does or knows.
Do this by sorting by creation time as an attempt at flattening the
layer tree into an ordered list. It won't be correct if a parent layer
needed to be recreated, but it's more likely to be correct otherwise.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
There was a race condition if a goroutine accessed to layerStore public
methods under RO lock and at the same time ReloadIfChanged was called.
In real life, it can occurr when there are two concurrent PlayKube
requests in Podman.
Signed-off-by: Mikhail Khachayants <tyler92@inbox.ru>
We now use the golang error wrapping format specifier `%w` instead of the
deprecated github.com/pkg/errors package.
Signed-off-by: Sascha Grunert <sgrunert@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>
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>
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>
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>
When we're creating a layer using another layer as a template, add the
new layer's uncompressed and compressed digest to the maps we use to
index layers using those digests.
When we forgot to do that, searching for a layer by either would still
turn up the original template, so this didn't really break anything.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
We mistakenly mixed up the uncompressed and compressed digests when
populating the by-uncompressed-digest map.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
When we create a copy of an image's top layer that's intended to be
identical to the top layer, except for having some set of ID mappings
already applied to it, copy over the template layer's compressed and
uncompressed digest and size information, compression information,
tar-split data, and lists of used UIDs and GIDs, if we have them.
The lack of sizing information was forcing ImageSize() to regenerate the
diffs to determine the size of the mapped layers, which shouldn't have
been necessary.
Teach the overlay DiffGetter to look for files in the diff directories
of lower layers if we can't find them in the current layer, so that
tar-split can retrieve content that we didn't have to pull up.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Adds AddNames and RemoveNames so operations which are invoked in parallel
manner can use it without destroying names from storage.
For instance
We are deleting names which were already written in store.
This creates faulty behavior when builds are invoked in parallel manner, as
this removes names for other builds.
To fix this behavior we must append to already written names and
override if needed. But this should be optional and not break public API
Following patch will be used by parallel operations at podman or buildah end, directly or indirectly.
Signed-off-by: Aditya R <arajan@redhat.com>
Account for the "diff != nil" path; try to remove even
the metadata of a layer on a failure saving.
Not that there's _much_ hope to be able to save
the version without the new layer when we weren't able
to save the version with the new layer.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Currently, if the attempts to recover from a failure
themselves fail, we don't record that at all.
That makes diagnosing the situation, or hypothetically
detecting that the cleanup could never work, much
harder.
So, log the errors.
Alternatively, we could include those failures as extra
text in the returned error; that's less likely to be lost
(e.g. a Go caller would have the extra text available, without
setting up extra infrastructure to capture logs), but possibly
harder to follow.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Don't ReadAll() from a Reader to create a buffer and then create another
Reader to read from that buffer.
Don't close a file and a decompressor that we're using to read the
file's contents when we we may still need to read from them after the
current function returns.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
This allows callers of Store.PutLayer to provide the values if
they have already computed them, so that ApplyDiff does not need
to compute them again.
This could quite significantly reduce CPU usage.
The code is a bit clumsy in the use of compressedWriter; it
might make sense to implement a ReadCounter counterpart to the
existing WriteCounter.
(Note that it remains the case that during pulls, both c/image/storage
and ApplyDiff decompress the stream; c/image/storage stores the
compressed, not the decompressed, version in a temporary file.
Nothing changes about that here - it's not obvious that changing it
is worth it, and anyway it's a different concept for a different
PR/discussion.)
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Have it count the input to idLogger instead of uncompressedDigester;
they should get exactly the same data, but we are going to make
uncompressedDigester optional.
Also make the uncompressedDigester use a separate line so that we
can later change it more easily.
Should not change (observable) behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It is a tiny bit expensive, but most importantly
this moves the uses of {un,}compressedDigester so that
we can later make them optional.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Have one section deal with detecting compression and re-assembling
the original stream, and another with computing the length and digest
of the original stream.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
When we're applying a diff, we compress the headers and stash them
elsewhere so that we can use them to correctly reconstruct the layer if
we need to extract its contents later.
By default, the compression uses a 1MB block, and up to GOMAXPROCS
threads, which results in allocating GOMAXPROCS megabytes of memory up
front. That can be much more than we need, especially if the system has
many, many cores. Drop it down to 1 megabyte.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
This helps long running processes like CRI-O to determine changes to the
local storage, while handling corrupted images automatically.
The corresponding fix in Podman [0] handles corrupt layers by reloading
the image. This does not work for CRI-O, because it will not reload the
storage on subsequent calls of pullImage if no storage modification has
been done.
[0]: b4bd886fcc
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Currently, layers aquired from additional layer store cannot be exported
(e.g. `podman save`, `podman push`).
This is because the current additional layer store exposes only *extracted view*
of layers. Tar is not reproducible so the runtime cannot reproduce the tar
archive that has the same diff ID as the original.
This commit solves this issue by introducing a new API "`blob`" to the
additional layer store. This file exposes the raw contents of that layer. When
*(c/storage).layerStore.Diff is called, it acquires the diff contents from this
`blob` file which the same digest as the original layer.
Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
Nalin Dahyabhai