MountStubsCleaner: preserve timestamps by AkihiroSuda · Pull Request #3149 · moby/buildkit

AkihiroSuda · 2022-10-04T16:42:21Z

thaJeztah · 2022-10-05T15:15:00Z

Ah, dang, looks like continuity doesn't have a stub / implementation for Windows; not sure if relevant already; if not we may have to stub it in this repo (or only run those tests on non-Windows)

AkihiroSuda · 2022-10-05T23:03:16Z

Added windows version of Atime() in util/system

thaJeztah · 2022-10-05T23:54:13Z

Added windows version of Atime() in util/system

Works for me (for now); perhaps would be good to also upstream it to containerd/continuity 👍

tonistiigi

Not sure I understand this. Iiuc this PR resets the timestamp that was there before the removal of the directory but why didn't the dir timestamp also get updated when the file/dir was added in first place.

I would have expected that timestamp needs to be saved for each parent path when this function is called and then restored in the callback.

@AkihiroSuda Do you also want to take #2884 if you are going over these cases?

AkihiroSuda · 2022-10-12T02:09:52Z

why didn't the dir timestamp also get updated when the file/dir was added in first place.

Because they are mounted, not added, IIUC

tonistiigi · 2022-10-12T02:11:05Z

The mountpoint is created before the mount can happen. I believe that is happening in runc.

tonistiigi · 2022-11-03T04:20:27Z

-			os.Remove(p)
+			// Back up the timestamps of the dir for reproducible builds
+			// https://github.com/moby/buildkit/issues/3148
+			dir := filepath.Dir(p)


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

Covered in the test for tmp/foo2/bar

tonistiigi

^

tonistiigi · 2022-11-09T05:01:44Z

+			"/tmp/foo2",
+			"/tmp/foo2/bar",
+		}),
+		llb.AddMount("/tmp/foo", llb.Scratch(), llb.Tmpfs()),


Does this test still pass if you remove this mount?

Fails 😞

client_test.go:8008: Error Trace: client_test.go:8008 run.go:86 run.go:189 Error: Not equal: expected: 1234567890 actual : 1667970326 Test: TestIntegration/TestMountStubsTimestamp/worker=oci Messages: tmp/

I assume this has been fixed?

Not really. Hard to cover all corner cases 😞 .
I guess it is ok to just support basic cases such as /etc for now.

I think we should debug a bit what is happening in #3149 (review) . It shouldn't be that hard to figure out the multi-parent case if it is not working already. If the mountpoint doesn't exist, we should check if the parent exists as well.

The test turned out to be wrong. Directory entries must have the '/' suffix in their names.
Will update the test.

tonistiigi · 2022-11-14T21:29:23Z

+	}
+	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


I'm a bit confused about what actually removes this parent dir of the mount. The described behavior is correct but I can't find the code that actually does that.

Me too, shall we remove this check?

The test turned out to be wrong. Directory entries must have the '/' suffix in their names.
Will update the test.

Fix issue 3148 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

tonistiigi · 2022-11-21T23:37:16Z

+		"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/,


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).

~~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

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

buildkit/executor/stubs.go

Line 32 in 09b4613

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?

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

archive: set WithModTimeUpperBound when WithSourceDateEpoch is set containerd/containerd#7710

tonistiigi · 2022-11-21T23:38:43Z

+		if errors.Is(err, io.EOF) {
+			break
+		}
+		require.NoError(t, err)


Why did you remove the mustNotExist cases?

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/)

tonistiigi

Lets make sure to recheck #3149 (comment) case before GA. Please create an issue for it in the milestone.

AkihiroSuda · 2022-11-23T23:40:49Z

Lets make sure to recheck #3149 (comment) case before GA. Please create an issue for it in the milestone.

Opened an issue #3309 .

AkihiroSuda added the area/reproducible-builds label Oct 4, 2022

AkihiroSuda force-pushed the fix-touch-etc branch 2 times, most recently from 6abd4e1 to 3827376 Compare October 5, 2022 05:49

AkihiroSuda added needs-cherry-pick/v0.10 and removed needs-cherry-pick/v0.10 labels Oct 5, 2022

thaJeztah reviewed Oct 5, 2022

View reviewed changes

Comment thread executor/stubs.go Outdated

AkihiroSuda force-pushed the fix-touch-etc branch from 3827376 to 7f5cfe4 Compare October 5, 2022 11:48

AkihiroSuda force-pushed the fix-touch-etc branch from 7f5cfe4 to 6db57dc Compare October 5, 2022 23:02

AkihiroSuda force-pushed the fix-touch-etc branch from 6db57dc to 2128b62 Compare October 5, 2022 23:15

AkihiroSuda requested a review from tonistiigi October 10, 2022 23:02

tonistiigi reviewed Oct 12, 2022

View reviewed changes

AkihiroSuda force-pushed the fix-touch-etc branch from 2128b62 to 78bb1fa Compare November 3, 2022 01:13

tonistiigi reviewed Nov 3, 2022

View reviewed changes

tonistiigi added this to the v0.11.0 milestone Nov 7, 2022

tonistiigi requested changes Nov 7, 2022

View reviewed changes

AkihiroSuda force-pushed the fix-touch-etc branch from 78bb1fa to 4eb66e4 Compare November 9, 2022 04:56

tonistiigi reviewed Nov 9, 2022

View reviewed changes

AkihiroSuda force-pushed the fix-touch-etc branch 3 times, most recently from 03eeb56 to 27f95f3 Compare November 11, 2022 03:59

tonistiigi reviewed Nov 14, 2022

View reviewed changes

AkihiroSuda marked this pull request as draft November 18, 2022 09:23

MountStubsCleaner: preserve timestamps

0b5a315

Fix issue 3148 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda force-pushed the fix-touch-etc branch from 27f95f3 to 0b5a315 Compare November 18, 2022 09:31

AkihiroSuda marked this pull request as ready for review November 18, 2022 09:32

tonistiigi reviewed Nov 21, 2022

View reviewed changes

tonistiigi approved these changes Nov 23, 2022

View reviewed changes

tonistiigi merged commit 54e3957 into moby:master Nov 23, 2022

AkihiroSuda mentioned this pull request Nov 23, 2022

Find a way to support RUN touch --date=@TIMESTAMP /MOUNT/POINT #3309

Open

thaJeztah mentioned this pull request Jun 21, 2024

fs: implement Atime for Windows containerd/continuity#241

Merged

thaJeztah mentioned this pull request Oct 29, 2024

util/system: deprecate Atime implementation for containerd/continuity/fs #5465

Merged

Conversation

AkihiroSuda commented Oct 4, 2022

Uh oh!

Uh oh!

thaJeztah commented Oct 5, 2022

Uh oh!

AkihiroSuda commented Oct 5, 2022

Uh oh!

thaJeztah commented Oct 5, 2022

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Oct 12, 2022

Uh oh!

tonistiigi commented Oct 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Nov 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AkihiroSuda Nov 22, 2022 •

edited

Loading