GH-768 Store config uniqueness enforced by StoreManager by mkeen · Pull Request #1555 · TraceMachina/nativelink

mkeen · 2024-12-29T05:01:26Z

Description

Ensuring that when new Stores are added to StoreManager, that memory, experimental, filesystem, grpc, and redis_store stores will not experience overlap within a StoreManager instance. The same data store may be used for multiple values for the listed types only if relevant StoreSpec configuration values differ (such as connection details, key namespace, etc). Dictated by disallow_duplicates_digest implementation being present on the relevant StoreSpec enum type.

Primary changes:

Added a new helper: make_and_add_store_to_manager to make sure the store_factory function is not used for config that overlaps on enforced Store entities
Fortified add_store so that duplicate-named items cannot be added
Ensuring that unique-enforced StoreSpec entities are defined in stores.rs and enforced by store_manager.rs.
Ensured that StoreSpec entities that don't need to be unique aren't encumbered by uniqueness enforcement

DRAFT STATUS: Need to finalize tests and do a bit more cleanup where possible

Fixes #768

Type of change

[ x ] Bug fix (non-breaking change which fixes an issue)
[ x ] New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to
not work as expected)
[ x ] This change requires a documentation update

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

Updated documentation if needed
[ x ] Tests added/amended
[ x ] bazel test //... passes locally
PR is contained in a single commit, using git amend see some docs

This change is

CLAassistant · 2024-12-29T05:01:32Z

All committers have signed the CLA.

…ta source)

MarcusSorealheis

Beyond simplification requested, everything looks good.

MarcusSorealheis · 2024-12-29T22:56:32Z

