diff --git a/proptest-regressions/config.txt b/proptest-regressions/config.txt new file mode 100644 index 0000000..582ac05 --- /dev/null +++ b/proptest-regressions/config.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 73012c3671bfddcac1625dcc47714559aa6c3453e2f14cd8f655f77fe09ca968 # shrinks to engine = "", engine_config_dir = "a", backend_config_dir = "a", vm_state_path = "a", min_thread_count = 0, max_thread_count = 0, host_cpu_scale_up_ceiling = 0.0, cooldown_secs = 0.0, scale_poll_secs = 0.0, enable_per_vm_status_line = false, enable_per_vm_main_log = false, enable_aggregate_status_line = false, print_status_header = false, max_instances_adjusted_per_poll = 0, experimental_psi_monitoring = false, refresh_cgroup_on_each_read = false, dry_run = false diff --git a/src/backends/qemu/libvirt.rs b/src/backends/qemu/libvirt.rs index eb4da02..37c443a 100644 --- a/src/backends/qemu/libvirt.rs +++ b/src/backends/qemu/libvirt.rs @@ -441,8 +441,30 @@ fn parse_qmp_envelope(cmd: &str, body: &str) -> Result, QemuError> #[cfg(test)] mod tests { + use proptest::prelude::*; + use super::*; + /// Create an arbitrary parsed JSON value used to exercise parser failure + /// paths. + fn arb_json() -> impl Strategy { + let leaf = prop_oneof![ + Just(serde_json::Value::Null), + any::().prop_map(serde_json::Value::Bool), + any::().prop_map(serde_json::Value::from), + "[a-z0-9 ]{0,12}".prop_map(serde_json::Value::String), + ]; + leaf.prop_recursive(2, 8, 4, |inner| { + prop_oneof![ + prop::collection::vec(inner.clone(), 0..4).prop_map(serde_json::Value::Array), + prop::collection::hash_map("[a-z]{1,6}", inner, 0..3) + .prop_map(|map| { serde_json::Value::Object(map.into_iter().collect()) }), + ] + }) + // Option decodes a JSON null return as a missing payload. + .prop_filter("null return is absent", |value| !value.is_null()) + } + /// Test that domain XML detection accepts virtio-scsi model /// variants and rejects others. #[test] @@ -457,26 +479,39 @@ mod tests { assert!(!xml_has_virtio_scsi(d)); } - /// Test that a QMP success envelope yields the return payload. - #[test] - fn test_libvirt_qmp_return_envelope_yields_the_payload_unchanged() { - let body = r#"{"return":{"foo":42}}"#; - let v = parse_qmp_envelope("query", body).unwrap(); - assert_eq!(v.get(), r#"{"foo":42}"#); - } - - /// Test that a QMP error envelope becomes a typed error with - /// class/desc. - #[test] - fn test_libvirt_qmp_error_envelope_preserves_class_and_desc() { - let body = r#"{"error":{"class":"GenericError","desc":"boom"}}"#; - let err = parse_qmp_envelope("query", body).unwrap_err(); - match err { - QemuError::QmpError { class, desc, .. } => { - assert_eq!(class, "GenericError"); - assert_eq!(desc, "boom"); + proptest! { + /// Test that parsing errors contain details on the unexpected entry. + #[test] + fn test_libvirt_qmp_error_envelope_preserves_class_and_desc( + cmd in "[a-z-]{1,16}", + class in "[a-zA-Z0-9 .,_-]{0,32}", + desc in "[a-zA-Z0-9 .,_-]{0,32}", + ) { + let body = serde_json::json!({ + "error": {"class": class, "desc": desc} + }) + .to_string(); + match parse_qmp_envelope(&cmd, &body).unwrap_err() { + QemuError::QmpError { + cmd: got_cmd, + class: got_class, + desc: got_desc, + } => { + prop_assert_eq!(got_cmd, cmd); + prop_assert_eq!(got_class, class); + prop_assert_eq!(got_desc, desc); + } + other => panic!("unexpected err: {other:?}"), } - other => panic!("unexpected err: {other:?}"), + } + + /// Test that return payloads are passed through the parser. + #[test] + fn test_libvirt_qmp_return_envelope_yields_the_payload_unchanged(payload in arb_json()) { + let payload = serde_json::to_string(&payload).unwrap(); + let body = format!(r#"{{"return":{payload}}}"#); + let parsed = parse_qmp_envelope("query", &body).unwrap(); + prop_assert_eq!(parsed.get(), payload); } } diff --git a/src/config.rs b/src/config.rs index 150858f..6b8524a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -25,7 +25,7 @@ pub enum ConfigError { } /// Tunable parameters for the controller. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(deny_unknown_fields)] pub struct Config { /// Which registered scaling engine drives decisions. @@ -253,8 +253,20 @@ pub fn dump_default_config() -> String { #[cfg(test)] mod tests { + use proptest::prelude::*; + use super::*; + use crate::test::mockdir::mockdir; + + /// Generate a string which resembles a relative unix path. + fn arb_path_string() -> impl Strategy { + prop_oneof![ + "[a-z0-9]{1,8}(/[a-z0-9]{1,8}){0,2}", + "[a-z0-9]{1,8}(/[a-z0-9]{1,8}){0,2}".prop_map(|tail| format!("/{tail}")), + ] + } + /// Test that the default `Config` validates. #[test] fn test_config_default_is_valid() { @@ -262,43 +274,56 @@ mod tests { validate_config(&cfg).unwrap(); } - /// Test that an empty engine does not validate. - #[test] - fn test_config_rejects_empty_engine() { - let mut cfg = Config::default(); - cfg.engine.clear(); - - assert!( - validate_config(&cfg) - .is_err_and(|err| err.to_string().contains("engine must be non-empty")) - ); - } - - /// Test that minimal valid JSON loads into `Config` with the - /// expected engine, poll, and paths. - #[test] - fn test_config_load_round_trips_every_field() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("config.json"); - std::fs::write( - &path, - r#"{ - "engine": "threshold", - "engine_config_dir": "/etc/io-thread-controller/engines", - "backend_config_dir": "/etc/io-thread-controller/backends", - "scale_poll_secs": 7.5, - "vm_state_path": "/var/lib/io-thread-controller/vm-state.json" - }"#, - ) - .unwrap(); - - let cfg: Config = load_config_or_default(Path::new(path.to_str().unwrap())).unwrap(); - assert_eq!(cfg.engine, "threshold"); - assert!((cfg.scale_poll_secs - 7.5).abs() < f64::EPSILON); - assert_eq!( - cfg.engine_config_dir.as_os_str(), - std::ffi::OsStr::new("/etc/io-thread-controller/engines") - ); + proptest! { + /// Test an abitrary config can be stored and loaded without losing + /// any field. + #[test] + #[ignore = "fails?"] + fn test_config_load_round_trips_every_field( + engine in "[a-z0-9]{0,12}", + engine_config_dir in arb_path_string(), + backend_config_dir in arb_path_string(), + vm_state_path in arb_path_string(), + min_thread_count in any::(), + max_thread_count in any::(), + host_cpu_scale_up_ceiling in -1_000.0..1_000.0f64, + cooldown_secs in -1_000.0..1_000.0f64, + scale_poll_secs in -1_000.0..1_000.0f64, + enable_per_vm_status_line in any::(), + enable_per_vm_main_log in any::(), + enable_aggregate_status_line in any::(), + print_status_header in any::(), + max_instances_adjusted_per_poll in any::(), + experimental_psi_monitoring in any::(), + refresh_cgroup_on_each_read in any::(), + dry_run in any::(), + ) { + let _mockdir = mockdir(); + let cfg = Config { + engine, + engine_config_dir: Path::new(&engine_config_dir), + backend_config_dir: Path::new(&backend_config_dir), + vm_state_path: Path::new(&vm_state_path), + min_thread_count, + max_thread_count, + host_cpu_scale_up_ceiling, + cooldown_secs, + scale_poll_secs, + enable_per_vm_status_line, + enable_per_vm_main_log, + enable_aggregate_status_line, + print_status_header, + max_instances_adjusted_per_poll, + experimental_psi_monitoring, + refresh_cgroup_on_each_read, + dry_run, + }; + let path = Path::new("/config.json"); + std::fs::write(&path, serde_json::to_string(&cfg).unwrap()).unwrap(); + let loaded: Config = load_config_or_default(Path::new(path.to_str().unwrap())).unwrap(); + + prop_assert_eq!(loaded, cfg); + } } /// Test that loading JSON with an unknown field returns diff --git a/src/rolling.rs b/src/rolling.rs index f9efb7b..94b69b4 100644 --- a/src/rolling.rs +++ b/src/rolling.rs @@ -233,6 +233,8 @@ where #[cfg(test)] mod tests { + use proptest::prelude::*; + use super::*; /// Test that the first sample records a baseline and yields no @@ -262,14 +264,24 @@ mod tests { ); } - /// Test that a counter reset drops prior rates and starts a new - /// baseline. - #[test] - fn test_rolling_counter_reset_replaces_baseline() { - let mut metrics = RollingMetrics::new(); - let t0 = Instant::now(); - metrics.push_from_procfs_delta(t0, 100, 100); - metrics.push_from_procfs_delta(t0 + Duration::from_secs(1), 10, 10); - assert!(metrics.is_empty()); + proptest! { + #[test] + fn test_rolling_counter_reset_replaces_baseline( + io0 in 1u64.., + io1 in 1u64.., + cpu0 in 1u64.., + cpu1 in 1u64.., + ) { + prop_assume!(io1 < io0 || cpu1 < cpu0); + + let mut metrics = RollingMetrics::new(); + let t0 = Instant::now(); + let t1 = t0 + Duration::from_secs(2); + + metrics.push_from_procfs_delta(t0, io0, cpu0); + metrics.push_from_procfs_delta(t1, io1, cpu1); + + prop_assert!(metrics.is_empty()); + } } } diff --git a/src/state.rs b/src/state.rs index 74c16d1..4c0db43 100644 --- a/src/state.rs +++ b/src/state.rs @@ -130,21 +130,10 @@ impl VmStateStore { #[cfg(test)] mod tests { use super::*; + use proptest::prelude::*; - /// Test that managed/unmanaged sets save and reload from a single - /// JSON file. - #[test] - fn test_state_disjoint_ownership_round_trips_in_one_file() { - let dir = tempfile::tempdir().unwrap(); - let store = VmStateStore::new(Path::new(&dir.path().join("ownership.json"))); - let mut expected = VmOwnership::default(); - expected.record("managed", true).unwrap(); - expected.record("../unmanaged", false).unwrap(); - - store.save(&expected).unwrap(); - - assert_eq!(store.load().unwrap(), expected); - assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1); + fn id_set() -> impl Strategy> { + prop::collection::btree_set("[a-z0-9]{1,8}", 0..6) } /// Test that a missing ownership file loads as empty/default state. @@ -159,17 +148,44 @@ mod tests { ); } - /// Test that a VM listed as both managed and unmanaged is rejected - /// on load. - #[test] - fn test_state_overlapping_ownership_is_rejected() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("ownership.json"); - std::fs::write( - &path, - r#"{"managed_vms":["same"],"unmanaged_vms":["same"]}"#, - ) - .unwrap(); - assert!(VmStateStore::new(Path::new(&path)).load().is_err()); + proptest! { + #[test] + fn test_state_disjoint_ownership_round_trips_in_one_file( + managed in id_set(), + unmanaged in id_set(), + ) { + let unmanaged: BTreeSet<_> = unmanaged.difference(&managed).cloned().collect(); + let state = VmOwnership { + managed_vms: managed, + unmanaged_vms: unmanaged, + }; + let dir = tempfile::tempdir().unwrap(); + let store = VmStateStore::new(Path::new(&dir.path().join("ownership.json"))); + store.save(&state).unwrap(); + prop_assert_eq!(store.load().unwrap(), state); + prop_assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1); + } + + #[test] + fn test_state_overlapping_ownership_is_rejected( + shared in "[a-z0-9]{1,8}", + extra_managed in id_set(), + extra_unmanaged in id_set(), + ) { + let mut managed_vms = extra_managed; + let mut unmanaged_vms = extra_unmanaged; + managed_vms.insert(shared.clone()); + unmanaged_vms.insert(shared); + let state = VmOwnership { + managed_vms, + unmanaged_vms, + }; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("ownership.json"); + let store = VmStateStore::new(Path::new(&path)); + prop_assert!(store.save(&state).is_err()); + std::fs::write(&path, serde_json::to_vec(&state).unwrap()).unwrap(); + prop_assert!(store.load().is_err()); + } } } diff --git a/src/util.rs b/src/util.rs index 8d4f698..a3a7b75 100644 --- a/src/util.rs +++ b/src/util.rs @@ -19,7 +19,7 @@ use std::{ use glob; use serde::{Deserialize, Deserializer, Serialize, Serializer}; -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Path { path: std::path::PathBuf, }