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>
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>
lockfile.LastWrite allows callers to explicitly
track state across *LockFile.{GetLastWrite,ModifiedSince,
RecordWrite}.
*LockFile.{Modified,Touch} are now implemented
in terms of these APIs.
NOTE: This changes behavior, Touch() now updates
the built-in state only on success. Either way there
can be suprious modification reports.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Add lockfile.GetLockFile and lockfile.GetROLockFile (with upper-case
F, as opposed to existing methods with lower-case f) which return a
*LockFile struct, instead of the Locker interface.
This follows the general Go design heuristic: "return structs, accept
interfaces"; more importantly, it allows us to add more methods to the lock
object without breaking the Go API by extending a pre-existing interface.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This actually relaxes some of the rules, contrary to previous
documentation, but some of those relaxed rules were already
assumed.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This will replace Lockfile.Locked for the sanity-checking
purposes.
It is, just like Locked never was, NOT a way for a caller
to check the current state. Hence the panic() instead of an
error return.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
If we ever check the last-writer of a lock file and get back a zero
value, don't only check if subsequent last-writer values match its first
zero bytes, which is always true, as a test to see if the last-writer
value has remained unchanged.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
A newly created lock-file is of size zero, which happens for example
if you reset the storage state. In this state, lockfile.Modified()
always returns true, because it thinks any short read means something
changed.
I ran into this with the image store, which was empty (using images
from a different store), and it keept re-reading the image store all
the time even if nothing changed (i.e. both lock file reads returned
zero bytes).
The proper way to handle this seems to be to compare partial reads
also.
Signed-off-by: Alexander Larsson <alexl@redhat.com>
There's at least one Go platform where that is not true.
Making that assumption without any warning is just a trap for future readers.
Instead, just do the obviously correct thing, per the API documentation.
On macOS, this compiles to _literally exactly the same binary code_.
So, let the compiler be clever so that we don't have to.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
unix.Open returns a bare error (like EPERM). This is worked around in
on user, (*lockfile).lock, where we panic, but not in the other user,
createLockerForPath, and as a result such a bare error might be returned
from GetLockfile or GetROLockfile.
Wrap it into os.PathError and remove the workaround from lockfile.lock.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 162a0bf730, openLock creates a file in case ro
argument is set, but the file is created with 0 permission bits.
Fix this, and unify the file creation logic while at it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 162a0bf730, openLock creates a file even if ro is
set, but the documentation says otherwise.
Since commit 16de6d32b4, openLock also creates file's parent
directories if needed, but the documentation doesn't say that.
Fix both issues.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.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>
... by including the time, PID, and a per-process counter.
Pad the rest with rando values for continuity.
This feels a bit like overkill, OTOH adding the PID might be
useful for debugging, so there's that.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to have only one place that generates the value,
and make it easier to change how it is constructed.
For now, this is just a simple function call forwarding,
but we'll change that shortly.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Users are setting up readonly shares and sometimes they forget, or do
not even know that they need to create the lockfile.
Even in the quay.io/podman/stable images Dockerfile, I have to manually
create these files.
Fixes: https://github.com/containers/storage/issues/1029
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
If the lockfile directory gets removed but the container runtime is
still running with an open storage instance, then we can trigger a
runtime panic by:
1. Starting CRI-O
2. Removing the lockfile dir, e.g.:
`sudo rm -rf /var/lib/containers/storage/overlay-images`
3. Running `crictl images`
```
panic: error opening "/var/lib/containers/storage/overlay-images/images.lock": no such file or directory
goroutine 93 [running]:
github.com/containers/storage/pkg/lockfile.(*lockfile).lock(0xc0006b83c0, 0x2000000)
/go/src/github.com/cri-o/cri-o/vendor/github.com/containers/storage/pkg/lockfile/lockfile_unix.go:107 +0x388
github.com/containers/storage/pkg/lockfile.(*lockfile).RLock(0xc0006b83c0)
/go/src/github.com/cri-o/cri-o/vendor/github.com/containers/storage/pkg/lockfile/lockfile_unix.go:146 +0x37
github.com/containers/storage.(*imageStore).RLock(0xc00006e420)
/go/src/github.com/cri-o/cri-o/vendor/github.com/containers/storage/images.go:778 +0x33
github.com/containers/storage.(*store).Images(0xc0003f3b80, 0x0, 0x0, 0x0, 0x0, 0x0)
/go/src/github.com/cri-o/cri-o/vendor/github.com/containers/storage/store.go:3089 +0x1dc
```
We now ensure the parent lockfile directory, but only if it does not
exist to not degrade the performance of c/stroage.
Unit tests around that case have been added as well.
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
drop a call to Seek and use the Pread and Pwrite syscalls instead that
already accept an offset.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Fix a race where the state lock in a Unixy lockfile structure wasn't
being used to protect access to all of the structure's state fields.
Fix usage of sync/atomic in the corresponding unit tests to not mix
atomic adds and reads with non-atomic reads, something which the race
detector complained about.
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
do not flush the modified metadata to disk each time the store is
modified.
It is not needed for other processes stat'ing the lock file to see the
updated mtime.
introduced with commit 9b0633295b
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
We're seeing some panics with Podman on Ubuntu 19 in this code,
and debugging without knowing what's going wrong is very
difficult.
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
Kir Kolyshkin