Skip to content

feat(kvp): add store trait and Hyper-V KVP storage crate#288

Open
peytonr18 wants to merge 16 commits intoAzure:mainfrom
peytonr18:probertson/kvp-store-trait
Open

feat(kvp): add store trait and Hyper-V KVP storage crate#288
peytonr18 wants to merge 16 commits intoAzure:mainfrom
peytonr18:probertson/kvp-store-trait

Conversation

@peytonr18
Copy link
Copy Markdown
Contributor

@peytonr18 peytonr18 commented Mar 9, 2026

What this PR introduces

  • Adds workspace crate libazureinit-kvp.
  • Adds libazureinit-kvp/src/lib.rs, libazureinit-kvp/src/store.rs, and libazureinit-kvp/src/error.rs.
  • Adds crate dependencies: libc, serde, serde_json, sysinfo.
  • This is step 1 of the abstraction — the crate exists and is exercised by its own tests, but is not yet wired into libazureinit. A follow-up PR will replace libazureinit/src/kvp.rs with this implementation.

KVP design

Single concrete type — no trait indirection.

  • KvpPoolStore: file-backed KVP pool store.
  • KvpPool: enum identifying one of the five Hyper-V pool indices.
  • PoolMode: policy for write-path size limits.
  • KvpError: error type.

KvpPoolStore API

Construction:

  • KvpPoolStore::new(pool, mode) — uses the default directory /var/lib/hyperv.
  • KvpPoolStore::new_in(pool, dir, mode) — uses a caller-supplied directory (file name derived from pool).

Reads / queries (do not enforce mode limits on the key argument; a Safe-mode store can read keys written in Unsafe mode):

  • read(key) -> Result<Option<String>, KvpError>
  • entries() -> Result<HashMap<String, String>, KvpError> (deduplicated, last-write-wins)
  • dump() -> Result<Vec<(String, String)>, KvpError> (on-disk order, preserves duplicates)
  • dump_json(path) -> Result<(), KvpError> (writes deduplicated entries as JSON)
  • len() -> Result<usize, KvpError> (on-disk record count, not unique keys)
  • is_empty() -> Result<bool, KvpError>
  • is_stale() -> Result<bool, KvpError>

Writes (enforce PoolMode key and value size limits):

  • insert(key, value) -> Result<(), KvpError> — upsert; overwrites first match in place, removes any further duplicates, and enforces a 1024 unique-key cap for new keys.
  • append(key, value) -> Result<(), KvpError> — blind append; no duplicate check, no unique-key cap. Intended for log/telemetry workloads.
  • delete(key) -> Result<bool, KvpError> — removes all records matching the key; returns true if any were present.
  • clear() -> Result<(), KvpError> — truncates the file.
  • clear_if_stale() -> Result<(), KvpError> — convenience: is_stale() + clear().

Metadata accessors:

  • pool() -> KvpPool
  • mode() -> PoolMode
  • path() -> &Path
  • max_key_size() -> usize
  • max_value_size() -> usize

Pool identity

KvpPool enumerates the five standard Hyper-V KVP pool indices:

Variant File Role
External .kvp_pool_0 Host-to-guest, pushed by host admin
Guest .kvp_pool_1 Guest-to-host; where cloud-init / azure-init write
Auto .kvp_pool_2 Guest intrinsics generated by the daemon
AutoExternal .kvp_pool_3 Host-originated data describing the host
AutoInternal .kvp_pool_4 Undocumented; no pool file exists

KvpPool::default_dir() returns /var/lib/hyperv. KvpPool::default_path() returns the full default path for a pool.

On-disk format and limits

Hyper-V wire format: fixed-size records, identical across environments.

  • key field: 512 bytes (zero-padded)
  • value field: 2048 bytes (zero-padded)
  • record size: 2560 bytes
  • maximum unique keys: 1024 (enforced by insert, not append)

PoolMode controls write-side size limits:

Mode Max key Max value Notes
Safe 254 bytes 1022 bytes Recommended for Linux kernel compatibility
Unsafe 512 bytes 2048 bytes Full wire-format maximums

Read-side operations do not enforce these limits, so a Safe-mode store can read keys written in Unsafe mode.

Locking change (flock -> fcntl OFD)

  • Replaces fs2 advisory locks with libc::fcntl open file description (OFD) locks.
  • Lock behavior:
    • shared read lock: fcntl(F_OFD_SETLKW, F_RDLCK)
    • exclusive write lock: fcntl(F_OFD_SETLKW, F_WRLCK)
    • unlock: fcntl(F_OFD_SETLK, F_UNLCK)
  • Why: Linux flock and fcntl locks are separate lock domains and do not coordinate. OFD locks are also per-fd (safe across threads sharing a process) while still being compatible with the POSIX fcntl locks used by hv_kvp_daemon and cloud-init.
  • Result: the KVP store coordinates correctly with other Linux components that use fcntl-style locks.
  • fcntl_unlock returns io::Result<()> so callers can choose to surface unlock failures or ignore them (e.g. in Drop impls, where returning errors isn't possible).

File I/O behavior

Standard file I/O via OpenOptions, read_exact, write_all, set_len, seek, flush.

  • open_for_read_write_create() explicitly sets .truncate(false) to avoid implicit truncation on open.
  • Iteration is handled by an internal KvpPoolIter that holds the file, keeps the lock for its lifetime, and supports in-place value overwrite and swap-remove. The lock is released when the iterator is dropped.

Flows:

  • insert — open read+write+create, take exclusive lock, iterate records (collecting unique keys into a set). If the key exists, overwrite the first match in place and remove any further duplicates. If the key is new, enforce the 1024 unique-key cap and append at EOF. Flush, unlock on drop.
  • append — open read+write+create, take exclusive lock, seek to EOF, write a new record. No duplicate check, no unique-key cap. Flush, unlock on drop.
  • delete — open read+write, take exclusive lock, iterate records. For each match, swap with the final record and truncate by one. Flush if any were removed, unlock on drop.
  • clear — open read+write, take exclusive lock, set_len(0). Unlock error surfaces only if the truncate succeeded (via write_result.and(unlock_result)).
  • read / entries / dump — open read-only, take shared lock, decode records. Unlock on drop.

Stale-data handling

  • is_stale() compares the pool file's mtime to the system boot time (derived from sysinfo::System::uptime()).
  • clear_if_stale() is a convenience that clears the file only when stale.
  • Stale detection is explicit and caller-controlled — no automatic truncation occurs during construction.

Validation

Key and value validation are implemented as free functions:

  • validate_key(key, max) — checks non-empty, no null bytes, and length ≤ max.
  • validate_value(value, max) — checks length ≤ max.

Both are called by append and insert using limits derived from the store's PoolMode. read and delete do not validate the key argument — absent keys simply return None / false.

Error model

KvpError variants:

  • EmptyKey
  • KeyContainsNull
  • KeyTooLarge { max, actual }
  • ValueTooLarge { max, actual }
  • MaxUniqueKeysExceeded { max }
  • Io(io::Error)

Test coverage

88 tests cover:

  • Key/value validation boundaries in both Safe and Unsafe modes.
  • Pool identity: file names, default dir, default path, per-variant paths.
  • insert semantics: upsert, duplicate collapse, unique-key cap (allows 1024 then rejects 1025th, allows overwrite at cap, allows new key after delete, cap uses unique keys not record count).
  • append semantics: no cap, duplicate-key last-wins on read, size validation.
  • read leniency: accepts over-limit keys, returns None for missing keys without validation errors.
  • delete: removes single and duplicate records, returns false for missing file and missing key, no key validation.
  • clear, clear_if_stale, len, is_empty, is_stale, is_stale_at_boot.
  • dump_json round-trips with and without data.
  • Iterator correctness: yield order, empty file, malformed file error, mid-iteration drop releases lock, size hint tracks iteration, read errors on truncated files.
  • In-place overwrite: adjacent records untouched, multiple overwrites in one pass, error if called before first next().
  • Remove: swap-and-continue, remove-last (no swap), remove-only-record, error if called before first next().
  • File-size invariants: unchanged on overwrite, grows on new key, preserves other records.
  • File integrity after mixed insert/append/delete operations.
  • Concurrent reader/writer, writer/writer, append-only, and mixed insert+append scenarios (8 threads).
  • Permission-denied error paths for read, delete, clear, entries, dump, len, is_stale, is_stale_at_boot.
  • fcntl failure paths (lock on mismatched fd modes, FIFO truncation / seek errors on Linux).
  • KvpError display, source, and From<io::Error> conversion.

Coverage Report

image

@peytonr18 peytonr18 force-pushed the probertson/kvp-store-trait branch from b134a98 to f435f70 Compare March 9, 2026 19:19
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread doc/libazurekvp.md Outdated
Comment thread doc/libazurekvp.md Outdated
Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new standalone workspace crate (libazureinit-kvp) that defines a storage abstraction (KvpStore) and a production Hyper-V pool-file implementation (HyperVKvpStore), along with updated technical reference documentation.

Changes:

  • Introduces libazureinit-kvp crate with KvpStore and KvpLimits (Hyper-V/Azure presets).
  • Implements Hyper-V binary pool-file backend with flock-based concurrency and stale-file truncation support.
  • Rewrites doc/libazurekvp.md as a technical reference for the new crate/API.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
Cargo.toml Adds libazureinit-kvp to the workspace members.
libazureinit-kvp/Cargo.toml Defines the new crate and its dependencies (fs2, sysinfo).
libazureinit-kvp/src/lib.rs Defines KvpStore, KvpLimits, and exports HyperVKvpStore plus size constants.
libazureinit-kvp/src/hyperv.rs Implements Hyper-V pool-file encoding/decoding and the KvpStore backend, plus unit tests.
doc/libazurekvp.md Updates documentation to describe the new crate’s record format, semantics, truncation behavior, and limits.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Comment thread doc/libazurekvp.md Outdated
Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Comment thread doc/libazurekvp.md Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Comment thread libazureinit-kvp/src/hyperv.rs Outdated
Comment thread libazureinit-kvp/src/azure.rs Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
@peytonr18 peytonr18 requested a review from cadejacobson March 27, 2026 19:25
Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
Comment thread libazureinit-kvp/src/store.rs Outdated
Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
Copy link
Copy Markdown

@bpryan99 bpryan99 left a comment

Choose a reason for hiding this comment

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

My comments were addressed, LGTM! Great work.

Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
Comment thread libazureinit-kvp/src/lib.rs Outdated
Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
#[repr(u8)]
pub enum KvpPool {
/// Host-to-guest data pushed by the host admin (`.kvp_pool_0`).
External = 0,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these should be the file paths so we don't new two params to new()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are kinda two ways to do this, depending if you want KvpPoolStore to be pool oriented or path oriented.

The first is this one, which just does an impl and new() will just take in the path (or a custom path if the user wants instead)

libazureinit-kvp/src/kvp_pool.rs
/// Hyper-V KVP pool indices.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
#[repr(u8)]
pub enum KvpPool {
   /// Host-to-guest data pushed by the host admin (`.kvp_pool_0`).
   External = 0,
   /// Guest-to-host data; cloud-init / azure-init write here (`.kvp_pool_1`).
   Guest = 1,
   // ...
}
impl From<KvpPool> for PathBuf {
   fn from(pool: KvpPool) -> Self {
       pool.default_path()
   }
}
impl KvpPoolStore {
   /// Create a new KVP pool store.
   ///
   /// - `path`: pool file path. Pass a [`KvpPool`] variant for a
   ///   well-known pool (e.g. `KvpPool::Guest`), or any
   ///   [`PathBuf`]/[`&Path`] for a custom location.
   /// - `mode`: size-limit policy (see [`PoolMode`]).
   /// - `clear_if_stale`: if `true`, clears stale data from a
   ///   previous boot.
   pub fn new(
       path: impl Into<PathBuf>,
       mode: PoolMode,
       clear_if_stale: bool,
   ) -> Result<Self, KvpError> {
       let store = Self {
           path: path.into(),
           mode,
       };
       if clear_if_stale && store.is_stale()? {
           store.clear()?;
       }
       Ok(store)
   }
}

Or, we can semantically change KvpPoolStore to be path oriented as a whole instead.

use std::path::{Path, PathBuf};
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum KvpPool {
   External,
   Guest,
   Auto,
   AutoExternal,
   AutoInternal,
}
impl KvpPool {
   pub fn as_path(self) -> &'static Path {
       match self {
           Self::External => Path::new("/var/lib/hyperv/.kvp_pool_0"),
           Self::Guest => Path::new("/var/lib/hyperv/.kvp_pool_1"),
           Self::Auto => Path::new("/var/lib/hyperv/.kvp_pool_2"),
           Self::AutoExternal => Path::new("/var/lib/hyperv/.kvp_pool_3"),
           Self::AutoInternal => Path::new("/var/lib/hyperv/.kvp_pool_4"),
       }
   }
}
impl From<KvpPool> for PathBuf {
   fn from(pool: KvpPool) -> Self {
       pool.as_path().to_path_buf()
   }
}

I was leaning towards the first approach, but am open to either depending on what you envisioned here!

Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
Comment thread libazureinit-kvp/src/kvp_pool.rs Outdated
/// - `mode`: size-limit policy (see [`PoolMode`]).
/// - `truncate_on_stale`: if `true`, clears stale data from a
/// previous boot.
pub fn new(
Copy link
Copy Markdown
Contributor

@cjp256 cjp256 Apr 15, 2026

Choose a reason for hiding this comment

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

let's leave path out of the default new....

impl KvpPool {
    pub fn filename(self) -> &'static str {
        match self {
            Self::External => ".kvp_pool_1",
            Self::Guest => ".kvp_pool_2",
        }
    }

    pub fn default_dir() -> &'static Path {
        Path::new("/var/lib/hyperv")
    }

    pub fn default_path(self) -> PathBuf {
        Self::default_dir().join(self.filename())
    }
}

#[derive(Debug)]
pub struct KvpStore {
    pool: KvpPool,
    path: PathBuf,
    mode: PoolMode,
}

impl KvpStore {
    /// Open using the default Hyper-V directory.
    pub fn new(pool: KvpPool, mode: PoolMode) -> Result<Self, KvpError> {
        Self::new_in(pool, KvpPool::default_dir(), mode)
    }

    /// Open using a custom directory (filename derived from pool).
    pub fn new_in(
        pool: KvpPool,
        dir: impl AsRef<Path>,
        mode: PoolMode,
    ) -> Result<Self, KvpError> {
        let path = dir.as_ref().join(pool.filename());
        Ok(Self { pool, path, mode })
    }

    pub fn path(&self) -> &Path {
        &self.path
    }
...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done!

Comment thread libazureinit-kvp/src/lib.rs Outdated
pub use kvp_pool::KvpPoolStore;

/// Key-value store with Hyper-V KVP semantics.
pub trait KvpStore: Send + Sync {
Copy link
Copy Markdown
Contributor

@cjp256 cjp256 Apr 15, 2026

Choose a reason for hiding this comment

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

let's remove this in favor of the definition in kvp_pool.rs

and let's do a little restructuring, e.g.:

src/
├── lib.rs
├── error.rs
└── store.rs

and lib.rs is something like:

mod error;
mod store;

pub use error::KvpError;
pub use store::{KvpPool, KvpStore, PoolMode};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done!

@peytonr18 peytonr18 force-pushed the probertson/kvp-store-trait branch from 3fa165a to cfc44ac Compare April 16, 2026 18:54
Comment thread libazureinit-kvp/src/store.rs Outdated
/// Release a lock on the entire file.
///
/// Unlock errors are non-actionable (the fd is about to close, which
/// releases the lock unconditionally), so the return value is discarded.
Copy link
Copy Markdown
Contributor

@cjp256 cjp256 Apr 21, 2026

Choose a reason for hiding this comment

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

it seems like this should raise the error, but then have the caller ignore it if it's not needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.34%. Comparing base (fd14207) to head (680057c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
+ Coverage   84.77%   89.34%   +4.56%     
==========================================
  Files          17       19       +2     
  Lines        3435     4907    +1472     
==========================================
+ Hits         2912     4384    +1472     
  Misses        523      523              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@peytonr18 peytonr18 force-pushed the probertson/kvp-store-trait branch from c5531ac to 4e1f57a Compare April 23, 2026 21:13
…ics and JSON dump

Overhaul the KvpStore trait into a pure customer-facing interface with
no default implementations or backend_* methods. All logic (validation,
file I/O, locking) now lives in the KvpPoolStore implementation.

Key changes:
- Replace append-based write with upsert (insert-or-update) so each key
  has exactly one record in the file. Eliminates entries_raw and
  last-write-wins deduplication.
- Move validate_key/validate_value from trait defaults to private
  KvpPoolStore methods.
- Decouple read validation from PoolMode: reads accept keys up to the
  full wire-format max (512 bytes) regardless of mode; only writes are
  restricted by PoolMode.
- Add len(), is_empty(), and dump() (JSON via serde_json) to the trait.
- Add 4 multi-threaded concurrency tests covering concurrent upserts to
  different keys, same key, mixed readers/writers, and key cap boundary.
… locks

Use libc fcntl(F_OFD_SETLKW/F_OFD_SETLK) for shared/exclusive/unlock operations so pool file locking interoperates with Linux KVP consumers that use fcntl record locks. Also replace fs2 with libc and keep existing pool behavior/tests intact.
Introduce explicit insert (overwrite existing key) and append (always add record) APIs, refactor pool operations to iterator-based in-place updates/deletes, and document record-count vs unique-key behavior with expanded concurrency/integrity test coverage.
…verage

- Split kvp_pool.rs into error.rs (KvpError) and store.rs (KvpStore trait + KvpPoolStore)
- lib.rs reduced to module declarations and re-exports
- Replace infallible dump_json map_err with expect
- Inline get_uptime into boot_time
- Add permission-denied, stale-clear, and missing-key-delete tests
- 98.5% line coverage; remaining gaps are untestable kernel-level fcntl error paths
Add 8 tests for uncovered paths: invalid UTF-8 decoding, size_hint,
iterator read errors, fcntl lock failures, and FIFO error propagation.

Minor production cleanups (no behavior change): boot_time() uses
.expect instead of unreachable .map_err, lock context message moved
into fcntl_lock_* functions, fcntl_unlock changed to void return.
- Replace validate_key_for_read and per-method validation with
  consistent standalone validate_key/validate_value free functions.
- Remove key validation from read and delete (read-path leniency
  allows Safe-mode stores to read keys written in Unsafe mode).
- Move is_empty default impl to the KvpStore trait.
- Narrow decode_record/encode_record visibility to private.
- Use open_for_read_write helper in clear().
- Merge KvpStore trait methods directly into impl KvpPoolStore. The
  trait added indirection without a consumer; testability is already
  covered by KvpPoolStore + TempDir. Can be re-introduced if a second
  backend is ever needed.
- fcntl_unlock now returns io::Result<()> so callers can propagate or
  ignore unlock errors explicitly. clear() surfaces unlock errors via
  write_result.and(unlock_result), preserving the primary error when
  both fail. Drop impl ignores the result (can't return from Drop);
  KvpPoolIter::new error paths rely on fd-drop for lock release,
  consistent with how its other ? paths already work.
- Update lib.rs exports and doc links to reflect the removed trait.
@peytonr18 peytonr18 force-pushed the probertson/kvp-store-trait branch from 4e1f57a to 680057c Compare April 23, 2026 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants