store: add support to split filesystem using `imagestore` by flouthoc · Pull Request #1549 · containers/storage

flouthoc · 2023-04-03T07:48:16Z

Allow storage users to split the filesystem of containers vs image store, imagestore if configured will pull images in image storage instead of the graphRoot while keeping the other parts still in the originally configured graphRoot.

rhatdan · 2023-04-03T12:33:33Z

I would prefer this to be ImageStore rather then PullStore. You even differentiate between the two within the code.

rhatdan · 2023-04-03T12:35:21Z

	imgStoreRoot := s.pullStore
	if imgStoreRoot == "" {
		imgStoreRoot = s.graphRoot
	}

flouthoc · 2023-04-03T15:26:06Z

imagestore SGTM I'll amend this.

giuseppe

Will the RO layer be found when Image Store is used?

e.g. another client is configured to not use the Image Store, and will create all the layers in the regular store. How would it work when the Image Store is set

pkg/config/config.go

flouthoc · 2023-04-04T12:59:07Z

Will the RO layer be found when Image Store is used?

e.g. another client is configured to not use the Image Store, and will create all the layers in the regular store. How would it work when the Image Store is set

I tried replaying this scenario, no RO layer will be not present if user switches to --imagestore in between, since --imagestore switches to a completely new image store. In such cases if users wants to use layers from previous store he should use additional-image-store and use original store as an additional-image-store manually.

giuseppe · 2023-04-04T13:34:57Z

Will the RO layer be found when Image Store is used?
e.g. another client is configured to not use the Image Store, and will create all the layers in the regular store. How would it work when the Image Store is set

I tried replaying this scenario, no RO layer will be not present if user switches to --imagestore in between, since --imagestore switches to a completely new image store. In such cases if users wants to use layers from previous store he should use additional-image-store and use original store as an additional-image-store manually.

I'd prefer the opposite to happen. If a --imagestore is specified, then images are pulled there and it is internally handled as an additional image store. The lookup would work in the same way it does now, first looking in the current layers then looking it up in the additional image stores.

When I build from a Containerfile, where would the layers end up?

flouthoc · 2023-04-05T07:51:50Z

@giuseppe I made a modification where when --image-store is used exisintg graphRoot is automatically used as an secondary additionalStore internal allowing users to use images both from newly added image-store and the original graphRoot, I have a added an integration test: split-store - use graphRoot as an additional store by default which tests this behavior. Do you think this suffice the use-case mentioned above.

flouthoc · 2023-04-05T11:39:45Z

Tagging @mrunalp for inputs.

mrunalp · 2023-04-07T20:14:27Z

Looks fine to me. Did we test with two disks?

flouthoc · 2023-04-08T02:49:47Z

I think it should work without any issues but let me test it out with two separate disk to confirm.

flouthoc · 2023-04-10T07:30:17Z

Okay I did test it with imagestore on a different disk than my original graphroot and it works fine. I used an external usb stick as imagestore and my current disk as original graphroot.

# Check if we mounted external usb
[fl@fedora ~]$ lsblk
NAME        MAJ:MIN RM   SIZE RO TYPE MOUNTPOINT
loop0         7:0    0  32.3M  1 loop /var/lib/snapd/snap/snapd/12704
loop1         7:1    0  55.4M  1 loop /var/lib/snapd/snap/core18/2128
loop2         7:2    0  13.9M  1 loop /var/lib/snapd/snap/universal-ctags/552
sdb           8:16   1  14.3G  0 disk 
├─sdb1        8:17   1  14.3G  0 part /run/media/fl/12a7bc12-59b6-4aa2-b40a-76f67faf28b7
└─sdb2        8:18   1     1M  0 part /run/media/fl/UEFI_NTFS
zram0       252:0    0     8G  0 disk [SWAP]
nvme0n1     259:0    0 476.9G  0 disk 
├─nvme0n1p1 259:1    0   600M  0 part /boot/efi
├─nvme0n1p2 259:2    0     1G  0 part /boot
└─nvme0n1p3 259:3    0 475.4G  0 part /home

# Check if external use contains directory for podman-storage
[fl@fedora ~]$ ls  /run/media/fl/12a7bc12-59b6-4aa2-b40a-76f67faf28b7
lost+found  podman-storage

# Check our storage.conf
[fl@fedora ~]$ cat /home/fl/.config/containers/storage.conf
[storage]

imagestore = "/run/media/fl/12a7bc12-59b6-4aa2-b40a-76f67faf28b7/podman-storage"

# First set imagestore to `none` and pull ubuntu
# Second set imagestore to `usb` and pull alpine

# podman images should show image from both original graph root and usb-stick
[fl@fedora bin]$ ./podman images
WARN[0000] The storage 'driver' option should be set in /home/fl/.config/containers/storage.conf. A driver was picked automatically. 
REPOSITORY                TAG         IMAGE ID      CREATED      SIZE        R/O
docker.io/library/alpine  latest      9ed4aefc74f6  11 days ago  7.34 MB     false
docker.io/library/ubuntu  latest      08d22c0ceb15  4 weeks ago  80.3 MB     true

# run container where image is not on imagestore but on original graph root
[fl@fedora bin]$ ./podman run --pull=never -it --rm ubuntu sh
WARN[0000] The storage 'driver' option should be set in /home/fl/.config/containers/storage.conf. A driver was picked automatically. 
# exit
[fl@fedora bin]$ 

# run container where image is on imagestore but not on original graph root
[fl@fedora bin]$ ./podman run --pull=never -it --rm alpine sh
WARN[0000] The storage 'driver' option should be set in /home/fl/.config/containers/storage.conf. A driver was picked automatically. 
/ # exit
[fl@fedora bin]$

flouthoc · 2023-04-10T07:31:01Z

@giuseppe @mrunalp ^

rhatdan · 2023-04-10T10:32:55Z

You need to update the containers-storage.conf.5.md

When we added splitstore support via `--imagestore` the `graphRoot` was added as an `rw` additionalImageStore so lets do same for the `vfs` driver. See PR for more details: * containers#1549 * containers#1578 Signed-off-by: Aditya R <arajan@redhat.com>

mtrmac

I am probably too sleep-deprived but I just can’t understand how this works, starting with dir2.

Why are all callers of dir2 special-casing this? I would expect a single location in this package to contain some code to find an existing image, which callers can then use without any conditionals. And I would expect a single location in this package to contain some code to determine where to create new images or store more data, which callers can then use without any conditionals.

This is clearly not that, and I can’t understand how this is intended to work.

Again, maybe it is just me. But please someone take another look.

mtrmac · 2023-04-21T20:57:01Z

drivers/overlay/overlay.go

 }

-func (d *Driver) dir2(id string) (string, bool) {
+func (d *Driver) dir2(id string) (string, string, bool) {


What are the return values of this function?

I would strongly recommend a habit of documenting the intended purpose, all parameters, and all return values, for every function written.

mtrmac · 2023-04-21T21:06:18Z

drivers/overlay/overlay.go

 	}
+	workDirBase := dir
+	if imagestore != "" {
+		if _, err := os.Stat(dir); err != nil {


Why is this checking dir? Based on the Stat just a few lines above, this is not going to fail.

mtrmac · 2023-04-21T21:14:39Z

drivers/overlay/overlay.go

+		"WorkDir":   path.Join(workDirBase, "work"),
+		"MergedDir": path.Join(workDirBase, "merged"),
+		"UpperDir":  path.Join(workDirBase, "diff"),


This is all using the values dependent on s.imageStore.

But create is using workDirBase/"work", but dir/"merged". UpdateLayerIDMap is using dir/"work". Everything seems to be using dir/"diff".

What’s going on here?

If it is hard to keep track of the right directory, introduce methods like workDir(imageID) and have everything call them.

mtrmac · 2023-04-21T21:18:21Z

drivers/overlay/overlay.go

 	newpath := path.Join(d.home, id)
+	imageStore := ""
+	if d.imageStore != "" {
+		imageStore = path.Join(d.imageStore, id)


So we now have an “image store” variable, which is an individual per-image value, but d.imageStore is a per-store global.

And this function creates this {s.imageStore}/{id} value, but never checks it for any purpose. Why?

If {d.home}/{id} exists, this function returns that as the first return value

{d.home}/{id} does not exist but one of the “additional image stores” contain an /{id} path, this function returns that additional one.

What role does {s.imageStore}/{id} play here?

Before, this function fairly clearly returned “which directory contains the image data if the image exists, or where the data should be created if it doesn’t exist anywhere; is the image in an additional store”.

Now it returns … ???

In particular the various parent lookups only check in the {s.imageStore}/{id} location, outright failing if the image is not there. But this function does not check at all whether anything exists in that location.

flouthoc · 2023-04-22T03:35:46Z

I'll open a new PR to address these refactor comments.

nalind · 2023-05-10T21:27:35Z

I am probably too sleep-deprived but I just can’t understand how this works, starting with dir2.

Why are all callers of dir2 special-casing this? I would expect a single location in this package to contain some code to find an existing image, which callers can then use without any conditionals. And I would expect a single location in this package to contain some code to determine where to create new images or store more data, which callers can then use without any conditionals.

This is clearly not that, and I can’t understand how this is intended to work.

Again, maybe it is just me. But please someone take another look.

Looking at it now, I don't think I fully understand it, either. With the option set, when the overlay driver creates and removes layers under the graph root, continuing to record things like the list of lowers there, it also seems to be creating and removing directories under the image store location, and it's neither obvious nor commented why it puts things in one place or the other. If my mostly-guesswork view of what the overlay driver is doing is correct, then the overlay driver is not correctly setting disk quotas.
I definitely don't understand what's supposed to be happening differently in the vfs driver.

Add support for `--imagestore` in podman which allows users to split the filesystem of containers vs image store, imagestore if configured will pull images in image storage instead of the graphRoot while keeping the other parts still in the originally configured graphRoot. This is an implementation of containers/storage#1549 in podman. Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc marked this pull request as draft April 3, 2023 07:48

flouthoc force-pushed the pull-store branch from 7dae996 to e10126c Compare April 3, 2023 10:29

flouthoc force-pushed the pull-store branch from e10126c to 2ce5ab7 Compare April 4, 2023 07:32

flouthoc changed the title ~~store: add support to split filesystem using pull-store~~ store: add support to split filesystem using imagestore Apr 4, 2023

flouthoc force-pushed the pull-store branch 5 times, most recently from e8973b8 to 72dca55 Compare April 4, 2023 10:03

flouthoc marked this pull request as ready for review April 4, 2023 10:03

giuseppe reviewed Apr 4, 2023

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

flouthoc force-pushed the pull-store branch from 72dca55 to 1ef9b8b Compare April 4, 2023 12:56

flouthoc force-pushed the pull-store branch from 1ef9b8b to 158470c Compare April 5, 2023 07:41

flouthoc force-pushed the pull-store branch 3 times, most recently from 18482ed to c57b081 Compare April 5, 2023 09:03

flouthoc requested a review from mrunalp April 5, 2023 11:39

flouthoc force-pushed the pull-store branch from c57b081 to bfbaad2 Compare April 10, 2023 06:15

flouthoc mentioned this pull request Apr 21, 2023

vfs,splitstore: add graphRoot as additionalStore #1580

Merged

mtrmac reviewed Apr 21, 2023

View reviewed changes

mtrmac mentioned this pull request May 22, 2023

New buildah/timeout/external container flakes containers/podman#18631

Closed

flouthoc mentioned this pull request Aug 23, 2023

storage: add support to split filesystem using imagestore cri-o/cri-o#7238

Merged

Conversation

flouthoc commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhatdan commented Apr 3, 2023

Uh oh!

rhatdan commented Apr 3, 2023

Uh oh!

flouthoc commented Apr 3, 2023

Uh oh!

giuseppe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flouthoc commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppe commented Apr 4, 2023

Uh oh!

flouthoc commented Apr 5, 2023

Uh oh!

flouthoc commented Apr 5, 2023

Uh oh!

mrunalp commented Apr 7, 2023

Uh oh!

flouthoc commented Apr 8, 2023

Uh oh!

flouthoc commented Apr 10, 2023

Uh oh!

flouthoc commented Apr 10, 2023

Uh oh!

rhatdan commented Apr 10, 2023

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

mtrmac Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

mtrmac Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

mtrmac Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

flouthoc commented Apr 22, 2023

Uh oh!

nalind commented May 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

flouthoc commented Apr 3, 2023 •

edited

Loading

flouthoc commented Apr 4, 2023 •

edited

Loading