-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Update printer.go for used/not-used ETCD storage percentage #17871
Conversation
Hi @Scotchman0. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Example output:
|
Hi @Scotchman0, thanks for your contribution. While we await a maintainer to review this suggestion, would you mind signing your commit? So the developer certificate of origin (DCO) check passes, i.e:
Ref: https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#commit-your-change |
Latest update:
|
etcdctl/ctlv3/command/printer.go
Outdated
@@ -211,7 +211,7 @@ func makeEndpointHealthTable(healthList []epHealth) (hdr []string, rows [][]stri | |||
} | |||
|
|||
func makeEndpointStatusTable(statusList []epStatus) (hdr []string, rows [][]string) { | |||
hdr = []string{"endpoint", "ID", "version", "storage version", "db size", "db size in use", "is leader", "is learner", "raft term", | |||
hdr = []string{"endpoint", "ID", "version", "storage version", "db size", "in use", "not used", "quota", "is leader", "is learner", "raft term", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "not in use"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed - agreed that's clearer
/ok-to-test |
etcdctl/ctlv3/command/printer.go
Outdated
@@ -221,6 +221,8 @@ func makeEndpointStatusTable(statusList []epStatus) (hdr []string, rows [][]stri | |||
status.Resp.StorageVersion, | |||
humanize.Bytes(uint64(status.Resp.DbSize)), | |||
humanize.Bytes(uint64(status.Resp.DbSizeInUse)), | |||
fmt.Sprint(100-(status.Resp.DbSizeInUse*100/status.Resp.DBSize)) + "%", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is better to keep this number format consistent with in use
one. They can be both byte size, or percentage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack - would the following format handling also succeed here? - pushed to check it but I can also revisit how I'm calculating the percentage with more of a robust option if it doesn't like this:
humanize.Bytes(uint64(100-(status.Resp.DBsizeInUse*100/status.Resp.DBSize))) + "%",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like no. 🤔 I'll explore some alternatives
/retest-required (oops wrong button) |
validated local build; removed additional percent symbol; confirmed output works the way I want it to (though I don't have a build running the version with the quota to check, I'll confirm that separately) latest should be where it needs to be pending another review.
|
Is it important to have |
I'd be inclined to change the name; The goal is to add an easy marker for identification of unused disk space which might help to illustrate why the cluster defrags often or whether or not a defrag is expected / could benefit performance. Mostly this is for easy visibility into this logic: https://github.com/openshift/cluster-etcd-operator/blob/cbfb856ec8892687a303989b84e01c8f34c1967e/pkg/operator/defragcontroller/defragcontroller.go#L181-L224 To be consistent, I think percentage handling is preferable. I've changed the name to Thanks! |
oof - I accidentally broke stuff while forcing DCO 🤦 1s I'll revert. **fixed - not sure how I pulled 64 commits in but latest should be correct 😅 |
re-syncing with main - @siyuanfoundation let me know if any additional changes are required; thanks! |
@Scotchman0 can you squash your commits? |
edb9ec3
to
77a8ffe
Compare
/test pull-etcd-unit-test |
Signed-off-by: Will Russell <will.russell.01@gmail.com>
also /lgtm (non-binding) from my side, thanks for the help @Scotchman0 :) |
@Scotchman0 can you please provide an example output using latest |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for your first contribution!
based on OMC's output for etcdctl endpoint health reporting, add output that includes how much storage is used/not in use and a percentage for better visualization of data within the database. Help with analysis for defrag rates and visual output.
https://github.com/gmeghnag/omc/blob/e97cc10586cefbf0c626ef43b61597730f2b82cf/cmd/etcd/etcdctl.go#L35-L50\
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.