This form is available since Go 1.22 (see
https://tip.golang.org/ref/spec#For_range) and will probably be seen
more and more in the new code.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Removes the duplicate copy*Map function using the general function newMapFrom.
Reduces the allocation of empty maps using the copyMapPrefferingNil function.
This change may affect the behavior so that instead of an empty allocated map, a nil will be returned.
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Removes duplicate copy*Slice functions using a generic copy function
or replaces them with the slices.Clone function.
Also simplifies the stringSliceWithoutValue function.
These changes should not change the behavior.
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
because it does not return nil when the slice length is 0.
This behavior caused the slices.Clone function to allocate
a unnecessary amount of memory when the slice length is 0,
and the c/common tests failed.
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Use the "slices", "maps" standard library packages, or other
readily-available features.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Conservatively use Index* + Delete to delete the
first element where it's not obvious that the code would really
want to delete all instances.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
gofumpt is a superset of gofmt, enabling some more code formatting
rules.
This commit is brought to you by
gofumpt -w .
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The lockfile's write record is now updated prior to the actual write
operation. This ensures that, in the event of an unexpected
termination, other processes are correctly notified of an initiated
write operation.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
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>
Replace the newer nameLooksLikeID() function with calls to
stringid.Validate(), which does the same thing.
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 is AFAICS correct),
use a very traditional read-write lock to protect the fields of containerStore.
This is primarily so that all three stores use the same design.
Also explicitly document how concurrent access to fields of containerStore
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>
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>
This splits up the containers.json file in the containers store into
two, adding the new file `volatile-containers.json`. This new file is
saved using the NoSync options, which is faster but isn't robust in
the case of an unclean shutdown.
In the standard case, only containers marked as "volatile" (i.e. those
started with `--rm`) are stored in the volatile json file. This means
such containers are faster, but may get lost in case of an unclean
shutdown. This is fine for these containers though, as they are not
meant to persist.
In the transient store case, all containers are stored in the volatile
json file, and it (plus the matching lock file) is stored on runroot
(i.e. tmpfs) instead of the regular directory. This mean all
containers are fast to write, and none are persisted across boot.
Signed-off-by: Alexander Larsson <alexl@redhat.com>
This could, hypothetically, make the functions readers that are safe
to use concurrently with other readers.
Well, except for the SetBigData call...
So it's just a microoptimization.
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 constructor, so use a variant
of 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 ReloadIfChanges, and makes it clearer that the responsibility
for maintaining locking details is with the containerStore; we can change it
in a single place.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This integrates ReloadIfChanges, and makes it clearer that the responsibility
for maintaining locking details is with the containerStore; 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>
Use constants for the names of flags that we set in Flags maps that we
store in layer/image/container records, to make it easier to avoid
possible breakages due to typos in the future.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
openshift-merge-bot[bot]
Kir Kolyshkin
Jan Rodák
Miloslav Trmač
Giuseppe Scrivano
Nalin Dahyabhai
Alexander Larsson
Daniel J Walsh