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 healthcheck and release ip if not successful #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ibotty
Copy link
Contributor

@ibotty ibotty commented Jun 18, 2021

This is not backwards-compatible in that it will run the healthcheck (and demote the peer) by default. That can of course be changed.

@@ -11,6 +11,8 @@ ENV KARP_SERVER_ID 10
ENV KARP_PASSWORD RAnDoM_max16char
ENV KARP_UPSCRIPT /etc/ucarp/vip-up-default.sh
ENV KARP_DOWNSCRIPT /etc/ucarp/vip-down-default.sh
ENV KARP_DISABLE_HEALTHCHECK=
ENV KARP_HEALTHCHECK_URL=https://localhost:6443/livez
Copy link
Owner

Choose a reason for hiding this comment

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

Since the container is running with host networking, this is the Kubernetes API server. If we check its health, the IP is released to another node that might hold a working API server. This is why you've introduced this PR, am I right?

If yes, the KARP_HEALTHCHECK_URL could be KARP_K8S_HEALTHCHECK_URL to be more descriptive. At first, I was thinking for a moment this is some kind of built-in Ucarp feature :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I can, of course, change the environment name. But I personally prefer KARP_HEALTHCHECK_URL because it might check any other Service as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe KARP_CHECK_URL to make it more explicit? Naming is hard.

Copy link
Owner

Choose a reason for hiding this comment

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

I can't think of any better name than KARP_APICHECK_URL if this is a general API health check to release the IP. I just want to separate the term health from uCarp's own health - that's what caused me the confusion in the first place.

Comment on lines +41 to +45
demote_ucarp() {
kill -USR2 "$(pidof ucarp)"
sleep 1
$KARP_DOWNSCRIPT "$KARP_INTERFACE" "$KARP_VIRTUAL_IP" "$KARP_SUBNET"
}
Copy link
Owner

Choose a reason for hiding this comment

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

This function seems to be the same as cleanup, we could delete one of them

}

do_healthcheck() {
[ "$(wget --no-check-certificate -q -O - "$KARP_HEALTHCHECK_URL")" = ok ]
Copy link
Owner

Choose a reason for hiding this comment

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

This wget returns a 401 error when being run in an existing kube-karp pod in my cluster, so it won't be "ok" ever.

/ # wget --no-check-certificate -q -O - https://localhost:6443/livez
wget: server returned error: HTTP/1.1 401 Unauthorized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's strange. OpenShift allows this URI without authorization.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm using RKE (Rancher Kubernetes Engine), so this is a nice implicit test case but a truly blocker one if it doesn't pass

@ibotty
Copy link
Contributor Author

ibotty commented Apr 6, 2022

Sorry, I missed the notification last year.

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