-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Fix offline defrag in etcdctl #13792
Conversation
You have already submitted another PR pull/13793 to remove the code, then why do you submit this PR? |
The PR commit to release-3.5. in release 3.5, it's still 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.
LGTM
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.
cc @serathius
@@ -46,6 +46,7 @@ func defragCommandFunc(cmd *cobra.Command, args []string) { | |||
if err != nil { | |||
cobrautl.ExitWithError(cobrautl.ExitError, err) | |||
} | |||
return |
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.
Why this was not detected by e2e tests? Even with etcdctl defrag --data-dir
removed, I think we should still double check that we have tests that cover etcdutl defrag
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.
In test, it's just check
etcd/tests/e2e/ctl_v3_defrag_test.go
Line 61 in 35c3cea
lines := []string{"finished defragmenting directory"} |
If not return, etcdctl still connect to endpoint and print error log. but e2e test will pass.
./etcdctl defrag --data-dir default.etcd/
Use etcdutl defrag
instead. The --data-dir is going to be decomissioned in v3.6.
2022-03-16T21:43:07+08:00 info backend/backend.go:482 defragmenting {"path": "default.etcd/member/snap/db", "current-db-size-bytes": 20480, "current-db-size": "20 kB", "current-db-size-in-use-bytes": 12288, "current-db-size-in-use": "12 kB"}
2022-03-16T21:43:07+08:00 info backend/backend.go:534 finished defragmenting directory {"path": "default.etcd/member/snap/db", "current-db-size-bytes-diff": 0, "current-db-size-bytes": 20480, "current-db-size": "20 kB", "current-db-size-in-use-bytes-diff": 0, "current-db-size-in-use-bytes": 12288, "current-db-size-in-use": "12 kB", "took": 0.009815448}
{"level":"warn","ts":"2022-03-16T21:43:12.776+0800","logger":"etcd-client","caller":"v3/retry_interceptor.go:64","msg":"retrying of unary invoker failed","target":"etcd-endpoints://0xc00000a1e0/127.0.0.1:2379","method":"/etcdserverpb.Maintenance/Defragment","attempt":0,"error":"rpc error: code = DeadlineExceeded desc = latest balancer error: last connection error: connection error: desc = "transport: Error while dialing dial tcp 127.0.0.1:2379: connect: connection refused""}
Failed to defragment etcd member[127.0.0.1:2379]. took 5.000801871s. (context deadline exceeded)
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.
ok, did the command fail (return non-zero error code)? Do our e2e tests allow commands to fail?
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.
ok, did the command fail (return non-zero error code)?
command will failed..(return error code: 1)
os.Exit(cobrautl.ExitError) |
Do our e2e tests allow commands to fail?
e2e test will pass.
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.
Ok, I think that e2e expect should return error if the called command failed when it was not expected to do so. Will open issue for that.
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
It should return in etcdctl when offline defrag.
cc @ptabor @spzala @serathius @ahrtr