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

Azure Archive is broken #505

Closed
sbixl opened this issue Jan 28, 2023 · 7 comments · Fixed by #511
Closed

Azure Archive is broken #505

sbixl opened this issue Jan 28, 2023 · 7 comments · Fixed by #511
Labels

Comments

@sbixl
Copy link

sbixl commented Jan 28, 2023

Hi,

the Azure archive handling is broken, because the implementation depends on a (in the meantime) deprecated pip package azure-storage [1]. The import of BlockBlobService in the new pip package azure-storage-blob is not supported and the ContainerClient shall be used instead [2]. I studied the implementation of the class AzureArchive a little bit and it seems to be a bit more refactoring is required in order to switch to the new API. So I did not think I could provide a suitable pull request.

[1] https://stackoverflow.com/questions/58900507/upload-and-delete-azure-storage-blob-using-azure-storage-blob-or-azure-storage/58900508#58900508

[2] https://azuresdkdocs.blob.core.windows.net/$web/python/azure-storage-blob/12.0.0b5/azure.storage.blob.html#azure.storage.blob.ContainerClient

PS: If there is a bugfix available I can help with testing of up- and downloads of artifacts.

Regards,
Sebastian

@jkloetzke jkloetzke added the bug label Jan 30, 2023
@jkloetzke
Copy link
Member

I guess Bob should move to the new API. Reimplementing the REST-API is probably not a viable option.

@jkloetzke
Copy link
Member

@sbixl I made some tests but it would be nice if you could test the changes too...

@sbixl
Copy link
Author

sbixl commented Mar 3, 2023

I am currently having problems with my CI environment but at first glance the changes are working. The upload to the Azure account works without problems (tested with this). The download I can currently not test. I hope I get the problem solved over the weekend and can give feedback by the end of next week.

@sbixl
Copy link
Author

sbixl commented Mar 4, 2023

Uploading and downloading to an Azure blob container works fine. :-) The only thing I noticed is that downloading an artifact takes much longer when working with the archive type: azure. When I download via http from the Azure archive it is much faster.

Archive type: azure

tests::cmake::greeter-cross: Duration: 0:13:16.857533, 3 checkouts (0 overrides active), 5 packages built, 3 downloaded. tests::cmake::greeter-host: Duration: 0:33:30.627560, 3 checkouts (0 overrides active), 5 packages built, 1 downloaded. tests::python::test Duration: 0:01:52.666133, 2 checkouts (0 overrides active), 2 packages built, 1 downloaded.

Archive type: http

tests::cmake::greeter-cross: Duration: 0:02:05.800365, 3 checkouts (0 overrides active), 5 packages built, 3 downloaded. tests::cmake::greeter-host: Duration: 0:02:08.481711, 3 checkouts (0 overrides active), 5 packages built, 1 downloaded. tests::python::test: Duration: 0:01:09.372338, 2 checkouts (0 overrides active), 2 packages built, 1 downloaded.

Do you have any idea why? I suspect that this has nothing to do with the implementation.

@jkloetzke
Copy link
Member

I think I found a possible problem that could cause the download speed issues. The Azure classes seem to implement a synchronous read() which implies a full roundtrip to the server. I added a buffer that should hide this problem. Let me know if this improves the performance for you.

@sbixl
Copy link
Author

sbixl commented Mar 7, 2023

Looks much better now:

tests::cmake::greeter-cross: Duration: 0:03:39.353156, 3 checkouts (0 overrides active), 5 packages built, 3 downloaded. tests::cmake::greeter-host: Duration: 0:03:08.453847, 3 checkouts (0 overrides active), 5 packages built, 1 downloaded. tests::python::test: Duration: 0:00:54.736444, 2 checkouts (0 overrides active), 2 packages built, 1 downloaded.

Thanks for fixing that issue! :-)

@jkloetzke
Copy link
Member

Thanks for testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants