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.

Sign up for GitHub

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

Jump to bottom

overlay: support AdditionalImagesStore on vfs #1899

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

overlay: support AdditionalImagesStore on vfs #1899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85c7e53
drivers: AdditionalImageStores uses different backends
giuseppe Apr 22, 2024
01a0bc2
store: ignore additional stores that do not exist
giuseppe Apr 23, 2024
ef0f37e
vfs: export graphdriver name
giuseppe Apr 22, 2024
4791448
overlay: support additional image store on vfs
giuseppe Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions drivers/aufs/aufs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,8 @@ func (a *Driver) ListLayers() ([]string, error) {
}

// AdditionalImageStores returns additional image stores supported by the driver
func (a *Driver) AdditionalImageStores() []string {
return nil
func (a *Driver) AdditionalImageStores() ([]string, []string) {
return nil, nil
}

// CreateFromTemplate creates a layer with the same contents and parent as another layer.
Expand Down
4 changes: 2 additions & 2 deletions drivers/btrfs/btrfs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,6 @@ func (d *Driver) ListLayers() ([]string, error) {
}

// AdditionalImageStores returns additional image stores supported by the driver
func (d *Driver) AdditionalImageStores() []string {
return nil
func (d *Driver) AdditionalImageStores() ([]string, []string) {
return nil, nil
}
2 changes: 1 addition & 1 deletion drivers/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ type ProtoDriver interface {
Cleanup() error
// AdditionalImageStores returns additional image stores supported by the driver
// This API is experimental and can be changed without bumping the major version number.
AdditionalImageStores() []string
AdditionalImageStores() ([]string, []string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know, I’m being annoying:

drivers.Driver is a publicly visible API, reachable by external callers via Store.GraphDriver().

OTOH, yes, neither Podman nor CRI-O call this function. Meh. I’ll defer to actual c/storage owners.

}

// DiffDriver is the interface to use to implement graph diffs
Expand Down
103 changes: 72 additions & 31 deletions drivers/overlay/overlay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
graphdriver "github.com/containers/storage/drivers"
"github.com/containers/storage/drivers/overlayutils"
"github.com/containers/storage/drivers/quota"
"github.com/containers/storage/drivers/vfs"
"github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/chrootarchive"
"github.com/containers/storage/pkg/directory"
Expand Down Expand Up @@ -977,7 +978,7 @@ func (d *Driver) Create(id, parent string, opts *graphdriver.CreateOpts) (retErr
}

func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnly bool) (retErr error) {
dir, homedir, _ := d.dir2(id, readOnly)
dir, homedir, _, _ := d.dir2(id, false, false)

disableQuota := readOnly

Expand Down Expand Up @@ -1007,15 +1008,6 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl
if err := idtools.MkdirAllAndChownNew(path.Dir(dir), 0o755, idPair); err != nil {
return err
}
if parent != "" {
parentBase := d.dir(parent)
st, err := system.Stat(filepath.Join(parentBase, "diff"))
if err != nil {
return err
}
rootUID = int(st.UID())
rootGID = int(st.GID())
}

if err := fileutils.Lexists(dir); err == nil {
logrus.Warnf("Trying to create a layer %#v while directory %q already exists; removing it first", id, dir)
Expand Down Expand Up @@ -1066,11 +1058,19 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl
}

if parent != "" {
parentBase := d.dir(parent)
st, err := system.Stat(filepath.Join(parentBase, "diff"))
parentBase, _, singleLayer, _ := d.dir2(parent, false, true)
var d string
if singleLayer {
d = parentBase
} else {
d = filepath.Join(parentBase, "diff")
}
st, err := system.Stat(d)
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
rootUID = int(st.UID())
rootGID = int(st.GID())
perms = os.FileMode(st.Mode())
}

Expand Down Expand Up @@ -1142,13 +1142,17 @@ func (d *Driver) parseStorageOpt(storageOpt map[string]string, driver *Driver) e
}

func (d *Driver) getLower(parent string) (string, error) {
parentDir := d.dir(parent)
parentDir, _, singleLayer, _ := d.dir2(parent, false, true)

// Ensure parent exists
if err := fileutils.Lexists(parentDir); err != nil {
return "", err
}

if singleLayer {
return parent, nil
}

// Read Parent link fileA
parentLink, err := os.ReadFile(path.Join(parentDir, "link"))
if err != nil {
Expand All @@ -1175,19 +1179,27 @@ func (d *Driver) getLower(parent string) (string, error) {
}

func (d *Driver) dir(id string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

From a quick skim it seems to me that various other callers of this also need updating:

  • e.g. getComposefsData, getAdditionalLayerPathByID would now read user-controlled contents of the VFS layer
  • calls to the effect of filepath.Join(d.dir(id), "diff") are also fairly frequent

There seem to be enough examples that I didn’t look into that exhaustively.

Worrying about every caller handling all the cases is becoming unwieldy … I’m starting to think there should be a layer abstraction, to handle the differences between native overlay / VFS / FUSE additionalLayerStore / composeFS.

p, _, _ := d.dir2(id, false)
p, _, _, _ := d.dir2(id, false, false)
Copy link
Collaborator

@mtrmac mtrmac May 21, 2024 •
edited

Choose a reason for hiding this comment

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

So it seems that https://github.com/containers/storage/pull/1899/files#r1576924079 was resolved by just not finding a VFS layer when dir is called, and instead returning an otherwise-uninitialized primaryPath. So some callers looking for a layer with the same ID will see a VFS layer, and some will see a different, empty directory?

Why is that safe and consistent?

After looking a bit, it seems that only parent layers can be VFS layers (because non-parent layers would presumably be handled by the VFS driver directly, not overlay?) … but then isParent is calling .dir and not dir2, and it will not find VFS layers?

I’m sorry, I just can’t deal with the complexity. Too many options, too many interactions, too many things that seem like they might be possible even if something elsewhere in the call stack probably prevents them. Most of that with documentation of neither intent nor maintained invariants.

I straight can’t approve any PR adding a feature adding any more indirections or alternatives to the overlay driver any more. I’m not smart enough to keep up.

It seems to me that most non-trivial PRs here would require me to spend something like 2 days just graphing out and understanding the interactions and possibilities. At some point, it must pay off to instead spend 2-3 weeks to restructure and explicitly document, so that PRs are again a matter of a small number of hours, and I’m now past the point where I’m willing to enable the current state to persist, and to keep paying for the complexity those 2 days at a time, every time.

I don’t know what is the path forward. Probably easiest, find some other reviewer who is up to the task.

Or, maybe, aggressively simplify. Build abstractions where they can be built and don’t leak. Find identifiable and isolated utilities that can be cut out of the >400-line functions. Rename things to be clear (dir vs. dir2 could never be clear), maybe findParentLayerPossiblyInOtherDriver vs. findLayerWeOwn vs … (yes, that is surely unproductive overkill, but the naming needs to first be clear and only afterwards succinct [I have no idea whether that’s the distinction that matters here, those names are not intended to be just used because I had to come up with an example]).

*shrug* it’s also possible that I’m overreacting and that a few sentences can make the picture clear. I’m not smart enough to figure that out myself.

return p
}

func (d *Driver) getAllImageStores() []string {
additionalImageStores := d.AdditionalImageStores()
additionalImageStores, _ := d.AdditionalImageStores()
if d.imageStore != "" {
additionalImageStores = append([]string{d.imageStore}, additionalImageStores...)
}
return additionalImageStores
}

func (d *Driver) dir2(id string, useImageStore bool) (string, string, bool) {
// dir2 returns the directory path and home directory path associated with the given layer ID.
// It also returns two boolean values indicating if the store is using the VFS graph driver, and
// whether the directory is found in an image store.
//
// - string: The directory path for the layer.
// - string: The storage home directory path.
// - bool: A boolean indicating whether the store is using VFS, thus doesn't require stacking of layers.
// - bool: A boolean indicating whether the directory is found in an additional image store.
func (d *Driver) dir2(id string, useImageStore, useVFS bool) (string, string, bool, bool) {
var homedir string

if useImageStore && d.imageStore != "" {
Expand All @@ -1196,18 +1208,27 @@ func (d *Driver) dir2(id string, useImageStore bool) (string, string, bool) {
homedir = d.home
}

newpath := path.Join(homedir, id)
primaryPath := path.Join(homedir, id)
if err := fileutils.Exists(primaryPath); err == nil {
return primaryPath, homedir, false, false
}

if err := fileutils.Exists(newpath); err != nil {
for _, p := range d.getAllImageStores() {
l := path.Join(p, d.name, id)
err = fileutils.Exists(l)
if err == nil {
return l, homedir, true
allImageStores := d.getAllImageStores()
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
for _, p := range allImageStores {
l := path.Join(p, d.name, id)
if err := fileutils.Exists(l); err == nil {
return l, homedir, false, true
}
if useVFS {
l = path.Join(p, vfs.Name, "dir", id)
if err := fileutils.Exists(l); err == nil {
return l, homedir, true, true
}
}
}
return newpath, homedir, false

// It does not exist, return where to create it.
return primaryPath, homedir, false, false
}

func (d *Driver) getLowerDirs(id string) ([]string, error) {
Expand Down Expand Up @@ -1417,7 +1438,7 @@ func (d *Driver) Get(id string, options graphdriver.MountOpts) (string, error) {
}

func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountOpts) (_ string, retErr error) {
dir, _, inAdditionalStore := d.dir2(id, false)
dir, _, _, inAdditionalStore := d.dir2(id, false, true)
if err := fileutils.Exists(dir); err != nil {
return "", err
}
Expand Down Expand Up @@ -1529,6 +1550,9 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
composeFsLayersDir := filepath.Join(dir, "composefs-layers")
maybeAddComposefsMount := func(lowerID string, i int, readWrite bool) (string, error) {
composefsBlob := d.getComposefsData(lowerID)
if composefsBlob == "" {
return "", nil
}
if err := fileutils.Exists(composefsBlob); err != nil {
if os.IsNotExist(err) {
return "", nil
Expand Down Expand Up @@ -1577,7 +1601,8 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
lower := ""
newpath := path.Join(d.home, l)
if st, err := os.Stat(newpath); err != nil {
for _, p := range d.getAllImageStores() {
allImageStores := d.getAllImageStores()
for _, p := range allImageStores {
lower = path.Join(p, d.name, l)
if st2, err2 := os.Stat(lower); err2 == nil {
if !permsKnown {
Expand All @@ -1586,6 +1611,15 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
}
break
}
// now try in the vfs store
lower = path.Join(p, vfs.Name, "dir", l)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall, I’d very much like the interdependencies between this and VFS to be transparent to IDEs (to make it clear what needs to be updated on format changes).

Maybe create drivers/internal/vfs.go, with func VFSLayersDir(vfsDriverHomeDir string) string) and func VFSLayerDir(vfsDriverHomeDir, layerID string) string; and call one or the other in every single location, with only exactly one place hard-coding the "dir" string.

if st2, err2 := os.Stat(lower); err2 == nil {
if !permsKnown {
perms = os.FileMode(st2.Mode())
permsKnown = true
}
break
}
lower = ""
}
// if it is a "not found" error, that means the symlinks were lost in a sudden reboot
Expand All @@ -1610,7 +1644,10 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO

linkContent, err := os.Readlink(lower)
if err != nil {
return "", err
if !errors.Is(err, unix.EINVAL) { // Not a symlink, a raw path
return "", err
}
linkContent = lower
}
lowerID := filepath.Base(filepath.Dir(linkContent))
composefsMount, err := maybeAddComposefsMount(lowerID, i+1, readWrite)
Expand Down Expand Up @@ -1835,10 +1872,14 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO

// Put unmounts the mount path created for the give id.
func (d *Driver) Put(id string) error {
dir, _, inAdditionalStore := d.dir2(id, false)
dir, _, singleLayer, inAdditionalStore := d.dir2(id, false, true)
if err := fileutils.Exists(dir); err != nil {
return err
}
if singleLayer {
return nil
}

mountpoint := path.Join(dir, "merged")
if count := d.ctr.Decrement(mountpoint); count > 0 {
return nil
Expand Down Expand Up @@ -2005,7 +2046,7 @@ func (g *overlayFileGetter) Close() error {
}

func (d *Driver) getStagingDir(id string) string {
_, homedir, _ := d.dir2(id, d.imageStore != "")
_, homedir, _, _ := d.dir2(id, d.imageStore != "", false)
return filepath.Join(homedir, stagingDir)
}

Expand Down Expand Up @@ -2282,8 +2323,8 @@ func (d *Driver) Changes(id string, idMappings *idtools.IDMappings, parent strin
}

// AdditionalImageStores returns additional image stores supported by the driver
func (d *Driver) AdditionalImageStores() []string {
return d.options.imageStores
func (d *Driver) AdditionalImageStores() ([]string, []string) {
return d.options.imageStores, []string{d.name, vfs.Name}
}

// UpdateLayerIDMap updates ID mappings in a from matching the ones specified
Expand Down
17 changes: 10 additions & 7 deletions drivers/vfs/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,20 @@ import (
"github.com/vbatts/tar-split/tar/storage"
)

const defaultPerms = os.FileMode(0o555)
const (
defaultPerms = os.FileMode(0o555)
Name = "vfs"
)

func init() {
graphdriver.MustRegister("vfs", Init)
graphdriver.MustRegister(Name, Init)
}

// Init returns a new VFS driver.
// This sets the home directory for the driver and returns NaiveDiffDriver.
func Init(home string, options graphdriver.Options) (graphdriver.Driver, error) {
d := &Driver{
name: "vfs",
name: Name,
home: home,
idMappings: idtools.NewIDMappingsFromMaps(options.UIDMaps, options.GIDMaps),
imageStore: options.ImageStore,
Expand Down Expand Up @@ -87,7 +90,7 @@ type Driver struct {
}

func (d *Driver) String() string {
return "vfs"
return Name
}

// Status is used for implementing the graphdriver.ProtoDriver interface. VFS does not currently have any status information.
Expand Down Expand Up @@ -297,11 +300,11 @@ func (d *Driver) ListLayers() ([]string, error) {
}

// AdditionalImageStores returns additional image stores supported by the driver
func (d *Driver) AdditionalImageStores() []string {
func (d *Driver) AdditionalImageStores() ([]string, []string) {
if len(d.additionalHomes) > 0 {
return d.additionalHomes
return d.additionalHomes, []string{d.name}
}
return nil
return nil, []string{d.name}
}

// SupportsShifting tells whether the driver support shifting of the UIDs/GIDs in an userNS
Expand Down
4 changes: 2 additions & 2 deletions drivers/windows/windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -971,8 +971,8 @@ func (d *Driver) DiffGetter(id string) (graphdriver.FileGetCloser, error) {
}

// AdditionalImageStores returns additional image stores supported by the driver
func (d *Driver) AdditionalImageStores() []string {
return nil
func (d *Driver) AdditionalImageStores() ([]string, []string) {
return nil, nil
}

// UpdateLayerIDMap changes ownerships in the layer's filesystem tree from
Expand Down
4 changes: 2 additions & 2 deletions drivers/zfs/zfs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,6 @@ func (d *Driver) ListLayers() ([]string, error) {
}

// AdditionalImageStores returns additional image stores supported by the driver
func (d *Driver) AdditionalImageStores() []string {
return nil
func (d *Driver) AdditionalImageStores() ([]string, []string) {
return nil, nil
}