fix(defrag): close temp file in case of error #18851
Conversation
|
Hi @ghouscht. 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-sigs/prow repository. |
|
/assign @ahrtr Can you have a look at this please? I'm assigning you because you started the discussion in the other PR. As written in the description this single line change is enough to ensure the temp file is always closed. This error branch was the only one that left open the file. |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files
... and 28 files with indirect coverage changes @@ Coverage Diff @@
## main #18851 +/- ##
==========================================
+ Coverage 68.76% 68.87% +0.11%
==========================================
Files 420 420
Lines 35524 35531 +7
==========================================
+ Hits 24427 24471 +44
+ Misses 9671 9636 -35
+ Partials 1426 1424 -2 Continue to review full report in Codecov by Sentry.
|
| @@ -499,6 +499,7 @@ func (b *backend) defrag() error { | |||
| tdbp := temp.Name() | |||
| tmpdb, err := bolt.Open(tdbp, 0600, &options) | |||
| if err != nil { | |||
| temp.Close() | |||
There was a problem hiding this comment.
we also need to remove the temp file, refer to below
etcd/server/storage/backend/backend.go
Line 526 in 4ad9261
There was a problem hiding this comment.
Is this really needed? etcd anyway removes the db.tmp.* files at startup.
There was a problem hiding this comment.
It's the tmp file instead of db.tmp.*
EDIT: the tmp file locates under b.db.Path(). It would be better to cleanup it sooner
There was a problem hiding this comment.
So should I simply add a os.Remove(temp.Name()) after the call to Close?
There was a problem hiding this comment.
Yes, I think so. Please also log an error message if the removing fails
There was a problem hiding this comment.
I added something, let me know what you think. Is there also a need to write a unit test for this?
In my opinion the error log doesn't really provide any value to an etcd user so personally I would omit it. As a user I do not really care if there is an old temp file lying around (that will be deleted on next etcd restart anyway) that is why I think it is not needed. However, I'm fine with adding it if you think it is needed 🙂
There was a problem hiding this comment.
Is it possible to use defer here for cleanup? I saw the line 525 does the same thing. If there are multiple places to handle, maybe we can consider to use defer here.
There was a problem hiding this comment.
Is it possible to use defer here for cleanup?
We don't need to close & remove it in normal case. The tmp will be renamed to the bbolt db file.
In my opinion the error log doesn't really provide any value to an etcd user so personally I would omit it.
It's for debug purpose. It should be very rare. Overall not a big problem, and doesn't deserve too much time to debate on this.
There was a problem hiding this comment.
We don't need to close & remove it in normal case. The tmp will be renamed to the bbolt db file.
I mean,
defer func() {
if returnErr != nil {
dosomething
}
}()
It's just an option.
Signed-off-by: Thomas Gosteli <thomas.gosteli@protonmail.ch>
|
/ok-to-list |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, fuweid, ghouscht The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
could you backport this PR to 3.5 and 3.4? |
See issue #18841 and the discussion #18822 (comment) for details.
As written in the issue the temp file is closed if the defragmentation works as expected. The only case where it is left open is when the
bolt.Opencall fails. I think it is enough to simply include the close there, in my opinion there is no need to complicate this more.Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.