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

go: Add key manager worker status to node status #4883

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

peternose
Copy link
Contributor

Task
Currently the key manager worker does not report any status as part of the GetStatus node control gRPC API call. It should provide some useful information about the operation of the key manager node.

Test
Tested locally using default fixture, simple-keymanager and minimal-runtime:

  • Key manager status is shown only for keymanager nodes and not for compute, client and validator nodes.
  • Status changes to ready once consensus is synced and key manager is registered.
  • Client runtimes and access list populates with a short delay.
  • Private peers are shown if we set them using the worker.keymanager.private_peer_pub_keys flag.

@peternose peternose force-pushed the peternose/feature/keymanager-worker-status branch from 2aa1fa4 to 1fd5d63 Compare August 8, 2022 07:52
@peternose peternose changed the title go: Add key manager worker status to node control go: Add key manager worker status to node status Aug 8, 2022
@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #4883 (fdaa307) into master (d0bb246) will increase coverage by 0.01%.
The diff coverage is 68.18%.

❗ Current head fdaa307 differs from pull request most recent head 8fc6860. Consider uploading reports for the commit 8fc6860 to get more accurate results

@@            Coverage Diff             @@
##           master    #4883      +/-   ##
==========================================
+ Coverage   66.54%   66.55%   +0.01%     
==========================================
  Files         452      454       +2     
  Lines       50418    50524     +106     
==========================================
+ Hits        33551    33627      +76     
- Misses      12705    12726      +21     
- Partials     4162     4171       +9     
Impacted Files Coverage Δ
go/worker/keymanager/api/api.go 32.43% <32.43%> (ø)
go/control/control.go 71.42% <50.00%> (-1.08%) ⬇️
go/worker/keymanager/status.go 82.60% <82.60%> (ø)
go/oasis-node/cmd/node/control.go 65.45% <100.00%> (+1.30%) ⬆️
go/worker/keymanager/init.go 61.53% <100.00%> (+0.60%) ⬆️
go/worker/keymanager/worker.go 64.94% <100.00%> (+1.08%) ⬆️
go/common/sgx/common.go 66.01% <0.00%> (-3.89%) ⬇️
go/worker/common/committee/keymanager.go 88.33% <0.00%> (-3.34%) ⬇️
go/runtime/registry/host.go 69.38% <0.00%> (-2.86%) ⬇️
go/worker/keymanager/p2p/client.go 80.95% <0.00%> (-2.39%) ⬇️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@peternose peternose force-pushed the peternose/feature/keymanager-worker-status branch from 1fd5d63 to 643cf77 Compare August 8, 2022 07:54
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

One minor thing, otherwise looks good.

go/worker/keymanager/api/api.go Outdated Show resolved Hide resolved
@kostko
Copy link
Member

kostko commented Aug 9, 2022

Also do squash the two commits as the second one actually fixes an issue introduced by the first one :)

@peternose peternose force-pushed the peternose/feature/keymanager-worker-status branch from fdaa307 to 8fc6860 Compare August 10, 2022 07:33
@peternose peternose requested a review from kostko August 10, 2022 08:01
@peternose peternose merged commit 3fa5096 into master Aug 11, 2022
@peternose peternose deleted the peternose/feature/keymanager-worker-status branch August 11, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants