Show keeper sync type in status #636
Conversation
dyson
force-pushed
the
dyson-status-show-keeper-sync-type
branch
from
e23fbe2 to
959fb1c
Compare
|
@dyson Thanks for the PR! In #628 was also added support for json output. To support both your changes should be firstly added to the struct here: https://github.com/sorintlab/stolon/pull/628/files#diff-ca7581917eb32f79f0111a1e9f3217e9R66 /cc @hmac |
dyson
force-pushed
the
dyson-status-show-keeper-sync-type
branch
2 times, most recently
from
1d0194c to
3762f25
Compare
|
Thanks @sgotti for looking at this! I've updated the PR to also support adding the role to the JSON formatted output. |
dyson
force-pushed
the
dyson-status-show-keeper-sync-type
branch
from
3762f25 to
4a8f33c
Compare
dyson
force-pushed
the
dyson-status-show-keeper-sync-type
branch
from
b55f52a to
ba55e97
Compare
| keepers := make([]KeeperStatus, 0) | ||
| kssKeys := cd.Keepers.SortedKeys() | ||
| keeperRoles := getKeeperRoles(status.Cluster.MasterDBUID, cd) |
There was a problem hiding this comment.
status.Cluster.MasterDBUID could be empty making getKeeperRoles panicking when doing db := cd.DBs[dbuid]
cmd/stolonctl/cmd/status.go
Outdated
| keeperRoles[dbuid] = "async" | ||
| } | ||
| db := cd.DBs[dbuid] | ||
| followers := db.Spec.Followers |
There was a problem hiding this comment.
Instead of making a recursive function can you just iterate over all dbs and use their followConfig to detect the parent?
cmd/stolonctl/cmd/status.go
Outdated
| keeperRoles[dbuid] = "external sync" | ||
| } else { | ||
| keeperRoles[dbuid] = "async" | ||
| } |
There was a problem hiding this comment.
There can be also a case where a keeper is not set to be a standby (when exceeding MaxStandbys) si it should be noted as "none"
cmd/stolonctl/cmd/status.go
Outdated
| for _, kuid := range kssKeys { | ||
| k := cd.Keepers[kuid] | ||
| db := cd.FindDB(k) | ||
| dbListenAddress := "(no db assigned)" | ||
| role := "" |
There was a problem hiding this comment.
Better to set it to something like none instead of an empty value
There was a problem hiding this comment.
@sgotti, would setting the none value to no role assigned be acceptable? The reason of proposing this is that I found it a bit confusing displaying (none) next to the keeper. This is how it looks like in both cases:
With none:
...
===== Keepers/DB tree =====
keeper0 (master)
keeper1 (sync)
keeper2 (none)
With no role assigned:
...
===== Keepers/DB tree =====
keeper0 (master)
keeper1 (sync)
keeper2 (no role assigned)
Which do you think works better?
|
@sgotti no problem at all! Thanks for the review. We'll work through these and get it in shape. Cheers! |
This commit fixes a couple of panicking bugs when empty or nil variables are accessed without protection in some corner cases, like having an empty `status.Cluster.MasterDBUID`, or having an empty `dbuid` passed to the `getKeeperRoles()` function. It also displayes more readable information in case a node has no role in the cluster yet instead of displaying an empty string. This takes into consideration whether information is being displayed in the default format, or in the JSON format.
Adds the type of sync a keeper is to the stolonctl status output which can be really useful.
Currently we are doing this through passing the cluster data through jq which isn't ideal.