Move analyzer before detector #197

jabrown85 · 2021-02-24T21:12:10Z

First take at this rfc

platform.md

ekcasey · 2021-02-24T23:35:12Z

platform.md

-  [-cache-image <cache-image>]
+  [-cache-dir <cache-dir>] \
+  [-cache-image <cache-image>] \
+  [-daemon] \ # sets <daemon> 


I think this gets confusing in conjunction with our registry only cache image. Maybe we can use the analysis metadata to record whether the previous image is a daemon or registry image?

Also, this might be a really good opportunity to make a long desired change and move from have a single reference key in analyzed.toml that stores either an imageID or digest reference, to having two explicit keys (image_id, registry_digest)

Apologies I thought I updated this comment with an edit. I realized existing analyzer already has this issue (if you want to call it an issue) and this suggestion should probably be out of scope for now. But I wanted to leave these thoughts somewhere, in case we want to incorporate them in future changes.

jabrown85 · 2021-03-03T19:14:52Z

@jabrown85 - Add missing items from RFC to support analyzer features not yet represented

First take at [this rfc](https://github.com/buildpacks/rfcs/blob/main/text/0075-move-analyze-phase.md) Signed-off-by: Jesse Brown <jabrown85@gmail.com>

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

platform.md

ekcasey · 2021-03-12T20:28:32Z

platform.md


- The lifecycle MUST write [analysis metadata](#analyzedtoml-toml) to `<analyzed>` if `<image>` is accessible.
 - **If** `<skip-layers>` is `true` the lifecycle MUST NOT perform layer analysis.


Suggested change

- **If** `<skip-layers>` is `true` the lifecycle MUST NOT perform layer analysis.

- For each buildpack in `<group>`, if persistent metadata for that buildpack exists in the analysis metadata, lifecycle MUST write a toml representation of the persistent metadata to `<layers>/<buildpack-id>/store.toml`

- **If** `<skip-layers>` is `true` the lifecycle MUST NOT perform layer analysis.

We also need to modify the data format for io.buildpacks.lifecycle.metadata[https://github.com/buildpacks/spec/blob/main/platform.md#iobuildpackslifecyclemetadata-json] to included persistent metadata in the format currently expected by the lifecycle.

What has changed here? Sorry, I don't see what needs to be modified.

Got all of the ones I could do from GitHub Signed-off-by: Jesse Brown <jabrown85@gmail.com> Co-authored-by: Emily Casey <emilykimballcasey@gmail.com>

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

natalieparellano · 2021-03-15T17:39:25Z

platform.md

+  [-analyzed <analyzed>] \
+  [-daemon] \ # sets <daemon>
+  [-gid <gid>] \
+  [-group <group>] \


From the lifecycle PR: it looks like group is only defined for earlier platform apis, is that intended?

Now that I'm remembering how all of this works... we can't have group if analyze goes first...

Suggested change

[-group <group>] \

We can have group, but not order

We can't have group right? There is no group until detection.

I think the future we will support stack-group in order to validate mixins, but not group?

I think I confused myself with the names. We need <order>, not <group>. Fixed in 6480d2e

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

Removed cache-dir/cache-image from analyzer Detail what image compatibility means for previous-image and run-image Updated stack.toml with build-image's stack and mixin information Updated analyzed.toml with run and build image mixin information Updated analzyed.toml with run image reference Signed-off-by: Jesse Brown <jabrown85@gmail.com>

jabrown85 · 2021-03-29T14:31:59Z

I updated the spec after discussions last week that were loosely documented here.

cc @ekcasey @jkutner

ekcasey · 2021-04-09T13:45:12Z

platform.md

+  [-analyzed <analyzed>] \
+  [-daemon] \ # sets <daemon>
+  [-gid <gid>] \
+  [-group <group>] \


We can't have group right? There is no group until detection.

platform.md

ekcasey · 2021-04-09T13:53:48Z

platform.md

+
+- **If** `<daemon>` is `false`, `<image>` MUST be a valid image reference
+- **If** `<daemon>` is `true`, `<image>` MUST be either a valid image reference or an imageID
+- **If** `<run-image>` is not provided by the platform the value will be resolved from the contents of stack


Suggested change

- **If** `<run-image>` is not provided by the platform the value will be resolved from the contents of stack

- **If** `<run-image>` is not provided by the platform the lifecycle MUST [resolve](#run-image-resolution) the run image from the contents of `stack` or fail if `stack` does not contain a valid run image.

Added in 6480d2e

ekcasey · 2021-04-09T14:01:06Z

platform.md

+| `<analyzed>`      | `CNB_ANALYZED_PATH`   | `<layers>/analyzed.toml` | Path to output analysis metadata (see [`analyzed.toml`](#analyzedtoml-toml)
+| `<daemon>`        | `CNB_USE_DAEMON`      | `false`                  | Analyze image from docker daemon
+| `<gid>`           | `CNB_GROUP_ID`        |                          | Primary GID of the stack `User`
+| `<group>`         | `CNB_GROUP_PATH`      | `<layers>/group.toml`    | Path to group definition (see [`group.toml`](#grouptoml-toml))


Suggested change

| `<group>` | `CNB_GROUP_PATH` | `<layers>/group.toml` | Path to group definition (see [`group.toml`](#grouptoml-toml))

Removed in 6480d2e

ekcasey · 2021-04-09T16:35:05Z

platform.md

+  - Stack IDs must match
+  - Platform/Architecture must be the same
+  - `<run-image>` `mixins` must be a superset of `<previous-image>` mixins
+- The lifecycle MUST ensure registry read/write access to `<run-image>`, `<previous-image>`, `<tag>`


Suggested change

- The lifecycle MUST ensure registry read/write access to `<run-image>`, `<previous-image>`, `<tag>`

- The lifecycle MUST ensure registry write access to `<image>` and any provided `<tag>`s.

We don't need write access to the previous image or the run image.

Added in 6480d2e

ekcasey · 2021-04-09T17:21:52Z

platform.md

+[build-image]
+  mixins = ["jq", "libgc", "libpq"]


Suggested change

[build-image]

mixins = ["jq", "libgc", "libpq"]

[build-image]

stack-id = "<string>"

mixins = ["jq", "libgc", "libpq"]

I want to add this meanly for symmetry with stack.toml. It could be useful eventually if we wanted to ensure that the detector and builder are in fact running on stack provided to analyzer.

Should we bubble this up so stack-id is shared between build and run? When I tried implementing this in pack, it was a little awkward because we don't usually scope a stack ID to only build or run.

@jkutner I could get behind that idea but I would insist that we make the change in stack.toml as well. We should still check that the run-image actually has the declared stack ID though.

I added stack-id in 6480d2e

Let's go with [build-image] stack-id, in this PR and I'll open a follow-up PR to change (then we can debate that separately from this PR).

platform.md

ekcasey · 2021-04-09T17:38:22Z

platform.md

-##### Layer analysis
-When analyzing a given layer the lifecycle SHALL:
+##### Layer Restoration
+When restoring a given layer the lifecycle SHALL:
 - **If** `build=true`, `cache=false`:
    - Do nothing
 - **Else if** `launch=true`:
    - Write layer metadata read from the analyzed image to `<layers>/<buildpack-id>/<layer-name>.toml`
    - Write the layer diffID from the analyzed image to `<layers>/<buildpack-id>/<layer-name>.sha`


Suggested change

- Write the layer diffID from the analyzed image to `<layers>/<buildpack-id>/<layer-name>.sha`

We don't need the .sha files anymore.

Removed in 6480d2e

ekcasey · 2021-04-09T17:42:38Z

platform.md

 - **If** `build=true`, `cache=false`:
    - Do nothing
 - **Else if** `launch=true`:
    - Write layer metadata read from the analyzed image to `<layers>/<buildpack-id>/<layer-name>.toml`
    - Write the layer diffID from the analyzed image to `<layers>/<buildpack-id>/<layer-name>.sha`
+- **Else if** `cache=true` AND the cache DOES NOT contain a layer with matching diffID
+    - Must not write layer metadata
 - **Else if** `cache=true`:
    - Write layer metadata read from the cache to `<layers>/<buildpack-id>/<layer-name>.toml`


Seems like we are missing a description of when to restore cached layer contents? In the interest of normalized independent API maybe we should just refer out to https://github.com/buildpacks/spec/blob/main/buildpack.md#layer-types for a description of what to restore when. Then, any future changes could be a buildpack API concern. We merely need to say that the lifecycle must use the provided cache-dir or cache-image to retrieve layer contents.

I took a stab at removing all the detail here, but I might've removed too much. Let me know what you think. Fixed in 6480d2e

ekcasey · 2021-04-09T17:45:12Z

platform.md

+| [exit status]      | (see Exit Code table below for values)
+| `/dev/stdout`      | Logs (info)
+| `/dev/stderr`      | Logs (warnings, errors)
+| `<analyzed>`       | Analysis metadata (see [`analyzed.toml`](#analyzedtoml-toml)


Now that we are writing run-image metadata in this output. analyzed.toml needs to be an input to exporter and stack.toml can be removed as an exporter input.

<analyzed> is already an input, I removed <stack> in 6480d2e

ekcasey · 2021-04-09T17:48:35Z

platform.md

@@ -1003,6 +1018,9 @@ Where:
 [run-image]
 image = "<image>"
 mirrors = ["<mirror>", "<mirror>"]
+[build-image]
+ stack-id = "<string>"
+ mixins = ["jq", "libgc", "libpq"]


Suggested change

mixins = ["jq", "libgc", "libpq"]

mixins = ["<mixin name>"]

jq etc. are very example-y and we generally don't do that in the data formats. See https://github.com/buildpacks/spec/blob/main/buildpack.md#buildpacktoml-toml

Removed in 6480d2e

platform.md

natalieparellano · 2021-04-14T21:33:30Z

platform.md

-| `<layers>/<buidpack-id>/<layer>.toml` ||                       | Files containing the layer content metadata of each analyzed layer (see data format in [Buildpack Interface Specification](buildpack.md))
+- For each buildpack in `<group>`, if persistent metadata for that buildpack exists in the analysis metadata, lifecycle MUST write a toml representation of the persistent metadata to `<layers>/<buildpack-id>/store.toml`
+- **If** `<skip-layers>` is `true` the lifecycle MUST NOT perform layer restoration.
+- **Else** the lifecycle MUST perform [layer restoration](#layer-restoration) for any app image layers or cached layers created by any buildpack present in the provided `<group>`.


Does this apply to app image layers? #layer-restoration only talks about the cache.

This is a re-do of #505, porting to the recently updated main branch. [RFC](buildpacks/rfcs#135) [Spec PR](buildpacks/spec#197) Signed-off-by: Jesse Brown <jabrown85@gmail.com>

platform.md

jkutner · 2021-04-26T15:56:01Z

@jabrown85 dco

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

Signed-off-by: Jesse Brown <jabrown85@gmail.com> Co-authored-by: Emily Casey <emilykimballcasey@gmail.com>

For mixing validation, detector will need the mixins written by analyzer and the buildpacks on the builder image. Signed-off-by: Jesse Brown <jabrown85@gmail.com>

platform.md

Signed-off-by: Jesse Brown <jabrown85@gmail.com> Co-authored-by: Emily Casey <emilykimballcasey@gmail.com>

jabrown85 · 2021-04-28T18:14:22Z

@sclevine can you take a look at this? We have the approvals but the change is large enough it wouldn't hurt to have more 👀 on it prior to merge.

Thanks!

Run `analyzer` before `detector` [Related RFC](buildpacks/rfcs#135) [Spec PR](buildpacks/spec#197) Signed-off-by: Jesse Brown <jabrown85@gmail.com> Co-authored-by: Natalie Arellano <narellano@vmware.com> Co-authored-by: Yael Harel <yharel@vmware.com>

See [here](buildpacks#197 (comment)). It was intended to remove `<image>` in favor of `-previous-image` and `-tag` but `<image>` got left in by accidently.

See [here](buildpacks#197 (comment)). It was intended to remove `<image>` in favor of `-previous-image` and `-tag` but `<image>` got left in by accidently. Signed-off-by: Jesse Brown <jabrown85@gmail.com>

jabrown85 mentioned this pull request Feb 24, 2021

Run analyzer before detector buildpacks/lifecycle#505

Closed

ekcasey reviewed Feb 24, 2021

View reviewed changes

jabrown85 force-pushed the patch-2 branch from 421821c to 55d2538 Compare February 25, 2021 01:19

hone marked this pull request as ready for review March 3, 2021 19:13

hone requested a review from a team as a code owner March 3, 2021 19:13

jabrown85 added 4 commits March 3, 2021 13:48

Move analyzer before detector

df6ac6f

First take at [this rfc](https://github.com/buildpacks/rfcs/blob/main/text/0075-move-analyze-phase.md) Signed-off-by: Jesse Brown <jabrown85@gmail.com>

PR Changes

326541a

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

added analyzed path to restorer

4cdddd7

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

Added more fields from the RFC

83178a2

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

jabrown85 force-pushed the patch-2 branch from de9d5a6 to 83178a2 Compare March 3, 2021 19:48

jabrown85 mentioned this pull request Mar 9, 2021

[RFC 0075] Move analyze phase before detect #194

Closed

jkutner approved these changes Mar 10, 2021

View reviewed changes

jkutner requested a review from a team March 10, 2021 15:31

hone approved these changes Mar 11, 2021

View reviewed changes

ekcasey added the api/platform label Mar 11, 2021

ekcasey linked an issue Mar 11, 2021 that may be closed by this pull request

[RFC 0075] Move analyze phase before detect #194

Closed

Removing fields to put into another spec PR

d0e9221

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

ekcasey requested changes Mar 12, 2021

View reviewed changes

jabrown85 and others added 2 commits March 12, 2021 16:20

Apply suggestions from code review

f1ab6a5

Got all of the ones I could do from GitHub Signed-off-by: Jesse Brown <jabrown85@gmail.com> Co-authored-by: Emily Casey <emilykimballcasey@gmail.com>

More PR changes

449ce31

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

natalieparellano reviewed Mar 15, 2021

View reviewed changes

nebhale requested a review from a team March 17, 2021 18:09

nebhale added this to the Platform 0.7 milestone Mar 17, 2021

adding back all the things missing from RFC for review

c8b108e

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

jkutner mentioned this pull request Mar 25, 2021

Set build-image metadata in stack.toml of builder buildpacks/pack#1118

Closed

2 tasks

jabrown85 requested review from jkutner and hone March 30, 2021 16:10

nebhale requested a review from a team April 7, 2021 18:10

nebhale approved these changes Apr 7, 2021

View reviewed changes

ekcasey changed the base branch from main to platform/0.7 April 9, 2021 13:42

ekcasey requested changes Apr 9, 2021

View reviewed changes

natalieparellano reviewed Apr 12, 2021

View reviewed changes

platform.md Show resolved Hide resolved

natalieparellano mentioned this pull request Apr 14, 2021

[RFC 0075] Move analyze phase before detect buildpacks/lifecycle#530

Closed

natalieparellano reviewed Apr 14, 2021

View reviewed changes

jabrown85 mentioned this pull request Apr 15, 2021

Run analyzer before detector buildpacks/lifecycle#582

Merged

natalieparellano reviewed Apr 16, 2021

View reviewed changes

platform.md Show resolved Hide resolved

This was referenced Apr 19, 2021

[Restore] Layer metadata restorer should not write layer.sha files for platform API >= 0.7 buildpacks/lifecycle#584

Closed

[Detect] Fail fast if buildpack.toml mixins are not satisfied by stack buildpacks/lifecycle#578

Closed

ekcasey reviewed Apr 21, 2021

View reviewed changes

platform.md Outdated Show resolved Hide resolved

platform.md Show resolved Hide resolved

platform.md Outdated Show resolved Hide resolved

ekcasey requested a review from sclevine April 21, 2021 18:11

jabrown85 and others added 3 commits April 26, 2021 11:04

Apply suggestions from code review

323cb33

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

Update platform.md

05aa33a

Signed-off-by: Jesse Brown <jabrown85@gmail.com> Co-authored-by: Emily Casey <emilykimballcasey@gmail.com>

Added analyzed to detector inputs

a3be6e5

For mixing validation, detector will need the mixins written by analyzer and the buildpacks on the builder image. Signed-off-by: Jesse Brown <jabrown85@gmail.com>

jabrown85 force-pushed the patch-2 branch from 349aa39 to a3be6e5 Compare April 26, 2021 16:05

ekcasey approved these changes Apr 28, 2021

View reviewed changes

platform.md Outdated Show resolved Hide resolved

Update platform.md

dd17824

Signed-off-by: Jesse Brown <jabrown85@gmail.com> Co-authored-by: Emily Casey <emilykimballcasey@gmail.com>

sclevine approved these changes Apr 28, 2021

View reviewed changes

Merge branch 'platform/0.7' into patch-2

5a9efbb

jkutner merged commit c82dd28 into buildpacks:platform/0.7 Apr 29, 2021

jabrown85 added a commit to jabrown85/spec that referenced this pull request May 13, 2021

Removing <image> from analyzer

35929b3

See [here](buildpacks#197 (comment)). It was intended to remove `<image>` in favor of `-previous-image` and `-tag` but `<image>` got left in by accidently.

jabrown85 mentioned this pull request May 13, 2021

Removing <image> from analyzer #225

Closed


		- The lifecycle MUST write [analysis metadata](#analyzedtoml-toml) to `<analyzed>` if `<image>` is accessible.
		- If `<skip-layers>` is `true` the lifecycle MUST NOT perform layer analysis.

	- If `<skip-layers>` is `true` the lifecycle MUST NOT perform layer analysis.
	- For each buildpack in `<group>`, if persistent metadata for that buildpack exists in the analysis metadata, lifecycle MUST write a toml representation of the persistent metadata to `<layers>/<buildpack-id>/store.toml`
	- If `<skip-layers>` is `true` the lifecycle MUST NOT perform layer analysis.

	- If `<run-image>` is not provided by the platform the value will be resolved from the contents of stack
	- If `<run-image>` is not provided by the platform the lifecycle MUST [resolve](#run-image-resolution) the run image from the contents of `stack` or fail if `stack` does not contain a valid run image.

	- The lifecycle MUST ensure registry read/write access to `<run-image>`, `<previous-image>`, `<tag>`
	- The lifecycle MUST ensure registry write access to `<image>` and any provided `<tag>`s.

Move analyzer before detector #197

Move analyzer before detector #197

Conversation

jabrown85 commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jabrown85 commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jabrown85 commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekcasey Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkutner commented Apr 26, 2021

jabrown85 commented Apr 28, 2021

ekcasey Apr 9, 2021 •

edited

Loading