init s3 storage #29

Omarabdul3ziz · 2023-09-25T22:58:49Z

Implement S3 blob store #26

Also a lot of clean up and refactor

Next: performance improvments

also fix a file size bug

add cmdline to do unpacking

Also update docs

muhamadazmy · 2023-09-28T07:01:46Z

src/store/s3.rs

+use tokio::io::AsyncReadExt;
+
+fn get_config() -> Result<(String, String, Credentials)> {
+    // TODO: get these from .env?


Why env? the idea is that all stores can be defined in a url string. this can simply be

s3://[key:secret@]hostname[/bucket

So start by parsing the url, and extract the pieces that u need.

muhamadazmy · 2023-09-28T07:05:50Z

src/store/s3.rs

+
+        let client = S3Client::new_with(dispatcher, provider, region);
+
+        let create_bucket_request = CreateBucketRequest {


Do we need to create the bucket? i really think that should be already done by the user before as part of the operation. For example, a zdb store won't create the namespace but will use it. Same applies here

muhamadazmy · 2023-09-28T08:00:46Z

src/store/s3.rs

+        let put_object_request = PutObjectRequest {
+            bucket: self.bucket.clone(),
+            key: hex::encode(key),
+            body: Some(ByteStream::from(blob.to_owned())),
+            ..Default::default()
+        };


Why did u decide to use rusoto_s3? this one feels inefficient that u need to copy everything to make a PutObjectRequest (i see u copy the bucket, and also the blob itself which is a lot of data to be copied with every set call)

Please find another library that is better. checkout the s3 sdk from amazon or anything else.

this one looks easy enough https://docs.rs/rust-s3/0.33.0/s3/ and doesn't require u to copy everything all the time. U can create a single bucket object on ::new() and keep it on the store, then use that to do all the set/get calls directly

example here
https://github.com/durch/rust-s3/blob/master/examples/minio-tokio.rs

…ster_s3

…tions

muhamadazmy · 2023-10-04T11:48:07Z

src/store/mod.rs

@@ -52,6 +55,13 @@ pub enum Error {
    #[error("encryption error")]
    EncryptionError,

+    #[error("bucket creation error")]


I really don't like that the generic Error type for store has "implementation specific" errors. Those errors need to be not that specific for s3.

For example, there is Url error, there is also Other error which can hold an anyhow::Error so basically any error that does not fit the other error types.

muhamadazmy · 2023-10-04T11:50:39Z

src/store/s3store.rs

+fn get_config(url: &str) -> Result<(Credentials, Region, String)> {
+    let url = url::Url::parse(url.as_ref())?;
+
+    let (access_key, access_secret, endpoint, bucket_name, region_name) = match url.host() {


This is unreadable, instead this can go like this.

use anyhow::{Context}; // then in function do let host = url.host().context("no host in url")?; // continue working with the host value

instead of this

muhamadazmy · 2023-10-04T11:53:42Z

src/store/s3store.rs

+            let access_secret = url
+                .password()
+                .ok_or(Error::InvalidConfigs(String::from(
+                    "did not find secret key",
+                )))?
+                .to_string();


Is really the secret is required? I can see already u rewrap it in Some means it's not required by the s3 api, no ? This can simplified to

let access_secret = url.password().context("passowrd is required")?;

BUT since access_secret is already optional as seen below at line 56 this may be can be something like

let access_secret = url.password().map(|s| s.to_owned());

then access_secret will become Some(String)

muhamadazmy · 2023-10-04T11:56:17Z

src/store/s3store.rs

+            // rust-s3 implementation force tls unless it found `://` in the endpoint
+            // check rust-s3/aws-region/src/region.rs #fn scheme
+            let scheme = match url.query_pairs().find(|(key, _)| key == "tls") {
+                Some((_, value)) if value == "false" => "http://",
+                _ => "",
+            };
+


What is actually going on here?

If the api requires always the url has https schema why don't rewrite the url immediately to https ?. Why would i pass the tls as a query instead of doing something like
s3+tls:// as a schema or s3:// if we really need to support both https and http

muhamadazmy · 2023-10-04T12:00:36Z

src/store/s3store.rs

+}
+
+impl S3Store {
+    pub async fn new(


this does not need to be async since it doesn't do any await in code

muhamadazmy · 2023-10-04T12:01:50Z

src/store/s3store.rs

+#[async_trait::async_trait]
+impl Store for S3Store {
+    async fn get(&self, key: &[u8]) -> super::Result<Vec<u8>> {
+        let response = self.bucket.get_object(std::str::from_utf8(key)?).await?;


Key is not granted to be convertable to utf8 if the get_object REQUIRES the key to be a string, I assume it's better to do a hex of the provided binary. doing utf8(key) will most definitely fail when running

- use the generic Error::Other error - better support for both http and https - more readable error handling - hex encode the key in set/get

- add openssl crate - handle the `=` sign for params in the first parser

muhamadazmy

All looks good and works fine, can u just (beside the comment i left on code) update the readme.md to explain how to use the s3 store.

muhamadazmy · 2023-10-26T10:47:42Z

src/store/s3store.rs

+        .query_pairs()
+        .find(|(key, _)| key == "region")
+        .map(|(_, value)| value.to_string())
+        .context("region name not found")?;


Can't we instead have a default value? or even empty if not set?

muhamadazmy added 27 commits September 15, 2023 13:33

WIP: rewrite fl scehma

c8d902b

update routes

3f39b68

WIP: refactor for separate stores

bb44194

Separation of cache and store

1ce3cad

Also a lot of clean up and refactor

WIP: pack function

4281572

complete the pack/unpack cycle

46d5880

Store needs to return their url and range

0d959b9

fix bug in router and improve tests for pack/unpack

917f2d8

Apply clippy

00b2e3b

Assert routes creation

86ed118

improve tests

7b973e2

dynamic creation of stores based on url

8617e5e

start fs mount based on stores from the fl

833e781

first working version

f08ac50

Next: performance improvments

implement create

5bdc98d

also fix a file size bug

use different impl pack

4d909b0

use blake2

fb6462a

use blake2_simd implementation

75db699

fix bug in parent ino

cb5becc

implement unpack with no recurssion

f0390fe

add cmdline to do unpacking

add support to use multiple stores from cmdlin

0a706db

clean up dependencies

8df4b97

updating docs

5cdd2b0

fix typos

94d38fb

implement preserver-ownership flag

f27801f

make sure on get stores are tried in random order

cb795e6

Also update docs

fix ci

14b12e4

Omarabdul3ziz marked this pull request as ready for review September 27, 2023 12:24

Omarabdul3ziz requested a review from muhamadazmy September 27, 2023 12:24

Parallel upload

a8ed37b

Omarabdul3ziz added 2 commits September 27, 2023 16:33

init s3 storage

47d3ac1

better error handling and runnable code

5bb0268

Omarabdul3ziz force-pushed the master_s3 branch from 0039dd5 to 5bb0268 Compare September 27, 2023 13:37

Omarabdul3ziz changed the base branch from master to v2-meta-rewrite September 27, 2023 13:38

Omarabdul3ziz added 2 commits September 27, 2023 16:46

move to store directory and update cargo files

b74ebd8

wip: refactor s3store to implement store and register it

acdadc9

Omarabdul3ziz marked this pull request as draft September 27, 2023 15:17

muhamadazmy requested changes Sep 28, 2023

View reviewed changes

Base automatically changed from v2-meta-rewrite to master October 2, 2023 12:14

Omarabdul3ziz added 6 commits October 3, 2023 01:08

wip: use s3 crate and parse config from url

991d4f9

Merge branch 'master' of https://github.com/threefoldtech/rfs into ma…

0e9865b

…ster_s3

update lock file

80851b9

add test_get_config

2f45892

add region_name to the url as a param and better error propagation

cabe4a0

handle requests without tls and add test for setting and getting func…

fd0af8a

…tions

muhamadazmy requested changes Oct 4, 2023

View reviewed changes

Omarabdul3ziz added 4 commits October 4, 2023 22:55

resolve comments:

4f63125

- use the generic Error::Other error - better support for both http and https - more readable error handling - hex encode the key in set/get

- clearer errors

c6b78d5

- add openssl crate - handle the `=` sign for params in the first parser

use regex to capture range and url

fbd1533

add regex crate

119f56d

Omarabdul3ziz marked this pull request as ready for review October 16, 2023 07:11

muhamadazmy requested changes Oct 26, 2023

View reviewed changes

Omarabdul3ziz added 2 commits October 26, 2023 14:29

update docs, and make region optional

80064a5

change to unwrap_or_default

047c4cd

muhamadazmy approved these changes Oct 26, 2023

View reviewed changes

muhamadazmy merged commit 51faf1f into master Oct 26, 2023
1 check passed

muhamadazmy deleted the master_s3 branch October 26, 2023 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

init s3 storage #29

init s3 storage #29

Omarabdul3ziz commented Sep 25, 2023 •

edited

Loading

muhamadazmy Sep 28, 2023

muhamadazmy Sep 28, 2023

muhamadazmy Sep 28, 2023

muhamadazmy Sep 28, 2023

muhamadazmy Sep 28, 2023

muhamadazmy Sep 28, 2023

muhamadazmy Oct 4, 2023

muhamadazmy Oct 4, 2023

muhamadazmy Oct 4, 2023

muhamadazmy Oct 4, 2023

muhamadazmy Oct 4, 2023

muhamadazmy Oct 4, 2023

muhamadazmy Oct 4, 2023

muhamadazmy left a comment

muhamadazmy Oct 26, 2023


		let client = S3Client::new_with(dispatcher, provider, region);

		let create_bucket_request = CreateBucketRequest {

init s3 storage #29

init s3 storage #29

Conversation

Omarabdul3ziz commented Sep 25, 2023 • edited Loading

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

muhamadazmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Omarabdul3ziz commented Sep 25, 2023 •

edited

Loading