Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 root buildpack as interface for app image extension RFC #77
Add root buildpack as interface for app image extension RFC #77
Changes from 15 commits
b97ea49
abf9731
f11dec6
6c8b006
0f4b7de
77a9802
e1d8d91
95afa8d
ac23238
338e77f
f2dd707
4d1651f
046628b
d500e76
7a3a1a0
cb2be5a
f122212
8c38d14
fd89506
416a410
16e452f
ee2bed1
196eaaf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was correcting the spelling of presence (currently precense).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could a better name for these buildpacks be stack buildpacks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm fine with either. I think to most end-users they will just be "buildpacks", so it's probably more a question for implementors of those buildpacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually, we could support
silces
to split up kaniko's userspace snapshots (and improve performance).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually proposed that
[[slices]]
would work as normal a few lines after this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that root buildpack authors need to know what paths the packages the buildpack installs write to for layer exclusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they want to slim down the run image, then yes they do. Otherwise they can just ignore this file. This is a good thing to point out because this proposal is tilting toward giving the buildpack authors more responsibility. But with that, I think we keep a highly orthogonal interface (i.e. everything is a buildpack).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah just thinking allowed. I think that might work well for cases where a root buildpack is specific to a particular package - say, installing curl - but not so well if they support arbitrary packages based on files in the application repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By
do we mean that the snapshotted layer will be stored in the cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but as @matthewmcnew pointed out, we could choose to store it as an intermediate build image (no need for snapshot in that case I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After more thought, I'm unsure if we can get away from the snapshot layer entirely. We may need it to restrict what changes are preserved (for example, we do not want to preserve changes to the
/layers
or/workspace
dirs)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are allow root buildpacks to use slices, are we assuming that the app dir is excluded from the snapshot layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the app dir should be excluded from the snapshot layer. I'll call that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale behind this constraint? What is the concern it's addressing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addresses a concern that @sclevine had about root buildpacks proliferating through the ecosystem. Like, if we give this tool to people, they will use it too frequently. If we instead restrict its use, it forces buildpacks authors to explore alternative solutions.
I don't necessarily agree with that concern, but I'm willing to accept it in compromise (in part because it's easy to reverse).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
detect.buildpacks
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's not a table in the latest definition of project.toml. So I'm proposing that the root buildpacks be listed in the same list as non-root buildpacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify what the "ephemeral run image" is here? How does it relate to the result of extend phase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused about this also. Maybe
was supposed to be replaced by the "new extend phase" below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the wording a bit confusing also. Is this what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused about how we generate run image layers using the snapshotting mechanism. Wouldn't we need to run the root buildpacks on the run image itself to capture the correct snapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also how does a root buildpack opt into reusing a run-image layer? Typically, we use the presence of a
layer.toml
file combined with the absence of layer content to signal layer reuse, but that doesn't work if the contents are not being written to the layers dir.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the root buildpacks must run twice: once on the build image and once on the run image. I'm not sure about reusing the run-image layer. It might not be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should cache the previous run image layers locally if they can be reused. They need to be built locally anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would information about these layers (such as their digest) be stored? Is that the responsibility of the platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the lifecycle would manage these layers (the same way as if you are storing the cache in an image). But I do need to address how they are stored when the cache is a volume. I'll fix this up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the restorer re-apply the snapshotted build layer? Does that make it hard for the buildpack to remove and rebuild that layer if it is out of date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the buildpack cannot rebuild with the snapshot layer applied, then it is not idempotent and the yet-to-be-named key in the
buildpack.toml
must specify this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand the allure of a pack upgrade flow, I feel pretty strongly that this should just be accomplished by a complete rebuild. An "upgrade" that re-executes some but not all buildpacks seems arbitrary and confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
pack upgrade
flow is just supposed to run the root buildpacks, and only on the run image. This does bring up a serious issue though: the app isn't accessible during a rebase.@jkutner I think root buildpacks will need something like a
/bin/rebase
executable and/or some way of storing metadata about the app. Otherwise rebase would require the original source code.Alternatively, we could only give root buildpacks access to metadata in project.toml and not the app directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @matthewmcnew, if I'm understanding the sentiment correctly.
My comment:
I propose that we eliminate the notion of
upgrade
and still only have the concept of(re)build
andrebase
.If you envision the layers as below and then associate the two operations, it becomes easy to understand and eliminates the complexity being introduced by
upgrade
.As an additional (possible) upside, by not providing additional "upgradability" for these set of layers we steer buildpack authors/developers to push recurring dependencies to the stack layers to improve performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @jromero and @matthewmcnew . I think adding an
upgrade
would be a confusion for new consumers, who know they need to do something for their app, but aren't sure what. If there's an explicit need at a later point, great, but adding it preemptively to me sounds like the confusion risks outweigh the benefits.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't executing arbitrary "root buildpacks" before a rebase no longer allow rebase to be a safe operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it makes it possible that rebase is unsafe. paging @sclevine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this. This is probably clear by now, but the idea is that we would re-play the root buildpacks on the new run image before rebasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't this be a positive outcome?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be positive, but it's an open question because we aren't defining how it would work.