Skip to content

Update grpc, maintain health check interface; requries go update #689

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

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

Conversation

zalegrala
Copy link
Contributor

What this PR does:

Here we update the grpc version, which requires two changes. First a Go update to 1.23 and second an interface update to include List() method. The implementation simply wraps the Check method.

I invented a key here, which seemed to suit the change, but perhaps "default" could be as well. I also notice that we have an individual check for all services wrapped in a closure, but perhaps a future change might modify the List method to include the status of each of the manager's services.

Which issue(s) this PR fixes:

Dependabot PRs updating dependencies which include newer grpc. For an example: grafana/tempo#5097

Fixes #

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Collaborator

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Any idea why tests fail?

@zalegrala
Copy link
Contributor Author

zalegrala commented May 6, 2025

No I'm a little confused why the tests are failing. They timeout locally, but I don't PR this repo often enough to know what's normal. I'll dig around some today.

@zalegrala
Copy link
Contributor Author

zalegrala commented May 6, 2025

Note that if I run the test individually which are failing in this PR, they pass locally.

❯ go test -race -cover ./crypto/tls/test/ -count=1 -run=".*"
ok  	github.com/grafana/dskit/crypto/tls/test	2.878s	coverage: 76.7% of statements

@aknuds1
Copy link
Collaborator

aknuds1 commented May 6, 2025

It's not flakiness is it @zalegrala?

@zalegrala
Copy link
Contributor Author

Hmm, possibly yes. I do get a failure if I run with -count=100 -failfast.

go test -race -cover ./crypto/tls/test/ -count=100 -failfast -run=".*"

@aknuds1
Copy link
Collaborator

aknuds1 commented May 6, 2025

I guess failing tests need some un-flaking then :)

@zalegrala
Copy link
Contributor Author

At least one of the failures looks like localhost meaning different TCP networks being resolved between the server and client. I'm taking a look at what we can do here and trying to not break existing tests in the process.

@zalegrala
Copy link
Contributor Author

I've raised #691. Let's see what happens in CI over there.

@zalegrala zalegrala force-pushed the grpcHealthUpdate branch from 1076657 to 7299073 Compare May 8, 2025 14:02
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