-        .await?,
-    );
+        &StoreSpec::filesystem(FilesystemSpec {
+            content_path: current_dir


This is too complicated. Would be simpler to keep MemorySpec here. I know it isn't pure but we don't have consistent way to test FilesystemStore. We need one.

…'s model anyway

MarcusSorealheis

Looking good @mkeen There's a few things to tidy up and I also want one of two other maintainers to look at the rename and deletion of store_factory. After lots of thought, I'm in favor of that change, but we simply need to boost awareness.

I want to think just for a few more hours also about what the error text should say when there are conflicting store configurations. What you have is fine, but can it be improved. I'm not sure.

MarcusSorealheis · 2024-12-30T21:42:57Z

 async fn make_store_manager() -> Result<Arc<StoreManager>, Error> {
    let store_manager = Arc::new(StoreManager::new());
-    store_manager.add_store(
+    make_and_add_store_to_manager(


For my understanding, what is the reason why you moved away from store_factory rather than modifying store_factory?

Still using store_factory to build the Store, but wrapping up in make_and_add_store_to_manager because it makes building and adding a Store to a StoreManager a single step from a public-facing perspective.

MarcusSorealheis · 2024-12-30T21:44:40Z

    PublishBuildToolEventStreamRequest, PublishLifecycleEventRequest, StreamId,
 };
 use nativelink_service::bep_server::BepServer;
-use nativelink_store::default_store_factory::store_factory;


this seems heavy at first read but maybe it is an improvement to both the naming and the functionality. store_factory as a name says nothing about what its function is, but developers could go look at the definition. make_and_add_store_to_manager is definitely clearer.

store_factory is still used but I made private and think that make_and_add_store_to_manager is better as a front-facing API because it makes and associates the Store with a StoreManager -- rather than leaving them as separate steps. The door is still open to manually build any Store instance though and call add_store on StoreManager.

MarcusSorealheis · 2024-12-30T21:45:04Z

@@ -0,0 +1,48 @@
+// Copyright 2024 The NativeLink Authors. All rights reserved.


MarcusSorealheis · 2024-12-30T21:46:15Z


+
+
+


what are these spaces for?

I will remove these and make sure formatting gets run again. My mistake on these spaces here.

MarcusSorealheis · 2024-12-30T21:46:25Z

 use std::collections::{HashMap, HashSet};
+
+
+


and these? I mean empty lines.

MarcusSorealheis · 2024-12-30T21:46:33Z

+
+
+
+


MarcusSorealheis · 2024-12-30T21:47:28Z

 use nativelink_service::execution_server::ExecutionServer;
 use nativelink_service::health_server::HealthServer;
 use nativelink_service::worker_api_server::WorkerApiServer;
-use nativelink_store::default_store_factory::store_factory;


@allada and @aaronmondal you two are going to need to sign off on moving away from store_factory. I would say this new name is more self-describing for sure.

The big thing here guys is that make_and_add_store_to_manager enforces that all factory-built stores are part of a StoreManager so it's less likely that an unmanaged Store would be hanging around. Still possible to build them separately and pass into add_store this way though (useful for testing and probably other reasons). The old store_factory is still there and used by the new make_and_add_store_to_manager function, but store_factory is no longer public in this changeset.

mkeen · 2024-12-30T23:46:52Z

 // limitations under the License.

 use std::collections::HashMap;
+use std::ptr;


We don't need this.. will remove

mkeen · 2024-12-30T23:48:51Z

+        }
+
+        for existing_store in stores.values().into_iter() {
+            if ptr::eq(&store, existing_store) {


Will remove this check. Thanks to rust's memory model and the fact that in this design, ownership of the struct instance is passed to the StoreManager on line 58. We don't have Store instance pointer being passed around with a risk of passing the same pointer N times. We have a single instance of which ownership is transferred exactly once in guaranteed fashion (thanks Rust).

So instance protection is no longer a goal or needed. But I do think the config overlap detection is valuable.

Thanks Rust is an underrated comment.

allada

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 14 files reviewed, and pending CI: Bazel Dev / macos-13, Bazel Dev / macos-14, Bazel Dev / ubuntu-24.04, NativeLink.com Cloud / Remote Cache / macos-14, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-22.04, and 12 discussions need to be resolved

nativelink-config/src/stores.rs line 435 at r11 (raw file):

}

impl StoreSpec {

nit: We don't allow complex functions in the config. We use these configs as simple definitions with some macro magic (like number->string conversions and such in serde).

mkeen added 2 commits December 28, 2024 19:41

disallow name overlaps for stores

324df36

store flow improvements / anti-collision

9f67019

mkeen temporarily deployed to production December 29, 2024 05:01 — with GitHub Actions Inactive

mkeen had a problem deploying to production December 29, 2024 05:01 — with GitHub Actions Failure

mkeen commented Dec 29, 2024

View reviewed changes

Comment thread nativelink-store/src/default_store_factory.rs Outdated

remove toolchain toml from changeset

ccca3a6

mkeen temporarily deployed to production December 29, 2024 05:32 — with GitHub Actions Inactive

mkeen had a problem deploying to production December 29, 2024 05:32 — with GitHub Actions Failure

mkeen changed the title ~~GH-768 Store config uniqueness enforiced by StoreManager~~ GH-768 Store config uniqueness enforced by StoreManager Dec 29, 2024

fix deadlock and tests now failing for the right reason (duplicate da…

6bb7cc2

…ta source)

mkeen temporarily deployed to production December 29, 2024 17:58 — with GitHub Actions Inactive

mkeen had a problem deploying to production December 29, 2024 17:58 — with GitHub Actions Failure

tests looking 👍

01f23a7

mkeen temporarily deployed to production December 29, 2024 19:11 — with GitHub Actions Inactive

mkeen had a problem deploying to production December 29, 2024 19:11 — with GitHub Actions Error

mkeen had a problem deploying to production December 29, 2024 19:11 — with GitHub Actions Failure

adhere to formatting rules

4a4d11a

mkeen temporarily deployed to production December 29, 2024 19:20 — with GitHub Actions Inactive

mkeen had a problem deploying to production December 29, 2024 19:20 — with GitHub Actions Error

formatting in the .bazel file

d361abf

some test fixes

91a3557

mkeen temporarily deployed to production December 29, 2024 20:12 — with GitHub Actions Inactive

mkeen had a problem deploying to production December 29, 2024 20:12 — with GitHub Actions Failure

mkeen had a problem deploying to production December 29, 2024 20:12 — with GitHub Actions Error

use working dir for filesystem store in test

9adeef3

mkeen temporarily deployed to production December 29, 2024 20:25 — with GitHub Actions Inactive

mkeen had a problem deploying to production December 29, 2024 20:26 — with GitHub Actions Failure

MarcusSorealheis requested review from aaronmondal, allada and caass and removed request for caass December 29, 2024 21:08

use defaults for Filesystem store

0022bdf

mkeen temporarily deployed to production December 29, 2024 21:14 — with GitHub Actions Inactive

mkeen had a problem deploying to production December 29, 2024 21:14 — with GitHub Actions Failure

MarcusSorealheis requested changes Dec 29, 2024

View reviewed changes

MarcusSorealheis mentioned this pull request Dec 29, 2024

Make CAS and AC server not to use the same store #1175

Closed

6 tasks

still not final -- will remove ptr check -- not needed thanks to rust…

aff36b8

…'s model anyway

mkeen temporarily deployed to production December 30, 2024 21:20 — with GitHub Actions Inactive

mkeen had a problem deploying to production December 30, 2024 21:20 — with GitHub Actions Failure

MarcusSorealheis reviewed Dec 30, 2024

View reviewed changes

mkeen commented Dec 30, 2024

View reviewed changes

allada reviewed Dec 31, 2024

View reviewed changes

		@@ -0,0 +1,48 @@
		// Copyright 2024 The NativeLink Authors. All rights reserved.

Conversation

mkeen commented Dec 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist

Uh oh!

CLAassistant commented Dec 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

MarcusSorealheis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcusSorealheis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allada left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkeen commented Dec 29, 2024 •

edited

Loading

CLAassistant commented Dec 29, 2024 •

edited

Loading