Skip to content
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

Support for multi-platform artifacts when creating builders and buildpack package #295

Merged
merged 17 commits into from
Apr 10, 2024

Conversation

jjbustamante
Copy link
Member

@jjbustamante jjbustamante commented Sep 14, 2023

readable

Fixes pack #1459

@buildpack-bot
Copy link
Member

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)

@jjbustamante jjbustamante force-pushed the jjbustamante/feature/multi-arch-phase-2 branch 4 times, most recently from 3da0d9b to 06118b6 Compare September 25, 2023 20:47
@jjbustamante jjbustamante marked this pull request as ready for review October 2, 2023 22:18
@jjbustamante jjbustamante added team/platform scope/team RFC scoped to a sub-team as opposed to the entire project. status/needs-steward type/rfc labels Oct 2, 2023
text/0000-multiarch-builders-and-package.md Show resolved Hide resolved
text/0000-multiarch-builders-and-package.md Outdated Show resolved Hide resolved
text/0000-multiarch-builders-and-package.md Outdated Show resolved Hide resolved
Comment on lines 217 to 220
- Add a new boolean `--multi-arch` flag to indicate pack it must create multiples OCI artifacts and combine them with an [image index](https://github.com/opencontainers/image-spec/blob/master/image-index.md),
this flag must be used in conjunction with `--publish` or `--format file` flags and error out when `daemon` is selected.
> **Note**
> I got feedback to avoid creating an Image Index if users are not expecting that, the flag is an acknowledgment for creating the index
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar thoughts here. Could this be inferred based on the presence of multiple targets? I get that we don't want to just suddenly start making image indexes as that changes things. But couldn't we only do this if multiple targets with different arch's are specified? Or is there a case where you'd have multiple target/arch's and still not want that behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to think of who would be specifying multiple targets today and I'm guessing it would just be bash buildpacks. Maybe we could print a warning if multiple targets are defined but there is only one ./bin/detect or ./bin/build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenarios I was thinking were these:

  1. I do not have targets on my buildpack.toml
    • In this case, the idea is to pack buildpack package do the same thing as it is doing today.
  2. I do have targets on my buildpack.toml
  • Daemon case: pack should build my buildpack package with the platform defined by the daemon (as it is doing today) without requiring any new flag.
  • Registry or File case:
    • If more than 1 target is defined: pack will create the Image Index, If I just want to create 1 single package, I will need to use --target flag to defined which one.
    • If only 1 target is available, nothing changes

text/0000-multiarch-builders-and-package.md Outdated Show resolved Hide resolved
Comment on lines 333 to 442
```bash
pack buildpack package <buildpack> --config ./package.toml --publish --platform linux/arm64 --platform linux/amd64 --multi-arch
A multi-arch buildpack package will be created for platforms: 'linux/amd64', 'linux/arm64'
Successfully published package <buildpack> and saved to registry

# Or
pack buildpack package <buildpack> --config ./package.toml --format file --platform linux/arm64 --platform linux/amd64 --multi-arch
A multi-arch buildpack package will be created for platforms: 'linux/amd64', 'linux/arm64'
Successfully created package <buildpack> and saved to file
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this command could be used if one hasn't updated to use targets yet? i.e. still using stacks?

What about buildpacks that are migrating from stacks to no-stacks and have both present? This use case concerns me the most, cause I know we (Paketo) will be in this state for a while.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be interested to hear more in general about this state of migrating from stacks to no-stacks. What commands would use the old stack metadata? what commands would use the new target info? I'm assuming we'd keep the two synchronized, but just curious more about how this would all work.

Or am I overthinking this, and the net result (i.e. the images generated) will be the same regardless of stacks or no stacks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, regarding the stack removal plan we are planning to support both scenarios (stacks and run, build images), based on your feedback the idea will be to clarify how a world with:

  • stacks
  • targets
  • multi-arch support
    will work


## Buildpack Package

As a quick summary, our current process to create a buildpack package involves:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would anything change for a composite/meta buildpack? I'd assume that you would need to have multi-arch images for every buildpack in the composite buildpack. Otherwise, would it work the same as a regular component buildpack?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, I will add a new example with the composite builpack, but we are assuming you'd need to have multi-arch images for every buildpack in the composite buildpack

text/0000-multiarch-builders-and-package.md Outdated Show resolved Hide resolved
text/0000-multiarch-builders-and-package.md Outdated Show resolved Hide resolved
text/0000-multiarch-builders-and-package.md Outdated Show resolved Hide resolved
text/0000-multiarch-builders-and-package.md Outdated Show resolved Hide resolved

# List of targets operating systems, architectures and versions

[[targets]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for opening this can of worms, but technically "target" when used in the context of say CNB_TARGET_OS or CNB_TARGET_ARCH refers exclusively to the run image. Right now, all target data for the build image and the run image must match - but perhaps that won't always be the case? What will we do then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natalieparellano what do you think we should use here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure that the current architecture will accommodate this. What would probably end up happening is that buildpack.toml would have a list of [[targets]] and the buildpack os/arch/bin directories would be a subset of that list. Then at build time (somehow, but probably in run.toml) you'd have a list of run images to fulfill all the os/arch combinations in [[targets]] and by iterating through that list, you have a multi-arch application image. The key is that the mapping between buildpack.toml [[targets]] and the buildpack os/arch/bin directories would not necessarily be 1:1, whereas today we assume it is.

- For each `target` an OCI image will be created, following these rules
- `pack` will copy everything in the root folder excluding all `{os}/**` folder
- `pack` will determine the **target root binary folder**, this is the specific root folder for a given `target` (based on the `targets.path` in the buildpack.toml or inferring it from a folder structure similar to the one show above)
- `pack` will copy everything from the **target root binary folder** into the base buildpack package folder, in case a conflict file name is found, `pack` will **override** the existing file with the latest one
Copy link
Member

@joshwlewis joshwlewis Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will these two copy steps outlined happen in different image layers? It seems like the idea here is to save disk space for files that are common across architectures. To achieve that, I think you'd need an image layer with the common files, that images for each architecture could include in their layer hierarchy. Maybe this was the idea and I missed it. If so, maybe worth mentioning somewhere.

Otherwise, if common files are in the same image layer as the architecture-specific files, clients would essentially be downloading and storing the common files once for each architecture.

Copy link
Member

@joshwlewis joshwlewis Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I did find the answer a bit further on: https://github.com/buildpacks/rfcs/pull/295/files#diff-cf9fc6f40b9a670966257d2475741958b49fb15684dd37cf8019514fe779885bR799. It looks like common and architecture specific files will be in one image layer.

Copy link
Member

@joshwlewis joshwlewis Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the common files are duplicated across architecture images anyway, does the common file inheritance have much value? End users of buildpacks and builders won't really get much benefit. And for the CNBs I work on (Heroku's), they won't have many (maybe 0) files shared across architectures.

Just thinking common file inheritance is a heavy feature with some potential gotchas, and buildpack authors who want/need to dedupe files could probably get pretty far just by symlinking or hardlinking files into multiple {os}/{arch} folders.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @joshwlewis

Thank you very much for your feedback! I think that at least the buildpack.toml and the package.toml will be two common files shared across architectures, that's actually a win for a Buildpack Authors, right? because they do not need to keep separate files for each os/arch they want to support.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common file inheritance is a heavy feature with some potential gotchas

I agree. Maybe it makes sense to take it out of this RFC and add it later if buildpack authors want it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that at least the buildpack.toml and the package.toml will be two common files shared across architectures

I feel like buildpack.toml could be the one and only special case. IDK if package.toml is something that we even expect to be inside a buildpack root directory at build time. It's just easier to comprehend the packaging step if we don't move too much stuff around inside the directory tree. Perhaps we could spec out that we'd resolve symlinks when creating the layer if folks want to take advantage of that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if I take a look at some buildpacks for Paketo for example, they have an structure like this one in the root folder:

├── LICENSE
├── NOTICE
├── README.md
├── buildpack.toml
├── package.toml
├── resources
│ ├── config.properties

In these cases, I think they want to shared those files on each platform images right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these cases, I think they want to shared those files on each platform images right?

They could copy, symlink, or hard link files into each platform folder. I don't think that's a big challenge for buildpack authors (they probably script this in some way anyway). Honestly, I think it might be a bit more straightforward for them to understand, too. Copy, symlink, and hardlink are established concepts; they won't need to read this RFC or the buildpack spec to know how to do that.

I still think there's convenience in this shared file logic. I'm mostly just thinking you could simplify the initial implementation, long term maintenance, and probably this RFC without it.


We propose:
- Add a new `--target` flag with the follwing format `[os][/arch][/variant]:[name@version]` to build for a particular target, once the `platform.os` field is removed,
this will be the way for end-users to specify the platform for which they want to create single OCI artifact.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to think of --target as more of a helpful way to narrow down the os/arch when buildpack.toml has multiple definitions, or to avoid creating a manifest list. Is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly when you actually want to avoid creating multiple images, then target will help you

text/0000-multiarch-builders-and-package.md Show resolved Hide resolved
text/0000-multiarch-builders-and-package.md Show resolved Hide resolved

```json
{
"architecture": "",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how it works today, but this feels like a bug. The OCI image spec says this field is required.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is a bug, but apparently nobody is complaining about it! I didn't want to start filling this attribute and break the current behavior for end-users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this did come up recently (here and here) I think we said this RFC would fix those cases but maybe we assumed first the buildpack author would upgrade to use targets. In any case, it probably doesn't hurt... I'm fine to leave it out of this RFC though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think is what @jericop is trying to fix also here

```bash
pack builder create <builder> --config ./builder.toml
Warning: "stack" has been deprecated, prefer "targets" instead: https://github.com/buildpacks/rfcs/blob/main/text/0096-remove-stacks-mixins.md
Warning: A new '--target' flag is available to set the target platform for the builder, using 'linux/amd64' as default
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that the build image is a manifest list, no? Because if the build image is linux/arm64 (only) then we create an arm64 image, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the pack builder create command assumes the build, run and buildpack images are multi-arch, and when building for a particular platform, pack should validate it

@natalieparellano
Copy link
Member

Awesome work @jjbustamante. I would really prefer if we kept "shared" files out of this RFC, it would make it a lot simpler to understand and implement. But otherwise this looks great.

```bash
pack buildpack package <buildpack> --config ./package.toml --publish --target linux/arm64 --target linux/amd64
A multi-arch buildpack package will be created for target platforms: 'linux/amd64', 'linux/arm64'
Successfully published package <buildpack> and saved to registry
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually the current messaged showed in pack

@jjbustamante jjbustamante force-pushed the jjbustamante/feature/multi-arch-phase-2 branch from 2a5741f to 44b9cca Compare March 27, 2024 16:55
jjbustamante and others added 17 commits March 27, 2024 11:55
Signed-off-by: Juan Bustamante <[email protected]>
…e the path for binaries in buildpack.toml

Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
…ic when exporting multi-arch buildpack packages as files

Signed-off-by: Juan Bustamante <[email protected]>
Co-authored-by: Rune Soerensen <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Co-authored-by: Rune Soerensen <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
@jjbustamante jjbustamante force-pushed the jjbustamante/feature/multi-arch-phase-2 branch from 44b9cca to abd3ba2 Compare March 27, 2024 16:56
natalieparellano added a commit that referenced this pull request Apr 10, 2024
[#295]

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano natalieparellano merged commit faa92a2 into main Apr 10, 2024
7 checks passed
@natalieparellano natalieparellano deleted the jjbustamante/feature/multi-arch-phase-2 branch April 10, 2024 14:44
Comment on lines +98 to +100
[[targets.distributions]]
name = "windows"
versions = ["10.0.20348.1970"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjbustamante I believe this should be [[targets.distros]] plus versions should be version (singular) and a string not an array?

(Plus the same change made to the 3-4 similar references in this file)

Copy link

@schneems schneems Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering what automated checks around non-code PRs such as a buildpack.toml linter (for example) might look like.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjbustamante I believe this should be [[targets.distros]] plus versions should be version (singular) and a string not an array?

(Plus the same change made to the 3-4 similar references in this file)

Yes!! You are right @edmorley! I will ask how could I do the fix now that this was merged

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw you opened a PR to fix a previous RFC, is it possible for you to open one for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - I've opened #312.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!! @edmorley

edmorley added a commit to edmorley/rfcs that referenced this pull request Apr 11, 2024
Updates RFC 0128 to use the correct `[[targets.distros]]` schema as defined
already in the spec:
https://github.com/buildpacks/spec/blob/main/buildpack.md#buildpacktoml-toml

I've not added an `## Amended` section, since:
1. The RFC has only just merged (this is effectively a review comment
   that missed the merge by a day)
2. The parts of the RFC being fixed are existing concepts already in the spec,
   rather than design decisions relating to the purpose of the RFC itself.

Fixes:
buildpacks#295 (comment)
edmorley added a commit to edmorley/rfcs that referenced this pull request Apr 11, 2024
Updates RFC 0128 to use the correct `[[targets.distros]]` schema as defined
already in the spec:
https://github.com/buildpacks/spec/blob/main/buildpack.md#buildpacktoml-toml

I've not added an `## Amended` section, since:
1. The RFC has only just merged (this is effectively a review comment
   that missed the merge by a day)
2. The parts of the RFC being fixed are existing concepts already in the spec,
   rather than design decisions relating to the purpose of the RFC itself.

Fixes:
buildpacks#295 (comment)
Signed-off-by: Ed Morley <[email protected]>
enrichman pushed a commit to enrichman/rfcs that referenced this pull request Aug 8, 2024
[buildpacks#295]

Signed-off-by: Natalie Arellano <[email protected]>
enrichman pushed a commit to enrichman/rfcs that referenced this pull request Aug 8, 2024
Updates RFC 0128 to use the correct `[[targets.distros]]` schema as defined
already in the spec:
https://github.com/buildpacks/spec/blob/main/buildpack.md#buildpacktoml-toml

I've not added an `## Amended` section, since:
1. The RFC has only just merged (this is effectively a review comment
   that missed the merge by a day)
2. The parts of the RFC being fixed are existing concepts already in the spec,
   rather than design decisions relating to the purpose of the RFC itself.

Fixes:
buildpacks#295 (comment)
Signed-off-by: Ed Morley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/team RFC scoped to a sub-team as opposed to the entire project. status/voting team/platform type/rfc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants