Skip to content

Conversation

@lukasberger
Copy link
Contributor

  • Analyzer restores metadata for cache and app layers in /layers:
    • Restores metadata and sha for all cache=true layers.
    • Restores metadata and sha for all launch=true layers.
    • Does not restore metadata for cache=false and build=true layers.
  • Restorer attempts to restore all cache layers in /layers.
    • Restores data for all cache=true layers that are in the cache.
    • Removes metadata for all cache=true layers that are not in cache.
    • Leaves all cache=false metadata as they are.
  • Remove obsolete analyzer testdata.
  • Remove unused classifiyCache.

Part 2 of the implementation of buildpacks/rfcs#21.

@lukasberger lukasberger force-pushed the switch-analyze-restore branch from 28b8b2c to fbd6b13 Compare November 1, 2019 22:37
@sclevine sclevine requested review from ekcasey and sclevine November 6, 2019 20:09
Copy link
Member

@ekcasey ekcasey left a comment

Choose a reason for hiding this comment

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

This looks great! I do think we need to handle the no-op cache case in analyzer before we can merge. restorer can be skipped when there is no cache but analyzer should always run and a platform should be able to use the lifecycle w/o supporting providing a cache.

* Analyzer restores metadata for cache and app layers in /layers:
  * Restores metadata and sha for all cache=true layers.
  * Restores metadata and sha for all launch=true layers.
  * Does not restore metadata for cache=false and build=true layers.
* Restorer attempts to restore all cache layers in /layers.
  * Restores data for all cache=true layers that are in the cache.
  * Removes metadata for all cache=true layers that are not in cache.
  * Leaves all cache=false metadata as they are.
* Remove obsolete analyzer testdata.
* Remove unused classifiyCache.

Signed-off-by: Lukas Berger <[email protected]>
* If not cache flag is specified warn and use empty cache metadata.
* Change info log messages to debug.
* Remove duplicate test case from analyzer.
* Remove misleading tests from restorer.

Signed-off-by: Lukas Berger <[email protected]>
@lukasberger lukasberger force-pushed the switch-analyze-restore branch from fbd6b13 to d61bdae Compare November 13, 2019 03:01
@ekcasey
Copy link
Member

ekcasey commented Nov 13, 2019

@lukasberger It occurs to me that there is a race condition here when an image is used as a cache. The image digest at the given tag can change between the execution of analyzer and restorer.

Once upon a time there was a similar race condition between analyzer and exporter. The "previous image" at a given tag may have changed during the time interval after it was analyzed and before we attempt to reuse layers. We solved that race condition by putting the digest reference to the previous image in the analyzed.toml file and using the immutable image at the digest reference from which to consume layers during export.

We may want to store the digest reference of a cache image during the analyze phase so that the restorer can use the exact same image. Otherwise, there could be some wacky edge cases where the metadata doesn't accurately describe the restored layer contents.

EDIT: retracted b/c of sha comparison

@lukasberger
Copy link
Contributor Author

lukasberger commented Nov 13, 2019

@lukasberger It occurs to me that there is a race condition here when an image is used as a cache. The image digest at the given tag can change between the execution of analyzer and restorer.

Once upon a time there was a similar race condition between analyzer and exporter. The "previous image" at a given tag may have changed during the time interval after it was analyzed and before we attempt to reuse layers. We solved that race condition by putting the digest reference to the previous image in the analyzed.toml file and using the immutable image at the digest reference from which to consume layers during export.

We may want to store the digest reference of a cache image during the analyze phase so that the restorer can use the exact same image. Otherwise, there could be some wacky edge cases where the metadata doesn't accurately describe the restored layer contents.

Since the restorer verifies the metadata sha restored by analyzer matches the layer sha in the cache image and deletes the layer metadata if the two don't match, is this really an issue? Presumably if the layer sha still matches, we don't care if the cache image changed since the layer data is the same.

@ekcasey
Copy link
Member

ekcasey commented Nov 13, 2019

@lukasberger good point. The previous case was different b/c there was no sha to compare. I retract my concern.

In restorer, change error to warning so that restorer can clean up
any accidentally-restored metadata.

Signed-off-by: Lukas Berger <[email protected]>
The function pulls in unnecessary dependencies.

Signed-off-by: Lukas Berger <[email protected]>
@ekcasey ekcasey merged commit c9f1056 into buildpacks:master Nov 26, 2019
@jromero jromero modified the milestones: 0.6.0, lifecycle-0.6.0 Jan 9, 2020
@zmackie zmackie added breaking change Known to be non-backwards compatible accepted labels Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Known to be non-backwards compatible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants