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

chore: Separation of node health and initialization state from rln_relay #2612

Merged
merged 7 commits into from
Apr 23, 2024

Conversation

NagyZoltanPeter
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter commented Apr 22, 2024

Description

In order to be able to give early status of node startup even during initialization, rest server start and /health endpoint installation must have been separated.
This separation allows users to use health endpoint right after nodes starts and can get comprehensive information about node health status.
Currently a "global" node health status is maintained and as we already had it endpoints gives updates on rln_relay state.

Changes

  • App initialization sequence by devide rest setup into two. Health endpoint initialized separately from node creation.
  • A sovereign WakuNodeHealthMonitor is added that can know about global state also about protocol states. Later can be extended.
  • /health endpoint now returns a structured health report

How to test

  1. start a node
  2. issue curl -X http://localhost:8645/health
    2/b. run ./script/chkhealth.sh (located in nwaku repositroy)

Issue

#2020

…tus. Make (only) health endpoint avail early and install others in the last stage of node setup.
@NagyZoltanPeter
Copy link
Contributor Author

NagyZoltanPeter commented Apr 22, 2024

  • Just about to add json structured health endpoint result.
  • Add a little helper script to check node health easy and properly.
  • Fix and enable /health endpoint test

Copy link

github-actions bot commented Apr 22, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2612-rln-v2-true

Built from 51b775e

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Approving not to be a blocker but I have the feeling we may need some further refactoring

Comment on lines +349 to +368
## Health REST API
installHealthApiHandler(server.router, nodeHealthMonitor)

restServerNotInstalledTab["admin"] =
"/admin endpoints are not available while initializing."
restServerNotInstalledTab["debug"] =
"/debug endpoints are not available while initializing."
restServerNotInstalledTab["relay"] =
"/relay endpoints are not available while initializing."
restServerNotInstalledTab["filter"] =
"/filter endpoints are not available while initializing."
restServerNotInstalledTab["lightpush"] =
"/lightpush endpoints are not available while initializing."
restServerNotInstalledTab["store"] =
"/store endpoints are not available while initializing."

server.start()
info "Starting REST HTTP server", url = "http://" & $address & ":" & $port & "/"

ok(some(server))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This block should fall under rest folder

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 agree, this past... Among with others... Needs refactoring. Here I wanted to keep changes minimum at once.

waku/node/health_monitor.nim Show resolved Hide resolved
apps/wakunode2/wakunode2.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Superrr! Thanks so much! 🤩

Adding some small questions, but not blockers

waku/waku_api/rest/health/handlers.nim Outdated Show resolved Hide resolved
waku/node/health_monitor.nim Show resolved Hide resolved
waku/factory/app.nim Show resolved Hide resolved
@Ivansete-status Ivansete-status marked this pull request as ready for review April 22, 2024 15:41
@SionoiS
Copy link
Contributor

SionoiS commented Apr 22, 2024

What do you think of the idea that each protocol should report their own "health"?

I envision each protocol/sub-system produce a report in a standard format to the rest server when asked.

Also to me the concept of node health does not make sense. A node is just a collection of protocols. If my node is un/healthy what does that mean?

My 0.02$

Copy link

github-actions bot commented Apr 23, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2612-rln-v2

Built from 3ef1d2d

Copy link

github-actions bot commented Apr 23, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2612-rln-v1

Built from 3ef1d2d

@NagyZoltanPeter
Copy link
Contributor Author

What do you think of the idea that each protocol should report their own "health"?

Yes, that what I think too, but nim OOP is a bit dumb and does not provide proper polymorphism.
But at the end it is the protocol instance to know its own state. WakuNodeHealthMonitor is to know about them and how to ask them.

I envision each protocol/sub-system produce a report in a standard format to the rest server when asked.

Note: WakuNodeHealthMonitor is in charge.

Also to me the concept of node health does not make sense. A node is just a collection of protocols. If my node is un/healthy what does that mean?

Well it does have meaning. During startup we faced with health report derived from (and only from) RlnRelay which does not have a constant state as it is time to time synchronizing. So I found it better to keep an overall state for the configured node.

Please notice: This PR is about the enhance the existing /health in two ways only (while keeping changes minimal as possible):

  • start rest server with /health endpoint enabled early.
  • Do not rely only on RlnRelay state.

My 0.02$

@SionoiS
Copy link
Contributor

SionoiS commented Apr 23, 2024

Please notice: This PR is about the enhance the existing /health in two ways only (while keeping changes minimal as possible):

Sorry, my comment was meant as next step discussion. I agree with the changes here 💯

@NagyZoltanPeter NagyZoltanPeter merged commit 6d135b0 into master Apr 23, 2024
13 checks passed
@NagyZoltanPeter NagyZoltanPeter deleted the chore-reorg-rest-server-init branch April 23, 2024 16:53
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