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

libindex: add fallback path for O_TMPFILE not being supported #1292

Merged
merged 2 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 26 additions & 7 deletions libindex/fetcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,6 @@ var (
_ indexer.DescriptionRealizer = (*FetchProxy)(nil)
)

// BUG(hank) On Linux, the [RemoteFetchArena] makes use of the O_TMPFILE flag to
// [open(2)], which requires version 3.11 or newer. There is no fallback for
// older kernels.
//
// [open(2)]: https://man7.org/linux/man-pages/man2/open.2.html

// RemoteFetchArena uses disk space to track fetched layers, removing them once
// all users are done with the layers.
type RemoteFetchArena struct {
Expand All @@ -54,11 +48,36 @@ type RemoteFetchArena struct {
}

// NewRemoteFetchArena returns an initialized RemoteFetchArena.
//
// If the "root" parameter is "", the advice in [file-hierarchy(7)] and ["Using
// /tmp/ and /var/tmp/ Safely"] is followed. Specifically, "/var/tmp" is used
// unless "TMPDIR" is set in the environment, in which case the contents of that
// variable are interpreted as a path and used.
//
// The RemoteFetchArena attempts to use [O_TMPFILE] and falls back to
// [os.CreateTemp] if that seems to not work. If the filesystem backing "root"
// does not support O_TMPFILE, files may linger in the event of a process
// crashing or an unclean shutdown. Operators should either use a different
// filesystem or arrange for periodic cleanup via [systemd-tmpfiles(8)] or
// similar.
//
// In a containerized environment, operators may need to mount a directory or
// filesystem on "/var/tmp".
//
// On OSX, temporary files are not unlinked from the filesystem upon creation,
// because an equivalent to Linux's "/proc/self/fd" doesn't seem to exist.
Comment on lines +67 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @hdonnay, do you mind elaborating on this or rephrasing the comment? I'm having a hard time grokking it.

... temporary files are not unlinked from the filesystem upon creation,
because an equivalent to Linux's "/proc/self/fd" doesn't seem to exist.

I couldn't find a reason "/proc/self/fd" was required for "unlinking" temporary files. I'm also not sure what unlinking in this context means; my assumption based on the word choice would be that the implementation does some sort of symlinking and we need to unlink them at some point, but I don't think that's the case.

temporary files are not unlinked from the filesystem upon creation

I'm particularly confused by this. If we're on Linux, do other temporary files get unlinked when we create our temporary files?


fwiw, I'm not sure if others do this, but I know Ross and I both run the ACS Scanner on macOS devices, so sorting this out would help us out a lot (in other words, I'm not trying to be pedantic with this comment 🙂)

Copy link
Contributor

Choose a reason for hiding this comment

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

(also happy to move this discussion to another PR or elsewhere if we want to unblock merging this)

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries.

"Unlink" in this context is a filesystem/syscall term-of-art: it means making a name no longer visible. This isn't exactly deleting a file because the semantics change based on the the type of object, the underlying filesystem, and outstanding file descriptions. Out of a combination of battle scars and habit, I tend to say "unlink" when I'm specifically thinking of unlink(2).

With regards to /proc/self/fd, that's a directory containing magic symlinks (actual terminology, see symlink(7)) representing open file descriptors. Opening one of them creates a new file descriptor backed by a new file description (see the NOTES section of open(2)). This means that unlike dup(2), the uses of the two file descriptors are completely independent (e.g. seeking one will not change the offset on the other).

The Linux code uses this to open files that aren't linked into the filesystem (meaning they'll never outlast the process, because only the process has them open) but can still be reopened as many times as needed to create independent file descriptors.

AFAICT, there's not a way to do this on OSX. Looking at FreeBSD man pages, fdescfs(5) acts the same way but only if mounted with nodup, which I don't think is the default. There is mention on that manpage about a O_EMPTY_PATH to openat(2), but I can't find any information on whether that exists on OSX and don't have a machine to test with. To work around this, the code simply doesn't unlink the file and re-opens it by name as needed. The downside is that the files are only removed as long as the program shuts down cleanly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the detailed explanation! I need to dive deeper into all this info, so please feel free to consider this discussion non-blocking for the PR

//
// On UNIX-unlike systems, none of the above logic takes place.
//
// [file-hierarchy(7)]: https://www.freedesktop.org/software/systemd/man/latest/file-hierarchy.html
// ["Using /tmp/ and /var/tmp/ Safely"]: https://systemd.io/TEMPORARY_DIRECTORIES/
// [O_TMPFILE]: https://man7.org/linux/man-pages/man2/open.2.html
// [systemd-tmpfiles(8)]: https://www.freedesktop.org/software/systemd/man/latest/systemd-tmpfiles.html
func NewRemoteFetchArena(wc *http.Client, root string) *RemoteFetchArena {
return &RemoteFetchArena{
wc: wc,
sf: &singleflight.Group{},
root: root,
root: fixTemp(root),
}
}

Expand Down
8 changes: 8 additions & 0 deletions libindex/tempdir_other.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//go:build !unix

package libindex

// FixTemp is a no-op.
func fixTemp(d string) string {
return d
}
18 changes: 18 additions & 0 deletions libindex/tempdir_unix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package libindex

import (
"os"
)

// FixTemp modifies "dir" according to the documented defaults.
//
// See [NewRemoteFetchArena].
func fixTemp(dir string) string {
if dir != "" {
return dir
}
if d, ok := os.LookupEnv("TMPDIR"); ok && d != "" {
return d

Check warning on line 15 in libindex/tempdir_unix.go

View check run for this annotation

Codecov / codecov/patch

libindex/tempdir_unix.go#L14-L15

Added lines #L14 - L15 were not covered by tests
}
return "/var/tmp"

Check warning on line 17 in libindex/tempdir_unix.go

View check run for this annotation

Codecov / codecov/patch

libindex/tempdir_unix.go#L17

Added line #L17 was not covered by tests
}
49 changes: 45 additions & 4 deletions libindex/tempfile_linux.go
Original file line number Diff line number Diff line change
@@ -1,24 +1,65 @@
package libindex

import (
"errors"
"fmt"
"os"
"sync"

"golang.org/x/sys/unix"
)

var tmpMap sync.Map

func canTmp(dir string) (ok, loaded bool) {
v, loaded := tmpMap.Load(dir)
if v == nil {
return false, loaded
}
return v.(bool), loaded
}

func setTmp(dir string, ok bool) {
tmpMap.Store(dir, ok)
}

type tempFile struct {
*os.File
}

func openTemp(dir string) (*tempFile, error) {
f, err := os.OpenFile(dir, os.O_WRONLY|unix.O_TMPFILE, 0644)
var f *os.File
var err error

ok, loaded := canTmp(dir)
switch {
case loaded && ok:
f, err = os.OpenFile(dir, os.O_WRONLY|unix.O_TMPFILE, 0644)
case loaded && !ok:
f, err = os.CreateTemp(dir, "fetcher.*")

Check warning on line 39 in libindex/tempfile_linux.go

View check run for this annotation

Codecov / codecov/patch

libindex/tempfile_linux.go#L38-L39

Added lines #L38 - L39 were not covered by tests
case !loaded:
f, err = os.OpenFile(dir, os.O_WRONLY|unix.O_TMPFILE, 0644)
if err == nil || !errors.Is(err, unix.ENOTSUP) {
ok = true
break
}
f, err = os.CreateTemp(dir, "fetcher.*")
default:
panic("unreachable")

Check warning on line 48 in libindex/tempfile_linux.go

View check run for this annotation

Codecov / codecov/patch

libindex/tempfile_linux.go#L46-L48

Added lines #L46 - L48 were not covered by tests
}
if !loaded {
setTmp(dir, ok)
}
if !ok && err == nil {
crozzy marked this conversation as resolved.
Show resolved Hide resolved
// This is just a best-effort action to keep files from accumulating.
// The correct way is to use the kernel feature for this: the O_TMPFILE flag.
_ = os.Remove(f.Name())

Check warning on line 56 in libindex/tempfile_linux.go

View check run for this annotation

Codecov / codecov/patch

libindex/tempfile_linux.go#L56

Added line #L56 was not covered by tests
}

if err != nil {
return nil, err
}
return &tempFile{
File: f,
}, nil
return &tempFile{File: f}, nil
}

func (t *tempFile) Reopen() (*os.File, error) {
Expand Down
Loading