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

fix(cf): scale app before building droplet #5692

Merged
merged 2 commits into from
May 13, 2022
Merged

Conversation

mattgogerly
Copy link
Member

Currently we set process config and scale the app/processes after building the droplet. This causes issues if the droplet requires more memory to build (e.g. lots of Python dependencies to install).

cf push uses a v2 endpoint to create applications which allows specifying scale when creating the app, whereas we are using a v3 endpoint which does not have those fields. Scaling is therefore a separate call.

@zachsmith1
Copy link
Contributor

I'd recommend doing a push with the same Python stuff via the CLI and with the verbose flag. Also check the sequence of api calls. I vaguely remember we had to do it this way but we should prob do whatever the CLI does

@mattgogerly
Copy link
Member Author

mattgogerly commented Apr 22, 2022

I'd recommend doing a push with the same Python stuff via the CLI and with the verbose flag. Also check the sequence of api calls. I vaguely remember we had to do it this way but we should prob do whatever the CLI does

The CF CLI is calling POST /v2/apps which accepts the full app definition (memory, disk, build packs etc). rather than v3, which we're calling, which only accepts the app name and lifecycle.

We've built an image with this change internally and we're able to deploy as normal with no issues, so if you have any idea what the specific problem was that we can test that'd be helpful!

edit: you might be thinking of the need to update processes before scaling, or to bind services before building the droplet? Both of those changes remain in place with this change

@nemesisOsorio
Copy link
Member

@mattgogerly we checked the cli calls using version 8.3.0 this version is using API v3
we noticed the cli is calling POST /v3/packages follow by POST /v3/builds(droplet)
we used the command cf push nginx0001 -v --docker-image nginx:alpine
and then we scaled with cf scale nginx0001 -i 3
we are worried about the order of the api calls
just to clarify can you confirm that you deployed a docker image using these changes?
as well can you provide us an example of your pipeline?

@mattgogerly
Copy link
Member Author

I don't have any way to test Docker stuff, which I assume means the issue @zachsmith1 pointed out is limited to that. Nonetheless, thanks for checking - I double checked and the /v3/builds endpoint has parameters for setting build sizing.

I've updated the PR to use those and reverted the ordering 👍

@mattgogerly mattgogerly force-pushed the release/cf-scale-ordering branch 2 times, most recently from a637127 to 50e5604 Compare May 12, 2022 10:17
Copy link
Member

@nemesisOsorio nemesisOsorio left a comment

Choose a reason for hiding this comment

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

LGTM, I believe you need to rename the pr

@link108 link108 added ready to merge Approved and ready for a merge backport-candidate Add to PRs to designate release branch patch candidates. labels May 12, 2022
@mergify mergify bot added the auto merged Merged automatically by a bot label May 12, 2022
@mergify mergify bot merged commit 9d3d51e into master May 13, 2022
@mergify mergify bot deleted the release/cf-scale-ordering branch May 13, 2022 00:31
@link108
Copy link
Member

link108 commented May 13, 2022

@Mergifyio backport release-1.28.x

mergify bot pushed a commit that referenced this pull request May 13, 2022
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 9d3d51e)
@mergify
Copy link
Contributor

mergify bot commented May 13, 2022

backport release-1.28.x

✅ Backports have been created

link108 added a commit that referenced this pull request May 18, 2022
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Matt <6519811+mattgogerly@users.noreply.github.com>
Co-authored-by: Cameron Motevasselani <cmotevasselani@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot backport-candidate Add to PRs to designate release branch patch candidates. ready to merge Approved and ready for a merge target-release/1.29
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants