pkg/archive: ignore mtime changes on directories #11422

vbatts · 2015-03-16T20:33:06Z

on overlay fs, the mtime of directories changes in a container where new
files are added in an upper layer (e.g. '/etc'). This flags the
directory as a change where there was none.

Related #9874

Ping @jfrazelle

Signed-off-by: Vincent Batts vbatts@redhat.com

jessfraz · 2015-03-16T20:34:26Z

omg cool!!!!

LK4D4 · 2015-03-16T20:37:50Z

That condition looks awful :E

vbatts · 2015-03-16T20:38:23Z

@LK4D4 so it goes

jessfraz · 2015-03-16T20:38:36Z

imma try it on overlay

vbatts · 2015-03-16T20:38:56Z

this fixes TestDiffEnsureOnlyKmsgAndPtmx and TestSaveDirectoryPermissions for me

vbatts · 2015-03-16T20:42:07Z

/me looking

vbatts · 2015-03-16T20:42:44Z

ah, TestChangesDirsMutated

LK4D4 · 2015-03-16T20:44:38Z

pkg/archive/changes.go

@@ -221,7 +221,7 @@ func (info *FileInfo) addChanges(oldInfo *FileInfo, changes *[]Change) {
 				oldStat.Rdev() != newStat.Rdev() ||
 				// Don't look at size for dirs, its not a good measure of change
 				(oldStat.Size() != newStat.Size() && oldStat.Mode()&syscall.S_IFDIR != syscall.S_IFDIR) ||
-				!sameFsTimeSpec(oldStat.Mtim(), newStat.Mtim()) ||
+				(!sameFsTimeSpec(oldStat.Mtim(), newStat.Mtim()) && oldStat.Mode()&syscall.S_IFDIR != syscall.S_IFDIR) ||


Can we rewrite it pls?
Like

oldStat.Mode()&syscall.S_IFDIR != 0 && ( oldStat.Size() != newStat.Size() || !sameFsTimeSpec(oldStat.Mtim(), newStat.Mtim()) )

jessfraz · 2015-03-16T20:53:41Z

yayyy this fixed integration tests :D

vbatts · 2015-03-16T20:54:15Z

@LK4D4 updated PTAL

LK4D4 · 2015-03-16T20:55:37Z

pkg/archive/changes.go

@@ -220,8 +220,8 @@ func (info *FileInfo) addChanges(oldInfo *FileInfo, changes *[]Change) {
 				oldStat.Gid() != newStat.Gid() ||
 				oldStat.Rdev() != newStat.Rdev() ||
 				// Don't look at size for dirs, its not a good measure of change
-				(oldStat.Size() != newStat.Size() && oldStat.Mode()&syscall.S_IFDIR != syscall.S_IFDIR) ||
-				!sameFsTimeSpec(oldStat.Mtim(), newStat.Mtim()) ||
+				(oldStat.Mode()&syscall.S_IFDIR != syscall.S_IFDIR &&


Why we comparing not with 0? Everywhere in other code we comparing with it.

Ah, because the line above wasnt comparing to zero... :-)
On Mar 16, 2015 4:55 PM, "Alexander Morozov" notifications@github.com
wrote:

In pkg/archive/changes.go
#11422 (comment):

@@ -220,8 +220,8 @@ func (info *FileInfo) addChanges(oldInfo *FileInfo, changes *[]Change) {
oldStat.Gid() != newStat.Gid() ||
oldStat.Rdev() != newStat.Rdev() ||
// Don't look at size for dirs, its not a good measure of change

(oldStat.Size() != newStat.Size() && oldStat.Mode()&syscall.S_IFDIR != syscall.S_IFDIR) ||

!sameFsTimeSpec(oldStat.Mtim(), newStat.Mtim()) ||

(oldStat.Mode()&syscall.S_IFDIR != syscall.S_IFDIR &&

Why we comparing not with 0? Everywhere in other code we comparing with
it.

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/11422/files#r26526731.

Look, your tests already failing because of this!

Eh?
On Mar 16, 2015 5:04 PM, "Alexander Morozov" notifications@github.com
wrote:

In pkg/archive/changes.go
#11422 (comment):

@@ -220,8 +220,8 @@ func (info *FileInfo) addChanges(oldInfo *FileInfo, changes *[]Change) {
oldStat.Gid() != newStat.Gid() ||
oldStat.Rdev() != newStat.Rdev() ||
// Don't look at size for dirs, its not a good measure of change

(oldStat.Size() != newStat.Size() && oldStat.Mode()&syscall.S_IFDIR != syscall.S_IFDIR) ||

!sameFsTimeSpec(oldStat.Mtim(), newStat.Mtim()) ||

(oldStat.Mode()&syscall.S_IFDIR != syscall.S_IFDIR &&

Look, your tests already failing because of this!

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/11422/files#r26527502.

That was joke :) But I still don't like it.

:-) well the parens weren't accepted on new lines, and splitting the next seemed even more confusing

jessfraz · 2015-03-16T21:21:06Z

you can change to closes #9874 so it actually closes it too :D

unclejack · 2015-03-16T22:06:14Z

This change needs to be checked with push and import on other graph drivers like aufs. We had some problems a while ago and I'm not sure this won't cause problems there again.

jessfraz · 2015-03-16T22:07:59Z

hmmm good call i can run it across the matrix

jessfraz · 2015-03-16T23:39:26Z

seems not to fail on aufs or btrfs for me

jessfraz · 2015-03-17T01:19:31Z

LGTM but @unclejack is the archive master here :)

unclejack · 2015-03-17T12:49:17Z

I can confirm that the test for the previously fixed issue with permissions is still there and it's still passing.

I'll test this on all graph drivers and comment again to confirm it's OK for the merge as far as I'm concerned.

@jfrazelle Thank you!

on overlay fs, the mtime of directories changes in a container where new files are added in an upper layer (e.g. '/etc'). This flags the directory as a change where there was none. Closes moby#9874 Signed-off-by: Vincent Batts <vbatts@redhat.com>

vbatts · 2015-03-17T14:54:39Z

also, i just updated the commit message to "closes ..", instead of "related ..."

vbatts · 2015-03-20T17:54:11Z

@unclejack results?

On Tue, Mar 17, 2015 at 8:49 AM, unclejack notifications@github.com wrote:

I can confirm that the test for the previously fixed issue with
permissions is still there and it's still passing.

I'll test this on all graph drivers and comment again to confirm it's OK
for the merge as far as I'm concerned.

@jfrazelle https://github.com/jfrazelle Thank you!

—
Reply to this email directly or view it on GitHub
#11422 (comment).

jessfraz · 2015-03-20T18:28:07Z

I think we should get this in for 1.6

unclejack · 2015-03-23T22:20:31Z

Overlay is working properly now. Thanks, @vbatts!

LGTM

pkg/archive: ignore mtime changes on directories

thaJeztah · 2015-03-23T22:28:47Z

@jfrazelle I see this is merged now, should the milestone be moved back to 1.6?

jessfraz · 2015-03-23T22:32:59Z

move it thanks @thaJeztah

Signed-off-by: John Howard <jhoward@microsoft.com> If fixes an error in sameFsTime which was using `==` to compare two times. The correct way is to use go's built-in timea.Equals(timeb). In changes_windows, it uses sameFsTime to compare mTim of a `system.StatT` to allow TestChangesDirsMutated to operate correctly now. Note there is slight different between the Linux and Windows implementations of detecting changes. Due to moby#9874, and the fix at moby#11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See moby#37982 for more information. The result in `TestChangesDirsMutated`, `dir3` is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second. With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows. It provides an implementation of `copyDir` in tests for Windows. To make a copy similar to Linux's `cp -a` while preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code.

Signed-off-by: John Howard <jhoward@microsoft.com> If fixes an error in sameFsTime which was using `==` to compare two times. The correct way is to use go's built-in timea.Equals(timeb). In changes_windows, it uses sameFsTime to compare mTim of a `system.StatT` to allow TestChangesDirsMutated to operate correctly now. Note there is slight different between the Linux and Windows implementations of detecting changes. Due to moby/moby#9874, and the fix at moby/moby#11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See moby/moby#37982 for more information. The result in `TestChangesDirsMutated`, `dir3` is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second. With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows. It provides an implementation of `copyDir` in tests for Windows. To make a copy similar to Linux's `cp -a` while preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code. Upstream-commit: 56b732058e4d55aa5559996cce03fe11e6ff3baa Component: engine

Signed-off-by: John Howard <jhoward@microsoft.com> If fixes an error in sameFsTime which was using `==` to compare two times. The correct way is to use go's built-in timea.Equals(timeb). In changes_windows, it uses sameFsTime to compare mTim of a `system.StatT` to allow TestChangesDirsMutated to operate correctly now. Note there is slight different between the Linux and Windows implementations of detecting changes. Due to moby#9874, and the fix at moby#11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See moby#37982 for more information. The result in `TestChangesDirsMutated`, `dir3` is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second. With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows. It provides an implementation of `copyDir` in tests for Windows. To make a copy similar to Linux's `cp -a` while preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code.

vbatts added the area/storage/overlay label Mar 16, 2015

GordonTheTurtle added status/0-triage dco/yes labels Mar 16, 2015

jessfraz added status/2-code-review and removed status/0-triage labels Mar 16, 2015

LK4D4 reviewed Mar 16, 2015
View reviewed changes

vbatts force-pushed the vbatts-overlay_dir_mtime_changes branch from 52d30b3 to 3c96c65 Compare March 16, 2015 20:48

vbatts force-pushed the vbatts-overlay_dir_mtime_changes branch from 3c96c65 to 7e23793 Compare March 16, 2015 20:54

LK4D4 reviewed Mar 16, 2015
View reviewed changes

jessfraz added this to the 1.6.0 milestone Mar 16, 2015

vbatts force-pushed the vbatts-overlay_dir_mtime_changes branch from 7e23793 to 2ce37f6 Compare March 17, 2015 14:53

jessfraz modified the milestones: 1.7.0, 1.6.0 Mar 23, 2015

jessfraz pushed a commit that referenced this pull request Mar 23, 2015

Merge pull request #11422 from vbatts/vbatts-overlay_dir_mtime_changes

9d00d81

pkg/archive: ignore mtime changes on directories

jessfraz merged commit 9d00d81 into moby:master Mar 23, 2015

jessfraz modified the milestones: 1.6.0, 1.7.0 Mar 23, 2015

crawford mentioned this pull request Sep 23, 2015

broken yum-based images coreos/bugs#615

Closed

vbatts deleted the vbatts-overlay_dir_mtime_changes branch April 27, 2016 18:03

lowenna mentioned this pull request Oct 8, 2018

pkg/archive fixes, and port most unit tests to Windows #37982

Merged

thaJeztah mentioned this pull request Jan 30, 2019

pkg/archive:CopyTo(): fix for long dest filename #38634

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/archive: ignore mtime changes on directories #11422

pkg/archive: ignore mtime changes on directories #11422

vbatts commented Mar 16, 2015

jessfraz commented Mar 16, 2015

LK4D4 commented Mar 16, 2015

vbatts commented Mar 16, 2015

jessfraz commented Mar 16, 2015

vbatts commented Mar 16, 2015

vbatts commented Mar 16, 2015

vbatts commented Mar 16, 2015

LK4D4 Mar 16, 2015

jessfraz commented Mar 16, 2015

vbatts commented Mar 16, 2015

LK4D4 Mar 16, 2015

vbatts Mar 16, 2015

LK4D4 Mar 16, 2015

vbatts Mar 16, 2015

LK4D4 Mar 16, 2015

vbatts Mar 17, 2015

jessfraz commented Mar 16, 2015

unclejack commented Mar 16, 2015

jessfraz commented Mar 16, 2015

jessfraz commented Mar 16, 2015

jessfraz commented Mar 17, 2015

unclejack commented Mar 17, 2015

vbatts commented Mar 17, 2015

vbatts commented Mar 20, 2015

jessfraz commented Mar 20, 2015

unclejack commented Mar 23, 2015

thaJeztah commented Mar 23, 2015

jessfraz commented Mar 23, 2015

pkg/archive: ignore mtime changes on directories #11422

pkg/archive: ignore mtime changes on directories #11422

Conversation

vbatts commented Mar 16, 2015

jessfraz commented Mar 16, 2015

LK4D4 commented Mar 16, 2015

vbatts commented Mar 16, 2015

jessfraz commented Mar 16, 2015

vbatts commented Mar 16, 2015

vbatts commented Mar 16, 2015

vbatts commented Mar 16, 2015

LK4D4 Mar 16, 2015

Choose a reason for hiding this comment

jessfraz commented Mar 16, 2015

vbatts commented Mar 16, 2015

LK4D4 Mar 16, 2015

Choose a reason for hiding this comment

vbatts Mar 16, 2015

Choose a reason for hiding this comment

LK4D4 Mar 16, 2015

Choose a reason for hiding this comment

vbatts Mar 16, 2015

Choose a reason for hiding this comment

LK4D4 Mar 16, 2015

Choose a reason for hiding this comment

vbatts Mar 17, 2015

Choose a reason for hiding this comment

jessfraz commented Mar 16, 2015

unclejack commented Mar 16, 2015

jessfraz commented Mar 16, 2015

jessfraz commented Mar 16, 2015

jessfraz commented Mar 17, 2015

unclejack commented Mar 17, 2015

vbatts commented Mar 17, 2015

vbatts commented Mar 20, 2015

jessfraz commented Mar 20, 2015

unclejack commented Mar 23, 2015

thaJeztah commented Mar 23, 2015

jessfraz commented Mar 23, 2015