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

Remove retrying of pack builder create #261

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

edmorley
Copy link
Member

Since:

  • The retry handling is broken, such that if the command fails after three retries, then the exit code is obscured and the CI run continues to the next step. This causes a confusing error message that's shown instead of the actual error.
  • In the case of legitimate failures, the retries prolong the time until the CI result shows the failure.
  • The builder creation step is pretty reliable now (I've inspected several dozen invocations and didn't see any failures).
  • If we start seeing intermittency in the future, I would rather know about it so we can improve reliability for everyone, rather than cover it up via retries.

GUS-W-11263169.

Since:
- The retry handling is broken, such that if the command fails after
  three retries, then the exit code is obscured and the CI run continues
  to the next step. This causes a confusing error message that's shown
  instead of the actual error.
- In the case of legitimate failures, the retries prolong the time until
  the CI result shows the failure.
- The builder creation step is pretty reliable now (I've inspected several
  dozen invocations and didn't see any failures).
- If we start seeing intermittency in the future, I would rather know about
  it so we can improve reliability for everyone, rather than cover it up via
  retries.

GUS-W-11263169.
@edmorley edmorley self-assigned this Jun 10, 2022
@edmorley
Copy link
Member Author

edmorley commented Jun 10, 2022

I should note a few weeks ago I also scaled up cnb-shim from one web dyno to two, plus enabled preboot -- to reduce chance of cnb-shim related errors during builder creation (or when people use it via --buildpacks).

@edmorley edmorley marked this pull request as ready for review June 10, 2022 14:23
@edmorley edmorley requested a review from a team as a code owner June 10, 2022 14:23
Copy link
Member

@joshwlewis joshwlewis left a comment

Choose a reason for hiding this comment

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

This may have also been helped by the go rewrite @jason-salesforce did.

@edmorley
Copy link
Member Author

Ah true! :-)

I'm presuming some other failure modes are errors or rate limits pulling from Docker Hub and/or ECR. However Circle CI now has exemption from Docker Hub rate limits even when not authed (I think there was a spell before that was implemented), and the ECR rate limit doesn't seem to be an issue in practice (whilst the limit is low, it's per Docker repo per IP, and unlike the buildpack cutlass tests, this CI doesn't do repeat pulls of the same Docker repo within the same job).

@edmorley
Copy link
Member Author

(And if we hit rate ECR limits again in the future, perhaps it's another reason to switch back to Docker Hub from ECR for our buildpack images.)

@edmorley edmorley merged commit b20f5b9 into main Jun 10, 2022
@edmorley edmorley deleted the edmorley/remote-builder-create-retries branch June 10, 2022 14:46
heroku-linguist bot added a commit that referenced this pull request Sep 4, 2024
## heroku/python

### Added

- Added initial support for the Poetry package manager. ([#261](heroku/buildpacks-python#261))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants