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

Conversation

AkihiroSuda
Copy link
Member

Fix #3148

executor/stubs.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

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)
Screenshot 2022-10-05 at 17 13 40

@AkihiroSuda
Copy link
Member Author

Added windows version of Atime() in util/system

@thaJeztah
Copy link
Member

Added windows version of Atime() in util/system

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

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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
Copy link
Member

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

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

@tonistiigi tonistiigi added this to the v0.11.0 milestone Nov 7, 2022
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

^

"/tmp/foo2",
"/tmp/foo2/bar",
}),
llb.AddMount("/tmp/foo", llb.Scratch(), llb.Tmpfs()),
Copy link
Member

Choose a reason for hiding this comment

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

Does this test still pass if you remove this mount?

Copy link
Member Author

Choose a reason for hiding this comment

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

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/

Copy link
Member

Choose a reason for hiding this comment

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

I assume this has been fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 test turned out to be wrong. Directory entries must have the '/' suffix in their names.
Will update the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@AkihiroSuda AkihiroSuda force-pushed the fix-touch-etc branch 3 times, most recently from 03eeb56 to 27f95f3 Compare November 11, 2022 03:59
}
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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Me too, shall we remove this check?

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 test turned out to be wrong. Directory entries must have the '/' suffix in their names.
Will update the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@AkihiroSuda AkihiroSuda marked this pull request as draft November 18, 2022 09:23
Fix issue 3148

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda AkihiroSuda marked this pull request as ready for review November 18, 2022 09:32
"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

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

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

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

@AkihiroSuda
Copy link
Member Author

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

Opened an issue #3309 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timestamp of /etc cannot be changed
3 participants