maurice/snix
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

refactor(tvix/castore): remove name from Nodes

Browse source 
Nodes only have names if they're contained inside a Directory, or if
they're a root node and have something else possibly giving them a name
externally.

This removes all `name` fields in the three different Nodes, and instead
maintains it inside a BTreeMap inside the Directory.

It also removes the NamedNode trait (they don't have a get_name()), as
well as Node::rename(self, name), and all [Partial]Ord implementations
for Node (as they don't have names to use for sorting).

The `nodes()`, `directories()`, `files()` iterators inside a `Directory`
now return a tuple of Name and Node, as does the RootNodesProvider.

The different {Directory,File,Symlink}Node struct constructors got
simpler, and the {Directory,File}Node ones became infallible - as
there's no more possibility to represent invalid state.

The proto structs stayed the same - there's now from_name_and_node and
into_name_and_node to convert back and forth between the two `Node`
structs.

Some further cleanups:

The error types for Node validation were renamed. Everything related to
names is now in the DirectoryError (not yet happy about the naming)

There's some leftover cleanups to do:
 - There should be a from_(sorted_)iter and into_iter in Directory, so
   we can construct and deconstruct in one go.
   That should also enable us to implement conversions from and to the
   proto representation that moves, rather than clones.

 - The BuildRequest and PathInfo structs are still proto-based, so we
   still do a bunch of conversions back and forth there (and have some
   ugly expect there). There's not much point for error handling here,
   this will be moved to stricter types in a followup CL.

Change-Id: I7369a8e3a426f44419c349077cb4fcab2044ebb6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12205
Tested-by: BuildkiteCI
Reviewed-by: yuka <yuka@yuka.dev>
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: benjaminedwardwebb <benjaminedwardwebb@gmail.com>
Reviewed-by: Connor Brewster <cbrewster@hey.com>
This commit is contained in:
Florian Klink 2024-08-14 22:00:12 +03:00 • committed by clbot
parent 04e9531e65
commit 49b173786c
46 changed files with 785 additions and 1002 deletions
Download patch file Download diff file Expand all files Collapse all files

7
tvix/store/src/bin/tvix-store.rs
View file

@ -351,9 +351,10 @@ async fn run_cli(cli: Cli) -> Result<(), Box<dyn std::error::Error + Send + Sync
// Create and upload a PathInfo pointing to the root_node,
// annotated with information we have from the reference graph.
let path_info = PathInfo {
node: Some(tvix_castore::proto::Node {
node: Some((&root_node).into()),
}),
node: Some(tvix_castore::proto::Node::from_name_and_node(
elem.path.to_string().into(),
root_node,
)),
references: Vec::from_iter(
elem.references.iter().map(|e| e.digest().to_vec().into()),
),

27
tvix/store/src/import.rs
View file

@ -1,10 +1,8 @@
use bstr::ByteSlice;
use std::path::Path;
use tracing::{debug, instrument};
use tvix_castore::{
blobservice::BlobService,
directoryservice::DirectoryService,
import::fs::ingest_path,
{NamedNode, Node},
blobservice::BlobService, directoryservice::DirectoryService, import::fs::ingest_path, Node,
};
use nix_compat::{
@ -29,12 +27,12 @@ impl From<CAHash> for nar_info::Ca {
}
}
pub fn log_node(node: &Node, path: &Path) {
pub fn log_node(name: &[u8], node: &Node, path: &Path) {
match node {
Node::Directory(directory_node) => {
debug!(
path = ?path,
name = ?directory_node.get_name(),
name = %name.as_bstr(),
digest = %directory_node.digest(),
"import successful",
)
@ -42,7 +40,7 @@ pub fn log_node(node: &Node, path: &Path) {
Node::File(file_node) => {
debug!(
path = ?path,
name = ?file_node.get_name(),
name = %name.as_bstr(),
digest = %file_node.digest(),
"import successful"
)
@ -50,7 +48,7 @@ pub fn log_node(node: &Node, path: &Path) {
Node::Symlink(symlink_node) => {
debug!(
path = ?path,
name = ?symlink_node.get_name(),
name = %name.as_bstr(),
target = ?symlink_node.target(),
"import successful"
)
@ -84,13 +82,14 @@ pub fn derive_nar_ca_path_info(
nar_size: u64,
nar_sha256: [u8; 32],
ca: Option<&CAHash>,
name: bytes::Bytes,
root_node: Node,
) -> PathInfo {
// assemble the [crate::proto::PathInfo] object.
PathInfo {
node: Some(tvix_castore::proto::Node {
node: Some((&root_node).into()),
}),
node: Some(tvix_castore::proto::Node::from_name_and_node(
name, root_node,
)),
// There's no reference scanning on path contents ingested like this.
references: vec![],
narinfo: Some(NarInfo {
@ -140,14 +139,14 @@ where
)
})?;
// rename the root node to match the calculated output path.
let root_node = root_node.rename(output_path.to_string().into_bytes().into());
log_node(&root_node, path.as_ref());
let name = bytes::Bytes::from(output_path.to_string());
log_node(name.as_ref(), &root_node, path.as_ref());
let path_info = derive_nar_ca_path_info(
nar_size,
nar_sha256,
Some(&CAHash::Nar(NixHash::Sha256(nar_sha256))),
output_path.to_string().into_bytes().into(),
root_node,
);

40
tvix/store/src/nar/import.rs
View file

@ -12,7 +12,7 @@ use tvix_castore::{
blobs::{self, ConcurrentBlobUploader},
ingest_entries, IngestionEntry, IngestionError,
},
PathBuf, {NamedNode, Node},
Node, PathBuf,
};
/// Ingests the contents from a [AsyncRead] providing NAR into the tvix store,
@ -97,9 +97,7 @@ where
let (_, node) = try_join!(produce, consume)?;
// remove the fake "root" name again
debug_assert_eq!(&node.get_name()[..], b"root");
Ok(node.rename("".into()))
Ok(node)
}
async fn produce_nar_inner<BS>(
@ -198,13 +196,7 @@ mod test {
.expect("must parse");
assert_eq!(
Node::Symlink(
SymlinkNode::new(
"".into(), // name must be empty
"/nix/store/somewhereelse".into(),
)
.unwrap()
),
Node::Symlink(SymlinkNode::new("/nix/store/somewhereelse".into(),).unwrap()),
root_node
);
}
@ -224,15 +216,11 @@ mod test {
.expect("must parse");
assert_eq!(
Node::File(
FileNode::new(
"".into(), // name must be empty
HELLOWORLD_BLOB_DIGEST.clone(),
HELLOWORLD_BLOB_CONTENTS.len() as u64,
false,
)
.unwrap()
),
Node::File(FileNode::new(
HELLOWORLD_BLOB_DIGEST.clone(),
HELLOWORLD_BLOB_CONTENTS.len() as u64,
false,
)),
root_node
);
@ -255,14 +243,10 @@ mod test {
.expect("must parse");
assert_eq!(
Node::Directory(
DirectoryNode::new(
"".into(), // name must be empty
DIRECTORY_COMPLICATED.digest(),
DIRECTORY_COMPLICATED.size(),
)
.unwrap()
),
Node::Directory(DirectoryNode::new(
DIRECTORY_COMPLICATED.digest(),
DIRECTORY_COMPLICATED.size(),
)),
root_node,
);

6
tvix/store/src/nar/mod.rs
View file

@ -37,13 +37,13 @@ pub enum RenderError {
#[error("failure talking to a backing store client: {0}")]
StoreError(#[source] std::io::Error),
#[error("unable to find directory {}, referred from {:?}", .0, .1)]
#[error("unable to find directory {0}, referred from {1:?}")]
DirectoryNotFound(B3Digest, bytes::Bytes),
#[error("unable to find blob {}, referred from {:?}", .0, .1)]
#[error("unable to find blob {0}, referred from {1:?}")]
BlobNotFound(B3Digest, bytes::Bytes),
#[error("unexpected size in metadata for blob {}, referred from {:?} returned, expected {}, got {}", .0, .1, .2, .3)]
#[error("unexpected size in metadata for blob {0}, referred from {1:?} returned, expected {2}, got {3}")]
UnexpectedBlobMeta(B3Digest, bytes::Bytes, u32, u32),
#[error("failure using the NAR writer: {0}")]

33
tvix/store/src/nar/renderer.rs
View file

@ -8,11 +8,7 @@ use tokio::io::{self, AsyncWrite, BufReader};
use tonic::async_trait;
use tracing::{instrument, Span};
use tracing_indicatif::span_ext::IndicatifSpanExt;
use tvix_castore::{
blobservice::BlobService,
directoryservice::DirectoryService,
{NamedNode, Node},
};
use tvix_castore::{blobservice::BlobService, directoryservice::DirectoryService, Node};
pub struct SimpleRenderer<BS, DS> {
blob_service: BS,
@ -103,6 +99,7 @@ where
walk_node(
nar_root_node,
proto_root_node,
b"",
blob_service,
directory_service,
)
@ -115,7 +112,8 @@ where
/// This consumes the node.
async fn walk_node<BS, DS>(
nar_node: nar_writer::Node<'_, '_>,
proto_node: &Node,
castore_node: &Node,
name: &[u8],
blob_service: BS,
directory_service: DS,
) -> Result<(BS, DS), RenderError>
@ -123,10 +121,10 @@ where
BS: BlobService + Send,
DS: DirectoryService + Send,
{
match proto_node {
Node::Symlink(proto_symlink_node) => {
match castore_node {
Node::Symlink(symlink_node) => {
nar_node
.symlink(proto_symlink_node.target())
.symlink(symlink_node.target())
.await
.map_err(RenderError::NARWriterError)?;
}
@ -154,19 +152,19 @@ where
.await
.map_err(RenderError::NARWriterError)?;
}
Node::Directory(proto_directory_node) => {
Node::Directory(directory_node) => {
// look it up with the directory service
match directory_service
.get(proto_directory_node.digest())
.get(directory_node.digest())
.await
.map_err(|e| RenderError::StoreError(e.into()))?
{
// if it's None, that's an error!
None => Err(RenderError::DirectoryNotFound(
proto_directory_node.digest().clone(),
proto_directory_node.get_name().clone(),
directory_node.digest().clone(),
bytes::Bytes::copy_from_slice(name),
))?,
Some(proto_directory) => {
Some(directory) => {
// start a directory node
let mut nar_node_directory = nar_node
.directory()
@ -180,15 +178,16 @@ where
// for each node in the directory, create a new entry with its name,
// and then recurse on that entry.
for proto_node in proto_directory.nodes() {
for (name, node) in directory.nodes() {
let child_node = nar_node_directory
.entry(proto_node.get_name())
.entry(name)
.await
.map_err(RenderError::NARWriterError)?;
(blob_service, directory_service) = Box::pin(walk_node(
child_node,
proto_node,
node,
name.as_ref(),
blob_service,
directory_service,
))

22
tvix/store/src/pathinfoservice/fs/mod.rs
View file

@ -3,7 +3,7 @@ use futures::StreamExt;
use tonic::async_trait;
use tvix_castore::fs::{RootNodes, TvixStoreFs};
use tvix_castore::{blobservice::BlobService, directoryservice::DirectoryService};
use tvix_castore::{Error, Node, ValidateNodeError};
use tvix_castore::{Error, Node};
use super::PathInfoService;
@ -58,25 +58,31 @@ where
.get(*store_path.digest())
.await?
.map(|path_info| {
path_info
let node = path_info
.node
.as_ref()
.expect("missing root node")
.try_into()
.map_err(|e: ValidateNodeError| Error::StorageError(e.to_string()))
.to_owned();
match node.into_name_and_node() {
Ok((_name, node)) => Ok(node),
Err(e) => Err(Error::StorageError(e.to_string())),
}
})
.transpose()?)
}
fn list(&self) -> BoxStream<Result<Node, Error>> {
fn list(&self) -> BoxStream<Result<(bytes::Bytes, Node), Error>> {
Box::pin(self.0.as_ref().list().map(|result| {
result.and_then(|path_info| {
path_info
let node = path_info
.node
.as_ref()
.expect("missing root node")
.try_into()
.map_err(|e: ValidateNodeError| Error::StorageError(e.to_string()))
.to_owned();
node.into_name_and_node()
.map_err(|e| Error::StorageError(e.to_string()))
})
}))
}

7
tvix/store/src/pathinfoservice/grpc.rs
View file

@ -132,9 +132,10 @@ where
let path_info = self
.grpc_client
.clone()
.calculate_nar(tvix_castore::proto::Node {
node: Some(root_node.into()),
})
.calculate_nar(tvix_castore::proto::Node::from_name_and_node(
"".into(),
root_node.to_owned(),
))
.await
.map_err(|e| Error::StorageError(e.to_string()))?
.into_inner();

16
tvix/store/src/pathinfoservice/lru.rs
View file

@ -108,11 +108,17 @@ mod test {
let mut p = PATHINFO_1.clone();
let root_node = p.node.as_mut().unwrap();
if let castorepb::Node { node: Some(node) } = root_node {
let n = node.to_owned();
*node = (&tvix_castore::Node::try_from(&n)
.unwrap()
.rename("11111111111111111111111111111111-dummy2".into()))
.into();
match node {
castorepb::node::Node::Directory(n) => {
n.name = "11111111111111111111111111111111-dummy2".into()
}
castorepb::node::Node::File(n) => {
n.name = "11111111111111111111111111111111-dummy2".into()
}
castorepb::node::Node::Symlink(n) => {
n.name = "11111111111111111111111111111111-dummy2".into()
}
}
} else {
unreachable!()
}

8
tvix/store/src/pathinfoservice/nix_http.rs
View file

@ -228,10 +228,10 @@ where
}
Ok(Some(PathInfo {
node: Some(castorepb::Node {
// set the name of the root node to the digest-name of the store path.
node: Some((&root_node.rename(narinfo.store_path.to_string().into())).into()),
}),
node: Some(castorepb::Node::from_name_and_node(
narinfo.store_path.to_string().into(),
root_node,
)),
references: pathinfo.references,
narinfo: pathinfo.narinfo,
}))

2
tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
View file

@ -74,7 +74,7 @@ where
&self,
request: Request<castorepb::Node>,
) -> Result<Response<proto::CalculateNarResponse>> {
let root_node = (&request.into_inner()).try_into().map_err(|e| {
let (_, root_node) = request.into_inner().into_name_and_node().map_err(|e| {
warn!(err = %e, "invalid root node");
Status::invalid_argument("invalid root node")
})?;

13
tvix/store/src/proto/mod.rs
View file

@ -9,7 +9,7 @@ use nix_compat::{
store_path::{self, StorePathRef},
};
use thiserror::Error;
use tvix_castore::{NamedNode, ValidateNodeError};
use tvix_castore::DirectoryError;
mod grpc_pathinfoservice_wrapper;
@ -39,7 +39,7 @@ pub enum ValidatePathInfoError {
/// Node fails validation
#[error("Invalid root node: {:?}", .0.to_string())]
InvalidRootNode(ValidateNodeError),
InvalidRootNode(DirectoryError),
/// Invalid node name encountered. Root nodes in PathInfos have more strict name requirements
#[error("Failed to parse {} as StorePath: {1}", .0.to_str_lossy())]
@ -160,12 +160,13 @@ impl PathInfo {
let root_nix_path = match &self.node {
None => Err(ValidatePathInfoError::NoNodePresent)?,
Some(node) => {
// TODO save result somewhere
let node: tvix_castore::Node = node
.try_into()
let (name, _node) = node
.clone()
.into_name_and_node()
.map_err(ValidatePathInfoError::InvalidRootNode)?;
// parse the name of the node itself and return
parse_node_name_root(node.get_name(), ValidatePathInfoError::InvalidNodeName)?
parse_node_name_root(name.as_ref(), ValidatePathInfoError::InvalidNodeName)?
.to_owned()
}
};

36
tvix/store/src/proto/tests/pathinfo.rs
View file

@ -6,11 +6,11 @@ use nix_compat::nixbase32;
use nix_compat::store_path::{self, StorePath, StorePathRef};
use rstest::rstest;
use tvix_castore::proto as castorepb;
use tvix_castore::ValidateNodeError;
use tvix_castore::{DirectoryError, ValidateNodeError};
#[rstest]
#[case::no_node(None, Err(ValidatePathInfoError::NoNodePresent))]
#[case::no_node_2(Some(castorepb::Node { node: None}), Err(ValidatePathInfoError::InvalidRootNode(ValidateNodeError::NoNodeSet)))]
#[case::no_node_2(Some(castorepb::Node { node: None}), Err(ValidatePathInfoError::InvalidRootNode(DirectoryError::NoNodeSet)))]
fn validate_pathinfo(
#[case] node: Option<castorepb::Node>,
@ -35,7 +35,7 @@ fn validate_pathinfo(
name: DUMMY_PATH.into(),
digest: Bytes::new(),
size: 0,
}, Err(ValidatePathInfoError::InvalidRootNode(tvix_castore::ValidateNodeError::InvalidDigestLen(0))))]
}, Err(ValidatePathInfoError::InvalidRootNode(DirectoryError::InvalidNode(DUMMY_PATH.into(), ValidateNodeError::InvalidDigestLen(0)))))]
#[case::invalid_node_name_no_storepath(castorepb::DirectoryNode {
name: "invalid".into(),
digest: DUMMY_DIGEST.clone().into(),
@ -74,7 +74,7 @@ fn validate_directory(
digest: Bytes::new(),
..Default::default()
},
Err(ValidatePathInfoError::InvalidRootNode(tvix_castore::ValidateNodeError::InvalidDigestLen(0)))
Err(ValidatePathInfoError::InvalidRootNode(DirectoryError::InvalidNode(DUMMY_PATH.into(), ValidateNodeError::InvalidDigestLen(0))))
)]
#[case::invalid_node_name(
castorepb::FileNode {
@ -226,24 +226,28 @@ fn validate_inconsistent_narinfo_reference_name_digest() {
/// Create a node with an empty symlink target, and ensure it fails validation.
#[test]
fn validate_symlink_empty_target_invalid() {
let node = castorepb::node::Node::Symlink(castorepb::SymlinkNode {
name: "foo".into(),
target: "".into(),
});
tvix_castore::Node::try_from(&node).expect_err("must fail validation");
castorepb::Node {
node: Some(castorepb::node::Node::Symlink(castorepb::SymlinkNode {
name: "foo".into(),
target: "".into(),
})),
}
.into_name_and_node()
.expect_err("must fail validation");
}
/// Create a node with a symlink target including null bytes, and ensure it
/// fails validation.
#[test]
fn validate_symlink_target_null_byte_invalid() {
let node = castorepb::node::Node::Symlink(castorepb::SymlinkNode {
name: "foo".into(),
target: "foo\0".into(),
});
tvix_castore::Node::try_from(&node).expect_err("must fail validation");
castorepb::Node {
node: Some(castorepb::node::Node::Symlink(castorepb::SymlinkNode {
name: "foo".into(),
target: "foo\0".into(),
})),
}
.into_name_and_node()
.expect_err("must fail validation");
}
/// Create a PathInfo with a correct deriver field and ensure it succeeds.

84
tvix/store/src/tests/nar_renderer.rs
View file

@ -22,9 +22,7 @@ async fn single_symlink(
write_nar(
&mut buf,
&Node::Symlink(
SymlinkNode::new("doesntmatter".into(), "/nix/store/somewhereelse".into()).unwrap(),
),
&Node::Symlink(SymlinkNode::new("/nix/store/somewhereelse".into()).unwrap()),
// don't put anything in the stores, as we don't actually do any requests.
blob_service,
directory_service,
@ -44,15 +42,11 @@ async fn single_file_missing_blob(
) {
let e = write_nar(
sink(),
&Node::File(
FileNode::new(
"doesntmatter".into(),
HELLOWORLD_BLOB_DIGEST.clone(),
HELLOWORLD_BLOB_CONTENTS.len() as u64,
false,
)
.unwrap(),
),
&Node::File(FileNode::new(
HELLOWORLD_BLOB_DIGEST.clone(),
HELLOWORLD_BLOB_CONTENTS.len() as u64,
false,
)),
// the blobservice is empty intentionally, to provoke the error.
blob_service,
directory_service,
@ -92,15 +86,11 @@ async fn single_file_wrong_blob_size(
// Test with a root FileNode of a too big size
let e = write_nar(
sink(),
&Node::File(
FileNode::new(
"doesntmatter".into(),
HELLOWORLD_BLOB_DIGEST.clone(),
42, // <- note the wrong size here!
false,
)
.unwrap(),
),
&Node::File(FileNode::new(
HELLOWORLD_BLOB_DIGEST.clone(),
42, // <- note the wrong size here!
false,
)),
blob_service.clone(),
directory_service.clone(),
)
@ -117,15 +107,11 @@ async fn single_file_wrong_blob_size(
// Test with a root FileNode of a too small size
let e = write_nar(
sink(),
&Node::File(
FileNode::new(
"doesntmatter".into(),
HELLOWORLD_BLOB_DIGEST.clone(),
2, // <- note the wrong size here!
false,
)
.unwrap(),
),
&Node::File(FileNode::new(
HELLOWORLD_BLOB_DIGEST.clone(),
2, // <- note the wrong size here!
false,
)),
blob_service,
directory_service,
)
@ -161,15 +147,11 @@ async fn single_file(
write_nar(
&mut buf,
&Node::File(
FileNode::new(
"doesntmatter".into(),
HELLOWORLD_BLOB_DIGEST.clone(),
HELLOWORLD_BLOB_CONTENTS.len() as u64,
false,
)
.unwrap(),
),
&Node::File(FileNode::new(
HELLOWORLD_BLOB_DIGEST.clone(),
HELLOWORLD_BLOB_CONTENTS.len() as u64,
false,
)),
blob_service,
directory_service,
)
@ -207,14 +189,10 @@ async fn test_complicated(
write_nar(
&mut buf,
&Node::Directory(
DirectoryNode::new(
"doesntmatter".into(),
DIRECTORY_COMPLICATED.digest(),
DIRECTORY_COMPLICATED.size(),
)
.unwrap(),
),
&Node::Directory(DirectoryNode::new(
DIRECTORY_COMPLICATED.digest(),
DIRECTORY_COMPLICATED.size(),
)),
blob_service.clone(),
directory_service.clone(),
)
@ -225,14 +203,10 @@ async fn test_complicated(
// ensure calculate_nar does return the correct sha256 digest and sum.
let (nar_size, nar_digest) = calculate_size_and_sha256(
&Node::Directory(
DirectoryNode::new(
"doesntmatter".into(),
DIRECTORY_COMPLICATED.digest(),
DIRECTORY_COMPLICATED.size(),
)
.unwrap(),
),
&Node::Directory(DirectoryNode::new(
DIRECTORY_COMPLICATED.digest(),
DIRECTORY_COMPLICATED.size(),
)),
blob_service,
directory_service,
)