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

Retry some commands when failed #377

Merged
merged 2 commits into from
Dec 7, 2016
Merged

Retry some commands when failed #377

merged 2 commits into from
Dec 7, 2016

Conversation

arm4b
Copy link
Member

@arm4b arm4b commented Dec 3, 2016

This PR is start of series to improve the builds in general and let them suck less
In 90% cases what we do to fix the build: retry manually

Brings function to retry failing command in Makefile.

And while pip has its own connection-retrying mechanism (with --retries <retries> default 5), most probably something wrong with it or it's not enough (?).
Anyways, experimenting. It's strange how frequently we have upstream connection errors like:


More technical debt that is good to address in future:

@arm4b
Copy link
Member Author

arm4b commented Dec 4, 2016

Surprisingly, it worked.

Could catch an example of this "super-intelligent auto-remediation" 😃 :

Here is the build where pip failed 5 times in different places (and was auto-retried):
https://circleci.com/gh/StackStorm/st2-packages/1483

Just a reminder to avoid using retry for anything not related to connection.
That can lead to "passed" tests for race conditions.

@arm4b arm4b added RFR and removed WIP labels Dec 4, 2016
@Kami
Copy link
Member

Kami commented Dec 4, 2016

Nice work 👍

Sadly there are still too many intermediate failures not directly related to our stuff and which we can't control so this should hopefully help a lot :)

@@ -30,6 +30,10 @@ else
DEB_DISTRO := $(shell lsb_release -cs)
endif

# Makefile function to retry the failed command 'N' times
# Example: $(call retry,3,some_script.sh)
retry = $(2) $(foreach t,$(shell seq 1 ${1}),|| (echo -e "\033[33m Failed ($$?): '$(2)'\n Retrying $t ... \033[0m"; $(2)))
Copy link
Member

Choose a reason for hiding this comment

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

Bash one liner!

I would prefer to make it just a normal function since it's more readable, but whatever :)

Copy link
Member Author

@arm4b arm4b Dec 4, 2016

Choose a reason for hiding this comment

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

Yeah, I don't like it too. But that's how it's usually done with make custom functions: https://www.gnu.org/software/make/manual/make.html#Call-Function

Multi-line/bash function would be even worse and easier to mess (ex: http://stackoverflow.com/questions/12774787/how-to-define-global-shell-functions-in-a-makefile).

I'll play with retries builds https://circleci.com/gh/StackStorm/st2-packages/tree/feature%2Fretry-on-failure more and if I'll find that 1 single command retry was enough in all cases,
maybe just:

pip wheel --wheel-dir=$(WHEELDIR) --find-links=$(WHEELDIR) -r requirements.txt || \
  pip wheel --wheel-dir=$(WHEELDIR) --find-links=$(WHEELDIR) -r requirements.txt

and I started with that first, but decided to go with something more re-usable.

So maybe retry function with echo-s and makefile alien syntax is overcomplication here.

@Kami
Copy link
Member

Kami commented Dec 4, 2016

Also, I don't know how easy it is to do, but it would be nice to have some kind of "test" for the retry function.

This way we can be positive that retry function which is not working correctly won't mask the failures and cause builds to succeed. I know the build would still probably fail somewhere else down the chain, but I would feel more comfortable with such "test" / check :)

@emedvedev
Copy link
Contributor

MAKE BUILDS GREAT AGAIN.

@arm4b
Copy link
Member Author

arm4b commented Dec 7, 2016

Considering our discussions @Kami @lakshmi-kannan, 👎 x 2 is too much, so I replaced it in favor of just command || command for simplicity.
Kept retry function only in 1 Makefile for future reference in case we'll need it in some future. So it's not used.

Let's see how this change behaves to get less failures and see what else we can adjust.

@Kami
Copy link
Member

Kami commented Dec 7, 2016

LGTM 👍

Let's try it and see how it goes :)

@arm4b arm4b merged commit d9a85e5 into master Dec 7, 2016
@arm4b arm4b deleted the feature/retry-on-failure branch December 7, 2016 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants