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

MountStubsCleaner: preserve timestamps #3149

Merged
merged 1 commit into from
Nov 23, 2022
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
70 changes: 70 additions & 0 deletions client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ func TestIntegration(t *testing.T) {
testSBOMScan,
testSBOMScanSingleRef,
testMultipleCacheExports,
testMountStubsTimestamp,
)
}

Expand Down Expand Up @@ -7902,6 +7903,75 @@ func testMultipleCacheExports(t *testing.T, sb integration.Sandbox) {
ensureFileContents(t, filepath.Join(destDir, "unique"), string(uniqueFile))
}

// https://github.com/moby/buildkit/issues/3148
func testMountStubsTimestamp(t *testing.T, sb integration.Sandbox) {
c, err := New(sb.Context(), sb.Address())
require.NoError(t, err)
defer c.Close()

const sourceDateEpoch = int64(1234567890) // Fri Feb 13 11:31:30 PM UTC 2009
st := llb.Image("busybox:latest").Run(
llb.Args([]string{"/bin/touch", fmt.Sprintf("--date=@%d", sourceDateEpoch),
"/bin",
"/etc",
"/var",
"/var/foo",
"/tmp",
"/tmp/foo2",
"/tmp/foo2/bar",
}),
llb.AddMount("/var/foo", llb.Scratch(), llb.Tmpfs()),
llb.AddMount("/tmp/foo2/bar", llb.Scratch(), llb.Tmpfs()),
)
def, err := st.Marshal(sb.Context())
require.NoError(t, err)

tmpDir := t.TempDir()
tarFile := filepath.Join(tmpDir, "out.tar")
tarFileW, err := os.Create(tarFile)
require.NoError(t, err)
defer tarFileW.Close()

_, err = c.Solve(sb.Context(), def, SolveOpt{
Exports: []ExportEntry{
{
Type: ExporterTar,
Output: fixedWriteCloser(tarFileW),
},
},
}, nil)
require.NoError(t, err)
tarFileW.Close()

tarFileR, err := os.Open(tarFile)
require.NoError(t, err)
defer tarFileR.Close()
tarR := tar.NewReader(tarFileR)
touched := map[string]*tar.Header{
"bin/": nil, // Regular dir
"etc/": nil, // Parent of file mounts (etc/{resolv.conf, hosts})
"var/": nil, // Parent of dir mount (var/foo/)
"tmp/": nil, // Grandparent of dir mount (tmp/foo2/bar/)
// No support for reproducing the timestamps of mount point directories such as var/foo/ and tmp/foo2/bar/,
Copy link
Member

Choose a reason for hiding this comment

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

Could you try fixing this? #3149 (comment) to check the parents makes sense? Otherwise, we at least need to track it as a separate bug(hopefully in the same milestone).

Copy link
Member Author

@AkihiroSuda AkihiroSuda Nov 22, 2022

Choose a reason for hiding this comment

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

I'm not sure how it would be possible.
Maybe, we would need to modify runc to mkdir the mountpoints with the specified SOURCE_DATE_EPOCH.

An alternative way would be to add a new differ opt (in a separate PR) to support if tm > SOURCE_DATE_EPOCH { tm = SOURCE_DATE_EPOCH } #3296

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand. Isn't it just that in

if errors.Is(err, os.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) {
we also need to check for the parent of that path and add it to paths if it doesn't exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

The timestamp specified in RUN touch is lost when the executor container exits, so it seems hard to retain custom timestamps.

SOURCE_DATE_EPOCH support for mount point dirs (and any arbitrary dirs/files) will be covered in

// because the touched timestamp value is lost when the mount is unmounted.
}
for {
hd, err := tarR.Next()
if errors.Is(err, io.EOF) {
break
}
require.NoError(t, err)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the mustNotExist cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

The mustNotExist test was simply wrong.

// WRONG
	mustNotExist := map[string]struct{}{
		"var/foo":  {}, // Created on mounting var/foo, and removed on unmounting it
		"tmp/foo2": {}, // Created on mounting tmp/foo2/bar, and removed on unmounting it
	}

var/foo was a typo of var/foo/, and this mountpoint directory is not removed by BuildKit nor by runc.
Same applies to tmp/foo2 (tmp/foo2/)

if x, ok := touched[hd.Name]; ok && x == nil {
touched[hd.Name] = hd
}
}
for name, hd := range touched {
t.Logf("Verifying %q (%+v)", name, hd)
require.NotNil(t, hd, name)
require.Equal(t, sourceDateEpoch, hd.ModTime.Unix(), name)
}
}

func ensureFile(t *testing.T, path string) {
st, err := os.Stat(path)
require.NoError(t, err, "expected file at %s", path)
Expand Down
26 changes: 25 additions & 1 deletion executor/stubs.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"syscall"

"github.com/containerd/continuity/fs"
"github.com/moby/buildkit/util/system"
"github.com/sirupsen/logrus"
)

func MountStubsCleaner(dir string, mounts []Mount) func() {
Expand Down Expand Up @@ -43,7 +45,29 @@ func MountStubsCleaner(dir string, mounts []Mount) func() {
if st.Size() != 0 {
continue
}
os.Remove(p)
// Back up the timestamps of the dir for reproducible builds
// https://github.com/moby/buildkit/issues/3148
dir := filepath.Dir(p)
Copy link
Member

Choose a reason for hiding this comment

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

What if the parent dir or the mount also did not exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

Covered in the test for tmp/foo2/bar

dirSt, err := os.Stat(dir)
if err != nil {
logrus.WithError(err).Warnf("Failed to stat %q (parent of mount stub %q)", dir, p)
continue
}
mtime := dirSt.ModTime()
atime, err := system.Atime(dirSt)
if err != nil {
logrus.WithError(err).Warnf("Failed to stat atime of %q (parent of mount stub %q)", dir, p)
atime = mtime
}

if err := os.Remove(p); err != nil {
logrus.WithError(err).Warnf("Failed to remove mount stub %q", p)
}

// Restore the timestamps of the dir
if err := os.Chtimes(dir, atime, mtime); err != nil {
logrus.WithError(err).Warnf("Failed to restore time time mount stub timestamp (os.Chtimes(%q, %v, %v))", dir, atime, mtime)
}
}
}
}
21 changes: 21 additions & 0 deletions util/system/atime_unix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//go:build !windows
// +build !windows

package system

import (
"fmt"
iofs "io/fs"
"syscall"
"time"

"github.com/containerd/continuity/fs"
)

func Atime(st iofs.FileInfo) (time.Time, error) {
stSys, ok := st.Sys().(*syscall.Stat_t)
if !ok {
return time.Time{}, fmt.Errorf("expected st.Sys() to be *syscall.Stat_t, got %T", st.Sys())
}
return fs.StatATimeAsTime(stSys), nil
}
17 changes: 17 additions & 0 deletions util/system/atime_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package system

import (
"fmt"
iofs "io/fs"
"syscall"
"time"
)

func Atime(st iofs.FileInfo) (time.Time, error) {
stSys, ok := st.Sys().(*syscall.Win32FileAttributeData)
if !ok {
return time.Time{}, fmt.Errorf("expected st.Sys() to be *syscall.Win32FileAttributeData, got %T", st.Sys())
}
// ref: https://github.com/golang/go/blob/go1.19.2/src/os/types_windows.go#L230
return time.Unix(0, stSys.LastAccessTime.Nanoseconds()), nil
}