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 is AFAICS correct),
use a very traditional read-write lock to protect the fields of imageStore.
This is primarily so that all three stores use the same design.
Also explicitly document how concurrent access to fields of imageStore
is managed.
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>
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>
They don't, as the code assumes, test whether the store
is writable; they return true for read-only stores as well.
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>
... for a bit of extra safety. That requires us to be a bit
more explicit in one of the users.
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 imageStore; 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 imageStore; 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>
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>
Don't call Validate() on a digest a second time if it returns an error
the first time, when it's easy to just remember the error it returned
the first time.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
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>
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>
The "if err == nil" is always true, because
there are no prior assignments to err; but it
creates a new scope, making the "err = r.Save()"
call write to an ignored variable.
Remove the unnecessary condition to avoid that.
(Note that the code still continues on after a failure
to save, i.e. it returns a reference to a created in-memory-only
container along with the error value.)
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
currently it is not possible to delete a mapped layer as it is always
referenced by the image.
Change it allow deleting the mapping if it is not used by any
container or other layers.
It will be useful in CRI-O as a lot of thrown away containers will be
created and the mapped images would keep growing without a way of
cleaning them up.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
This commit adds a new `NamesHistory` field to the `images.json`, which
is basically a deduped list of names the image had in the past. The
first entry of the list is the latest history entry.
The main use case for this feature is to tell the end-user which
names/tags an image had in the past if it does not contain any `names`
any more.
Detailed use case:
1. Pulling `image:v1` into the local registry: `names: [ "image:v1" ]`
2. Pushing a new image as `image:v1` into the remote registry
3. Pulling `image:v1` again into the local registry:
- first image: `names: [ "image:v1" ]`
- previous v1 image: `names: [], names-history: [ "image:v1" ]`
4. An consumer of the storage API can now process the image name and
still display `image` as REPOSITORY, like:
* Before:
```
> podman images
REPOSITORY TAG IMAGE ID CREATED SIZE
image v1 25b62d1b654a 13 seconds ago 2.07 kB
<none> <none> b134eff7b955 17 seconds ago 2.07 kB
```
* After:
```
> podman images
REPOSITORY TAG IMAGE ID CREATED SIZE
image v1 25b62d1b654a 13 seconds ago 2.07 kB
image <none> b134eff7b955 17 seconds ago 2.07 kB
```
5. Since the `NamesHistory` is a slice we would be able to tell the
end-user which names an image ID had before.
The change should be backwards compatible with previous versions of
containers/storage.
Signed-off-by: Sascha Grunert <sgrunert@suse.com>
Wrap the ID or the digest to ErrImageUnknown errors to avoid ambiguity
which image is unknown. Consumers of the storage library may have
multiple subsequent calls to the storage API where it can be unclear
which image is unknown. Wrapping the ID and digest attempts to avoid
this ambiguity.
Related-to: github.com/containers/libpod/issues/2979
Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
Use RLock() to lock stores that we know are read-only, and panic in
Lock() if we know that we're not a read-write lock.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Add a RecursiveLock() API to allow for recursive acquisitions of a
writer lock within the same process space. This is yet another
requirement for the copy-detection mechanism in containers/image where
multiple goroutines can be pulling the same blob. Having a recursive
lock avoids a complex synchronization mechanism as the commit order is
determinted by the corresponding index in the image.
Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
Nalin Dahyabhai