Restorer changes for run image extension #1014

natalieparellano · 2023-02-17T16:13:56Z

Fixes #997

- The logic to update the default path for TOML files was repeated across phases - In general it is safe to provide default values for inputs that might not be relevant to the current phase, as these will be ignored when constructing a new service for the phase; e.g., platform.LifecycleInputs.OrderPath will be ignored when constructing a lifecycle.Exporter - As more inputs are shared across phases (e.g., analyzed.toml is now an input to the detect phase), duplicating the logic for providing default values is becoming more cumbersome Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

…= 0.10 Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

…kage Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

… the platform spec Signed-off-by: Natalie Arellano <narellano@vmware.com>

…mage.extend = <true or false, default false> Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

… should fail if the selected base image is not found in run.toml. Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-02-17T16:14:58Z

Still needs to add target data; blocking on #994

- digest ref for run image - target data for run image Additionally the restorer will download the run image manifest & config when extend is true Signed-off-by: Natalie Arellano <narellano@vmware.com>

acceptance/extender_test.go

Signed-off-by: Natalie Arellano <narellano@vmware.com>

imgutil/layout/sparse modifies the image media types which we don't want Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-02-28T23:41:38Z

This will be easier to review as a diff on top of runext/detect-996 ...I'm working on getting that one up-to-date with main, then I'll repoint. For now, the changes shown include commits from both feature branches.

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-03-01T17:46:29Z

Still need to bring this up-to-date with main (hence the huge diff)

Signed-off-by: Natalie Arellano <narellano@vmware.com>

cmd/lifecycle/restorer.go

natalieparellano · 2023-03-08T22:29:07Z

cmd/lifecycle/restorer.go

 	}
-	buildImageHash, err := remoteImage.Digest()
+	// get remote image
+	remoteImage, digest, err := newRemoteImage(imageRef, r.keychain)


If exporting to layout format we could look for the run image in the layout directory, but since the pack implementation currently requires the builder to exist in a registry (even when exporting to a daemon) we could keep this requirement for the run image and drop it whenever buildpacks/pack#1623 is completed.

cmd/lifecycle/restorer.go

joe-kimmel-vmw · 2023-03-09T02:51:11Z

platform/run_image.go

