layers: write read only layers to imagestore by giuseppe · Pull Request #2258 · containers/storage

giuseppe · 2025-02-14T15:48:59Z

when an imagestore is used, a R/O layer must be written to the layers.json under the imagestore, not graphroot.

The lock on the imagestore is already taken through the multipleLockFile{} mechanism in place.

Closes: #2257

openshift-ci · 2025-02-14T15:49:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2025-02-14T21:29:55Z

LGTM
@nalind @mtrmac @Luap99 PTAL

mtrmac

This does not touch drivers/overlay where I am refusing to review PRs adding new conditionals, but it’s pretty close… please find a reviewer who understands this.

I’m basically only reviewing the store.go API part.

As for the core of the thing, changing where layer metadata is stored seems like a Big Deal. If we were doing the wrong thing all this time, how come no-one has noticed until now?!

I suppose, this is preexisting (and fixing this will mean touching drivers/overlay…): I’m also pretty seriously uncomfortable about this code making a decision where to record metadata, vs. the overlay driver making a decision where to create the physical layer (or, if there are multiple layers with the same ID, which one to use), completely independently. That seems to be one of those things that is guaranteed to drift out of sync.

store.go

mtrmac · 2025-02-17T16:10:15Z

store.go

+	if !writeable && s.imageStoreDir != "" {
+		options.ImageStore = true
+	}


I don’t generally see why this logic should exist at this layer of the call stack. layerStore was already initialized with the imageStore location, it seems to me that it can make this decision on its own.

Alternatively… for imageStore, the search, and the decision where to write is managed at the store level. There is no multipleLockFile for images. This looks so different, and I don’t understand the design nearly enough to tell whether the difference is necessary.

the difference is necessary because images are always directed to the image store. Instead for layers, we write only the RO layers to the imagestore; container layers are written to the main store.

There are three calls to .create in store.go. What about the one in imageTopLayerForMapping?

should remapped images be always created on the image store?

I think it is better if they are created on the same store where their parent layer is. I've added a patch to the PR to implement this behavior

Fine, I can see how that is a good location to choose.

… but now, again, we have one part of the code that decides the location in layers.go and another here in store.go.
WDYT about layerStore explicitly tracking whether imageDir is set (compare #2258 (comment) ), and then this condition can be moved inside the new pickLocation.

and then this condition can be moved

no, that doesn’t work that easily because, AFAICS, we can’t currently tell between “ordinary” RO layers and the ID-mapping layers from imageTopLayerForMapping; we would still need to add a new option to create. Still, an thisIsAnIDMappingLayer option set in store.Go, and all location decisions centralized in pickLocations, seems worth considering.

layers.go

giuseppe · 2025-02-17T20:35:08Z

As for the core of the thing, changing where layer metadata is stored seems like a Big Deal. If we were doing the wrong thing all this time, how come no-one has noticed until now?!

nobody tried to use the image store as a self store, instead of an additional store.

I suppose, this is preexisting (and fixing this will mean touching drivers/overlay…): I’m also pretty seriously uncomfortable about this code making a decision where to record metadata, vs. the overlay driver making a decision where to create the physical layer (or, if there are multiple layers with the same ID, which one to use), completely independently. That seems to be one of those things that is guaranteed to drift out of sync.

I was against this feature when it was first added, because it is a mess and makes everything more complicated. IMO this could have been an additional store and the decision where to pull an image handled at a higher level, but it was needed for Kubernetes and now we have to deal with it.

mtrmac · 2025-02-18T14:48:13Z

now we have to deal with it.

… :| I am deeply dissatisfied with how the current implementation requires store.go, layers.go and drivers/overlay to precisely orchestrate the various layer stores, lookups, and locking, with no explicit visibility into the interdependencies. But, also, I don’t know what a good design would look like and I don’t know what it would take to figure that out.

[I guess something along the lines of: would want to just have one “ordinary” layerStore per directory, and have lookups etc. all managed by the “search list” in store go? The part that does not trivially work would, I think, be mounting a layer, which would need to be passed some kind of a “handle” to all parent layers across several stores. I don’t know what that would look like (locking? locking order?), and I can’t immediately see how to split that redesign into smaller steps.]

layers.go

giuseppe · 2025-02-19T10:30:24Z

took care of the last comments and pushed a new version

mtrmac · 2025-02-19T13:07:40Z

AFAICS #2258 (comment) and some variant of #2258 (comment) are still outstanding.

giuseppe · 2025-02-19T14:26:56Z

AFAICS #2258 (comment) and some variant of #2258 (comment) are still outstanding.

if the goal is to not have duplication of the logic in store.go, then I think we can just pass down writeble, so that now all the logic is in the same function in layers.go

giuseppe · 2025-02-19T15:03:42Z

working on a fix for the failing tests...

mtrmac

Thanks!

LGTM, just please check that I understand the removal of the “inherit from parent” code. If so, feel free to merge.

layers.go

when an imagestore is used, a R/O layer must be written to the layers.json under the imagestore, not graphroot. The lock on the imagestore is already taken through the multipleLockFile{} mechanism in place. Closes: containers#2257 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

mtrmac

/lgtm

Thanks!

I guess I tricked myself into doing a full review after all :)

openshift-ci bot added the approved label Feb 14, 2025

giuseppe mentioned this pull request Feb 14, 2025

Podman invocations using different --roots cannot share an --imagestore #2257

Closed

giuseppe force-pushed the fix-image-store-layer branch 4 times, most recently from d06baeb to 73bde82 Compare February 14, 2025 20:12

mtrmac reviewed Feb 17, 2025

View reviewed changes

giuseppe force-pushed the fix-image-store-layer branch from 73bde82 to 6fa178e Compare February 17, 2025 20:33

giuseppe force-pushed the fix-image-store-layer branch from 6fa178e to 0fa4e24 Compare February 18, 2025 15:44

mtrmac reviewed Feb 18, 2025

View reviewed changes

layers.go Outdated Show resolved Hide resolved

layers.go Outdated Show resolved Hide resolved

mtrmac reviewed Feb 18, 2025

View reviewed changes

layers.go Outdated Show resolved Hide resolved

giuseppe force-pushed the fix-image-store-layer branch from 0fa4e24 to 37f6acc Compare February 18, 2025 16:55

giuseppe force-pushed the fix-image-store-layer branch 2 times, most recently from a7b64c7 to 22169ad Compare February 19, 2025 14:26

giuseppe force-pushed the fix-image-store-layer branch 3 times, most recently from f948f09 to dc090bc Compare February 19, 2025 15:32

mtrmac reviewed Feb 19, 2025

View reviewed changes

layers.go Show resolved Hide resolved

layers.go Outdated Show resolved Hide resolved

layers.go Outdated Show resolved Hide resolved

giuseppe force-pushed the fix-image-store-layer branch from dc090bc to a50114c Compare February 19, 2025 20:36

giuseppe force-pushed the fix-image-store-layer branch from a50114c to 11a5b7b Compare February 20, 2025 08:25

mtrmac reviewed Feb 20, 2025

View reviewed changes

openshift-ci bot assigned mtrmac Feb 20, 2025

openshift-ci bot added the lgtm label Feb 20, 2025

openshift-merge-bot bot merged commit e5728ab into containers:main Feb 20, 2025
20 checks passed

Conversation

giuseppe commented Feb 14, 2025

Uh oh!

openshift-ci bot commented Feb 14, 2025

Uh oh!

rhatdan commented Feb 14, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtrmac Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

giuseppe Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

mtrmac Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

giuseppe Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

mtrmac Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giuseppe commented Feb 17, 2025

Uh oh!

mtrmac commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giuseppe commented Feb 19, 2025

Uh oh!

mtrmac commented Feb 19, 2025

Uh oh!

giuseppe commented Feb 19, 2025

Uh oh!

giuseppe commented Feb 19, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtrmac Feb 18, 2025 •

edited

Loading

mtrmac commented Feb 18, 2025 •

edited

Loading