Suzuka Deletion Util and Followers Test Cleanup #660

l-monninger · 2024-10-04T18:56:47Z

Summary

RFCs: $\emptyset$.
Categories: misc.

Creates a suzuka-util CLI. Provides a subcommand to delete a syncing resource. Uses said subcommand to clean up test sync buckets in test-followers overlays.

Testing

Easiest Way to Test the Deletion Util

Compile the util:

cargo build -p suzuka-util # should compile binary to ./target/debug/suzuka-util

Run the simple interaction tests with a unique MOVEMENT_SYNC bucket:

MOVEMENT_SHARED_RANDOM_1=$(openssl rand -hex 16)
MOVEMENT_SYNC=leader::follower-test-$MOVEMENT_SHARED_RANDOM_1<=>{maptos,maptos-storage,suzuka-da-db}/**
# ensure you environment has AWS creds before running the below
just suzuka-full-node native build.setup.eth-local.celestia-local.test

Check that an s3 bucket was created [INTERNAL ONLY]
Run the deletion util.

DOT_MOVEMENT_PATH=./.movement ./target/debug/suzuka-util syncing delete-resource

Tests Added

Unit tests for MOVEMENT_SYNC parsing addded.
Unit tests for syncing preserved.
process-compose and docker-compose tests updated to include deletion.

Outstanding issues

.github/workflows/build-push-containers-all.yml

networks/suzuka/suzuka-config/src/syncing.rs

mzabaluev · 2024-10-07T14:20:56Z

networks/suzuka/suzuka-config/src/syncing.rs

+		};
+
+		// Split sync pattern on "<=>", expect exactly two parts (bucket and glob)
+		let mut bucket_arrow_glob = sync_pattern_part.split("<=>");


This also could use split_once.

mzabaluev · 2024-10-07T14:24:00Z

networks/suzuka/suzuka-config/src/syncing.rs

+		// Split sync pattern on "<=>", expect exactly two parts (bucket and glob)
+		let mut bucket_arrow_glob = sync_pattern_part.split("<=>");
+		let bucket = bucket_arrow_glob.next().context(
+			"MOVEMENT_SYNC environment variable must be in the format <bucket><=> <glob>",


This help is currently misleading due to a space on one side of the <=> separator (it would be aesthetically better to separate also in front?), while the parsing code does not trim whitespace from the separated values.

Do you want for me to trim?

I think it's more robust to trim if whitespaces could be tolerated by the syntax, and it costs almost nothing.

mzabaluev · 2024-10-07T14:25:34Z

networks/suzuka/suzuka-util/src/main.rs

@@ -0,0 +1,13 @@
+#![forbid(unsafe_code)]


This only has effect on this binary, better add this directive to lib.rs as well.

mzabaluev · 2024-10-07T14:29:02Z

util/dot-movement/src/sync.rs

+/*use movement_types::application;
+use syncup::{syncup, Target};*/

 impl DotMovement {
-	pub async fn sync(
+	/*pub async fn syncup(


Please clean up the dead code or restore depending on the eventual intent.

mzabaluev · 2024-10-07T14:29:33Z

util/dot-movement/src/sync.rs

+
+	pub async fn delete_sync_bucket(
+		&self,
+		bucket: String,
+	) -> Result<impl std::future::Future<Output = Result<(), anyhow::Error>>, anyhow::Error> {
+		let sync_task =
+			syncup(false, self.0.clone(), "", Target::S3(bucket), application_id).await?;
+		Ok(sync_task)
+	}*/


Is this also added as temporarily unused code?

mzabaluev · 2024-10-07T14:33:19Z

util/syncador/src/backend/s3/shared_bucket/mod.rs

+pub async fn create_random_with_application_id(
+	bucket: String,
+	application_id: application::Id,
+	pull_destination: PathBuf,
+) -> Result<(push::Push, pull::Pull), anyhow::Error> {


The proliferation of create_* functions look like a builder API could be a better solution.

I agree. Do you want that included in this PR?

Not sure, this is not a blocker.

mzabaluev · 2024-10-07T14:39:29Z

util/syncador/src/backend/s3/shared_bucket/mod.rs

-	application_id: application::Id,
-	pull_destination: PathBuf,
-) -> Result<(push::Push, pull::Pull), anyhow::Error> {
+pub async fn destroy_with_load_from_env(bucket: String) -> Result<(), anyhow::Error> {


The coupling of operation methods with a specific config load strategy is error-prone.

Consider something like this, maybe in conjunction with a builder pattern as suggested in the preceding comment:

let conn = BucketConnection::load_from_env("my_bucket").await?; conn.destroy().await?;

These helpers are being used for bucket operations from different processes, e.g., full node runtime and CLI.

Does your comment still apply under that pretense?

Yes, just perhaps a more composable API would be good where the loading of configuration from environment and connecting to AWS (the load_from_env part) is common between the paths that create and destroy the specified bucket (the create and destroy methods, respectively). This also wraps the AWS API more closely.

mzabaluev · 2024-10-07T14:42:08Z

util/syncador/src/backend/s3/shared_bucket/mod.rs

 	bucket: String,
 	pull_destination: PathBuf,
+	metadata: metadata::Metadata,
 ) -> Result<(push::Push, pull::Pull), anyhow::Error> {


I don't like how Push, Pull, and Metadata need to be always imported tautologically as push::Push, pull::Pull, and metadata::Metadata. I think it's better to re-export these names at this module level and make the push, pull, metadata modules a private implementation detail.

Okay, I will move them up.

Co-authored-by: Mikhail Zabaluev <mikhail.zabaluev@movementlabs.xyz>

l-monninger · 2024-10-08T17:42:06Z

DOT_MOVEMENT=/path/to/dot/movement suzuka-util syncing delete-reource

…z/movement into l-monninger/deletion-util

l-monninger · 2024-10-10T13:01:42Z

Ah, committing that suggestion is probably why you had build issues @0xPrimata

:wq#

…r/deletion-util

l-monninger added 5 commits October 3, 2024 16:40

fix: expand movement sync configuration.

f9acfe8

chore: refactor syncing API to facilitate suzuka-util.

e05cb08

fix: syncing is not particular to bucket.

0fccbad

fix: cleanup help.

d3c1e74

debug: load from env flaking.

d80f658

l-monninger requested review from andyjsbell, 0xmovses and mzabaluev as code owners October 4, 2024 18:56

l-monninger added cicd:suzuka-multi-node-local CI/CD for running multi node local tests. and removed cicd:suzuka-multi-node-local CI/CD for running multi node local tests. labels Oct 4, 2024

chore: add deletion to docker as well.

b25be5c

mzabaluev requested changes Oct 7, 2024

View reviewed changes

l-monninger and others added 2 commits October 7, 2024 08:39

fix: typo

07a9e88

Co-authored-by: Mikhail Zabaluev <mikhail.zabaluev@movementlabs.xyz>

fix: use split once

618ab24

Co-authored-by: Mikhail Zabaluev <mikhail.zabaluev@movementlabs.xyz>

l-monninger added 2 commits October 10, 2024 05:51

feat: disable non-leader upsync.

1e6bb89

Merge branch 'l-monninger/deletion-util' of github.com:movementlabsxy…

67fb000

…z/movement into l-monninger/deletion-util

fix: suggested commit.

551d12b

l-monninger added cicd:suzuka-multi-node-local CI/CD for running multi node local tests. and removed cicd:suzuka-multi-node-local CI/CD for running multi node local tests. labels Oct 10, 2024

l-monninger mentioned this pull request Oct 10, 2024

Remove Sync Task Participation from Followers #683

Merged

l-monninger added 7 commits October 11, 2024 03:34

fix: process compose.

32b84bd

fix: typo.

fbea7e3

fix: reintroduce path fix.

0e71d05

:wq#

fix: ammended commit.

0e8208f

Merge branch 'l-monninger/third-party-syncing-issues' into l-monninge…

67bec82

…r/deletion-util

fix: merge.

02ca804

fix: remove trailing space.

fe09f1c

fix: merge.

46bed8d

l-monninger requested a review from musitdev as a code owner November 15, 2024 14:51

l-monninger merged commit f81c95c into main Nov 15, 2024
44 of 48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suzuka Deletion Util and Followers Test Cleanup #660

Suzuka Deletion Util and Followers Test Cleanup #660

l-monninger commented Oct 4, 2024 •

edited

Loading

mzabaluev Oct 7, 2024

mzabaluev Oct 7, 2024

l-monninger Oct 7, 2024

mzabaluev Oct 7, 2024

mzabaluev Oct 7, 2024

mzabaluev Oct 7, 2024

mzabaluev Oct 7, 2024

mzabaluev Oct 7, 2024

l-monninger Oct 7, 2024

mzabaluev Oct 7, 2024

mzabaluev Oct 7, 2024

l-monninger Oct 7, 2024

l-monninger Oct 7, 2024

mzabaluev Oct 7, 2024 •

edited

Loading

mzabaluev Oct 7, 2024 •

edited

Loading

l-monninger Oct 7, 2024

l-monninger commented Oct 8, 2024

l-monninger commented Oct 10, 2024

Suzuka Deletion Util and Followers Test Cleanup #660

Suzuka Deletion Util and Followers Test Cleanup #660

Conversation

l-monninger commented Oct 4, 2024 • edited Loading

Summary

Testing

Outstanding issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

mzabaluev Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l-monninger commented Oct 8, 2024

l-monninger commented Oct 10, 2024

l-monninger commented Oct 4, 2024 •

edited

Loading

mzabaluev Oct 7, 2024 •

edited

Loading

mzabaluev Oct 7, 2024 •

edited

Loading