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

add unit tests for caching run and copy #888

Merged
merged 1 commit into from
Dec 13, 2019

Conversation

cvgw
Copy link
Contributor

@cvgw cvgw commented Nov 27, 2019

Description

Adds unit tests for caching copy and run

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

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

Release Notes

@cvgw
Copy link
Contributor Author

cvgw commented Nov 27, 2019

I think I may have found a bug, which is that CachingCopy and Copy return different values for FilesUsedFromContext. The result of FilesUsedFromContext is included in the cache key which results in CachingCopy and Copy generating different cache keys. This also seem to be true of CachingRun and Run.

https://github.com/GoogleContainerTools/kaniko/pull/888/files#diff-44df3b0bdda7ba139f5aeadce2431721R301

Here it will be the non caching version of the command

files, err := command.FilesUsedFromContext(&cfg, s.args)

Here it will be the caching version of the command

files, err := command.FilesUsedFromContext(&s.cf.Config, s.args)

@cvgw
Copy link
Contributor Author

cvgw commented Nov 27, 2019

I think I may have found a bug, which is that CachingCopy and Copy return different values for FilesUsedFromContext. The result of FilesUsedFromContext is included in the cache key which results in CachingCopy and Copy generating different cache keys. This also seem to be true of CachingRun and Run.

https://github.com/GoogleContainerTools/kaniko/pull/888/files#diff-44df3b0bdda7ba139f5aeadce2431721R301

Here it will be the non caching version of the command

files, err := command.FilesUsedFromContext(&cfg, s.args)

Here it will be the caching version of the command

files, err := command.FilesUsedFromContext(&s.cf.Config, s.args)

After thinking about this more; I don't think it is technically a bug since CachingCopy and CachingRun commands are not pushed to the cache (since they are already there) so the cache keys produced for the caching and non-caching version may have never been intended to be the same. Where this becomes an issue are possible changes where we start using the cache key of layer A in the cache key of layer B (IE COPY --from)

So probably not a bug, but perhaps unexpected.

@cvgw
Copy link
Contributor Author

cvgw commented Nov 28, 2019

I think I may have found a bug, which is that CachingCopy and Copy return different values for FilesUsedFromContext. The result of FilesUsedFromContext is included in the cache key which results in CachingCopy and Copy generating different cache keys. This also seem to be true of CachingRun and Run.
https://github.com/GoogleContainerTools/kaniko/pull/888/files#diff-44df3b0bdda7ba139f5aeadce2431721R301
Here it will be the non caching version of the command

files, err := command.FilesUsedFromContext(&cfg, s.args)

Here it will be the caching version of the command

files, err := command.FilesUsedFromContext(&s.cf.Config, s.args)

After thinking about this more; I don't think it is technically a bug since CachingCopy and CachingRun commands are not pushed to the cache (since they are already there) so the cache keys produced for the caching and non-caching version may have never been intended to be the same. Where this becomes an issue are possible changes where we start using the cache key of layer A in the cache key of layer B (IE COPY --from)

So probably not a bug, but perhaps unexpected.

So this actually is a bug in the case of caching copy commands.

CopyCommand and CachingCopyCommand produce a different cachekey. This means that the cache key used to look up the cached layer (in stagebuilder.optimize) is different than the cache key used to write the layer to the cache (in stagebuilder.build).

The bug becomes apparent when you have a dockerfile that contains a cached copy command followed by other commands like RUN. This is due to the fact that we continuously append the composite cache with new keys. This means that after the cached copy command, optimize and build produce different cache keys. Example below

FROM foo
COPY . ./ 
RUN apt-get install -y bar
optimize ->
COPY . ./  <- is CopyCommand, cache key includes FilesUsedFromContext
RUN apt-get install bar <- cache key includes COPY . ./ FilesUsedFromContext

build ->
COPY . ./ is CachingCopyCommand, cache key does not include FilesUsedFromContext
RUN apt-get install bar <- cache key does not include COPY . ./ FilesUsedFromContext

This results in RUN apt-get install bar producing a different cache key for read (in optimize) vs write (in build). It never finds the cache key it wrote.

This bug is filed as #899

@cvgw cvgw force-pushed the u/cvgw/caching_unit_tests branch 3 times, most recently from d31eff2 to e517fe4 Compare December 9, 2019 23:28
@tejal29 tejal29 merged commit 6cdd9b7 into GoogleContainerTools:master Dec 13, 2019
@cvgw cvgw deleted the u/cvgw/caching_unit_tests branch December 14, 2019 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants