build, run: record hash or digest in image history for sources used in `--mount` #5691

flouthoc · 2024-08-18T17:00:44Z

When using --mount=type=bind or --mount=type=cache the hash or digest of source in these flags should be added to image history so buildah can burst cache if files on host or image which is being used as source is changed.

Closes

podman build not invalidating layers after a change in a --mount=type=bind directory podman#23382
buildah never invalidates the cache of a RUN --mount=type=bind layer #5400

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

build, run: record hash or digest in image history for sources used in `--mount`

flouthoc · 2024-08-18T17:01:18Z

Need to add tests, will undraft then.

flouthoc · 2024-08-18T23:58:38Z

Will rebase after this #5693

nalind · 2024-09-17T17:52:45Z

I'm surprised that we'd care about the contents of caches. I'd be inclined to archive the contents of a directory (and create a single-entry archive for a non-directory) to account for different permissions/ownership/datestamps/xattrs and to safely handle soft and hard links.

flouthoc · 2024-09-17T18:49:10Z

I'm surprised that we'd care about the contents of caches. I'd be inclined to archive the contents of a directory (and create a single-entry archive for a non-directory)

If i'm understanding this correctly, did you mean instead of computeDirectoryHash or computeFileHash separately and recursively I should just create a temporary archive of the context dir and write its hash in history ? And on everyrun I can do the same process again to check if hash is matching with what is written in the history ? The idea sounds good to me.

nalind · 2024-09-17T19:10:29Z

I'm surprised that we'd care about the contents of caches. I'd be inclined to archive the contents of a directory (and create a single-entry archive for a non-directory)

If i'm understanding this correctly, did you mean instead of computeDirectoryHash or computeFileHash separately and recursively I should just create a temporary archive of the context dir and write its hash in history?

I wouldn't expect the archive to be written anywhere, but the digest of an archive is something we already use as a way of describing contents, when handling COPY and ADD instructions. I don't know yet about doing this over the entire build context or additional build context if only a portion of it is being used at that point (i.e., if "src" is set to a subdirectory).

flouthoc · 2024-09-17T19:57:05Z

I'm surprised that we'd care about the contents of caches. I'd be inclined to archive the contents of a directory (and create a single-entry archive for a non-directory)

If i'm understanding this correctly, did you mean instead of computeDirectoryHash or computeFileHash separately and recursively I should just create a temporary archive of the context dir and write its hash in history?

I wouldn't expect the archive to be written anywhere, but the digest of an archive is something we already use as a way of describing contents, when handling COPY and ADD instructions. I don't know yet about doing this over the entire build context or additional build context if only a portion of it is being used at that point (i.e., if "src" is set to a subdirectory).

This sounds good to me, i will amend the PR.

sanmai-NL · 2024-10-03T10:49:15Z

@flouthoc

I'm seeing this behavior with podman build, will this be fixed by this change too?

ARG PATH_1=mydirectory
ARG SELINUXRELABEL=,z
ARG DISTRO=PATH_1
RUN --mount=type=bind,source=${PATH_1:?},target=/tmp/${PATH_1:?}${SELINUXRELABEL:?} \
  echo Nop

Subsequently, /tmp/mydirectory ends up in an image layer rather than being ephemeral and only mounted at build time. This bloats the image and leaks information.

flouthoc · 2024-10-03T17:14:58Z

RUN --mount=type=bind,source=${PATH_1:?},target=/tmp/${PATH_1:?}${SELINUXRELABEL:?}

@sanmai-NL Are contents of PATH_1 ending up in your final built image as well ? A layer is created so that i can be used later but I don't think contents of RUN --mount are part of your image.

sanmai-NL · 2024-10-04T06:50:30Z

Thanks for your response. Yes indeed, they do end up in there, which I find surprising. And which is the reason for my comment.

Some factors which may cause this in case the problem isn't general
This happens when running podman build from a containerized Podman. Maybe ID mapping also causes some sort of copy operation instead of a bind mount.

sanmai-NL · 2024-10-04T06:52:55Z

RUN --mount=type=bind,source=${PATH_1:?},target=/tmp/${PATH_1:?}${SELINUXRELABEL:?}
@sanmai-NL Are contents of PATH_1 ending up in your final built image as well ? A layer is created so that i can be used later but I don't think contents of RUN --mount are part of your image.

Re-reading your comment... So you do find this ending up in a layer by-design, do you? But this way, information leaks and the image bloats.

flouthoc · 2024-10-10T04:26:09Z

RUN --mount=type=bind,source=${PATH_1:?},target=/tmp/${PATH_1:?}${SELINUXRELABEL:?}

@sanmai-NL Are contents of PATH_1 ending up in your final built image as well ? A layer is created so that i can be used later but I don't think contents of RUN --mount are part of your image.

Re-reading your comment... So you do find this ending up in a layer by-design, do you? But this way, information leaks and the image bloats.

@sanmai-NL What you are describing is a different bug, would it be possible to create a small reproducer and open a new issue ?

github-actions · 2024-11-10T00:14:03Z

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

flouthoc · 2024-12-18T13:23:36Z

Since #5693 is merged, i'll get back to this.

flouthoc · 2025-01-14T22:03:40Z

@nalind @containers/buildah-maintainers PTAL

internal/parse/parse.go

internal/util/util.go

flouthoc · 2025-01-15T18:39:09Z

@nalind Could you PTAL again.

nalind

Be very sure you want to use sha256 over an entire directory tree. It can be an expensive computation if the tree is large.

nalind · 2025-01-15T21:12:10Z

imagebuildah/stage_executor.go

-			if imgID, ref, err = s.commit(ctx, s.getCreatedBy(nil, ""), emptyLayer, s.output, s.executor.squash || s.executor.confidentialWorkload.Convert, lastStage); err != nil {
+			createdBy, err := s.getCreatedBy(nil, "")
+			if err != nil {
+				return "", nil, false, err


Get in the habit of adding context to errors being passed up from calls to methods that returned them, to make it easier to follow the call stack when troubleshooting and debugging. Not just here, but throughout this PR.

imagebuildah/stage_executor.go

internal/util/util.go

imagebuildah/stage_executor.go

internal/util/util.go

internal/parse/parse.go

nalind · 2025-01-15T21:24:07Z

internal/util/util.go

+		return "", err
+	}
+
+	tarWriter.Close()


Move this to just above the if err != nil check, if its return value isn't going to be checked.

sanmai-NL · 2025-01-16T00:38:49Z

https://github.com/BLAKE3-team/BLAKE3

https://github.com/lukechampine/blake3

nalind · 2025-01-16T19:09:23Z

In its current form, this considers caches where a "src" option is used to select a subdirectory, but not otherwise. It should either check the mount type and skip caches, or make arrangements to handle those cases.

nalind

I'm not actually sure of whether or not we should be considering the contents of "cache" mounts here.

imagebuildah/util.go

imagebuildah/util_test.go

nalind · 2025-01-22T22:18:36Z

tests/bud.bats

+Helloworld2
+_EOF
+
+  # on third run since we have added new file `anotherfile` so cache must burst.


Update this comment.

imagebuildah/util.go

imagebuildah/stage_executor.go

nalind

Looking better, but the arg parsing still has a potential crasher in it.

imagebuildah/stage_executor.go

imagebuildah/util.go

nalind · 2025-01-23T19:50:01Z

imagebuildah/util_test.go

+	data := []byte("Hello, world!")
+	if _, err := tempFile.Write(data); err != nil {
+		t.Fatalf("Failed to write data to temp file: %v", err)
+	}


Not a blocker: unless the filename being randomized is important to the test, using os.WriteFile() would be shorter.

imagebuildah/stage_executor.go

imagebuildah/util_test.go

When using `--mount=type=bind` or `--mount=type=cache` the hash or digest of source in these flags should be added to image history so buildah can burst cache if files on host or image which is being used as source is changed. Signed-off-by: flouthoc <[email protected]>

flouthoc · 2025-01-23T23:45:07Z

@nalind Could you PTAL again.

nalind · 2025-01-24T16:10:34Z

/lgtm

flouthoc · 2025-01-24T17:00:26Z

@rhatdan @containers/build-maintainers PTAL

rhatdan · 2025-01-25T11:31:55Z

/approve

openshift-ci · 2025-01-25T11:32:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the approved label Aug 18, 2024

flouthoc marked this pull request as draft August 18, 2024 17:01

openshift-ci bot added the do-not-merge/work-in-progress label Aug 18, 2024

flouthoc mentioned this pull request Aug 18, 2024

buildah cache with --layers does not work when in RUN --mount=type=bind uses image or additional-build-context as source #5692

Closed

flouthoc mentioned this pull request Aug 21, 2024

stage_executor: set avoidLookingCache only if mounting stage and not additional build context #5693

Merged

wegel mentioned this pull request Sep 25, 2024

buildah never invalidates the cache of a RUN --mount=type=bind layer #5400

Closed

github-actions bot added the stale-pr label Nov 10, 2024

nalind mentioned this pull request Nov 21, 2024

RUN instruction with bind mounted image is never cached containers/podman#24640

Open

flouthoc force-pushed the burst-cache-mount branch from b1aa69d to 2b42840 Compare January 14, 2025 21:20

openshift-ci bot removed the approved label Jan 14, 2025

flouthoc marked this pull request as ready for review January 14, 2025 21:21

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 14, 2025

flouthoc force-pushed the burst-cache-mount branch from 2b42840 to 4e114bd Compare January 14, 2025 21:55

flouthoc removed the stale-pr label Jan 14, 2025

flouthoc requested a review from nalind January 14, 2025 22:06

nalind reviewed Jan 14, 2025

View reviewed changes

internal/parse/parse.go Outdated Show resolved Hide resolved

nalind reviewed Jan 14, 2025

View reviewed changes

internal/util/util.go Outdated Show resolved Hide resolved

flouthoc force-pushed the burst-cache-mount branch from 4e114bd to 03fd1c1 Compare January 14, 2025 23:30

flouthoc requested a review from nalind January 15, 2025 00:13

flouthoc force-pushed the burst-cache-mount branch from 03fd1c1 to 78a884c Compare January 15, 2025 14:40

nalind requested changes Jan 15, 2025

View reviewed changes

flouthoc force-pushed the burst-cache-mount branch 3 times, most recently from 7718e85 to e88f782 Compare January 18, 2025 18:31

flouthoc requested a review from nalind January 18, 2025 22:14

nalind requested changes Jan 22, 2025

View reviewed changes

flouthoc force-pushed the burst-cache-mount branch 3 times, most recently from 78cac13 to ef188af Compare January 23, 2025 18:36

nalind requested changes Jan 23, 2025

View reviewed changes

flouthoc force-pushed the burst-cache-mount branch from ef188af to 4383e34 Compare January 23, 2025 23:44

flouthoc requested a review from nalind January 23, 2025 23:44

openshift-ci bot assigned nalind Jan 24, 2025

openshift-ci bot added the lgtm label Jan 24, 2025

openshift-ci bot added the approved label Jan 25, 2025

openshift-merge-bot bot merged commit e08082f into containers:main Jan 25, 2025
32 checks passed

flouthoc mentioned this pull request Jan 25, 2025

podman build not invalidating layers after a change in a --mount=type=bind directory containers/podman#23382

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build, run: record hash or digest in image history for sources used in `--mount` #5691

build, run: record hash or digest in image history for sources used in `--mount` #5691

flouthoc commented Aug 18, 2024

flouthoc commented Aug 18, 2024

flouthoc commented Aug 18, 2024

nalind commented Sep 17, 2024

flouthoc commented Sep 17, 2024

nalind commented Sep 17, 2024

flouthoc commented Sep 17, 2024

sanmai-NL commented Oct 3, 2024

flouthoc commented Oct 3, 2024

sanmai-NL commented Oct 4, 2024 •

edited

Loading

sanmai-NL commented Oct 4, 2024

flouthoc commented Oct 10, 2024

github-actions bot commented Nov 10, 2024

flouthoc commented Dec 18, 2024

flouthoc commented Jan 14, 2025

flouthoc commented Jan 15, 2025

nalind left a comment

nalind Jan 15, 2025

nalind Jan 15, 2025

sanmai-NL commented Jan 16, 2025 •

edited

Loading

nalind commented Jan 16, 2025

nalind left a comment

nalind Jan 22, 2025

nalind left a comment

nalind Jan 23, 2025

flouthoc commented Jan 23, 2025

nalind commented Jan 24, 2025

flouthoc commented Jan 24, 2025

rhatdan commented Jan 25, 2025

openshift-ci bot commented Jan 25, 2025

build, run: record hash or digest in image history for sources used in --mount #5691

build, run: record hash or digest in image history for sources used in --mount #5691

Conversation

flouthoc commented Aug 18, 2024

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

flouthoc commented Aug 18, 2024

flouthoc commented Aug 18, 2024

nalind commented Sep 17, 2024

flouthoc commented Sep 17, 2024

nalind commented Sep 17, 2024

flouthoc commented Sep 17, 2024

sanmai-NL commented Oct 3, 2024

flouthoc commented Oct 3, 2024

sanmai-NL commented Oct 4, 2024 • edited Loading

sanmai-NL commented Oct 4, 2024

flouthoc commented Oct 10, 2024

github-actions bot commented Nov 10, 2024

flouthoc commented Dec 18, 2024

flouthoc commented Jan 14, 2025

flouthoc commented Jan 15, 2025

nalind left a comment

Choose a reason for hiding this comment

nalind Jan 15, 2025

Choose a reason for hiding this comment

nalind Jan 15, 2025

Choose a reason for hiding this comment

sanmai-NL commented Jan 16, 2025 • edited Loading

nalind commented Jan 16, 2025

nalind left a comment

Choose a reason for hiding this comment

nalind Jan 22, 2025

Choose a reason for hiding this comment

nalind left a comment

Choose a reason for hiding this comment

nalind Jan 23, 2025

Choose a reason for hiding this comment

flouthoc commented Jan 23, 2025

nalind commented Jan 24, 2025

flouthoc commented Jan 24, 2025

rhatdan commented Jan 25, 2025

openshift-ci bot commented Jan 25, 2025

build, run: record hash or digest in image history for sources used in `--mount` #5691

build, run: record hash or digest in image history for sources used in `--mount` #5691

sanmai-NL commented Oct 4, 2024 •

edited

Loading

sanmai-NL commented Jan 16, 2025 •

edited

Loading