Add additional metadata to app images

[dmikusa]

RFC that proposes to add additional metadata to app container images: ports and health check information.

Readable

[buildpack-bot]

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

[natalieparellano]

Is there any concern that this could cause confusion when multiple process types are defined? Do we need to warn / alert the user somehow about this?

That said I think this should be fine to enable. Buildpack authors don't have to support it if they don't want to.

[dmikusa]

The ports/health check information is specified by the buildpack per process type, so if I have web and scheduler process types, you can define different ports/health check info for each one. Unfortunately, there's not parity for that in the container image metadata. There's only the capacity to to put metadata for one process type in the container's ports/health check info so we have to pick one of them.

I think the default process type set by buildpacks seems most reasonable and least likely to be surprising (i.e. best choice if we put anything in this container metadata) because if you're going to docker run the image then that's what is run. Given that, I think it stands to reason that the metadata in the container image would reference the thing that is run in this manner.

I'm not sure what opportunities we have to insert some warnings/helpful logging. Where I see this being potentially confusing is that when you run the non-default process type, (I assume, I haven't checked) Docker is still going to see this metadata for the default process type and attempt to use it as ports/health check info (unless the user overrides it). That might be confusing if you don't remember (or know) that the ports/health check metadata is on the container image. Especially the health check. The container would start and could potentially fail if the health check is different between the two process types.

I'm not totally sure what we can do about it, especially at this level. Maybe pack could have an opt-out flag for including this metadata (although buildpacks could do that too)? Buildpacks could also log at build time if this metadata is being set. Ideally, there would be some kind of reminder that it's set at runtime but I don't know that is possible and it would be noise a lot of the time, like you'd only want a warning when something other than the default process type is run & there is metadata set on the container image.

I suppose the other possibility would be that we don't write the metadata if there are multiple process types. Then it's not confusing because there's only one process type. I think this would negate a lot of the benefit though. I can only speak for Paketo Java buildpacks, but I know that would rule out most or all of our buildpacks. We might be able to change things to only include one process type, but right now they write the same command to multiple different process types for some historical reason that predates me.

[AidanDelaney]

Should we provide a healthcheck binary, similar to launcher? Something that executes the correct heathcheck for the running process but allows HEALTHCHECK to be the healthcheck for the running process?

[dmikusa]

I like that idea. Is that possible? How would it know the correct process type to run?

If you put /cnb/healthcheck into HEALTHCHECK, when the health check is invoked /cnb/healthcheck would need to figure out the process type. It could default to the default process type, but I'm not sure how it would know if a different process type was being used. Is that metadata available somewhere?

[dmikusa]

Should we provide a healthcheck binary, similar to launcher? Something that executes the correct heathcheck for the running process but allows HEALTHCHECK to be the healthcheck for the running process?

This would actually be really helpful, IF we can make this process skip invoking the exec.d binaries. It should source the environment, but not run exec.d binaries. This is because some exec.d binaries have side effects when they run, so it's not really safe to execute them every time we run the health check, which could be multiple times a minute.

We actually learned this the hard way with the Paketo Health Checker buildpack :(

Having said that, it also seems like a tremendous amount of additional work, although please correct me if that's an invalid assumption, and I wonder if we couldn't just do something in the launcher that would allow us to launch the health check binary like it normally does, but skip the exec.d binaries.

If you put /cnb/healthcheck into HEALTHCHECK, when the health check is invoked /cnb/healthcheck would need to figure out the process type. It could default to the default process type, but I'm not sure how it would know if a different process type was being used. Is that metadata available somewhere?

I was thinking about this more and it seems like we'd know this if it were implemented in the launcher, since the launcher knows what process to run. So maybe that's not really a big deal.

@natalieparellano what do you think about the above two points?

[dmikusa]

Curious if there are any more thoughts/feedback on this one.

I'd also be interested in helping with the implementation of this. Not my normal area, but it seems like a good one to branch out if I can get some help/pointers to get started.

[samj1912]

I think the general proposal and implementation makes sense to me. I wonder if ports and healthchecks should be scopes inside processes table though.

[dmikusa]

Sorry, not sure I follow you @samj1912. Do you have an example of what you're thinking?

[tomkennedy513]

I really like this!