Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(defrag): close temp file in case of error #18851

Merged
merged 1 commit into from
Nov 7, 2024
+9 −0
Merged

fix(defrag): close temp file in case of error #18851

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions server/storage/backend/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,15 @@ func (b *backend) defrag() error {
tdbp := temp.Name()
tmpdb, err := bolt.Open(tdbp, 0600, &options)
if err != nil {
temp.Close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also need to remove the temp file, refer to below

etcd/server/storage/backend/backend.go

Line 526 in 4ad9261

if rmErr := os.RemoveAll(tmpdb.Path()); rmErr != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed? etcd anyway removes the db.tmp.* files at startup.

Copy link
Member

@ahrtr ahrtr Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I simply add a os.Remove(temp.Name()) after the call to Close?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so. Please also log an error message if the removing fails

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err := os.Remove(temp.Name()); err != nil && b.lg != nil {
b.lg.Error(
"failed to remove temporary file",
zap.String("path", temp.Name()),
zap.Error(err),
)
}

return err
}

Expand Down