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

Add liveness and readiness probes for the ipfailover dc. #9215

Merged
merged 1 commit into from
Jun 9, 2016

Conversation

ramr
Copy link
Contributor

@ramr ramr commented Jun 8, 2016

As per a message on the mailing list, fixing the warnings oc status -v shows for ipfailover.
Add readiness and liveness probes.

@smarterclayton PTAL thx

@smarterclayton
Copy link
Contributor

Debating whether you need a readiness probe. Do you expose ports? Is there any case where a service of ipfailover pods would be used to determine which nodes are ready? Are you using host network or pod network in ipfailover?

@ramr
Copy link
Contributor Author

ramr commented Jun 8, 2016

So this came about on the mailing list because the oc status -v command shows the readiness probe as missing for ipfailover and the registry.
To answer your questions - no, we don't expose ports and no, don't really need determination of which nodes are ready. And it is using the host network.

Does it hurt to have it? To play devil's advocate here, the probes did feel a bit weird as its checking for the process (which if it was dead would have killed the container off anyway). I chalk that up to it being late last night and the meds kicking in! Though that said, I guess we could check the keepalived processes instead of monitor.sh.

@smarterclayton
Copy link
Contributor

The warning may be too aggressive, at least if you don't plan to have
them. That's the challenge with all helpful messages.

I don't think readiness should be added unless we can detect when the VIP
node is now available for serving. However, since the VIP doesn't show up
in endpoints, it's probably not a great story today. It might be better
for ipfailover to periodically update endpoints to be the valid range of
IPs.

Liveness is valid if there's a way to ensure that the process is still
doing something useful.

On Jun 8, 2016, at 2:21 PM, Ram Ranganathan [email protected]
wrote:

So this came about on the mailing list because the oc status -v command
shows the readiness probe as missing for ipfailover and the registry.
To answer your questions - no, we don't expose ports and no, don't really
need determination of which nodes are ready. And it is using the host
network.

Does it hurt to have it? To play devil's advocate here, the probes did feel
a bit weird as its checking for the process (which if it was dead would
have killed the container off anyway). I chalk that up to it being late
last night and the meds kicking in! Though that said, I guess we could
check the keepalived processes instead of monitor.sh.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9215 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABG_p13oqkTkW598fnK_NkK_SUdLZpyBks5qJwghgaJpZM4IwoEL
.

@ramr
Copy link
Contributor Author

ramr commented Jun 8, 2016

@smarterclayton changed to just the liveness probe + use keepalived as the process to really check. PTAL thx

[test]

@smarterclayton
Copy link
Contributor

LGTM, you've manually tested to verify it stays up (we don't have an integrated test for it yet)?

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to fec14fb

@ramr
Copy link
Contributor Author

ramr commented Jun 8, 2016

@smarterclayton - yes manually verified that it stays up and also that it cleans up on termination (tested with oc scale dc/ipfailover --replicas=0).

@smarterclayton
Copy link
Contributor

[merge]

On Wed, Jun 8, 2016 at 5:19 PM, Ram Ranganathan [email protected]
wrote:

@smarterclayton https://github.com/smarterclayton - yes manually
verified that it stays up and also that it cleans up on termination (tested
with oc scale dc/ipfailover --replicas=0).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9215 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p9mC9w3TEnehmlkOm0s_f1_uRxsHks5qJzHggaJpZM4IwoEL
.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 8, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4629/) (Image: devenv-rhel7_4333)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to fec14fb

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4629/)

@openshift-bot openshift-bot merged commit 381559d into openshift:master Jun 9, 2016
@ramr ramr deleted the ipfprobes branch February 3, 2017 00:10
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.

3 participants