Allow platform operators to configure init #332
Conversation
|
Maintainers, As you review this RFC please queue up issues to be created using the following commands: Issues(none) |
Propose that platform operators can specify all images are generated with an init system Signed-off-by: adelaney21 <adelaney21@bloomberg.net>
AidanDelaney
force-pushed
the
feature/add-builder-init
branch
from
622458c to
79795a2
Compare
|
|
||
| `docker run --rm -it --entrypoint /bin/tini py -s -- /cnb/process/web` | ||
|
|
||
| We propose that the enablement of a subreaper is configured at the builder level. This allows platform operators to control whether or not they want to use a subreaper on their platform. |
There was a problem hiding this comment.
I like that the builder can provide an init process. I like that it can be any init process. Both nice features.
I don't like that whether this feature is enabled or not is at the builder/platform level. I don't think the builder/platform has a good perspective to know if an init process is actually required. There are some apps that will correctly run as PID1, so in this case the init process isn't needed. The platform is not introspecting each app so it would have no clue. It would just add the init process regardless. Not the end of the world, but one more thing running in the container.
If a platform operator did want to allow users to opt out of always having an init process, then they need to maintain two different builders. One with and one without. Then the user would need to pick which builder to use for which app, and that's kind of a pain/error prone.
I feel like there should be some way for this behavior to be overridden by a.) the buildpacks and b.) a user.
- Buildpacks do introspect the apps, so they will have a better idea on if the app is or isn't save.
- Ultimately, the dev may be the only one that knows if it is or isn't save, so they should have some way to adjust this behavior.
So my $0.02 would be that the builder can provide a init process and sets the default. Then buildpacks and users are able to override (in that order of precedence). It might be handy to add a label or annotation or something the operator can use to look at images and see who's overriding this. That way they can easily audit who's not using their default choice.
If that's too complicated, then my vote would be for builder default w/user override. I think that's going to give the most flexibility and accuracy.
There was a problem hiding this comment.
I think I understand your perspective. This RFC suggests a way to provide a default for a platform, and you would like to see a --init=enable or --init=disable in pack?
From my perspective, I have a strong use-case for the builder to provide a default. I think we can deal with user overrides of the default in a subsequent RFC. This allows us to control the scope of this work.
I don't yet understand how the decision of init/no-init can be made at a buildpack level, as we could have a situation where Buildpack A wants init and Buildpack B does not want it. I agree that the end-user programmer is well-placed to enable or disable init. However, our motivating examples use application written by a target end-user programmer who does not quite undersand process reaping or signal handling. So having a builder default does make sense.
Do we agree that this RFC provides enough functionality to implement pack --init={enable|disable} in the future?
There was a problem hiding this comment.
I think I understand your perspective. This RFC suggests a way to provide a default for a platform, and you would like to see a --init=enable or --init=disable in pack?
👍
It wasn't clear to me when I first read this that the builder would set a default. It sounded like the builder would set a value and that was it. I can think of reasons why an operator would want to enforce this and not allow an override, so I wasn't certain the exact intent.
I think if you clarify that the builder supplies the default, and that out of the box we're not intending to provide any way to override that default (i.e. MVP) but that in the future we may provide ways to override, then that would be good.
I don't yet understand how the decision of init/no-init can be made at a buildpack level, as we could have a situation where Buildpack A wants init and Buildpack B does not want it.
Yes, that's a little fuzzier in my head. Probably would be a situation where it would be tied to the process type, so like with process types, the last buildpack to update it would win. i.e. if buildpack A runs first and says it should have an init proc, then buildpack B runs and says the same process type should not have an init proc, it would not have an init proc.
I'm totally fine not doing this for now, since it is undefined and would increase the scope a lot.
Do we agree that this RFC provides enough functionality to implement pack --init={enable|disable} in the future?
I'd really like pack to include support for overriding this out of the box. It seems incomplete to have a default and offer no way to override said default, but I totally get where you're coming from too. That's not something you particularly need.
Like I said above, I'd be 👍 so long as the RFC is written to make it clear the builder value is a default and that we've given thought to allowing a pack override in the future and that it would be possible without breaking changes to what we're defining here, because I think there's a very strong chance someone will come along later and ask for this.
| init = /bin/tini | ||
| ``` | ||
|
|
||
| Where `direct = false`, or is unspecified, and an init binary is configured, then the init binary is used to launch the shell process that eventually forks the application process. |
There was a problem hiding this comment.
Do we still need to deal with direct = false? I thought this was removed, so there is only direct = true now.
There was a problem hiding this comment.
The distinction between launch with shell and launch directly is still in launcher.go. Here I'm trying to account for all the current options. I'll double check the status of direct=true.
There was a problem hiding this comment.
I was specifically thinking of https://github.com/buildpacks/rfcs/blob/main/text/0093-remove-shell-processes.md
| ``` | ||
| [[processes]] | ||
| type = "web" | ||
| init = /bin/tini |
There was a problem hiding this comment.
What's your thinking on putting init = with every entry vs an [init] section in launch.toml and allowing launcher to codify the direct behavior?
There was a problem hiding this comment.
The proposed [init] section is in builder.toml. Do I need to make this more clear? Every process in launch.toml can include init = binary_name or omit the key value pair to default to the current behavior.
There was a problem hiding this comment.
ah ok, I missed the each process could change the behavior part.
4 participants
Propose that platform operators can specify all images are generated with an init system
Readable