Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move locking responsibility from store.go to the container/image/layer stores #1395

Merged
merged 32 commits into from Oct 18, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Oct 17, 2022

Final big part of the preparatory infrastructure work on locking.

Move the conceptual responsibility for locking/loading containerStore, imageStore and layerStore from the caller (store) into the three individual C/I/L objects. And in those objects, centralize it.

I.e. this goes from

func (s *store) method(…) {
    …
    layerStore.Lock()
    defer layerStore.Unlock()
    if err := store.ReloadIfChanged(); err != nil { … }

to

func (s *store) method(…) {
    …
    if err := layerStore.startWriting(); err != nil { … }
    defer layerStore.stopWriting()

that will, finally, allow us to change the design in a single place, startReading / startWriting. E.g. to:


This should not change observable behavior, apart from the “Remove a redundant rwImageStore.Save() call” commit.

See individual commit messages for details.

(The need to change every lock caller in store by this PR is the primary motivation for preparing #1383 / #1384 , which decreased the number of places to update.)

This is organized by the individual store, changing each one separately; i.e. each conceptual change is repeated three times. If desired, I can instead reorganize this to do same conceptual change in a single commit, across all three stores.

This ends up removing roFileBasedStore / rwFileBasedStore, and declaring the {start,stop}{Reading,Writing} methods in each individual store. If preferred, I could change this PR to keep (or re-introduce) those interfaces again.

... so that we can modify/replace them.

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>
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>
The only callers are internal, have them access r.lockfile directly.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Callers should just use startReading/startWriting.

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>
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>
... 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>
... so that we can modify/replace them.

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>
... 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>
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>
The only callers are internal, have them access r.lockfile directly.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Callers should just use startReading/startWriting.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
rwImageStore.Create() already calls it.

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>
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>
... 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>
... so that we can modify/replace them.

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>
... for a bit of extra safety.

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>
The only callers are internal, have them access r.lockfile directly.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It is an internal helper for ReloadIfChanged, with
no external users.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Callers should just use startReading/startWriting.

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>
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>
.. 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>
Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rhatdan rhatdan merged commit bd3bbad into containers:main Oct 18, 2022
@mtrmac mtrmac deleted the locking-responsibility branch October 18, 2022 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants