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

Fix accessing containers and image locks #1369

Merged
merged 1 commit into from
Sep 28, 2022
Merged

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 27, 2022

Match containers and image lock to the new layer lock method. in #1351

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

@rhatdan
Copy link
Member Author

rhatdan commented Sep 27, 2022

@mtrmac PTAL

Match containers and image lock to the new layer lock method.
in containers#1351

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@@ -265,6 +263,8 @@ func newContainerStore(dir string) (ContainerStore, error) {
bylayer: make(map[string]*Container),
byname: make(map[string]*Container),
}
cstore.Lock()
defer cstore.Unlock()
if err := cstore.Load(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

It's somehow surprising that callers of a public API are responsible for locking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The {container,image,layer}Store objects implement public interfaces, but AFAICS there is no public way to access them (one would have to take a storage.Store and manually cast it to an interface that contains accessors to the three stores).

Compare #1345 .

I think the basic idea is that various storage.Store operations need to coordinate multiple operations on a single layerStore, so they need a consistent view, and a lock held across the layerStore calls.

See #1346 which tries to make the locking requirements much more explicit to make sure there are no mistakes there.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 28, 2022

@vrothberg I think @mtrmac has a PR to make all of these functions Private to containers/storage.

@nalind
Copy link
Member

nalind commented Sep 28, 2022

LGTM

@rhatdan rhatdan merged commit bb2504f into containers:main Sep 28, 2022
@mtrmac
Copy link
Collaborator

mtrmac commented Sep 29, 2022

@rhatdan thanks!

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