RFC to create a lifecycle crd #1483

tomkennedy513 · 2024-01-16T20:54:40Z

codecov-commenter · 2024-01-16T20:57:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e1722c0) 67.17% compared to head (1cb07da) 67.17%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1483   +/-   ##
=======================================
  Coverage   67.17%   67.17%           
=======================================
  Files         134      134           
  Lines        8276     8276           
=======================================
  Hits         5559     5559           
  Misses       2264     2264           
  Partials      453      453

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

natalieparellano · 2024-01-17T16:06:16Z

rfcs/0000-lifecycle-crd.md

+patched upstream, requires any user to build the kpack specific lifecycle image themselves if the kpack provided one
+has not been bumped.
+
+As we [move to deprecate windows](https://github.com/buildpacks-community/kpack/discussions/1366), there is an opportunity for us to utilize


We do ship "lifecycle images" for Windows, so in theory you could specify the Windows image if Windows support is desired.

if we kept windows support a user would need to create a ClusterLifecycle for windows and reference that one in their builder. We should have os status on the new lifecycle crd and validation when generating a builder. I can add something about that in the event that windows doesn't get deprecated before we do this

Do we want to support multi-arch lifecycle images? I.e. if I have an Image Index with linux/amd64, linux/arm64, and windows/amd64 images, do I need to break them out into 3 separate ClusterLifecycles?

That is a good question. I suspect the simpler approach would be to keep them separate, but I could be wrong. Would appreciate other more knowledgeable folks weighing in here.

natalieparellano · 2024-01-17T16:32:57Z

rfcs/0000-lifecycle-crd.md

+One open question is whether we should follow the Clusterstack model and keep this resource strictly cluster scoped or
+follow the ClusterBuildpack pattern and provide a namespaced scoped version of the CRD. 


What are the pros and cons here? I could imagine, con of providing additional namespaced scoped resource is increased complexity. Anything else here?

I'd vote to do whatever is simplest and add more stuff later if it is ever needed or requested.

the pro would be that a user that wants to use a particular version of the lifecycle that doesn't have cluster access can create a namespaced builder with a namespaced lifecycle. They would still run into the fact that stacks are cluster scoped only. I am in favor of only creating a cluster level resource but I feel pretty strongly that we should call it ClusterLifecycle to keep the door open for a namespaced one and to follow the naming convention of our other resources

natalieparellano · 2024-01-17T16:35:14Z

rfcs/0000-lifecycle-crd.md

+The existing lifecycle reconciler can be updated to reconcile this new CRD instead of ConfigMaps.
+
+A new field will be added to the ClusterBuilder/Builder resources allowing the user to provide a reference to a lifecycle CR.
+This field will be optional and will default to the highest semantic version that is supported by the buildpacks in the builder and the platform. 


Is that basically like how we hard-code the lifecycle version here?

kpack/hack/lifecycle/main.go

Line 35 in e1722c0

lifecycleVersion = "0.17.2"

Would the future world deprecate that part of the code entirely? Like, if no ClusterLifecycle is specified do you get a default ClusterLifecycle or do you fallback to pulling the layer out of the lifecycleImage?

ya this would get deprecated and we would pull in an upstream lifecycle in the ci pipeline instead of building our own like this file is doing.

we would still ship a default ClusterLifecycle to ease burden on our users and avoid a breaking change on upgrade

Signed-off-by: Tom Kennedy <[email protected]>

sambhav · 2024-04-18T19:52:49Z

A few unresolved qs from my side -

What happens when there are multiple lifecycle crds? Can we specify a default? Is this a cluster only resource? What happens when the lifecycle gets updated? Will we do a rebuild/rebase?

natalieparellano · 2024-05-06T17:16:41Z

What happens when there are multiple lifecycle crds? Can we specify a default?

I think @tomkennedy513 was suggesting we use a webhook for that

Is this a cluster only resource?

Yes (this is detailed here)

What happens when the lifecycle gets updated? Will we do a rebuild/rebase?

Good question. For rebuild, I think yes? This is philosophically the same as updating your builder which already triggers a rebuild. For rebase, I think no? There would be no benefit if the run image didn't change. @tomkennedy513 @chenbh could you confirm my understanding?

For rebase, I think no?

BUT we may want to explore (on the CNB-spec side) "launcher layer rebase" to update the go version in the launcher which (when outdated) can cause pain.

natalieparellano · 2024-05-06T17:17:09Z

@tomkennedy513 @chenbh I'd like to work on this. Is there an issue for it? Could we create one (pending the approval of this RFC)?

tomkennedy513 · 2024-05-06T18:12:46Z

What happens when there are multiple lifecycle crds? Can we specify a default?

I think @tomkennedy513 was suggesting we use a webhook for that

I was thinking we may go down the same path we did with clusterstack where the kp cli will use a lifecycle crd called default (and we will ship with one called default). Alternatively we were thinking that the kpack webhook could do the exact same thing where it looks for one called default.

Is this a cluster only resource?

Yes (this is detailed here)

What happens when the lifecycle gets updated? Will we do a rebuild/rebase?

Good question. For rebuild, I think yes? This is philosophically the same as updating your builder which already triggers a rebuild. For rebase, I think no? There would be no benefit if the run image didn't change. @tomkennedy513 @chenbh could you confirm my understanding?

For rebase, I think no?

BUT we may want to explore (on the CNB-spec side) "launcher layer rebase" to update the go version in the launcher which (when outdated) can cause pain.

It would cause a rebuild because the builder would be rewritten. You can mimic this today by updating the lifecycle in the configmap

tomkennedy513 · 2024-05-06T18:22:46Z

@tomkennedy513 @chenbh I'd like to work on this. Is there an issue for it? Could we create one (pending the approval of this RFC)?

I dont think we have one, are you ok making one

natalieparellano · 2024-05-06T20:30:53Z

Made an issue to track the work on this: #1609

natalieparellano · 2024-05-08T16:40:15Z

rfcs/0000-lifecycle-crd.md

+The existing lifecycle reconciler can be updated to reconcile this new CRD instead of ConfigMaps.
+
+A new field will be added to the ClusterBuilder/Builder resources allowing the user to provide a reference to a lifecycle CR.
+This field will be optional and will default to the highest semantic version that is supported by the buildpacks in the builder and the platform. 


Suggested change

This field will be optional and will default to the highest semantic version that is supported by the buildpacks in the builder and the platform.

This field will be optional and will default to the highest semantic version that is supported by the platform.

Buildpacks are configured by the operator, so I don't think we can know which Buildpack APIs will be required... right? In theory (assuming we think most buildpacks in the wild have upgraded to at least Buildpack API 0.7 - since lifecycle 0.18.x and higher drops support for earlier APIs) it should be the highest semantic version that supports the Platform APIs here...right? In practice, that is going to mean the latest available lifecycle (until we deprecate more Platform APIs).

we do validate that any buildpack referenced in the order is using buildpack apis supported by the lifecycle when constructing a builder

kpack/pkg/cnb/builder_builder.go

Lines 188 to 204 in ae5eb15

func (bb *builderBlder) validateBuilder(sortedBuildpacks []DescriptiveBuildpackInfo) error {

platformApis := append(bb.LifecycleMetadata.APIs.Platform.Deprecated, bb.LifecycleMetadata.APIs.Platform.Supported...)

err := validatePlatformApis(platformApis)

if err != nil {

return err

}

buildpackApis := append(bb.LifecycleMetadata.APIs.Buildpack.Deprecated, bb.LifecycleMetadata.APIs.Buildpack.Supported...)

for _, bpInfo := range sortedBuildpacks {

bpLayerInfo := bb.buildpackLayers[bpInfo].BuildpackLayerInfo

err := bpLayerInfo.supports(buildpackApis, bb.stackId, bb.mixins, relaxedMixinContract(platformApis))

if err != nil {

return errors.Wrapf(err, "validating buildpack %s", bpInfo)

}

}

return nil

}

Ah, I see - but that validation happens after the ClusterLifecycle is defined, no? So in the case that the provided lifecycle doesn't support a buildpack in the builder we just fail, right? (Instead of selecting a different lifecycle?)

I think the only check we'd want when the cluster lifecycle is reconciled is the platform api compatibility. The buildpack api compatibility will continue to happen at builder creation

natalieparellano · 2024-06-18T15:23:05Z

FYI the PR implementing the changes proposed here is ready for feedback: #1628

So, we should probably circle back to this RFC to see if it is acceptable as-is or if it needs changes.

cc @tomkennedy513 @chenbh @samj1912

natalieparellano reviewed Jan 17, 2024

View reviewed changes

natalieparellano approved these changes Jan 17, 2024

View reviewed changes

RFC to create a lifecycle crd

a4da42a

Signed-off-by: Tom Kennedy <[email protected]>

tomkennedy513 force-pushed the lifecycle-crd branch from 1cb07da to a4da42a Compare April 18, 2024 17:35

tomkennedy513 marked this pull request as ready for review April 18, 2024 17:35

tomkennedy513 requested a review from a team as a code owner April 18, 2024 17:35

natalieparellano mentioned this pull request May 6, 2024

Implement lifecycle CRD #1609

Open

natalieparellano reviewed May 8, 2024

View reviewed changes

natalieparellano mentioned this pull request May 29, 2024

Implement lifecycle CRD #1628

Open

natalieparellano mentioned this pull request Jun 18, 2024

Update kp cli to support lifecycle CRD #1656

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC to create a lifecycle crd #1483

RFC to create a lifecycle crd #1483

tomkennedy513 commented Jan 16, 2024

codecov-commenter commented Jan 16, 2024

natalieparellano Jan 17, 2024

tomkennedy513 Jan 17, 2024

chenbh Apr 19, 2024

natalieparellano May 6, 2024

natalieparellano Jan 17, 2024 •

edited

Loading

tomkennedy513 Jan 17, 2024

natalieparellano Jan 17, 2024

tomkennedy513 Jan 17, 2024

sambhav commented Apr 18, 2024 •

edited

Loading

natalieparellano commented May 6, 2024

natalieparellano commented May 6, 2024

tomkennedy513 commented May 6, 2024

tomkennedy513 commented May 6, 2024

natalieparellano commented May 6, 2024

natalieparellano May 8, 2024

tomkennedy513 May 8, 2024

natalieparellano May 8, 2024

tomkennedy513 May 8, 2024

natalieparellano commented Jun 18, 2024

		One open question is whether we should follow the Clusterstack model and keep this resource strictly cluster scoped or
		follow the ClusterBuildpack pattern and provide a namespaced scoped version of the CRD.

	This field will be optional and will default to the highest semantic version that is supported by the buildpacks in the builder and the platform.
	This field will be optional and will default to the highest semantic version that is supported by the platform.

	func (bb *builderBlder) validateBuilder(sortedBuildpacks []DescriptiveBuildpackInfo) error {
	platformApis := append(bb.LifecycleMetadata.APIs.Platform.Deprecated, bb.LifecycleMetadata.APIs.Platform.Supported...)
	err := validatePlatformApis(platformApis)
	if err != nil {
	return err
	}
	buildpackApis := append(bb.LifecycleMetadata.APIs.Buildpack.Deprecated, bb.LifecycleMetadata.APIs.Buildpack.Supported...)
	for _, bpInfo := range sortedBuildpacks {

	bpLayerInfo := bb.buildpackLayers[bpInfo].BuildpackLayerInfo
	err := bpLayerInfo.supports(buildpackApis, bb.stackId, bb.mixins, relaxedMixinContract(platformApis))
	if err != nil {
	return errors.Wrapf(err, "validating buildpack %s", bpInfo)
	}
	}
	return nil
	}

RFC to create a lifecycle crd #1483

Are you sure you want to change the base?

RFC to create a lifecycle crd #1483

Conversation

tomkennedy513 commented Jan 16, 2024

codecov-commenter commented Jan 16, 2024

Codecov Report

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 Jan 17, 2024 • 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

sambhav commented Apr 18, 2024 • edited Loading

natalieparellano commented May 6, 2024

natalieparellano commented May 6, 2024

tomkennedy513 commented May 6, 2024

tomkennedy513 commented May 6, 2024

natalieparellano commented May 6, 2024

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 Jun 18, 2024

natalieparellano Jan 17, 2024 •

edited

Loading

sambhav commented Apr 18, 2024 •

edited

Loading