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

Upload and download data from and to volumes #95

Merged
merged 14 commits into from
Nov 20, 2024
Merged

Upload and download data from and to volumes #95

merged 14 commits into from
Nov 20, 2024

Conversation

PizieDust
Copy link
Collaborator

closes #93 #92 #32

This PR adds the functionality to download data and upload data to and from block devices.
There's also some refactoring for modals where all is moved to a single component, same with buttons.

At the moment, we can't download a volume if it is in use by a running unikernel, we also can't upload data to a volume which is in use by a running unikernel.

@PizieDust PizieDust added the enhancement New feature or request label Nov 20, 2024
@PizieDust PizieDust self-assigned this Nov 20, 2024
@PizieDust
Copy link
Collaborator Author

@hannesm I don't know if this is intended but uploads which are larger in size than the volume are successful. For example uploading a 5MB file to a 1MB volume is successful. I don't know if this should be the case.
also, empty uploads are successful (uploads with no file).

@@ -579,10 +579,32 @@ async function createVolume() {
const block_compressed = document.getElementById("block_compressed").checked;
const block_data = document.getElementById("block_data").files[0];

if (!isValidName(block_name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for refining the error message. Now, I'd appreciate if the deployUnikernel() as well gets such nice informative error messages, and best have both share the code....

So, could "isValidName" return the error message?

@hannesm
Copy link
Contributor

hannesm commented Nov 20, 2024

Thanks for your PR:

@hannesm I don't know if this is intended but uploads which are larger in size than the volume are successful. For example uploading a 5MB file to a 1MB volume is successful.

This is strange, from the albatross source:

  | `Block_set (compressed, data) ->
    begin match Vmm_resources.find_block t.resources id with
      | None -> Error (`Msg "set block: not found")
      | Some (_, true) -> Error (`Msg "set block: is in use")
      | Some (size, false) ->
        let* data =
          if compressed then
            Vmm_compress.uncompress data
          else
            Ok data
        in
        let* size_in_bytes = Vmm_unix.bytes_of_mb size in
        let* () =
          if size_in_bytes >= String.length data then
            Ok ()
          else
            Error (`Msg "data exceeds block size")
        in
        let* () = Vmm_unix.destroy_block id in
        let* () = Vmm_unix.create_block ~data id size in
        Ok (t, `End (`Success (`String "set block device")))
    end

So, we first lookup the block name in the resources, which returns us a size (in MB), then we compare that this size (converted to bytes) is greater or equal to the length of the uncompressed data (String.length data). Does, when you uploaded a 5 MB data blob to a 1 MB block device the device size increase to 5 MB? was all data written?

I don't know if this should be the case. also, empty uploads are successful (uploads with no file).

I don't know exactly what you mean here. So, if albatross receives a "Block_set" with an empty string, it writes all 0s to the block device (which is always truncated to the size that was specified when it was created).

What does mollymawk emit to albatross in the case of "uploads with no file"?

@PizieDust
Copy link
Collaborator Author

PizieDust commented Nov 20, 2024 via email

match
Utils.Json.(get "block_name" json_dict, get "compression_level" json_dict)
with
| Some (`String block_name), Some (`Int compression_level) -> (
Copy link
Contributor

Choose a reason for hiding this comment

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

here we need a different interface for albatross, and have some streaming or chunk read & write - likely with a chunked transfer-encoding.

unikernel.ml Outdated
~data:"Couldn't find block name in json" `Bad_request

let upload_to_volume albatross reqd (user : User_model.user) =
read_multipart_data reqd >>= fun result ->
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to unikernel_deploy, we should (at one day in our live) have a streaming interface here as well, instead of requiring the entire data fitting into memory.

the downside (and we'll likely need to revise the protocol) is that in TLS we can embed only 16MB of certificates, this means that we can't write/set multi-gigabyte block devices. for now, this is a shortcoming -- I opened an issue at the albatross repository robur-coop/albatross#201

unikernel.ml Outdated Show resolved Hide resolved
Co-authored-by: Hannes Mehnert <[email protected]>
@hannesm hannesm merged commit 7933ad7 into main Nov 20, 2024
2 checks passed
@hannesm hannesm deleted the updated_volumes branch November 20, 2024 18:44
@hannesm
Copy link
Contributor

hannesm commented Nov 20, 2024

Thanks, let's address suggestions in subsequent PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Create volume dialog, error messages
2 participants