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
builder: Fix WORKDIR
with a trailing slash causing a cache miss
#47723
Conversation
b106d63
to
2fcb3ed
Compare
8c7bbde
to
217d538
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; left a small comment about t.Helper()
and a tiny nit (both not hard blockers)
integration/build/build_test.go
Outdated
// Test for #47627 | ||
func TestBuildWorkdirNoCacheMiss(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really minor nit here, that it can be more convenient to put a full link to the ticket here, and perhaps to describe the original issue (so that it may not be needed at all to refer to github. I guess "technically" this comment should also follow // TestBuildWorkdirNoCacheMiss ....
GoDoc format (but I don't think we have linters verifying GoDoc on tests)
Not a blocker; just if you still want to adjust before merging.
{name: "trailing slash", dockerfile: "FROM busybox\nWORKDIR /foo/"}, | ||
{name: "no trailing slash", dockerfile: "FROM busybox\nWORKDIR /foo"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment / thinking out loud; wondering how we do cache-matching on Windows, where directories are case-insensitive 🤔 (not sure if we should care.. it would be a corner-case at most, mostly curious)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think mixing the case sensitivity will produce a cache miss on Windows.
But that's out of scope of this PR, and probably not a too big deal, as we mostly care about caching the same Dockerfile builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, for sure! No need to spend time on that, unless it'd be an actual issue (but if we don't support it, it's likely been like that for years already, so .. 🤷)
integration/build/build_test.go
Outdated
func readBuildImageIDs(t *testing.T, rd io.Reader) string { | ||
decoder := json.NewDecoder(rd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be a t.Helper()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should look if we need proper helpers for build
tests; ISTR there's various implementations for doing these kind of things (and it's admittedly awkward to test these, as you need to handle the progress stream 😞)
I guess also somewhat related to;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, instead of working on proper helpers, I'd rather just work on a better API.. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, instead of working on proper helpers, I'd rather just work on a better API.. 😅
Yup, agreed, and I think we may have to start working on that, also in light of improvements in output / progress with multi-arch etc.
That said; we can't change old API versions, so we'd have to outweigh the amount of work needed to provide more decent helpers for the existing bits, or to fully ignore those, and only focus on "next".
8843125
to
c02e2eb
Compare
The `normalizeWorkdir` function has two branches, one that returns a result of `filepath.Join` which always returns a cleaned path, and another one where the input string is returned unmodified. To make these two outputs consistent, also clean the path in the second branch. This also makes the cleaning of the container workdir explicit in the `normalizeWorkdir` function instead of relying on the `SetupWorkingDirectory` to mutate it. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Don't mutate the container's `Config.WorkingDir` permanently with a cleaned path when creating a working directory. Move the `filepath.Clean` to the `translateWorkingDir` instead. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
c02e2eb
to
7532420
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [docker/docker](https://togithub.com/docker/docker) | minor | `26.0.2` -> `26.1.0` | --- ### Release Notes <details> <summary>docker/docker (docker/docker)</summary> ### [`v26.1.0`](https://togithub.com/moby/moby/releases/tag/v26.1.0) [Compare Source](https://togithub.com/docker/docker/compare/v26.0.2...v26.1.0) #### 26.1.0 For a full list of pull requests and changes in this release, refer to the relevant GitHub milestones: - [docker/cli, 26.1.0 milestone](https://togithub.com/docker/cli/issues?q=is%3Aclosed+milestone%3A26.1.0) - [moby/moby, 26.1.0 milestone](https://togithub.com/moby/moby/issues?q=is%3Aclosed+milestone%3A26.1.0) - Deprecated and removed features, see [Deprecated Features](https://togithub.com/docker/cli/blob/v26.1.0/docs/deprecated.md). - Changes to the Engine API, see [API version history](https://togithub.com/moby/moby/blob/v26.1.0/docs/api/version-history.md). ##### New - Add configurable OpenTelemetry utilities and basic instrumentation to commands. For more information, see [OpenTelemetry for the Docker CLI](https://docs.docker.com/config/otel). [docker/cli#4889](https://togithub.com/docker/cli/pull/4889) ##### Bug fixes and enhancements - Native Windows containers are configured with an internal DNS server for container name resolution, and external DNS servers for other lookups. Not all resolvers, including `nslookup`, fall back to the external resolvers when they get a `SERVFAIL` answer from the internal server. So, the internal DNS server can now be configured to forward requests to the external resolvers, by setting `"features": {"windows-dns-proxy": true }` in the `daemon.json` file. [moby/moby#47584](https://togithub.com/moby/moby/pull/47584) > \[!NOTE] > This will be the new default behavior in Docker Engine 27.0. > \[!WARNING] > The `windows-dns-proxy` feature flag will be removed in a future release. - Swarm: Fix `Subpath` not being passed to the container config. [moby/moby#47711](https://togithub.com/moby/moby/pull/47711) - Classic builder: Fix cache miss on `WORKDIR <directory>/` build step (directory with a trailing slash). [moby/moby#47723](https://togithub.com/moby/moby/pull/47723) - containerd image store: Fix `docker images` failing when any image in the store has unexpected target. [moby/moby#47738](https://togithub.com/moby/moby/pull/47738) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6am on monday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/earthly/dind). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMjEuMiIsInVwZGF0ZWRJblZlciI6IjM3LjMyMS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJyZW5vdmF0ZSJdfQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: idodod <ido@earthly.dev>
WORKDIR
is not cached by legacy builder with trailing slash #47627- What I did
- How I did it
Removed
SetupWorkingDirectory
side effect (mutating the container Config).- How to verify it
TestBuildWorkdirNoCacheMiss
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)