Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs: Blob transfer Architectural Decision Record #233

Merged
merged 46 commits into from
May 4, 2022

Conversation

ouphi
Copy link

@ouphi ouphi commented Apr 21, 2022

  • Added ADR explaining how Blob transfer should work.
  • Added sequence diagram to explain the data transfer flow.

The goal of this document is to explain what should be implemented. This document does not represents what is already implemented.
For example, right now DPF uses the storage account key (SHARED_KEY), but the SAS token is not used yet (not written and read from Vault).

@ouphi ouphi changed the title Docs/231 blob transfer adr Blob transfer ADR Apr 21, 2022
@github-actions
Copy link

github-actions bot commented Apr 21, 2022

Unit Test Results

     386 files   -     6       386 suites   - 6   8m 42s ⏱️ - 9m 43s
12 065 tests  - 130  12 046 ✔️  - 131  18 💤 ±0  1 +1 
12 126 runs   - 130  12 107 ✔️  - 131  18 💤 ±0  1 +1 

For more details on these failures, see this check.

Results for commit 6f41446. ± Comparison against base commit 6065693.

♻️ This comment has been updated with latest results.

@ouphi ouphi changed the title Blob transfer ADR Blob transfer Architectural Decision Record Apr 21, 2022
@ouphi ouphi marked this pull request as ready for review April 26, 2022 07:43
@ouphi ouphi removed the request for review from paullatzelsperger April 26, 2022 07:50
@ouphi ouphi changed the base branch from main to docs/1183-blob-transfer-adr April 26, 2022 07:51
@ouphi ouphi changed the title Blob transfer Architectural Decision Record Docs: Blob transfer Architectural Decision Record Apr 26, 2022
@zeier zeier linked an issue Apr 26, 2022 that may be closed by this pull request
7 tasks
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2022

Codecov Report

Merging #233 (6f41446) into docs/1183-blob-transfer-adr (6065693) will not change coverage.
The diff coverage is n/a.

@@                      Coverage Diff                       @@
##             docs/1183-blob-transfer-adr     #233   +/-   ##
==============================================================
  Coverage                          58.61%   58.61%           
  Complexity                          2731     2731           
==============================================================
  Files                                698      698           
  Lines                              15444    15444           
  Branches                            1045     1045           
==============================================================
  Hits                                9053     9053           
  Misses                              5967     5967           
  Partials                             424      424           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6065693...6f41446. Read the comment docs.

@zeier zeier requested a review from algattik April 26, 2022 08:44
@ouphi ouphi requested a review from algattik May 3, 2022 14:03
Copy link

@chrislomonico chrislomonico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved open comment

@chrislomonico
Copy link

@algattik @ouphi what is remaining in approving this PR and merging?

@algattik
Copy link

algattik commented May 4, 2022

12 and 13 (now 13 and 14) should be separate loops, these are not synchronised

@algattik
Copy link

algattik commented May 4, 2022

in (16) "deletes the container containing the blob and the SAS token" the token is not in the container. We should add an arrow (17) whereby the consumer deletes the Sas token in its vault.
"The ObjectStorageProvisioner is responsible for deprovisioning the container" and who is responsible for deprovisioning the SAS token?

@algattik
Copy link

algattik commented May 4, 2022

nit: it would make the puml clearer to have service activation boxes across 2/3/4/13 for Consumer, across 5/6 for Provider and across 6/7/8/9/10/11 for DPF and across 14/15 for Client, across 15/16/(new)17 for Consumer

@algattik
Copy link

algattik commented May 4, 2022

Pushed some proofreading fixes

@ouphi ouphi merged commit 7ce17a2 into docs/1183-blob-transfer-adr May 4, 2022
@ouphi ouphi deleted the docs/231-blob-transfer-adr branch May 4, 2022 14:54
algattik pushed a commit that referenced this pull request May 11, 2022
* Docs: Blob transfer Architectural Decision Record (#233)

Added Azure blob transfer Architectural Decision Record.

* Updated changelog.

* Fixed PR number in changelog.

* Applied suggestion.

Co-authored-by: Paul Latzelsperger <[email protected]>

* Applied suggestions.

Co-authored-by: Paul Latzelsperger <[email protected]>

* Update docs/developer/decision-records/2022-04-21-dpf-blob-data-transfer/README.md

Co-authored-by: Paul Latzelsperger <[email protected]>

* Update docs/developer/decision-records/2022-04-21-dpf-blob-data-transfer/README.md

Co-authored-by: Paul Latzelsperger <[email protected]>

* Update docs/developer/decision-records/2022-04-21-dpf-blob-data-transfer/README.md

Co-authored-by: Paul Latzelsperger <[email protected]>

* Moved client poll.

* Update docs/developer/decision-records/2022-04-21-dpf-blob-data-transfer/README.md

Co-authored-by: Paul Latzelsperger <[email protected]>

* Made 17 and 18 substep of 16.

Co-authored-by: Paul Latzelsperger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

End-to-end blob storage transfer is not working
7 participants