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

copying a file with a tree path doesn't set timestamps correctly #207

Closed
smira opened this issue Sep 16, 2024 · 0 comments · Fixed by #214
Closed

copying a file with a tree path doesn't set timestamps correctly #207

smira opened this issue Sep 16, 2024 · 0 comments · Fixed by #214
Assignees

Comments

@smira
Copy link

smira commented Sep 16, 2024

This issue appeared while debugging issue with buildkit creating images with wrong timestamps. It is a custom LLB frontend which outputs an instruction like:

COPY --timestamp=<FIXED> /src/file /dst/path1/path2/file

In the output, the /dst/path1/path2/file has proper timestamp <FIXED>, while /dst, /dst/path1, /dst/path1 all have wrong timestamps (current timestamps).

While debugging this, I got down to this library and put a unit-test (to copy_test.go) which fails:

func TestCopyFileWithPathTimestamp(t *testing.T) {
	timestamp := time.Unix(0, 0)
	apply := fstest.Apply(
		fstest.CreateDir("/foo/", 0755),
		fstest.CreateFile("/foo/bar", []byte{}, 0644),
	)

	t1 := t.TempDir()
	t2 := t.TempDir()

	require.NoError(t, apply.Apply(t1))
	require.NoError(t, Copy(context.TODO(), t1, "/foo/bar", t2, "/test1/test2/test3/bar", WithCopyInfo(CopyInfo{
		CopyDirContents: true,
		Utime:           &timestamp,
	})))

	for _, s := range []string{"/test1/test2/test3/bar", "/test1/test2/test3", "/test1/test2", "/test1"} {
		stat, _ := os.Stat(filepath.Join(t2, s))
		require.Equal(t, timestamp, stat.ModTime(), "path: %s", s)
	}
}

This test shows exactly same as I observed with buildkit - the final file has a correct timestamp, while all intermediate directories have a current timestamp.

The reason is that MkdirAll recursively calls itself, and when it creates the parent /path1, the /path1 timestamp will be set correctly with Utimes, but then MkdirAll will create /path1/path2, which will reset the timestamp of /path1, and so on.

The second issue is in the Copy function itself - when it creates the file, it doesn't revert the timestamp of the parent directory.

So I couldn't come up with an easy fix, so creating this issue to get some ideas how to fix it.

For me the most clean way would be to set the timestamp after all copy operations for all the created items to avoid dealing with parent-child file creation.

smira added a commit to smira/extensions that referenced this issue Sep 16, 2024
See tonistiigi/fsutil#207

The result of this issue is that we can't use `finalize` steps where
destination is a directory, so refactor things to pull in such steps
into the `install` step.

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/extensions that referenced this issue Sep 16, 2024
See tonistiigi/fsutil#207

The result of this issue is that we can't use `finalize` steps where
destination is a directory, so refactor things to pull in such steps
into the `install` step.

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/extensions that referenced this issue Sep 16, 2024
See tonistiigi/fsutil#207

The result of this issue is that we can't use `finalize` steps where
destination is a directory, so refactor things to pull in such steps
into the `install` step.

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/extensions that referenced this issue Sep 16, 2024
See tonistiigi/fsutil#207

The result of this issue is that we can't use `finalize` steps where
destination is a directory, so refactor things to pull in such steps
into the `install` step.

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/extensions that referenced this issue Sep 20, 2024
See tonistiigi/fsutil#207

The result of this issue is that we can't use `finalize` steps where
destination is a directory, so refactor things to pull in such steps
into the `install` step.

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
(cherry picked from commit 11f48c5)
@tonistiigi tonistiigi self-assigned this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants