tahoe-lafs/tahoe-lafs
tahoe-lafs
/
tahoe-lafs
2
0
Fork 0
Code Issues Pull Requests 23 Packages Projects Releases 1 Wiki Activity

More immutable support for HTTP storage API #1178

Merged
itamarst merged 27 commits from 3860-http-more-immutables into master 2022-03-07 14:12:57 +00:00
Conversation 1 Commits 27 Files Changed 7 +537 -185
itamarst commented 2022-02-10 22:08:39 +00:00 (Migrated from github.com)
Copy Link

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860

I decided to limit the scope of the spec in a few places in order to simplify the implementation.

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3860 I decided to limit the scope of the spec in a few places in order to simplify the implementation.
coveralls commented 2022-02-22 20:29:35 +00:00 (Migrated from github.com)
Copy Link

Coverage Status

Coverage increased (+0.06%) to 94.613% when pulling 87ab56426a on 3860-http-more-immutables into 2444c55b36 on master.

[![Coverage Status](https://coveralls.io/builds/47127246/badge)](https://coveralls.io/builds/47127246) Coverage increased (+0.06%) to 94.613% when pulling **87ab56426ad1634ef97cd2ca3805cc07038cb2a4 on 3860-http-more-immutables** into **2444c55b36691eb303f80fa9ccb312caaee5069b on master**.
exarkun (Migrated from github.com) approved these changes 2022-03-04 15:32:27 +00:00
exarkun (Migrated from github.com) left a comment
Copy Link

Thanks! Looks good. A few minor comments inline. Please address and merge.

Thanks! Looks good. A few minor comments inline. Please address and merge.
src/allmydata/storage/http_server.py
exarkun (Migrated from github.com) commented 2022-03-04 14:50:50 +00:00
Copy Link

Typo = "Mape"

Typo = "Mape"
exarkun (Migrated from github.com) commented 2022-03-04 15:06:11 +00:00
Copy Link

I'm not sure what the consequences are, but the regex will match some strings that si_a2b will reject. For example, nomd2a65ylxjbqzsw7gcfh4ivr or h3q2wnkd4slieprxomfbczu7g6.

I guess this just means to_python should handle the exception and turn it into a ValidationError.

I'm not sure what the consequences are, but the regex will match some strings that `si_a2b` will reject. For example, `nomd2a65ylxjbqzsw7gcfh4ivr` or `h3q2wnkd4slieprxomfbczu7g6`. I guess this just means `to_python` should handle the exception and turn it into a `ValidationError`.
src/allmydata/storage/http_server.py
@ -246,0 +262,4 @@
try:
finished = bucket.write(offset, data)
except ConflictingWriteError:
request.setResponseCode(http.CONFLICT)
exarkun (Migrated from github.com) commented 2022-03-04 15:07:11 +00:00
Copy Link

Nice.

Nice.
src/allmydata/storage/http_server.py
@ -295,2 +332,4 @@
# https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3872
data = bucket.read(offset, end - offset)
request.setResponseCode(http.PARTIAL_CONTENT)
exarkun (Migrated from github.com) commented 2022-03-04 15:18:02 +00:00
Copy Link

Also, in practice, share number cannot be greater than 255 because zfec only supports 256 shares and is zero-indexed. However, I'm not sure there's any reason a storage server should know or care about that - perhaps the client is eventually upgraded to support another encoding scheme without that limitation. So ... just fyi.

Also, in practice, share number cannot be greater than 255 because zfec only supports 256 shares and is zero-indexed. However, I'm not sure there's any reason a storage server should know or care about that - perhaps the client is eventually upgraded to support another encoding scheme without that limitation. So ... just fyi.
src/allmydata/storage_client.py
@ -1130,7 +1140,7 @@ class _HTTPStorageServer(object):
client=immutable_client,
storage_index=storage_index,
share_number=share_num,
exarkun (Migrated from github.com) commented 2022-03-04 14:48:51 +00:00
Copy Link

This key creates a link between multiple upload actions taken by a single client. Currently a client could be configured to use Tor or I2P and probably avoid the creation of such a link.

We've already discussed other strategies for determining an upload key so I expect that this approach is just a step on the path towards that goal, which is fine. Maybe at this point it's worth having a ticket for the ultimate goal and linking to it here.

This key creates a link between multiple upload actions taken by a single client. Currently a client could be configured to use Tor or I2P and probably avoid the creation of such a link. We've already discussed other strategies for determining an upload key so I expect that this approach is just a step on the path towards that goal, which is fine. Maybe at this point it's worth having a ticket for the ultimate goal and linking to it here.
src/allmydata/test/test_storage_http.py
exarkun (Migrated from github.com) commented 2022-03-04 14:06:24 +00:00
Copy Link

im_... first made me think of "instance method". Tahoe uses "imm" as an abbreviation for "immutable" in some places already, maybe that would be a better component for this name.

`im_...` first made me think of "instance method". Tahoe uses "imm" as an abbreviation for "immutable" in some places already, maybe that would be a better component for this name.
exarkun (Migrated from github.com) commented 2022-03-04 14:07:29 +00:00
Copy Link

though now I see this is an existing name that just got shuffled around a little bit, so not a blocker for this PR if you don't want

though now I see this is an existing name that just got shuffled around a little bit, so not a blocker for this PR if you don't want
src/allmydata/test/test_storage_http.py
@ -409,3 +524,3 @@
im_client.write_share_chunk(
self.imm_client.write_share_chunk(
storage_index,
share_number,
exarkun (Migrated from github.com) commented 2022-03-04 14:11:37 +00:00
Copy Link

I think this just moved around but this is the same as bytes(range(16)) right?

I think this just moved around but this is the same as `bytes(range(16))` right?
src/allmydata/test/test_storage_http.py
exarkun (Migrated from github.com) commented 2022-03-04 14:36:51 +00:00
Copy Link

Did the coverage proposed by test_bucket_allocated_with_new_shares get added somewhere? Or did it prove to be redundant?

Did the coverage proposed by `test_bucket_allocated_with_new_shares` get added somewhere? Or did it prove to be redundant?
src/allmydata/test/test_storage_http.py
exarkun (Migrated from github.com) commented 2022-03-04 14:35:21 +00:00
Copy Link

The docstring and the test method name don't seem to agree here. The docstring looks more accurate to me - but I am not an expert at eyeball parsing byte ranges. Does this test coverage the negative offset and length cases? If not, it looks like those tests are missing.

The docstring and the test method name don't seem to agree here. The docstring looks more accurate to me - but I am not an expert at eyeball parsing byte ranges. Does this test coverage the negative offset and length cases? If not, it looks like those tests are missing.
exarkun (Migrated from github.com) commented 2022-03-04 15:32:04 +00:00
Copy Link

(on further reading, I think "bytes=-2-9" is the negative offset case and "bytes=0--10" is the negative length case, if so, cool)

(on further reading, I think `"bytes=-2-9"` is the negative offset case and `"bytes=0--10"` is the negative length case, if so, cool)
itamarst (Migrated from github.com) reviewed 2022-03-07 13:22:57 +00:00
src/allmydata/test/test_storage_http.py
itamarst (Migrated from github.com) commented 2022-03-07 13:22:56 +00:00
Copy Link

May as well fix it before I wrote more tests.

May as well fix it before I wrote more tests.
itamarst (Migrated from github.com) reviewed 2022-03-07 13:27:29 +00:00
src/allmydata/test/test_storage_http.py
itamarst (Migrated from github.com) commented 2022-03-07 13:27:29 +00:00
Copy Link

test_allocate_buckets_second_time_different_shares covers this.

`test_allocate_buckets_second_time_different_shares` covers this.
itamarst (Migrated from github.com) reviewed 2022-03-07 13:29:06 +00:00
src/allmydata/storage_client.py
@ -1130,7 +1140,7 @@ class _HTTPStorageServer(object):
client=immutable_client,
storage_index=storage_index,
share_number=share_num,
itamarst (Migrated from github.com) commented 2022-03-07 13:29:06 +00:00
Copy Link

Hm. This is a... API mismatch, if I remember correctly. I will try to fix it now, if I can. If I can't I'll open a ticket with explanation of the problem.

Hm. This is a... API mismatch, if I remember correctly. I will try to fix it now, if I can. If I can't I'll open a ticket with explanation of the problem.
itamarst (Migrated from github.com) reviewed 2022-03-07 13:53:01 +00:00
src/allmydata/storage_client.py
@ -1130,7 +1140,7 @@ class _HTTPStorageServer(object):
client=immutable_client,
storage_index=storage_index,
share_number=share_num,
itamarst (Migrated from github.com) commented 2022-03-07 13:53:00 +00:00
Copy Link

Might be fixable, but will require some logic changes in allocating buckets on HTTP side, potentially with other security implications, so I will merge this without that and do it as follow-up ticket so it's easier to review.

Might be fixable, but will require some logic changes in allocating buckets on HTTP side, potentially with other security implications, so I will merge this without that and do it as follow-up ticket so it's easier to review.
Sign in to join this conversation.
Reviewers
No reviewers
exarkun
Labels
Clear labels   
Benchmarking and Performance

   
HTTP Storage Protocol

   
Nevow Removal

   
Python 3 Porting

   
not-for-merge
No Label
Benchmarking and Performance
HTTP Storage Protocol
Nevow Removal
Python 3 Porting
not-for-merge
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: tahoe-lafs/tahoe-lafs#1178
No description provided.
Delete Branch "3860-http-more-immutables"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?