fix(*): wait_for_dependency will wait forever for unhealthy container #551
Conversation
StarlightIbuki
force-pushed
the
fix/infinite-wait-dep
branch
from
bb4507b
to
8380240
Compare
StarlightIbuki
force-pushed
the
fix/infinite-wait-dep
branch
from
8380240
to
3fd2254
Compare
| @@ -625,20 +625,26 @@ function healthy { | |||
| fi | |||
|
|
|||
| local state | |||
| state=$(docker inspect "$iid") | |||
| state=$(docker inspect --format='{{.State.Health.Status}}' "$iid") | |||
There was a problem hiding this comment.
I'm hesitant with these types of changes, on how we validate status and how we collect that input. We've had plenty of issues in the past due to different Docker versions, or even non-Docket tools being used.
So are these changes necessary?
There was a problem hiding this comment.
The runner blocks until it times out when a container fails to start, and it gives no reason. This makes it hard to diagnose issues with container setup. I'm trying to improve the situation.
There was a problem hiding this comment.
I can find a use of this in a 7 years ago script: https://github.com/jordyv/wait-for-healthy-container/blob/master/wait-for-healthy-container.sh, so maybe we can consider it a stable interface.
There was a problem hiding this comment.
@StarlightIbuki don't get me wrong, I like the improvements, but what I'm hesitant about is changing the checks themselves as you do in the above lines.
| @@ -609,7 +609,7 @@ function compose { | |||
| } | |||
|
|
|||
|
|
|||
| function healthy { | |||
| function status { | |||
There was a problem hiding this comment.
Can you add comments here specifically what the returned values are?
Then I think there are issues here, it can return 0 for success (for example if health checks are disabled). Yet in the calling code, that is not checked, since it only checks "healthy" as return value.
So I think this currently breaks the "SERVICE_DISABLE_HEALTHCHECK" option
pongo.sh
Outdated
| msg "Waiting for $dep" | ||
|
|
||
| while ! healthy "$iid" "$dep"; do | ||
| while [ "$(status "$iid" "$dep")" == "starting" ]; do |
There was a problem hiding this comment.
should we bail out of this loop after a timeout period? eg. 60 seconds?
There was a problem hiding this comment.
That makes sense. I was blindly following the original behavior.
StarlightIbuki
force-pushed
the
fix/infinite-wait-dep
branch
from
3fd2254
to
fcb6b7d
Compare
|
@StarlightIbuki see #554 for an alternative implementation |
As the title says.