image/save: set a stable timestamp for assets #48611
Conversation
stevvooe
force-pushed
the
sjd/stable-save-timestamps
branch
from
7ff5326
to
74bf9dd
Compare
When saving a docker image with `docker save`, output may have the current timestamp, resulting in slightly changed content each time the `save` command gets run. This patch attemtps to stabilize that effort to clean up some spots where we've missed setting the timestamps. It's not totally clear that setting these timestamps to 0 is the correct behavior but it will fix the hash stability problem on output. Signed-off-by: Stephen Day <[email protected]>
stevvooe
force-pushed
the
sjd/stable-save-timestamps
branch
from
74bf9dd
to
a5d5914
Compare
| return distribution.Descriptor{}, errors.Wrap(err, "could not set layer timestamp") | ||
| } | ||
| for _, fname := range []string{outDir, layerPath} { | ||
| // todo: maybe save layer created timestamp? |
There was a problem hiding this comment.
This TODO was added back in 2015 by @tonistiigi in 01ba0a9#diff-9ac2cf7826d0cfc8316ca97dceae47447f431bc510fd4a4622ebeb53cc74d08aR295
I wonder if it's still clear what it's about, or if we should just drop it 🤔
There was a problem hiding this comment.
Not quite sure. This just does the nil check once but shouldn't change the behavior here. This PR still defers on a timestamp methodology, other than fixing the issue with the particular assets identified in docker/cli#5515.
The other approach here would be to do a full walk and set them all to the same value after assembling the save file.
There was a problem hiding this comment.
Yeah, was mostly wondering if we had a specific reason to preserve the layer timestamp, because if not, we might as well remove the TODO comment here.
Resetting to the image's timestamp (what I saw you did in this PR) makes sense to me; at least it makes things stable for the same image
| // mkdirAllWithChtimes is nearly an identical copy to the os.MkdirAll but | ||
| // tracks created directories and applies the provided mtime and atime using | ||
| // system.Chtimes. | ||
| func mkdirAllWithChtimes(path string, perm os.FileMode, atime, mtime time.Time) error { |
There was a problem hiding this comment.
Having a copy of os.MkdirAll feels meh.. But I can't think of a better way to do it.
Perhaps it's worth adding the specific Go tag the code was copied from. That would make it easier to update this code to match the Go's MkdirAll in case it changes.
vvoland
added
kind/enhancement
When saving a docker image with
docker save, output may have the current timestamp, resulting in slightly changed content each time thesavecommand gets run. This patch attemtps to stabilize that effort to clean up some spots where we've missed setting the timestamps.It's not totally clear that setting these timestamps to 0 is the correct behavior but it will fix the hash stability problem on output.
Closes docker/cli#5515
We still need some better testing for this and might need some discussion around which timestamp we actually use.