-
Notifications
You must be signed in to change notification settings - Fork 8
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
Make deploy.RunPod
wait for pod readiness rather than running phase
#346
base: main
Are you sure you want to change the base?
Conversation
76d34e3
to
00e3577
Compare
deploy.RunPod
wait for pod readiness rather than running phase
4e7747e
to
4f19c30
Compare
4f19c30
to
544fa20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the need for the BuildNginxPodWithoutProbes()
fixture for the disruptor tests a design smell. The idea of introducing a readiness probe in the test fixtures was to avoid the flakiness of certain tests, but it introduces more subtle problems as the disruptor tests will fail if this non-intuitive fixture is used.
The reason for these potential test failures is that the tests apply the disruption to all requests to a target pod or service. This includes the readiness/liveness probes if any, unless explicitly excluded. But which probes to exclude may not be obvious and depended on the fixture's definition
I think that either we reverse the situation and make the alternative BuildxxxWithProbes
fixture or we address the problem of automatically excluding the pod's probes from disruptions.
Description
This PR changes the
WaitPodRunning
function toWaitPodReady
, which checks whether the pod has theReady
condition set toTrue
instead of simply being in theRunning
phase.If a pod does not contain any container with a readiness probe defined,
WaitPodReady
should behave exactly the same asWaitPodRunning
did. However, if one or more containers have a readiness probe,WaitPodReady
will wait until they are satisfied.This is helpful to reduce flakiness on certain tests which previously attempted to initiate a connection to a pod once it was running, but before the pod had time to actually get started. By attaching a readiness probe to those pods, this scenario can be avoided, as
deploy.RunPod
will wait or error until the readiness probe passes.This PR also adds a helper
WithHTTPReadinessProbe
function toContainerBuilder
, that attaches an HTTP GET readiness probe to a container with reasonable defaults.Checklist:
make lint
) and all checks pass.make test
) and all tests pass.make integration-xxx
for affected packages)make e2e-xxx
fordisruptors
, orcluster
related changes)