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

feat(proof_data_handler): add a health check endpoint to the proof data handler #2278

Closed
wants to merge 1 commit into from

Conversation

pbeza
Copy link
Collaborator

@pbeza pbeza commented Jun 19, 2024

What ❔

Add /health endpoint to the proof_data_handler introduced in eca98cc (#1993).

Why ❔

The DevOps team asked for a health check endpoint so the in-cluster load balancer can use it. Details: https://matter-labs-workspace.slack.com/archives/C05MZ0ZUB6Z/p1718801741282109?thread_ts=1718191882.611349&cid=C05MZ0ZUB6Z

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.
  • Spellcheck has been run via zk spellcheck.

@pbeza pbeza force-pushed the tee/proof-data-handler-healthcheck-feature branch from d5924e9 to e29c022 Compare June 19, 2024 14:20
Copy link
Collaborator

@haraldh haraldh left a comment

Choose a reason for hiding this comment

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

LGTM

@pbeza
Copy link
Collaborator Author

pbeza commented Jun 19, 2024

@popzxc @RomanBrodetski is it too hacky? Should I use health_check crate instead?

Copy link
Member

@popzxc popzxc left a comment

Choose a reason for hiding this comment

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

You should use health check instead indeed. Let's not mix approaches.

@popzxc
Copy link
Member

popzxc commented Jun 19, 2024

Moreover, healthcheck layer is added unconditionally in the node, so isn't it already covered?

@thomasknauth
Copy link
Contributor

Moreover, healthcheck layer is added unconditionally in the node, so isn't it already covered?

Not sure. Proof-data-handler is started like so https://github.com/matter-labs/gitops-kubernetes/blob/main/apps/environments/era-stage-proofs/server-v2/server-v2-proof-data-handler.yaml - does node framework start the health check server automatically? If not, proof data handler should spawn it.

@thomasknauth
Copy link
Contributor

Moreover, healthcheck layer is added unconditionally in the node, so isn't it already covered?

Not sure. Proof-data-handler is started like so https://github.com/matter-labs/gitops-kubernetes/blob/main/apps/environments/era-stage-proofs/server-v2/server-v2-proof-data-handler.yaml - does node framework start the health check server automatically? If not, proof data handler should spawn it.

Never mind. Proof data handler has the health server, so we will integrate with it properly. ✅

@pbeza
Copy link
Collaborator Author

pbeza commented Jun 19, 2024

Closing as it turned out there is already a health check endpoint we can reuse.

@pbeza pbeza closed this Jun 19, 2024
@thomasknauth thomasknauth deleted the tee/proof-data-handler-healthcheck-feature branch June 20, 2024 07:59
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.

4 participants