Skip to content

Overlay cleanups after #666 #783

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

Merged
Luap99 merged 6 commits into from
Apr 27, 2026
+120 −151
Merged

Overlay cleanups after #666 #783

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f9896de
Collect constants in drivers/overlay by purpose
mtrmac Apr 23, 2026
4f46ac6
Document getLower and rename it
mtrmac Apr 23, 2026
727feee
Remove an always-true condition
mtrmac Apr 23, 2026
7fac8c2
Consolidate the lowerFile fallback to getLowerLayerIDs
mtrmac Apr 23, 2026
5c39710
Simplify and optimize isParent
mtrmac Apr 23, 2026
e4ee402
Move all the "lower"-related functions to single place.
mtrmac Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
271 changes: 120 additions & 151 deletions storage/drivers/overlay/overlay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ import (
var untar = chrootarchive.UntarUncompressed

const (
defaultPerms = os.FileMode(0o555)
mountProgramFlagFile = ".has-mount-program"
defaultPerms = os.FileMode(0o555)
)

// This backend uses the overlay union filesystem for containers
Expand Down Expand Up @@ -79,24 +78,32 @@ const (
// syscall. A hard upper limit of 500 lower layers is enforced to ensure
// that mounts do not fail due to length.

const (
linkDir = "l"
stagingDir = "staging"
tempDirName = "tempdirs"
lowerFile = "lower"
const ( // Paths within the driver’s home directory
mountProgramFlagFile = ".has-mount-program"
linkDir = "l"
stagingDir = "staging"
tempDirName = "tempdirs"
)

const ( // Paths within a per-layer directory
lowerFile = "lower"
// lowerLayersFile references lower layers directly by layer ID
// instead of going through the l/ symlinks. The code appends
// "/diff" itself when consuming entries. It is preferred over
// lowerFile when present. The old lowerFile is still written
// for backward compatibility with older tools.
lowerLayersFile = "lower-layers"
maxDepth = 500

stagingLockFile = "staging.lock"
)

const ( // Keys within DriverWithDifferOutput.Artifacts
tocArtifact = "toc"
fsVerityDigestsArtifact = "fs-verity-digests"
)

const stagingLockFile = "staging.lock"

const (
maxDepth = 500
// idLength represents the number of random characters
// which can be used to create the unique link identifier
// for every layer. If this value is too long then the
Expand Down Expand Up @@ -1145,42 +1152,22 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl
return idtools.MkdirAndChown(path.Join(dir, "empty"), 0o700, forcedSt.IDs)
}

lower, err := d.getLower(parent)
lower, err := d.getLowerForParent(parent)
if err != nil {
return err
}
if lower != "" {
if err := os.WriteFile(path.Join(dir, lowerFile), []byte(lower), 0o666); err != nil {
return err
}
if err := os.WriteFile(path.Join(dir, lowerFile), []byte(lower), 0o666); err != nil {
return err
}

// Write a lower-layers file referencing layers by ID instead of
// l/ symlink references. The reading side appends "/diff" itself.
parentDir := d.dir(parent)
layerLower := parent
parentLower, err := os.ReadFile(path.Join(parentDir, lowerLayersFile))
if err == nil {
layerLower += ":" + string(parentLower)
} else if !errors.Is(err, unix.ENOENT) {
parentLowerLayerIDs, err := d.getLowerLayerIDs(parent)
if err != nil {
return err
} else {
// Parent has no lower-layers file. Convert old-format lower
// entries (l/ symlinks) to layer IDs.
oldLower, err := os.ReadFile(path.Join(parentDir, lowerFile))
if err == nil {
for _, s := range strings.Split(string(oldLower), ":") {
target, err := os.Readlink(d.dir(s))
if err != nil {
return fmt.Errorf("reading symlink for lower %q: %w", s, err)
}
layerLower += ":" + filepath.Base(filepath.Dir(target))
}
} else if !errors.Is(err, unix.ENOENT) {
return err
}
}
if err := os.WriteFile(path.Join(dir, lowerLayersFile), []byte(layerLower), 0o666); err != nil {
layerLowerLayerIDs := strings.Join(append([]string{parent}, parentLowerLayerIDs...), ":")
if err := os.WriteFile(path.Join(dir, lowerLayersFile), []byte(layerLowerLayerIDs), 0o666); err != nil {
return err
}

Expand Down Expand Up @@ -1231,28 +1218,6 @@ func (d *Driver) parseStorageOpt(opts *graphdriver.CreateOpts, readOnly bool) (q
return res, nil
}

func (d *Driver) getLower(parent string) (string, error) {
parentDir := d.dir(parent)

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

parentLink, err := os.ReadFile(path.Join(parentDir, "link"))
if err != nil {
return "", err
}
lowers := []string{path.Join(linkDir, string(parentLink))}

parentLower, err := os.ReadFile(path.Join(parentDir, lowerFile))
if err == nil {
parentLowers := strings.SplitSeq(string(parentLower), ":")
lowers = slices.AppendSeq(lowers, parentLowers)
}
return strings.Join(lowers, ":"), nil
}

func (d *Driver) dir(id string) string {
p, _, _ := d.dir2(id, false)
return p
Expand Down Expand Up @@ -1294,36 +1259,98 @@ func (d *Driver) dir2(id string, useImageStore bool) (string, string, bool) {
return newpath, homedir, false
}

func (d *Driver) getLowerDirs(id string) ([]string, error) {
var lowersArray []string
dir := d.dir(id)
lowers, err := os.ReadFile(path.Join(dir, lowerLayersFile))
// getLowerForParent returns the contents of lowerFile for a child layer of parent.
//
// This should only be used to construct a lowerFile for compatibility;
// new code should rely on lowerLayersFile instead.
func (d *Driver) getLowerForParent(parent string) (string, error) {
parentDir := d.dir(parent)

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

parentLink, err := os.ReadFile(path.Join(parentDir, "link"))
if err != nil {
if !errors.Is(err, unix.ENOENT) {
return nil, err
}
lowers, err = os.ReadFile(path.Join(dir, lowerFile))
return "", err
}
lowers := []string{path.Join(linkDir, string(parentLink))}

parentLower, err := os.ReadFile(path.Join(parentDir, lowerFile))
if err == nil {
for s := range strings.SplitSeq(string(lowers), ":") {
lower := d.dir(s)
lp, err := os.Readlink(lower)
parentLowers := strings.SplitSeq(string(parentLower), ":")
lowers = slices.AppendSeq(lowers, parentLowers)
}
return strings.Join(lowers, ":"), nil
}

// getLowerLayerIDs returns a list of lower layer IDs for a layer id;
// typically the contents of lowerLayersFile, falling back to lowerFile.
// If the layer has neither of the files, returns an empty list without reporting an error.
func (d *Driver) getLowerLayerIDs(id string) ([]string, error) {
dir := d.dir(id)
lowerLayers, err := os.ReadFile(path.Join(dir, lowerLayersFile))
switch {
case err == nil:
return strings.Split(string(lowerLayers), ":"), nil

case errors.Is(err, fs.ErrNotExist):
lowers, err := os.ReadFile(path.Join(dir, lowerFile))
if err != nil {
if errors.Is(err, fs.ErrNotExist) {
return nil, nil
}
return nil, err
}
var res []string
Copy link
Copy Markdown
Member

@giuseppe giuseppe Apr 23, 2026

Choose a reason for hiding this comment

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

we know how many items are in the sequence, worth preallocating?

Copy link
Copy Markdown
Contributor Author

@mtrmac mtrmac Apr 23, 2026

Choose a reason for hiding this comment

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

The code uses SplitSeq, so we don’t actually trivially know. We could scan one more time to count that, and have everyone carefully think about the required +1… And anyway this is the legacy path…

Actually the lowerLayers path is currently unnecessarily creating an iterator and then allocating anyway. I’ll fix that.

Hypothetically all of these could return iterators instead of allocating slices, but that would complicate error handling and the current uses of strings.Join.

for relLowerLink := range strings.SplitSeq(string(lowers), ":") {
lowerLink := d.dir(relLowerLink) // This is an invalid use of dir() (the input is supposed to be a layer ID) but pre-existing
lp, err := os.Readlink(lowerLink)
if err != nil {
if errors.Is(err, syscall.EINVAL) {
// Not a symlink: layer ID, append /diff.
lowersArray = append(lowersArray, path.Join(lower, "diff"))
continue
}
return nil, err
}
lowersArray = append(lowersArray, path.Clean(d.dir(path.Join("link", lp))))
lowerID := filepath.Base(filepath.Dir(lp))
res = append(res, lowerID)
}
} else if !errors.Is(err, fs.ErrNotExist) {
return res, nil

default:
return nil, err
}
}

// getLowerDirs returns a list of lower directories for a layer id;
// the directories may be symbolic links (do not call redirectDiffIfAdditionalLayer).
func (d *Driver) getLowerDirs(id string) ([]string, error) {
lowerLayerIDs, err := d.getLowerLayerIDs(id)
if err != nil {
return nil, err
}
lowersArray := make([]string, 0, len(lowerLayerIDs))
for _, lowerID := range lowerLayerIDs {
lowerDir := d.dir(lowerID)
lowersArray = append(lowersArray, path.Join(lowerDir, "diff"))
}
return lowersArray, nil
}

// getLowerDiffPaths returns a list of lower diff paths for a layer id;
// the paths have redirectDiffIfAdditionalLayer applied.
func (d *Driver) getLowerDiffPaths(id string) ([]string, error) {
layers, err := d.getLowerDirs(id)
if err != nil {
return nil, err
}
for i, l := range layers {
layers[i], err = redirectDiffIfAdditionalLayer(l, false)
if err != nil {
return nil, err
}
}
return layers, nil
}

func (d *Driver) optsAppendMappings(opts string, uidMaps, gidMaps []idtools.IDMap) string {
if uidMaps != nil {
var uids, gids bytes.Buffer
Expand Down Expand Up @@ -1520,18 +1547,11 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
readWrite = false
}

lowers, err := os.ReadFile(path.Join(dir, lowerLayersFile))
lowerLayerIDs, err := d.getLowerLayerIDs(id)
if err != nil {
if !errors.Is(err, unix.ENOENT) {
return "", err
}
lowers, err = os.ReadFile(path.Join(dir, lowerFile))
if err != nil && !os.IsNotExist(err) {
return "", err
}
return "", err
}
splitLowers := strings.Split(string(lowers), ":")
if len(splitLowers) > maxDepth {
if len(lowerLayerIDs) > maxDepth {
return "", errors.New("max depth exceeded")
}

Expand Down Expand Up @@ -1621,46 +1641,17 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO

// For each lower, resolve its path, and append it and any additional diffN
// directories to the lowers list.
for i, l := range splitLowers {
if l == "" {
continue
for i, lowerID := range lowerLayerIDs {
lower := filepath.Join(d.dir(lowerID), "diff")
st, err := os.Stat(lower)
if err != nil {
return "", fmt.Errorf("can't stat (or find?) lower layer %q: %w", lower, err)
}

lower := ""
newpath := path.Join(d.home, l)
if st, err := os.Stat(newpath); err != nil {
for _, p := range d.getAllImageStores() {
lower = path.Join(p, d.name, l)
if st2, err2 := os.Stat(lower); err2 == nil {
if !permsKnown {
perms = st2.Mode()
permsKnown = true
}
break
}
lower = ""
}
if lower == "" {
return "", fmt.Errorf("can't stat lower layer %q: %w", newpath, err)
}
} else {
if !permsKnown {
perms = st.Mode()
permsKnown = true
}
lower = newpath
if !permsKnown {
perms = st.Mode()
permsKnown = true
}

linkContent, err := os.Readlink(lower)
if err != nil {
if !errors.Is(err, syscall.EINVAL) {
return "", err
}
// Not a symlink: layer ID from lower-layers, append /diff.
lower = path.Join(lower, "diff")
linkContent = lower
}
lowerID := filepath.Base(filepath.Dir(linkContent))
composefsMount, err := maybeAddComposefsMount(lowerID, i+1, readWrite)
if err != nil {
return "", err
Expand Down Expand Up @@ -2030,23 +2021,15 @@ func (d *Driver) ListLayers() ([]string, error) {

// isParent returns if the passed in parent is the direct parent of the passed in layer
func (d *Driver) isParent(id, parent string) bool {
lowers, err := d.getLowerDirs(id)
lowerLayerIDs, err := d.getLowerLayerIDs(id)
if err != nil {
return false
}
if parent == "" && len(lowers) > 0 {
return false
}

parentDir := d.dir(parent)
var ld string
if len(lowers) > 0 {
ld = filepath.Dir(lowers[0])
}
if ld == "" && parent == "" {
return true
actualParent := ""
if len(lowerLayerIDs) > 0 {
actualParent = lowerLayerIDs[0]
}
return ld == parentDir
return parent == actualParent
}

func (d *Driver) getWhiteoutFormat() archive.WhiteoutFormat {
Expand Down Expand Up @@ -2430,20 +2413,6 @@ func (d *Driver) getDiffPath(id string) (string, error) {
return redirectDiffIfAdditionalLayer(path.Join(dir, "diff"), false)
}

func (d *Driver) getLowerDiffPaths(id string) ([]string, error) {
layers, err := d.getLowerDirs(id)
if err != nil {
return nil, err
}
for i, l := range layers {
layers[i], err = redirectDiffIfAdditionalLayer(l, false)
if err != nil {
return nil, err
}
}
return layers, nil
}

// DiffSize calculates the changes between the specified id
// and its parent and returns the size in bytes of the changes
// relative to its base filesystem directory.
Expand Down
Loading