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

Fix multistage caching with COPY --from #2559

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

kt315
Copy link
Contributor

@kt315 kt315 commented Jun 12, 2023

Fixes #1244
Fixes #1877

Description

  • Removed block on use --cache-copy-layers with multistage builds (applied in feat: disable cache-copy-layers in multistage builds; closes 2065 #2227)
  • Removed using digest in composite key with command COPY --from
  • COPY --from command uses src as file context (only changed files will be reason for change hash)
  • ARG and ENV changed before COPY dont change composite key
  • Add and fix some tests
  • Caching work same as caching in docker buildx

Submitter Checklist

  • Includes unit tests
  • Adds integration tests if needed.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

--cache-copy-layers is available again for multistage builds

@aaron-prindle
Copy link
Collaborator

aaron-prindle commented Jun 12, 2023

Thanks for the PR here @kt-315! Haven't had a chance to go through the code yet, from the Unit Tests CI/CD run seems there is some lint errors from gofmt and goimports:

Log link:
https://github.com/GoogleContainerTools/kaniko/actions/runs/5242455713/jobs/9475118769?pr=2559#step:4:1381

Log Snippet:

Gofmt errors in files:
./pkg/executor/build_test.go
diff ./pkg/executor/build_test.go.orig ./pkg/executor/build_test.go
--- ./pkg/executor/build_test.go.orig
+++ ./pkg/executor/build_test.go
@@ -1592,7 +1592,7 @@
 		expectedCacheKey string
 	}{
 		{
-			description:      "multi-stage copy command",
+			description: "multi-stage copy command",
 			// dont use digest from previoust stage for COPY
 			command:          "COPY --from=0 foo.txt bar.txt",
 			expectedCacheKey: "COPY --from=0 foo.txt bar.txt",
FAILED /home/runner/work/kaniko/kaniko/scripts/../hack/gofmt.sh
RUN /home/runner/work/kaniko/kaniko/scripts/../hack/linter.sh
Installing GolangCI-Lint
golangci/golangci-lint info checking GitHub for tag 'v1.51.1'
golangci/golangci-lint info found version: 1.51.1 for v1.51.1/linux/amd64
golangci/golangci-lint info installed /home/runner/work/kaniko/kaniko/hack/bin/golangci-lint
level=warning msg="[runner] The linter 'interfacer' is deprecated (since v1.38.0) due to: The repository of the linter has been archived by the owner. "
level=warning msg="[runner] The linter 'maligned' is deprecated (since v1.38.0) due to: The repository of the linter has been archived by the owner.  Replaced by govet 'fieldalignment'."
level=warning msg="[runner] The linter 'golint' is deprecated (since v1.41.0) due to: The repository of the linter has been archived by the owner.  Replaced by revive."
pkg/executor/build_test.go:1595: File is not `goimports`-ed (goimports)
			description:      "multi-stage copy command",
Error: pkg/commands/run.go:211:1: receiver name cr should be consistent with previous receiver name r for CachingRunCommand (golint)
func (cr *CachingRunCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
^
Error: pkg/commands/run.go:243:1: receiver name cr should be consistent with previous receiver name r for CachingRunCommand (golint)
func (cr *CachingRunCommand) FilesToSnapshot() []string {
^
Error: pkg/commands/run.go:251:1: receiver name cr should be consistent with previous receiver name r for CachingRunCommand (golint)
func (cr *CachingRunCommand) String() string {
^
FAILED /home/runner/work/kaniko/kaniko/scripts/../hack/linter.sh
make: *** [Makefile:67: test] Error 1
Error: Process completed with exit code 2.

@kt315
Copy link
Contributor Author

kt315 commented Jun 12, 2023

Hi! I have fixed this mistakes

@aaron-prindle
Copy link
Collaborator

aaron-prindle commented Jun 14, 2023

Just tried this, seems to be working great! Left some small nit comments, LGTM! Will wait 1-2 days to merge if you want to fix nit. Thanks for your work here!!

Using the repro steps from #1244 at HEAD vs this PR I am seeing the multi-stage layers being cache:

Repro steps copied from above issue reproduced below:

  1. See https://github.com/thaituan/kaniko-cache
  2. Build docker image with cache on
  3. Update scripts in package.json (not change dependencies and devDependencies)
    Example: "test": "echo \"Error: no test specified\" && exit 10" -> "test": "echo \"Error: no test specified\" && exit 20"
  4. Re-build docker image with cache on

Running the repro steps @ HEAD:

first run -> cache-miss:
real	0m30.017s
user	0m0.049s
sys	0m0.021s
============
no file changes, cache-hit:
real	0m14.653s
user	0m0.035s
sys	0m0.031s
============
update package.json -> cache-miss:
real	0m32.029s # <--- this is what we hope improves
user	0m0.050s
sys	0m0.022s
=============
no file changes, cache-hit:
real	0m14.139s
user	0m0.036s
sys	0m0.032s

Running the repro steps at this PR:

new PR, no file changes, first run -> cache-miss (keys must be different now?) :
real	0m32.027s
user	0m0.027s
sys	0m0.030s
=============
new PR, no file changes, 2nd run-> cache-hit:
real	0m15.525s
user	0m0.040s
sys	0m0.026s
==============
new PR, update package.json, 1st run -> partial cache-hit (as hoped!) :
real	0m19.190s # <---- it improves!
user	0m0.030s
sys	0m0.038s

Copy link
Collaborator

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the PR here @kt-315. Left one small nit comment

* Removed block on use --cache-copy-layers with multistage builds
* Removed using digest in composite key with command COPY --from
* COPY --from command uses src as file context (only changed files will be reason for change hash)
* ARG and ENV changed before COPY dont change composite key
* Add and fix some tests
* Caching work same as caching in docker buildx
@aaron-prindle aaron-prindle merged commit eea12bd into GoogleContainerTools:main Jun 16, 2023
@kt315 kt315 deleted the fix_cache_with_copy branch June 17, 2023 16:22
kylecarbs pushed a commit to coder/kaniko that referenced this pull request Jul 12, 2023
* Removed block on use --cache-copy-layers with multistage builds
* Removed using digest in composite key with command COPY --from
* COPY --from command uses src as file context (only changed files will be reason for change hash)
* ARG and ENV changed before COPY dont change composite key
* Add and fix some tests
* Caching work same as caching in docker buildx

Co-authored-by: Sergei Kraev <skraev@tradingview.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants