mirror of
https://github.com/openobserve/openobserve.git
synced 2026-11-04 16:09:05 +00:00
fix(windows): strip \\?\ prefix from canonicalized paths before disk mount-point matching (#13045)
Fixes #13041. ## Root cause Two broken string comparisons, both caused by Windows path canonicalization: **1. `src/config/src/config.rs` — `check_disk_cache_config()`** `Path::canonicalize()` on Windows returns a verbatim extended-length path (`\\?\C:\OpenObserve\data\cache`), but `sysinfo` returns plain mount points (`C:\`). The `starts_with` check `"\\?\C:\...".starts_with("C:\\")` is always `false` → `disk_total = disk_free = 0` on every Windows deployment. **2. `src/job/stats.rs` — `update_node_disk_usage()`** The comparison was also reversed: `mount_point.starts_with(data_dir)` checks whether the mount point path begins with the (much longer) data directory path — `"C:\\".starts_with("C:\\OpenObserve\\data\\")` is always `false`. The correct direction is `data_dir.starts_with(mount_point)`. ## Consequences when disk sizes are 0 - Disk cache `max_size` is computed from near-zero `effective_free`, leaving `result_max_size = 0` - `disk.rs` GC panics at first tick: `max_size / result_max_size` evaluates before `max(1, …)` → divide by zero, GC task dies permanently - `check_disk_circuit_breaker()` always compares `0 > 0` → never trips → no self-protection against disk exhaustion on Windows ## Fix 1. **`config.rs`**: strip the `\\?\` prefix from the canonicalized cache dir string before the `starts_with` lookup against sysinfo mount points. 2. **`stats.rs`**: fix the reversed comparison (`mount_point.starts_with(data_dir)` → `data_dir.starts_with(mount_point)`) and apply the same `\\?\` prefix strip so canonicalized data dirs match correctly. Both fixes are no-ops on Linux/macOS where `canonicalize()` never produces the `\\?\` prefix. --------- Co-authored-by: Hengfei Yang <hengfei.yang@gmail.com>
This commit is contained in:
co-authored by
Hengfei Yang
parent
48b3f1f58a
commit
82a79e4741
@@ -3252,12 +3252,36 @@ fn check_memory_config(cfg: &mut Config) -> Result<(), anyhow::Error> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Strip the Windows extended-length prefix (`\\?\`) from a canonicalized path
|
||||
/// so it can be compared with sysinfo mount points that use the plain DOS form.
|
||||
///
|
||||
/// Uses [`std::path::Prefix`] to detect verbatim prefixes rather than
|
||||
/// manipulating the string directly, which would silently break on non-ASCII
|
||||
/// drive letters or UNC paths.
|
||||
pub fn deverbatim(path: &Path) -> std::borrow::Cow<'_, str> {
|
||||
#[cfg(windows)]
|
||||
{
|
||||
use std::path::{Component, Prefix};
|
||||
if let Some(Component::Prefix(p)) = path.components().next() {
|
||||
if let Prefix::VerbatimDisk(drive) = p.kind() {
|
||||
// \\?\C:\rest → C:\rest
|
||||
// p.as_os_str() is "\\?\C:" (6 bytes); the remainder of the
|
||||
// original string is "\rest", so prepend the plain drive letter.
|
||||
let after_prefix = &path.to_string_lossy()[p.as_os_str().len()..];
|
||||
return format!("{}:{}", drive as char, after_prefix).into();
|
||||
}
|
||||
}
|
||||
}
|
||||
path.to_string_lossy()
|
||||
}
|
||||
|
||||
fn check_disk_cache_config(cfg: &mut Config) -> Result<(), anyhow::Error> {
|
||||
std::fs::create_dir_all(&cfg.common.data_cache_dir).expect("create cache dir success");
|
||||
let cache_dir = Path::new(&cfg.common.data_cache_dir)
|
||||
let cache_dir_path = Path::new(&cfg.common.data_cache_dir)
|
||||
.canonicalize()
|
||||
.unwrap();
|
||||
let cache_dir = cache_dir.to_str().unwrap();
|
||||
let cache_dir_owned = deverbatim(&cache_dir_path).into_owned();
|
||||
let cache_dir = cache_dir_owned.as_str();
|
||||
|
||||
// disable disk cache for local disk storage
|
||||
if cfg.common.is_local_storage
|
||||
@@ -4333,4 +4357,34 @@ mod tests {
|
||||
let name = get_cluster_name();
|
||||
assert!(!name.is_empty(), "cluster name should not be empty");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deverbatim_plain_path_unchanged() {
|
||||
let p = std::path::Path::new("/data/openobserve");
|
||||
let result = deverbatim(p);
|
||||
assert_eq!(result, "/data/openobserve");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deverbatim_empty_path_unchanged() {
|
||||
let p = std::path::Path::new("");
|
||||
let result = deverbatim(p);
|
||||
assert_eq!(result, "");
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn test_deverbatim_verbatim_disk_stripped() {
|
||||
let p = std::path::Path::new(r"\\?\C:\data\openobserve");
|
||||
let result = deverbatim(p);
|
||||
assert_eq!(result, r"C:\data\openobserve");
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn test_deverbatim_plain_windows_path_unchanged() {
|
||||
let p = std::path::Path::new(r"C:\data\openobserve");
|
||||
let result = deverbatim(p);
|
||||
assert_eq!(result, r"C:\data\openobserve");
|
||||
}
|
||||
}
|
||||
|
||||
+10
-9
@@ -13,7 +13,7 @@
|
||||
// You should have received a copy of the GNU Affero General Public License
|
||||
// along with this program. If not, see <http://www.gnu.org/licenses/>.
|
||||
|
||||
use config::{cluster::LOCAL_NODE, get_config, spawn_pausable_job};
|
||||
use config::{cluster::LOCAL_NODE, deverbatim, get_config, spawn_pausable_job};
|
||||
use tokio::time;
|
||||
|
||||
use crate::service::{compact::stats::update_stats_from_file_list, db};
|
||||
@@ -112,21 +112,22 @@ async fn update_node_memory_usage() -> Result<(), anyhow::Error> {
|
||||
// update node disk usage metrics every 60 seconds
|
||||
async fn update_node_disk_usage() -> Result<(), anyhow::Error> {
|
||||
let cfg = get_config();
|
||||
let data_dir = std::path::Path::new(&cfg.common.data_dir);
|
||||
let data_dir_raw = std::path::Path::new(&cfg.common.data_dir)
|
||||
.canonicalize()
|
||||
.unwrap_or_else(|_| std::path::PathBuf::from(&cfg.common.data_dir));
|
||||
// Strip any Windows extended-length prefix (\\?\) returned by canonicalize
|
||||
// so comparisons with sysinfo mount points (plain DOS form) work correctly.
|
||||
let data_dir_norm = deverbatim(&data_dir_raw).into_owned();
|
||||
|
||||
loop {
|
||||
let disks = config::utils::sysinfo::disk::get_disk_usage();
|
||||
// Sum up all disks that contain subdirectories of the data directory
|
||||
// This handles cases where multiple disks are mounted at different subpaths
|
||||
// Sum up all disks whose mount point is a prefix of the data directory.
|
||||
// This finds the disk(s) that actually host the data directory.
|
||||
let mut total_space = 0_u64;
|
||||
let mut total_used = 0_u64;
|
||||
|
||||
for disk in disks {
|
||||
// Check if the disk mount point is under the data directory
|
||||
if disk
|
||||
.mount_point
|
||||
.starts_with(data_dir.to_string_lossy().as_ref())
|
||||
{
|
||||
if data_dir_norm.starts_with(disk.mount_point.as_str()) {
|
||||
total_space += disk.total_space;
|
||||
total_used += disk.total_space - disk.available_space;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user