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

health: add liveness server #461

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

health: add liveness server #461

wants to merge 8 commits into from

Conversation

roobre
Copy link
Contributor

@roobre roobre commented May 26, 2022

Right now, it is not possible for Kubernetes to know the state of the integration. While the integration currently exits whenever an error occurs, before entering the main loop it will be still waiting for some initialization to complete.

This PR adds a health server that can be used for the application to expose whether it is healthy or not. Currently, the main command of the integration sets the server to Healthy after the first successful scrape.

@roobre roobre force-pushed the liveness branch 3 times, most recently from 8bbb8f1 to 8727225 Compare May 26, 2022 14:41
cmd/nri-kubernetes/main.go Show resolved Hide resolved

{{- if not (include "nriKubernetes.controlPlane.hostNetwork" .) }}
ports:
- containerPort: 8999
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it worth it to make the health port configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've started implementing this, but I'm finding UX concerns that are pretty hard to solve. The main one is that, right now, it's easy to expose the containerPort and to disable everything if hostNetwork is on. If we expose the config, that logic gets more complicated, especially if we want to override this setting depending on hostNetwork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a big fan of the hardcoded probe as it is now either... Any ideas, @kang-makes ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should listen to localhost and it is disabled when hostNetwork is enabled so I have no big issues with hardcoding it.

As it could be set in the configuration file we could test in the .Values.common.config has a healthAddress and set it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any other already configurable port which may collide if it is set to 8999? If we finally keep it hardcoded, that is the only cause for concern I can think of...

Copy link
Contributor

@kang-makes kang-makes May 31, 2022

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to have preview but Github seems to be drunk (again)

Ports 8001, 8002, and 8003 are hardcoded right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! 👍

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