From 73b6c2327137592e4f6b2cff4959e830fca3fc79 Mon Sep 17 00:00:00 2001 From: Nikolay Date: Wed, 3 May 2023 10:47:02 +0200 Subject: [PATCH] lib/fs: do not panic at windows at dir deletion (#4132) Windows doesn't allow to remove dir with opened files. Usually it's a case for snapshots, hard cannot be removed if file is openned. With this change, dir will be renamed and properly deleted at the next process start. It's recommended to restart vmstorage/vmsingle for snapshots deletion completion periodically. https://github.com/VictoriaMetrics/VictoriaMetrics/issues/70 --- docs/CHANGELOG.md | 1 + lib/fs/fs.go | 12 +----------- lib/fs/fs_nix.go | 20 ++++++++++++++++++++ lib/fs/fs_openbsd.go | 20 ++++++++++++++++++++ lib/fs/fs_solaris.go | 16 ++++++++++++++++ lib/fs/fs_windows.go | 16 ++++++++++++++++ 6 files changed, 74 insertions(+), 11 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 6e3601a5c..0e5125cc8 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -42,6 +42,7 @@ The following tip changes can be tested by building VictoriaMetrics components f * BUGFIX: reduce the probability of sudden increase in the number of small parts on systems with small number of CPU cores. * BUGFIX: [vmctl](https://docs.victoriametrics.com/vmctl.html): fix performance issue when migrating data from VictoriaMetrics according to [these docs](https://docs.victoriametrics.com/vmctl.html#migrating-data-from-victoriametrics). Add the ability to speed up the data migration via `--vm-native-disable-retries` command-line flag. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4092). +* BUGFIX: do not panic at windows during snapshot deletion. Note, at windows process restart required for complete snapshot delete. See [this comment](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/70#issuecomment-1491529183) for details. * BUGFIX: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): fix a panic when the duration in the query contains uppercase `M` suffix. Such a suffix isn't allowed to use in durations, since it clashes with `a million` suffix, e.g. it isn't clear whether `rate(metric[5M])` means rate over 5 minutes, 5 months or 5 million seconds. See [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3589) and [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4120) issues. * BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert.html): retry failed read request on the closed connection one more time. This improves rules execution reliability when connection between vmalert and datasource closes unexpectedly. * BUGFIX: [vmui](https://docs.victoriametrics.com/#vmui): fix the display of the tenant selector. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4160). diff --git a/lib/fs/fs.go b/lib/fs/fs.go index 81dbf920f..2392b6178 100644 --- a/lib/fs/fs.go +++ b/lib/fs/fs.go @@ -219,17 +219,7 @@ func IsEmptyDir(path string) bool { // If the process crashes after the step 1, then the directory must be removed // on the next process start by calling MustRemoveTemporaryDirs on the parent directory. func MustRemoveDirAtomic(dir string) { - if !IsPathExist(dir) { - return - } - n := atomic.AddUint64(&atomicDirRemoveCounter, 1) - tmpDir := fmt.Sprintf("%s.must-remove.%d", dir, n) - if err := os.Rename(dir, tmpDir); err != nil { - logger.Panicf("FATAL: cannot move %s to %s: %s", dir, tmpDir, err) - } - MustRemoveAll(tmpDir) - parentDir := filepath.Dir(dir) - MustSyncPath(parentDir) + mustRemoveDirAtomic(dir) } var atomicDirRemoveCounter = uint64(time.Now().UnixNano()) diff --git a/lib/fs/fs_nix.go b/lib/fs/fs_nix.go index 96d37d77f..54dc570c4 100644 --- a/lib/fs/fs_nix.go +++ b/lib/fs/fs_nix.go @@ -4,9 +4,29 @@ package fs import ( + "fmt" + "os" + "path/filepath" + "sync/atomic" + + "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" "golang.org/x/sys/unix" ) func freeSpace(stat unix.Statfs_t) uint64 { return uint64(stat.Bavail) * uint64(stat.Bsize) } + +func mustRemoveDirAtomic(dir string) { + if !IsPathExist(dir) { + return + } + n := atomic.AddUint64(&atomicDirRemoveCounter, 1) + tmpDir := fmt.Sprintf("%s.must-remove.%d", dir, n) + if err := os.Rename(dir, tmpDir); err != nil { + logger.Panicf("FATAL: cannot move %s to %s: %s", dir, tmpDir, err) + } + MustRemoveAll(tmpDir) + parentDir := filepath.Dir(dir) + MustSyncPath(parentDir) +} diff --git a/lib/fs/fs_openbsd.go b/lib/fs/fs_openbsd.go index 1ab4b9358..27e3f9dac 100644 --- a/lib/fs/fs_openbsd.go +++ b/lib/fs/fs_openbsd.go @@ -1,9 +1,29 @@ package fs import ( + "fmt" + "os" + "path/filepath" + "sync/atomic" + + "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" "golang.org/x/sys/unix" ) func freeSpace(stat unix.Statfs_t) uint64 { return uint64(stat.F_bavail) * uint64(stat.F_bsize) } + +func mustRemoveDirAtomic(dir string) { + if !IsPathExist(dir) { + return + } + n := atomic.AddUint64(&atomicDirRemoveCounter, 1) + tmpDir := fmt.Sprintf("%s.must-remove.%d", dir, n) + if err := os.Rename(dir, tmpDir); err != nil { + logger.Panicf("FATAL: cannot move %s to %s: %s", dir, tmpDir, err) + } + MustRemoveAll(tmpDir) + parentDir := filepath.Dir(dir) + MustSyncPath(parentDir) +} diff --git a/lib/fs/fs_solaris.go b/lib/fs/fs_solaris.go index 385903896..c5cb04ec1 100644 --- a/lib/fs/fs_solaris.go +++ b/lib/fs/fs_solaris.go @@ -3,11 +3,27 @@ package fs import ( "fmt" "os" + "path/filepath" + "sync/atomic" "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" "golang.org/x/sys/unix" ) +func mustRemoveDirAtomic(dir string) { + if !IsPathExist(dir) { + return + } + n := atomic.AddUint64(&atomicDirRemoveCounter, 1) + tmpDir := fmt.Sprintf("%s.must-remove.%d", dir, n) + if err := os.Rename(dir, tmpDir); err != nil { + logger.Panicf("FATAL: cannot move %s to %s: %s", dir, tmpDir, err) + } + MustRemoveAll(tmpDir) + parentDir := filepath.Dir(dir) + MustSyncPath(parentDir) +} + func mmap(fd int, length int) (data []byte, err error) { return unix.Mmap(fd, 0, length, unix.PROT_READ, unix.MAP_SHARED) diff --git a/lib/fs/fs_windows.go b/lib/fs/fs_windows.go index fff0783c7..949829e9b 100644 --- a/lib/fs/fs_windows.go +++ b/lib/fs/fs_windows.go @@ -5,6 +5,7 @@ import ( "os" "reflect" "sync" + "sync/atomic" "unsafe" "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" @@ -23,6 +24,21 @@ var ( func mustSyncPath(path string) { } +func mustRemoveDirAtomic(dir string) { + if !IsPathExist(dir) { + return + } + n := atomic.AddUint64(&atomicDirRemoveCounter, 1) + tmpDir := fmt.Sprintf("%s.must-remove.%d", dir, n) + if err := os.Rename(dir, tmpDir); err != nil { + logger.Panicf("FATAL: cannot move %s to %s: %s", dir, tmpDir, err) + } + err := os.RemoveAll(tmpDir) + if err != nil { + logger.Warnf("cannot remove dir: %q: %s, restart VictoriaMetrics process to complete file deletion", tmpDir, err) + } +} + const ( lockfileExclusiveLock = 2 fileFlagNormal = 0x00000080