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 http health check #210

Merged
merged 2 commits into from Nov 15, 2019
Merged

Add http health check #210

merged 2 commits into from Nov 15, 2019

Conversation

nanzhong
Copy link
Member

We would like to be able to monitor the availability of the CSI driver. This PR adds a health check endpoint for this.

This is a similar change to digitalocean/digitalocean-cloud-controller-manager#293, but simpler since we have full control over how we configuring the driver.

Similar future consideration include:

  • tying the DO api health to the actual endpoints that we use

@nanzhong
Copy link
Member Author

🤔 there was an issue with the vendored packages and when I did a tidy + vendored a massive set of changes were pulled in, taking a look to see what's up.

@nanzhong nanzhong changed the base branch from master to bump-k8s-1.15 November 12, 2019 01:32
@nanzhong nanzhong changed the base branch from bump-k8s-1.15 to master November 12, 2019 01:39
@nanzhong
Copy link
Member Author

Figured out the reason for the vendor issues. It was related to some indirect dependencies that is introduced by adding the healthz package from apiserver. This PR is now updated to with the minimal set of module and vendor changes.

We are running with 1.12 dependencies; I've opened #212 to look into updating our dependencies to 1.15.

Copy link
Collaborator

@jcodybaker jcodybaker left a comment

Choose a reason for hiding this comment

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

LGTM

@nanzhong
Copy link
Member Author

@timoreimann would mind taking another look? I've stripped down the health checking to not need new things vendored.

@nanzhong
Copy link
Member Author

I will backport this to the other branch PRs after this is thumbsed.

Copy link
Collaborator

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM.

defer cancel()
_, _, err := c.account.Get(ctx)
if err != nil {
return fmt.Errorf("checking do health: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: wrapping (%w) probably not needed.

(This will likely also fail on older versions built with Go <1.13.)

@timoreimann
Copy link
Collaborator

Conflicts need to be resolved still.

@timoreimann
Copy link
Collaborator

Oh would be great if you could amend the change log real quick @nanzhong .

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.

None yet

3 participants