Skip to content

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

Open
peytonr18 wants to merge 5 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 5 commits intoAzure:mainfrom
peytonr18:probertson/kvp-store-trait

Conversation

@peytonr18
Copy link
Contributor

@peytonr18 peytonr18 commented Mar 9, 2026

Summary

Introduces libazureinit-kvp, a standalone workspace crate that defines the KvpStore storage trait and provides a production implementation backed by the Hyper-V binary pool file format.

This is the foundational layer (Layer 0) for the KVP architecture rework. It establishes the storage abstraction that higher layers (diagnostics, tracing, provisioning reports) will build on in follow-up PRs.

  • Adds new workspace crate libazureinit-kvp with KvpStore trait and HyperVKvpStore implementation.
  • Defines KvpLimits as a trait-level requirement so consumers and alternative implementations can query size constraints generically.
  • Rewrites doc/libazurekvp.md as a technical reference for the new crate.
  • No consumers wired yet — libazureinit still uses the existing kvp.rs module. Integration happens in follow-up PRs.

Changes

KvpStore trait

Storage abstraction with explicit semantics:

  • limits() — returns KvpLimits for the store (required by the trait so consumers can chunk/validate generically)
  • write(key, value) — validates against limits, then stores
  • read(key) — last-write-wins for duplicate keys
  • entries() — deduplicated HashMap view
  • delete(key) — removes all records for a key

HyperVKvpStore

Production implementation backed by the Hyper-V binary pool file:

  • Fixed-size 2,560-byte records (512-byte key + 2,048-byte value, zero-padded)
  • Append-only writes with flock-based concurrency (exclusive for writes, shared for reads)
  • truncate_if_stale() clears records from previous boots by comparing file mtime to boot time
  • Write validation rejects empty keys, oversized keys, and oversized values (no silent truncation)

KvpLimits

PR feedback asked for limits to be required at the trait level rather than only on the concrete type. This ensures:

  • Any KvpStore implementation must declare its constraints.
  • Generic consumer code (fn foo<S: KvpStore>(store: &S)) can call store.limits().max_value_size for chunking decisions without hardcoding constants or knowing the concrete type.

Exported as part of the trait contract with two presets:

  • KvpLimits::hyperv() — 512-byte key / 2,048-byte value (raw Hyper-V wire format)
  • KvpLimits::azure() — 512-byte key / 1,022-byte value (UTF-16: 511 chars + null terminator; matches Azure host read limit)

Higher layers are responsible for chunking oversized payloads before calling the store.

Next Steps

Follow-up PRs will add higher layers on top of this trait (Connected to #287):

  1. Diagnostics layer (typed events, value splitting at the 1,022-byte Azure limit)
  2. Tracing layer (tracing_subscriber::Layer adapter)
  3. Provisioning report accessor
  4. Integration into libazureinit/src/logging.rs to replace the existing kvp.rs module

@peytonr18 peytonr18 force-pushed the probertson/kvp-store-trait branch from b134a98 to f435f70 Compare March 9, 2026 19:19
pub const HYPERV_MAX_VALUE_BYTES: usize = 2048;
/// Azure key limit in bytes (policy/default preset).
pub const AZURE_MAX_KEY_BYTES: usize = 512;
/// Azure value limit in bytes (UTF-16: 511 characters + null terminator).
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a reference for the null termination of this? I opened up that link in the previous file, but did not see a reference to null termination

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was basing this off of what we have in our existing kvp.rs --

const HV_KVP_EXCHANGE_MAX_KEY_SIZE: usize = 512;
const HV_KVP_EXCHANGE_MAX_VALUE_SIZE: usize = 2048;
const HV_KVP_AZURE_MAX_VALUE_SIZE: usize = 1022;

and my old notes from that KVP process, but I'll look for the official documentation where it says that!

@cadejacobson cadejacobson self-requested a review March 9, 2026 20:42
Copy link

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.

@@ -1,126 +1,88 @@
# Azure-init Tracing System
# `libazureinit-kvp`
Copy link
Contributor

Choose a reason for hiding this comment

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

can we delete this file in favor of the source rustdocs for now? I'd rather focus on great docs in the source modules and keep this at really high level, or delete it. There's a lot of duplication as it stands...

pub use hyperv::HyperVKvpStore;

/// Hyper-V key limit in bytes (policy/default preset).
pub const HYPERV_MAX_KEY_BYTES: usize = 512;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be here (dupe of hyperv)

/// Key/value byte limits for write validation.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub struct KvpLimits {
pub max_key_size: usize,
Copy link
Contributor

@cjp256 cjp256 Mar 10, 2026

Choose a reason for hiding this comment

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

let's inline these into the trait

and have each implementation define them as their own const.

Copy link
Contributor

Choose a reason for hiding this comment

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

if you want to keep KvpLimits, make it a trait and implement Azure & HyperV variants

/// Consumers (e.g. diagnostics, tracing layers) should call this
/// instead of hardcoding size constants, so the limits stay correct
/// regardless of the underlying implementation.
fn limits(&self) -> KvpLimits;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove in favor of two consts.

///
/// Keys are deduplicated using last-write-wins semantics, matching
/// the behavior of [`read`](KvpStore::read).
fn entries(&self) -> io::Result<HashMap<String, String>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we expose one more entries_raw(), or dump() which yields a list of key/value pairs that aren't necessarily unique?

this may be useful for testing or some dump command

/// Storage abstraction for KVP backends.
///
/// Semantics:
/// - `write`: stores one key/value or returns validation/I/O error.
Copy link
Contributor

Choose a reason for hiding this comment

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

these details should be consolidated into the function docs. This should just be a high level doc for the trait.

e.g. key-value store that supports the semantics of Hyper-V's KVP while also being generic to support non-file-backed-pool-KVP in-memory implementation, for tests etc.

///
/// Returns an error if:
/// - The key is empty.
/// - The key exceeds the configured maximum key size.
Copy link
Contributor

Choose a reason for hiding this comment

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

what are allowed values for the key characters? are there restrictions?


/// Write a key-value pair into the store.
///
/// Returns an error if:
Copy link
Contributor

Choose a reason for hiding this comment

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

these errors should be defined here so we can be specific about what is being returned under what conditions

//! - No record-count header or explicit record cap in this layer.
//!
//! ## Behavior summary
//! - **`write`**: append-only; one record appended per call.
Copy link
Contributor

Choose a reason for hiding this comment

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

leave these to function docs

//! - No record-count header or explicit record cap in this layer.
//!
//! ## Behavior summary
//! - **`write`**: append-only; one record appended per call.
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the module docs relevant to the Hyper-V KVP pool / protocol and don't include any rust implementations details. The implementation should flow from this description and be detailed in the impl docs.

Comment on lines +98 to +99
pub fn new_autodetect(path: impl Into<PathBuf>) -> Self {
let limits = if is_azure_vm(None) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is clever, but let's not do this. The caller should decide with implementation to work with if it's separated in two different implementations. If it's one implementation, then constrain it to a required bool in the new() call, e.g. new(azure=true)

/// - The key exceeds the configured maximum key size.
/// - The value exceeds the configured maximum value size.
/// - An I/O error occurs during the write.
fn write(&self, key: &str, value: &str) -> io::Result<()>;
Copy link
Contributor

Choose a reason for hiding this comment

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

we'll actually want there to be an implementation here with common validations and then a call-out to the backend-processing.

for example:

trait KvpStore {
    const MAX_KEY_SIZE: usize;
    const MAX_VALUE_SIZE: usize;

    fn backend_read(&self, key: &str) -> Result<String, KvpError>;
    fn backend_write(&self, key: &str, value: &str) -> Result<(), KvpError>;

    fn read(&self, key: &str) -> Result<String, KvpError> {
        Self::validate_key(key)?;
        self.backend_read(key)
    }

    fn write(&self, key: &str, value: &str) -> Result<(), KvpError> {
        Self::validate_key(key)?;
        Self::validate_value(value)?;
        self.backend_write(key, value)
    }

    fn validate_key(key: &str) -> Result<(), KvpError> {
        let actual = key.len();
        let max = Self::MAX_KEY_SIZE;

        if actual > max {
            return Err(KvpError::KeyTooLarge { max, actual });
        }

        Ok(())
    }

    fn validate_value(value: &str) -> Result<(), KvpError> {
        let actual = value.len();
        let max = Self::MAX_VALUE_SIZE;

        if actual > max {
            return Err(KvpError::ValueTooLarge { max, actual });
        }

        Ok(())
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't preclude additional validations the backend might want to do, but keeps a shared implementation of the basics that will be common everywhere

const AZURE_CHASSIS_ASSET_TAG_PATH: &str =
"/sys/class/dmi/id/chassis_asset_tag";

fn is_azure_vm(tag_path: Option<&str>) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want this logic (yet) but if there is a reason to keep it, move it into its own module, e.g. platform.rs or identity.rs.

/// check-then-truncate sequence. If the lock cannot be acquired
/// immediately (another client holds it), the call returns `Ok(())`
/// without blocking.
pub fn truncate_if_stale(&self) -> io::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

there are two parts to this, truncate() and pool_is_stale(). let's split them apart.

I think the truncate() method can be part of the trait definition and implementable with trait methods or backend callouts for backend specific bits. It simply empties out the store (clears the hash map or truncates the file).

is_stale() can be separate. The in-memory case would return False and HyperV/Azure would check mtime as done here.

the new() constructor for hyperv/azure can have a parameter to truncate_on_stale which checks if stale then truncates.

truncate_on_stale: bool,
) -> Result<Self, KvpError> {
Ok(Self {
inner: HyperVKvpStore::new(path, truncate_on_stale)?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall, I think this composition setup works super well! This is the intended replacement for inheritance for Rust. My only request is that I would recommend updating inner to be a more descriptive term, maybe kvp_store, since I do not see many official references to using the inner name. This could get confusing if we ever extend this further.
https://trpl.rantai.dev/docs/part-iii/chapter-20/#2043-implementing-composition-in-rust

For example, if AzureKvpStore were ever put into a separate composition, the pattern of calling the composed object inner could remain and would result in a ExtendedAzureKvpStore.inner.inner.path calling, instead of ExtendedAzureKvpStore.azure_kvp_store.kvp_store.path which strikes me as more verbose.

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.

4 participants