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

[peytonr18]

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

[Copilot]

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.

[cjp256]

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

[peytonr18]

Done!

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.51%. Comparing base (5ff1d14) to head (7518bfa).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
+ Coverage   91.52%   94.51%   +2.98%     
==========================================
  Files          17       19       +2     
  Lines        3905     6031    +2126     
==========================================
+ Hits         3574     5700    +2126     
  Misses        331      331              

☔ 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.

[cjp256]

should we use iter_mut() here?

[peytonr18]

I think this is correct as is because delete() opens the file via open_for_read_write() where there isn't a create call and then the function exits with Ok(false) if the file is missing. using iter_mut() would open the file via open_for_read_write_create() and subsequently create an empty pool file as a side effect from trying to delete from it

[cjp256]

we want to take both fcntl and flock locks:

this lock works for hv_kvp_daemon, but cloud-init uses flock(LOCK_EX)

https://github.com/canonical/cloud-init/blob/c7ae6a4cc0dad5a411945e0644dab6926d41a997/cloudinit/reporting/handlers.py#L286

[cjp256]

maybe add a lock_for_reading() and lock_for_writing()

and the semantic here, if needed, perhaps should be read_only: bool, because we want to guarantee the locks for any write operations

[cjp256]

why last index? doesn't this reverse ordering if copying the last record and moving it to current?

[peytonr18]

I added some comments to make the iteration logic clearer here, but let me know if you still have questions/concerns and I can rework the approach!

[cjp256]

    /// Exposes a bug where `remove_current()` reorders duplicate
    /// records via its swap-with-last strategy, silently changing
    /// which duplicate `entries()` reports as last-write-wins.
    ///
    /// Initial on-disk order has `("dup", "C")` as the final record
    /// for key `"dup"`, so `entries()["dup"] == "C"`. After deleting
    /// the unrelated `"middle"` key, the swap moves `("dup", "C")`
    /// into the freed slot ahead of `("dup", "B")`, leaving `"B"` as
    /// the on-disk-last record for `"dup"`.
    #[test]
    fn test_delete_reorders_duplicates_breaking_last_write_wins() {
        let dir = TempDir::new().unwrap();
        let store = safe_store(dir.path());

        store
            .populate(pairs([
                ("dup", "A"),
                ("middle", "m"),
                ("dup", "B"),
                ("other", "x"),
                ("dup", "C"),
            ]))
            .unwrap();

        assert_eq!(
            store.entries().unwrap().get("dup"),
            Some(&"C".to_string()),
            "precondition: last-write-wins picks the final on-disk record",
        );

        assert!(store.delete("middle").unwrap());

        assert_eq!(
            store.entries().unwrap().get("dup"),
            Some(&"C".to_string()),
            "deleting an unrelated key must not change which duplicate \
             entries() reports as the winning value",
        );
    }

    /// Companion to
    /// [`test_delete_reorders_duplicates_breaking_last_write_wins`]
    /// for the other production caller of `remove_current()`: the
    /// duplicate-collapsing branch of [`KvpPoolStore::insert`].
    ///
    /// Initial on-disk order has `("dup", "C")` as the final record
    /// for key `"dup"`, so `entries()["dup"] == "C"`. Inserting
    /// `"ins"` overwrites the first `"ins"` record in place and then
    /// calls `remove_current()` on the second `"ins"`, which swaps
    /// the tail `("dup", "C")` forward of `("dup", "B")`, leaving
    /// `"B"` as the on-disk-last record for `"dup"`.
    #[test]
    fn test_insert_reorders_duplicates_breaking_last_write_wins() {
        let dir = TempDir::new().unwrap();
        let store = safe_store(dir.path());

        store
            .populate(pairs([
                ("ins", "i1"),
                ("ins", "i2"),
                ("dup", "B"),
                ("dup", "C"),
            ]))
            .unwrap();

        assert_eq!(
            store.entries().unwrap().get("dup"),
            Some(&"C".to_string()),
            "precondition: last-write-wins picks the final on-disk record",
        );

        store.insert("ins", "new").unwrap();

        assert_eq!(
            store.entries().unwrap().get("dup"),
            Some(&"C".to_string()),
            "insert collapsing its own duplicates must not change which \
             duplicate of an unrelated key entries() reports as winning",
        );
    }

failures:

---- store::tests::test_delete_reorders_duplicates_breaking_last_write_wins stdout ----

thread 'store::tests::test_delete_reorders_duplicates_breaking_last_write_wins' (1817184) panicked at libazureinit-kvp/src/store.rs:1341:9:
assertion `left == right` failed: deleting an unrelated key must not change which duplicate entries() reports as the winning value
  left: Some("B")
 right: Some("C")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- store::tests::test_insert_reorders_duplicates_breaking_last_write_wins stdout ----

thread 'store::tests::test_insert_reorders_duplicates_breaking_last_write_wins' (1817203) panicked at libazureinit-kvp/src/store.rs:1382:9:
assertion `left == right` failed: insert collapsing its own duplicates must not change which duplicate of an unrelated key entries() reports as winning
  left: Some("B")
 right: Some("C")


failures:
    store::tests::test_delete_reorders_duplicates_breaking_last_write_wins
    store::tests::test_insert_reorders_duplicates_breaking_last_write_wins

test result: FAILED. 100 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.72s

error: test failed, to rerun pass `-p libazureinit-kvp --lib`

[cjp256]

or for a dump-based view

    /// Exposes a bug where `remove_current()` reorders duplicate
    /// records via its swap-with-last strategy, silently changing
    /// which duplicate `entries()` reports as last-write-wins.
    ///
    /// Initial on-disk order has `("dup", "C")` as the final record
    /// for key `"dup"`, so `entries()["dup"] == "C"`. After deleting
    /// the unrelated `"middle"` key, the swap moves `("dup", "C")`
    /// into the freed slot ahead of `("dup", "B")`, leaving `"B"` as
    /// the on-disk-last record for `"dup"`.
    #[test]
    fn test_delete_reorders_duplicates_breaking_last_write_wins() {
        let dir = TempDir::new().unwrap();
        let store = safe_store(dir.path());

        store
            .populate(pairs([
                ("dup", "A"),
                ("middle", "m"),
                ("dup", "B"),
                ("other", "x"),
                ("dup", "C"),
            ]))
            .unwrap();

        assert_eq!(
            store.entries().unwrap().get("dup"),
            Some(&"C".to_string()),
            "precondition: last-write-wins picks the final on-disk record",
        );

        assert!(store.delete("middle").unwrap());

        assert_eq!(
            store.dump().unwrap(),
            pairs([
                ("dup", "A"),
                ("dup", "B"),
                ("other", "x"),
                ("dup", "C"),
            ]),
            "deleting an unrelated key must preserve the relative order \
             of every other record (so last-write-wins still picks C \
             for `dup`)",
        );
        assert_eq!(
            store.entries().unwrap().get("dup"),
            Some(&"C".to_string()),
            "deleting an unrelated key must not change which duplicate \
             entries() reports as the winning value",
        );
    }

    /// Companion to
    /// [`test_delete_reorders_duplicates_breaking_last_write_wins`]
    /// for the other production caller of `remove_current()`: the
    /// duplicate-collapsing branch of [`KvpPoolStore::insert`].
    ///
    /// Initial on-disk order has `("dup", "C")` as the final record
    /// for key `"dup"`, so `entries()["dup"] == "C"`. Inserting
    /// `"ins"` overwrites the first `"ins"` record in place and then
    /// calls `remove_current()` on the second `"ins"`, which swaps
    /// the tail `("dup", "C")` forward of `("dup", "B")`, leaving
    /// `"B"` as the on-disk-last record for `"dup"`.
    #[test]
    fn test_insert_reorders_duplicates_breaking_last_write_wins() {
        let dir = TempDir::new().unwrap();
        let store = safe_store(dir.path());

        store
            .populate(pairs([
                ("ins", "i1"),
                ("ins", "i2"),
                ("dup", "B"),
                ("dup", "C"),
            ]))
            .unwrap();

        assert_eq!(
            store.entries().unwrap().get("dup"),
            Some(&"C".to_string()),
            "precondition: last-write-wins picks the final on-disk record",
        );

        store.insert("ins", "new").unwrap();

        assert_eq!(
            store.dump().unwrap(),
            pairs([("ins", "new"), ("dup", "B"), ("dup", "C")]),
            "collapsing duplicate `ins` records must preserve the \
             relative order of unrelated `dup` records (so last-write-wins \
             still picks C for `dup`)",
        );
        assert_eq!(
            store.entries().unwrap().get("dup"),
            Some(&"C".to_string()),
            "insert collapsing its own duplicates must not change which \
             duplicate of an unrelated key entries() reports as winning",
        );
    }

failures:

---- store::tests::test_delete_reorders_duplicates_breaking_last_write_wins stdout ----

thread 'store::tests::test_delete_reorders_duplicates_breaking_last_write_wins' (1818095) panicked at libazureinit-kvp/src/store.rs:1341:9:
assertion `left == right` failed: deleting an unrelated key must preserve the relative order of every other record (so last-write-wins still picks C for `dup`)
  left: [("dup", "A"), ("dup", "C"), ("dup", "B"), ("other", "x")]
 right: [("dup", "A"), ("dup", "B"), ("other", "x"), ("dup", "C")]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- store::tests::test_insert_reorders_duplicates_breaking_last_write_wins stdout ----

thread 'store::tests::test_insert_reorders_duplicates_breaking_last_write_wins' (1818139) panicked at libazureinit-kvp/src/store.rs:1394:9:
assertion `left == right` failed: collapsing duplicate `ins` records must preserve the relative order of unrelated `dup` records (so last-write-wins still picks C for `dup`)
  left: [("ins", "new"), ("dup", "C"), ("dup", "B")]
 right: [("ins", "new"), ("dup", "B"), ("dup", "C")]

[cjp256]

I think we just have to bite the bullet and redo these to shift everything up (and delete dupe keys in the process). I think if we have to iterate through all entries anyways, the cost isn't really a problem. Callers should avoid introducing duplicate keys anyways so we likely won't hit the worst-case.

[cjp256]

why is this special cased and not handled by iter_mut()? we should be able to see the NotFound error [and ignore it] through that interface too?

[peytonr18]

Good call out - this should be fixed in the most recent commit!

[cjp256]

OK, I think I've come around on the drop pattern here.

this way if scope of iter is too long, it can be simply managed with something like:

let result = {
    let mut iter = self.iter_mut()?;
    iter.append(key, value)?;
};

to ensure it's unlocked when the caller needs it to be.

I think this is more generally safe than trying to ensure all callers run_with_iter(). And file closure depends on the same mechanism, so this makes it consistent.

[peytonr18]

Adopted this in the latest commit!