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

Use single step for all four Buildpacks lifecycle phases #491

Conversation

SaschaSchwarze0
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 commented Nov 23, 2020

FIxes #455

The change updates the buildstrategies for Buildpacks, Heroku as well as Paketo, to use a single step for the four lifecycle phases. It is inspired by the Tekton catalog entry for Buildpacks. It uses the functionality introduced by /cnb/lifecycle/creator.

I am using the resources of one old step now for the merged step.

I did some performance tests. The old build strategy assigns different resources to the different steps: prepare, detect and restore got 0.1 CPU cores and 128 MiB memory; build and export received 0.5 CPU cores and 512 MiB memory. The new build strategy with just one step for the Buildpacks lifecycle continued to assign 0.1 CPU cores and 128 MiB memory to the prepare step while the buildpacks step received 0.5 CPU cores and 512 MiB memory. The source code came from cf-acceptance-tests. The Buildpacks builder image was 0.1.35-full. I triggered ten buildruns at the same time. Four runtimes were tested:

Runtime Old build strategy New build strategy
Node 1m18.5s 1m14.6s
Go 3m20.7s 2m56.5s
Java 1m43.1s 1m33s
PHP 2m17s 2m12.6s

To confirm no difference with larger load, a single comparison with 30 parallel builds:

Runtime Old build strategy New build strategy
Java 2m57.7s 2m13.666666666s

@zhangtbj
Copy link
Contributor

A quick question, do we need to update the buildpacks step in this ADR document?: https://github.com/shipwright-io/build/blob/master/docs/proposals/buildstrategy-steps-resources.md#example Or just keep as before is enough?

@SaschaSchwarze0
Copy link
Member Author

A quick question, do we need to update the buildpacks step in this ADR document?: https://github.com/shipwright-io/build/blob/master/docs/proposals/buildstrategy-steps-resources.md#example Or just keep as before is enough?

Hm, good question. The proposal describes the distinction between strategy administrators and users, and discusses how we assign resources from the build strategy steps into container resources in the TaskRun. It also uses a buildpacks-v3-default strategy as a sample to help understanding it. The way the strategy looks, it does not match any existing build strategy (some steps have no resources, the buildpacks-v3 has resources for all steps also before I started modifying it, and they were the same for every step).

What do you think @qu1queee ? Should I update the sample in your proposal ?

@sbose78
Copy link
Member

sbose78 commented Nov 23, 2020

We could keep the proposal as is since this change doesn't fundamentally change how a BuildStrategy needs to be crafted.

@SaschaSchwarze0
Copy link
Member Author

@sbose78 I added the performance comparison results to the PR description.

@zhangtbj zhangtbj self-requested a review November 24, 2020 02:14
Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

This looks good, not approving yet because either bot will merge this and there are two minor questions might be important. Can you pls add https://github.com/buildpacks/rfcs/blob/main/text/0026-lifecycle-all.md in your PR description pls, for future references.

@SaschaSchwarze0 SaschaSchwarze0 force-pushed the sascha-455-buildpacks-single-step branch from f8a9225 to 5955ad6 Compare November 24, 2020 09:55
Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

Looks solid, thanks for the Verbesserung.

@qu1queee
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 24, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2020
@openshift-merge-robot openshift-merge-robot merged commit ebfe068 into shipwright-io:master Nov 24, 2020
@SaschaSchwarze0 SaschaSchwarze0 deleted the sascha-455-buildpacks-single-step branch November 24, 2020 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify buildpacks sample build strategy to use updated pattern from Tekton catalog
6 participants