+)
+
+type TargetMetadata struct {
+	ID           string          `toml:"id"`


heh -- i also just added this ID Field. but i think here you got rid of that "partial" struct i was playing with... pros and cons, but now it's decoupled from what happens in the buildpack side, which imo was the only value of that partial (to preserve & codify the coupling). I thought it was correct to keep them coupled (and add the ID field to both structs) but if we're not doing that then it's probably correct to delete the TargetPartial struct entirely from bp_descriptor.go.

I removed it just because it's simpler, but if we find we need it, we can put it back.

joe-kimmel-vmw · 2023-03-09T02:52:54Z

platform/run_image.go

+	Distribution *OSDistribution `toml:"distribution"`
+}
+
+type OSDistribution struct {


here too -- what's your guidance on de/coupling with the bp_descriptor.go file? My gut still says that they're reading & writing the same things, so it's better to have them using the same structs. But I also think it's somewhere between possible and likely that I'm missing something..

In this case the platform spec and the buildpack spec happen to match each other (and happily so), but there isn't any technical reason that they are tied together. By contrast where for example platform.BuildMetadata depends on buildpack.Label that schema is directly coming from the buildpack spec and how buildpacks define labels. At least, that is my rationalization.

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-03-09T22:35:43Z

Let's block this on #1030 as the changes from that PR will be useful here

We'll be able to fully remove internal/selective when we update tests for the extender as part of #998 Signed-off-by: Natalie Arellano <narellano@vmware.com>

Ensure we write a digest reference to analyzed.toml Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-03-27T15:47:15Z

cmd/lifecycle/exporter.go

@@ -64,12 +64,12 @@ func (e *exportCmd) DefineFlags() {
 	cli.FlagProcessType(&e.DefaultProcessType)
 	cli.FlagProjectMetadataPath(&e.ProjectMetadataPath)
 	cli.FlagReportPath(&e.ReportPath)
-	cli.FlagRunImage(&e.RunImageRef)
+	cli.FlagRunImage(&e.RunImageRef) // Note: this flag isn't valid on Platform 0.7 and later, and any provided value is ignored


Suggested change

cli.FlagRunImage(&e.RunImageRef) // Note: this flag isn't valid on Platform 0.7 and later, and any provided value is ignored

cli.FlagRunImage(&e.RunImageRef) // Note: this flag isn't valid on Platform 0.7 and later

(randomly found when reviewing the code) Actually, I don't think we even ignore the provided value :/ we should probably change this, especially when we formally deprecate platforms older than 0.7

wait what does that mean, that it's not valid but "we don't even ignore the provided value" -- what do we do with it?

This flag was supposed to have been removed from the exporter when we released Platform API 0.7. But, it looks like we forgot :( When I added the above comment I thought we ignored the provided value but we don't, we use it to construct the exported image. We could just remove it and "fix" the lifecycle but I worry that platforms may be relying on it even if it's not in the spec.

natalieparellano · 2023-03-27T15:47:28Z

cmd/lifecycle/exporter.go

 	cli.FlagStackPath(&e.StackPath)
 	cli.FlagUID(&e.UID)
 	cli.FlagUseDaemon(&e.UseDaemon)

-	cli.DeprecatedFlagRunImage(&e.DeprecatedRunImageRef)
+	cli.DeprecatedFlagRunImage(&e.DeprecatedRunImageRef) // Note: this flag isn't valid on Platform 0.7 and later, and any provided value is ignored


Suggested change

cli.DeprecatedFlagRunImage(&e.DeprecatedRunImageRef) // Note: this flag isn't valid on Platform 0.7 and later, and any provided value is ignored

cli.DeprecatedFlagRunImage(&e.DeprecatedRunImageRef) // Note: this flag isn't valid on Platform 0.7 and later

natalieparellano · 2023-03-27T15:48:12Z

platform/files.go

@@ -89,40 +102,6 @@ func (t *TargetMetadata) IsSatisfiedBy(o *buildpack.TargetMetadata) bool {
 	return true
 }

-func GetTargetFromImage(image imgutil.Image) (*TargetMetadata, error) {


Moved to run_image.go

natalieparellano · 2023-03-27T15:48:46Z

@jabrown85 @joe-kimmel-vmw I think this is ready for one more look

joe-kimmel-vmw

LGTM - left a couple small questions but no blockers, mostly just my own confusion :)

joe-kimmel-vmw · 2023-03-27T17:30:02Z

acceptance/exporter_test.go

-	// Values from image acceptance/testdata/exporter/container/layout-repo in OCI layout format
-	analyzedMD.RunImage = &platform.RunImage{
-		Reference: "/layout-repo/index.docker.io/library/busybox/latest@sha256:445c45cc89fdeb64b915b77f042e74ab580559b8d0d5ef6950be1c0265834c33",
+	regPlaceholders := []string{


what do you think about leaving this comment as a //comment in the code?

joe-kimmel-vmw · 2023-03-27T17:35:09Z

acceptance/restorer_test.go

@@ -80,7 +82,7 @@ func testRestorerFunc(platformAPI string) func(t *testing.T, when spec.G, it spe
 			})
 		})

-		when("called with -analyzed", func() {
+		when("called with -analyzed (on older platforms)", func() {


A+ update to test descriptions

joe-kimmel-vmw · 2023-03-27T18:29:35Z

cmd/lifecycle/exporter.go

@@ -64,12 +64,12 @@ func (e *exportCmd) DefineFlags() {
 	cli.FlagProcessType(&e.DefaultProcessType)
 	cli.FlagProjectMetadataPath(&e.ProjectMetadataPath)
 	cli.FlagReportPath(&e.ReportPath)
-	cli.FlagRunImage(&e.RunImageRef)
+	cli.FlagRunImage(&e.RunImageRef) // Note: this flag isn't valid on Platform 0.7 and later, and any provided value is ignored


wait what does that mean, that it's not valid but "we don't even ignore the provided value" -- what do we do with it?

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano added 12 commits February 14, 2023 13:03

Read values from environment

433bf3c

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Buildpack API: run.Dockerfiles are allowed instructions on versions >…

f40616e

…= 0.10 Signed-off-by: Natalie Arellano <narellano@vmware.com>

Platform API: the detector accepts a new -run flag

29a94a6

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Move responsibility for validating Dockerfiles into the buildpack pac…

569f985

…kage Signed-off-by: Natalie Arellano <narellano@vmware.com>

When verifying Dockerfiles, return the new base image name if necessary

c72a10e

Signed-off-by: Natalie Arellano <narellano@vmware.com>

When determining the new runtime base image, use criteria outlined in…

4c5a564

… the platform spec Signed-off-by: Natalie Arellano <narellano@vmware.com>

Platform API: the schema of analyzed.toml is updated to include run-i…

01ab4b6

…mage.extend = <true or false, default false> Signed-off-by: Natalie Arellano <narellano@vmware.com>

TESTME: Update analyzed.toml with new run image if needed

9ba40e3

Signed-off-by: Natalie Arellano <narellano@vmware.com>

If extensions are used to switch the runtime base image, the detector…

eea3182

… should fail if the selected base image is not found in run.toml. Signed-off-by: Natalie Arellano <narellano@vmware.com>

Add fixture to test re-writing of analyzed.toml

647a3db

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Move updating analyzed.toml into lifecycle package for easier testing

9157796

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano force-pushed the runext/restore-997 branch from fa89581 to 53dbf59 Compare February 17, 2023 16:18

Platform API: the restorer will update analyzed.toml with:

62411e8

- digest ref for run image - target data for run image Additionally the restorer will download the run image manifest & config when extend is true Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano force-pushed the runext/restore-997 branch from 53dbf59 to 62411e8 Compare February 17, 2023 17:18

natalieparellano commented Feb 17, 2023

View reviewed changes

acceptance/extender_test.go Outdated Show resolved Hide resolved

natalieparellano added 4 commits February 17, 2023 14:12

Update acceptance/extender_test.go

362e755

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Merge branch 'main' into runext/restore-997

15c4c06

Fix merge and restore selective package

8917ca0

imgutil/layout/sparse modifies the image media types which we don't want Signed-off-by: Natalie Arellano <narellano@vmware.com>

Update analyzed.toml with digest reference or target data if needed

ea13f10

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano marked this pull request as ready for review February 28, 2023 23:37

natalieparellano requested a review from a team as a code owner February 28, 2023 23:37

natalieparellano changed the base branch from main to runext/detect-996 February 28, 2023 23:38

natalieparellano changed the base branch from runext/detect-996 to main February 28, 2023 23:39

Merge branch 'main' into runext/detect-996

d0ce558

natalieparellano changed the base branch from main to runext/detect-996 March 1, 2023 17:37

Fix acceptance

9ba92d1

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano added 2 commits March 8, 2023 15:48

Fix acceptance

5cce13c

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Fix acceptance

c630e7d

Signed-off-by: Natalie Arellano <narellano@vmware.com>

jabrown85 approved these changes Mar 8, 2023

View reviewed changes

natalieparellano commented Mar 8, 2023

View reviewed changes

cmd/lifecycle/restorer.go Outdated Show resolved Hide resolved

natalieparellano commented Mar 8, 2023

View reviewed changes

cmd/lifecycle/restorer.go Outdated Show resolved Hide resolved

joe-kimmel-vmw reviewed Mar 9, 2023

View reviewed changes

cmd/lifecycle/restorer.go Outdated Show resolved Hide resolved

joe-kimmel-vmw reviewed Mar 9, 2023

View reviewed changes

natalieparellano added 5 commits March 9, 2023 15:03

Test that we don't update target data for older platforms

0f2305a

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Remove target partial and use helper function

5adcca5

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Fix acceptance

2c17e05

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Bump imgutil

4054ebd

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Fix acceptance

95dbc52

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano added 4 commits March 24, 2023 11:09

Merge branch 'main' into runext/restore-997

cc1a045

Use imgutil/sparse package instead of internal/selective package

c8398c3

We'll be able to fully remove internal/selective when we update tests for the extender as part of #998 Signed-off-by: Natalie Arellano <narellano@vmware.com>

Don't try to pull a builder image if it wasn't specified

ac6ad80

Ensure we write a digest reference to analyzed.toml Signed-off-by: Natalie Arellano <narellano@vmware.com>

Fix lint

da9e2ff

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano commented Mar 27, 2023

View reviewed changes

natalieparellano requested review from joe-kimmel-vmw and jabrown85 and removed request for joe-kimmel-vmw March 27, 2023 15:48

joe-kimmel-vmw approved these changes Mar 27, 2023

View reviewed changes

Add and update comment

f2a068e

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano merged commit 47594c2 into main Mar 28, 2023

natalieparellano deleted the runext/restore-997 branch March 28, 2023 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restorer changes for run image extension #1014

Restorer changes for run image extension #1014

natalieparellano commented Feb 17, 2023 •

edited

Loading

natalieparellano commented Feb 17, 2023

natalieparellano commented Feb 28, 2023 •

edited

Loading

natalieparellano commented Mar 1, 2023

natalieparellano Mar 8, 2023 •

edited

Loading

joe-kimmel-vmw Mar 9, 2023

natalieparellano Mar 9, 2023

joe-kimmel-vmw Mar 9, 2023

natalieparellano Mar 9, 2023

natalieparellano commented Mar 9, 2023

natalieparellano Mar 27, 2023

joe-kimmel-vmw Mar 27, 2023

natalieparellano Mar 27, 2023

natalieparellano Mar 27, 2023

natalieparellano Mar 27, 2023

natalieparellano commented Mar 27, 2023

joe-kimmel-vmw left a comment

joe-kimmel-vmw Mar 27, 2023

joe-kimmel-vmw Mar 27, 2023

joe-kimmel-vmw Mar 27, 2023

	cli.FlagRunImage(&e.RunImageRef) // Note: this flag isn't valid on Platform 0.7 and later, and any provided value is ignored
	cli.FlagRunImage(&e.RunImageRef) // Note: this flag isn't valid on Platform 0.7 and later

	cli.DeprecatedFlagRunImage(&e.DeprecatedRunImageRef) // Note: this flag isn't valid on Platform 0.7 and later, and any provided value is ignored
	cli.DeprecatedFlagRunImage(&e.DeprecatedRunImageRef) // Note: this flag isn't valid on Platform 0.7 and later

Restorer changes for run image extension #1014

Restorer changes for run image extension #1014

Conversation

natalieparellano commented Feb 17, 2023 • edited Loading

natalieparellano commented Feb 17, 2023

natalieparellano commented Feb 28, 2023 • edited Loading

natalieparellano commented Mar 1, 2023

natalieparellano Mar 8, 2023 • 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

natalieparellano commented Mar 9, 2023

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

natalieparellano commented Mar 27, 2023

joe-kimmel-vmw left a 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

natalieparellano commented Feb 17, 2023 •

edited

Loading

natalieparellano commented Feb 28, 2023 •

edited

Loading

natalieparellano Mar 8, 2023 •

edited

Loading