ui, server: prevent decommissioned nodes from displaying in ui as live #86252
Conversation
|
This change is |
Santamaura
changed the title
Santamaura
force-pushed
the
retired-nodes-ui-fix
branch
from
77f60e1
to
043e16a
Compare
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Santamaura and @zachlite)
pkg/server/admin.go line 2055 at r1 (raw file):
"known to gossip."
nit. Can you update comment to reflect current changes?
pkg/server/admin.go line 2066 at r1 (raw file):
return nil, serverError(ctx, err) } livenesses, err := s.server.nodeLiveness.GetLivenessesFromKV(ctx)
We call GetLivenessesFromKV function twice (one time in getLivenessStatusMap func and also here). My concern that now it does hit KV layer instead of cached in-memory values. Can it be refactored to call this function once (Probably introduce auxiliary function that returns both liveness and livenessStatusMap)?
This change resolves an issue where decommissioned nodes would in rare cases display as live in the admin ui. The changes are on both the frontend and backend: - The ui has been changed so that the way live nodes are filtered is now based on MembershipStatus which matches the way decommissioned nodes are filtered. - The server now uses GetLivenessesFromKV instead of GetLivenesses because the latter retrieves livenesses known to gossip which could be stale reads from in-memory cache while the former is read directly from the KV layer. Release justification: low risk, high benefit changes to existing functionality. Release note (ui change, api change): Filter live nodes based on MembershipStatus and retrieve livenesses directly from kv.
Santamaura
force-pushed
the
retired-nodes-ui-fix
branch
from
043e16a
to
6170dee
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/server/admin.go line 2055 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
"known to gossip."
nit. Can you update comment to reflect current changes?
Done
pkg/server/admin.go line 2066 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
We call
GetLivenessesFromKVfunction twice (one time ingetLivenessStatusMapfunc and also here). My concern that now it does hit KV layer instead of cached in-memory values. Can it be refactored to call this function once (Probably introduce auxiliary function that returns both liveness and livenessStatusMap)?
Done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
bors r+ |
|
bors cancel |
|
Canceled. |
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
bors retry |
|
Already running a review |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed: |
|
bors retry |
|
Build succeeded: |
This change resolves an issue where decommissioned nodes would
in rare cases display as live in the admin ui. The changes
are on both the frontend and backend:
filtered is now based on MembershipStatus which matches
the way decommissioned nodes are filtered.
GetLivenesses because the latter retrieves livenesses
known to gossip which could be stale reads from in-memory cache
while the former is read directly from the KV layer.
Part of #85445
Release justification: low risk, high benefit changes to existing functionality.
Release note (ui change, api change): Filter live nodes based on
MembershipStatus and retrieve livenesses directly from kv.