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.

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

Don't flock snapshot files #17199

Merged
merged 1 commit into from
Jan 8, 2024
Merged

Don't flock snapshot files #17199

merged 1 commit into from
Jan 8, 2024

Conversation

serathius
Copy link
Member

@serathius serathius commented Jan 4, 2024

Ref #17194

Flocking to snapshot files was added unintentionally in #1927.
Even at this time PurgeFile function was used for both wal and snapshot files

etcd/etcdserver/server.go

Lines 323 to 339 in 502396e

func (s *EtcdServer) purgeFile() {
var serrc, werrc <-chan error
if s.cfg.MaxSnapFiles > 0 {
serrc = fileutil.PurgeFile(s.cfg.SnapDir(), "snap", s.cfg.MaxSnapFiles, purgeFileInterval, s.done)
}
if s.cfg.MaxWALFiles > 0 {
werrc = fileutil.PurgeFile(s.cfg.WALDir(), "wal", s.cfg.MaxWALFiles, purgeFileInterval, s.done)
}
select {
case e := <-werrc:
log.Fatalf("etcdserver: failed to purge wal file %v", e)
case e := <-serrc:
log.Fatalf("etcdserver: failed to purge snap file %v", e)
case <-s.done:
return
}
}

Did minimal changes to avoid flocking files. Don't want to refactor it now to allow easy backport to older versions.

cc @ahrtr

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@fuweid
Copy link
Member

fuweid commented Jan 5, 2024

Not sure I understand correctly, if that server recovered from snapshot db, server will flock it by bbolt. If purge goroutine doesn't try lock, it will delete using data.

UPDATE: Before open backend, the file will be renamed to db.

@serathius
Copy link
Member Author

/retest

@serathius
Copy link
Member Author

ping @ahrtr

@serathius serathius merged commit a2eb17c into etcd-io:main Jan 8, 2024
40 checks passed
@ahrtr ahrtr mentioned this pull request Jan 8, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants