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

build with --squash-all + --timestamp clobbers timestamps in preexisting layers #14536

Closed
edsantiago opened this issue Jun 8, 2022 · 10 comments
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@edsantiago
Copy link
Member

[filing under podman, not buildah, because --squash-all is a podman-only option]

$ printf "FROM alpine\nRUN echo hi\n" | bin/podman build --squash-all --timestamp 0 -t foo -
STEP 1/2: FROM alpine
STEP 2/2: RUN echo hi
hi
COMMIT foo
Getting image source signatures
Copying blob f89409ba1b89 skipped: already exists  
Copying config 7350b81f59 done  
Writing manifest to image destination
Storing signatures
--> 7350b81f59a
Successfully tagged localhost/foo:latest
7350b81f59a6cb83bd33e1a5f36a404655b947fa8b47f876dfc9b9e14b041ccc
$ for i in alpine foo;do bin/podman run --rm $i ls -l /etc/profile;done
-rw-r--r--    1 root     root           857 Mar  2 19:00 /etc/profile
-rw-r--r--    1 root     root           857 Jan  1  1970 /etc/profile         <<<<< unexpected

Documentation for --timestamp includes:

All files committed to the layers of the image will be created with the timestamp.

...but to me that applies to files added/created in the build, not files in the existing image. The current behavior violates POLA.

@edsantiago
Copy link
Member Author

Here is my use case:

  1. Build a base image in which /home/podman/testimage-id has a precisely tuned timestamp.
  2. Build a new image based on the above image. This second image must use --timestamp, because of podman system prune support prune unused networks #14556. I would like to use --squash-all here, because otherwise I have to rework an existing image tree test.

--timestamp is a hard requirement, --squash-all is a wishlist. I will figure out a way to update the image tree test if necessary, I just don't think it should be, because I find it surprising that --squash-all clobbers timestamps on preexisting layers. My intuition is that those preexisting layers should be inviolate.

@flouthoc
Copy link
Collaborator

Hi @edsantiago , From last discussion I think fixing this and making --squash-all work with --timestamp would be hard since it also squashes all the upper layers including the base. Not sure if this solution is realistic but one very inefficient solution would be to create copy of the base as the most lower layer again ( without --timestamp and minus the diff from any other layers ) and include it in squashed result again.

@nalind @edsantiago If you are okay with it could we add a note for this behavior of --timestamp with --squash-all in the docs.

@nalind
Copy link
Member

nalind commented Jun 15, 2022

I thought about it some more, and I think that using the not-often-used storage Changes() method to gather a list of modified and added filepaths in the container, and using them to decide whether or not to reset the timestamp on an item that we read when we're tarring up the rootfs, could work. It would still be slower than the current implementation, but not prohibitively slower, and it would avoid the need for additional disk space that the approach I sketched out yesterday would have required.
I don't really have a strong preference between doing that and resolving this with a docs update, though, so whatever folks with stronger opinions prefer would be fine with me.

@edsantiago
Copy link
Member Author

Speaking as someone with strong opinions :-), my intuition is that using --squash-all with --timestamp is going to be rare -- this is definitely an obscure corner case -- but, when someone does use it, precision is going to be more important than speed.

(Disclaimer: conviction is poorly correlated with actual truth. My "strong opinions" really should yield to careful consideration from those better informed).

edsantiago added a commit to edsantiago/libpod that referenced this issue Jun 15, 2022
Changes:
 - use --timestamp option to produce 'created' stamps
   that can be reliably tested in the image-history test

 - podman now supports manifest & multiarch run, so we
   no longer need buildah

 - bump up base alpine & busybox images

This turned out to be WAY more complicated than it should've been,
because:

 - alpine 3.14 fixed 'date -Iseconds' to include a colon in
   the TZ offset ("-07:00", was "-0700"). This is now consistent
   with GNU date's --iso-8601 format, yay, so we can eliminate
   a minor workaround.

 - with --timestamp, all ADDed files are set to that timestamp,
   including the custom-reference-timestamp file that many tests
   rely on. So we need to split the build into two steps. But:

 - ...with a two-step build I need to use --squash-all, not --squash, but:

 - ... (deep sigh) --squash-all doesn't work with --timestamp (containers#14536)
   so we need to alter existing tests to deal with new image layers.

 - And, long and sordid story relating to --rootfs. TL;DR that option
   only worked by a miracle relating to something special in one
   specific test image; it doesn't work with any other images. Fix
   seems to be complicated, so we're bypassing with a FIXME (containers#14505).

And, unrelated:

 - remove obsolete skip and workaround in run-basic test (dating
   back to varlink days)
 - add a pause-image cleanup to avoid icky red warnings in logs

Fixes: containers#14456

Signed-off-by: Ed Santiago <[email protected]>
@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Jul 16, 2022

@nalind @flouthoc Did either of you look at this issue?

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Aug 16, 2022

@nalind @flouthoc Re-ping

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@edsantiago
Copy link
Member Author

This is unlikely ever to get fixed. I have a suboptimal workaround and can live with the bug.

@edsantiago edsantiago closed this as not planned Won't fix, can't repro, duplicate, stale Sep 3, 2024
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Dec 3, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Dec 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

No branches or pull requests

4 participants