API break: Remove Lockfile.Locked #1399
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Oct 18, 2022
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
This should very slightly improve efficiency because we no longer take the lock’s state mutex to determine if we can write to the store. |
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Oct 19, 2022
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Oct 19, 2022
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Oct 19, 2022
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Oct 19, 2022
DO NOT MERGE: Vendor containers/buildah#4350 Remove uses of Lockfile.Locked() One of them is entirely unnecessary, and the other one is avoidable. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Oct 19, 2022
DO NOT MERGE: Vendor containers/buildah#4350 Remove uses of Lockfile.Locked(): One of them is entirely unnecessary, and the other one is avoidable. [NO NEW TESTS NEEDED] Signed-off-by: Miloslav Trmač <mitr@redhat.com>
tyler92
reviewed
Oct 19, 2022
| @@ -302,17 +307,19 @@ func (r *containerStore) Load() error { | |||
| r.bylayer = layers | |||
| r.byname = names | |||
| if needSave { | |||
| if !lockedForWriting { | |||
| // Eventually, the callers should be modified to retry with a write lock, instead. | |||
| return errors.New("container store is inconsistent and the current caller does not hold a write lock") | |||
There was a problem hiding this comment.
How can callers know when they need to try again? I mean how to distinguish this error (which requires retry) from any other? I see ErrDuplicateImageNames in layerStore.Load()
There was a problem hiding this comment.
They can’t, currently.
I plan to add that (“try again”, instead of callers hard-coding specific error values) both to load and the callers simultaneously.
There was a problem hiding this comment.
The retries will be added in #1407 .
|
Please don’t merge until this is proven not to break Buildah and Podman tests: |
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Oct 19, 2022
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Oct 19, 2022
DO NOT MERGE: Vendor containers/buildah#4350 Remove uses of Lockfile.Locked(): One of them is entirely unnecessary, and the other one is avoidable. [NO NEW TESTS NEEDED] Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Oct 20, 2022
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
Buildah tests succeeded; Podman failures seem unrelated. |
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>
.Locked() will be going away. These are just smoke-tests, so use the .Assert* methods (which are not usable with other concurrent users). The AssertLockedForWriting() tests probably don't work on Windows, but that was the case previously as well. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead, have the callers of ReloadIfChanged provide the locking state. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead, have the callers of ReloadIfChanged provide the locking state. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead, have the callers of ReloadIfChanged provide the locking state. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It is fundamentally incorrect, and no users are remaining. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
added a commit
to mtrmac/buildah
that referenced
this pull request
Oct 21, 2022
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This was referenced Jan 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As detailed in #1388,
Lockfile.Locked()does not return the right values if there are multiple users of the same lock within a single process.So, replace the users by callers correctly tracking locking state; and add
LockFile.AssertLockedandLockfile.AssertLockedForWritingpurely for diagnostic purposes. These functions can trigger a panic in clearly invalid situations, but can’t be used by callers to avoid tracking state correctly. That’s a bit risky, so I’ll at least trigger Buildah and Podman CI runs with this changeFollowing the precedent of #1376 , this removes the
Lockedmethod, breaking API of c/storage.See individual commit messages for details.
Fixes #1388 (and makes another small step towards fixing #1136; the relevant places are now annotated).