Use the new Azure SDK for C++. #3910

teo-tsirpanis · 2023-02-17T16:14:56Z

Note This PR is not ready to merge; the acquisition of the new SDK is very clumsy and works only on Windows. But it is ready for review.

To communicate with Azure Storage we have been using the Azure C++ Storage Client Lite which is no longer in active development. This PR switches the Azure VFS to use the new Azure SDK for C++. The dependency to the old SDK is removed.

The migration is mostly a straightforward change from the old SDK calls to the new ones. The new SDK does not use futures, simplifying the code. As a bonus, we now pass more detailed error messages. Some stuff around configuration will now work differently, and I added relevant warnings.

Azure unit tests succeed on my machine. Let's see what CI thinks.

SC-21996
SC-25039
SC-25045
SC-25046

TYPE: IMPROVEMENT
DESC: Use the new Azure SDK for C++.

shortcut-integration · 2023-02-17T16:15:01Z

This pull request has been linked to:

tiledb/sm/filesystem/azure.h

teo-tsirpanis · 2023-02-20T10:25:52Z

I think I found the problem. For some reason the BlobServiceClient's endpoint URI gets reset (or we create one with unset parameters).

jabbera · 2023-03-07T14:48:33Z

Does this mean the azure backend will now be supported on Windows?

ihnorton · 2023-03-07T14:50:01Z

@jabbera, yes - @teo-tsirpanis develops primarily on Windows, so it will definitely be supported there.

teo-tsirpanis · 2023-03-08T12:51:59Z

Well, on Windows CI vcpkg does not run at all. 🥲

teo-tsirpanis · 2023-03-08T14:09:50Z

CI on Linux seems to fail because the Azurite version we use is old.

teo-tsirpanis · 2023-03-08T14:49:54Z

I think I know why vcpkg doesn't get imported on Windows; we are setting CMAKE_TOOLCHAIN_FILE in the workflow through an environment variable.

ihnorton · 2023-03-08T19:51:33Z

I think I know why vcpkg doesn't get imported on Windows; we are setting CMAKE_TOOLCHAIN_FILE in the workflow through an environment variable.

Ugh. Good catch. That was used to work around this problem. Fix: #3949

teo-tsirpanis · 2023-03-22T15:31:27Z

The [azure] unit tests succeed on a premium block blob storage account on the real Azure cloud.

I kept a branch with the older Windows-only way of acquiring the Azure SDK that works on my machine, and tested it from there.

shaunrd0

Great work! 👍 I was able to run the UTs locally against Azurite and all of them passed. I ran into some problems with a debug build, and noticed the Azure tests take a long time to retry requests up to failure.

tiledb/sm/cpp_api/config.h

cmake/TileDB-Superbuild.cmake

shaunrd0 · 2023-03-27T17:13:43Z

tiledb/sm/filesystem/azure.cc

-    client_ = make_shared<azure::storage_lite::blob_client>(
-        HERE(), account, thread_pool_->concurrency_level(), cert_file);
+  ::Azure::Storage::Blobs::BlobClientOptions options;
+  options.Retry.MaxRetries = constants::azure_max_attempts;


I added the following options here, after trying to run UTs and hanging for some time..

options.Retry.RetryDelay = std::chrono::milliseconds(100); options.Retry.MaxRetryDelay = std::chrono::milliseconds(1000);

Azure SDK will increase the RetryDelay up to MaxRetryDelay for-each retry.. The default options seem to allow for up to 120s of hang time for a request retry. With the retry count set to 10, failing this request 10 times using these defaults could take quite a while. The C++ docs offer very little information on this 😕 so the other link here is from JS docs.

@shaunrd0 and I decided to use the default Azure SDK retry policy (three retries starting from 800ms and growing exponentially).

Can we make these config options with defaults? We should also standardize the timing and retry count with what we have for AWS if we can, will just make it simplier to document and explain to users.

I added config options. Standardizing across all cloud providers is hard because each SDK allows configuring the retry policies differently and with varying degrees of freedom.

tiledb/sm/filesystem/azure.h

tiledb/sm/filesystem/azure.cc

teo-tsirpanis · 2023-04-04T11:56:57Z

@davisp do I have to copy the new Azure SDK ports to the ports folder? What is the procedure for this? A simple copy-paste caused build errors.

davisp · 2023-04-04T15:18:26Z

@teo-tsirpanis Can you point me at the version you were trying to use? It shouldn't be much more than a copy/paste but the version resolution ends up being mostly manual because I've used super old port versions to match our existing Find${project}_EP.cmake configuration. There might be quite a bit of finagling on what new azure might force us to upgrade.

…cified.

They map to what the Azure SDK exposes and have its default values.

… S3.

ihnorton · 2023-04-23T13:22:42Z

CI fixes in #4055

A follow-up to #3910, this PR fixes typos, improves a message, and infers the blob endpoint from `vfs.azure.storage_account_name` if `vfs.azure.blob_endpoint` was not specified, matching the behavior of the previous SDK. I also added `CONFIG` changelog entries that I forgot to do in #3910. --- TYPE: IMPROVEMENT DESC: Improvements to the Azure VFS. TYPE: CONFIG DESC: Add vfs.azure.max_retries, vfs.azure.retry_delay_ms and vfs.azure.max_retry_delay_ms options to control the retry policy when connecting to Azure. TYPE: CONFIG DESC: Remove the vfs.azure.use_https option; connecting to Azure with HTTP will require specifying the scheme in vfs.azure.blob_endpoint instead. === * Remove unnecessary headers. * Fix typos and improve a message. * Construct the blob endpoint from the account name if not specified. * Bring back `vfs.azure.storage_sas_token`. * Append the SAS token to the blob endpoint if specified. This keeps backwards compatibility with the previous Azure SDK. * Update documentation of config values. * Fix typo.

teo-tsirpanis requested a review from ihnorton February 17, 2023 16:14

teo-tsirpanis force-pushed the azure branch from 52725a8 to a62a79a Compare February 17, 2023 16:18

teo-tsirpanis commented Feb 17, 2023

View reviewed changes

tiledb/sm/filesystem/azure.h Outdated Show resolved Hide resolved

teo-tsirpanis changed the title ~~[NO MERGE] Use the maintained Azure SDK for C++.~~ [NO MERGE] Use the new Azure SDK for C++. Feb 17, 2023

ihnorton added the pending-2.16 label Feb 17, 2023

teo-tsirpanis force-pushed the azure branch 2 times, most recently from cba3e2c to 27635f9 Compare February 22, 2023 13:21

teo-tsirpanis force-pushed the azure branch from 27635f9 to 11128f9 Compare March 3, 2023 11:56

teo-tsirpanis force-pushed the azure branch from a055248 to 2a56191 Compare March 8, 2023 09:47

teo-tsirpanis changed the title ~~[NO MERGE] Use the new Azure SDK for C++.~~ Use the new Azure SDK for C++. Mar 8, 2023

teo-tsirpanis force-pushed the azure branch 2 times, most recently from c271697 to 7f1e0cf Compare March 10, 2023 12:14

teo-tsirpanis force-pushed the azure branch from 7f1e0cf to eb29151 Compare March 22, 2023 10:58

ihnorton requested a review from shaunrd0 March 22, 2023 15:45

shaunrd0 requested changes Mar 27, 2023

View reviewed changes

teo-tsirpanis force-pushed the azure branch 2 times, most recently from 9f5c961 to 9844027 Compare March 28, 2023 14:39

teo-tsirpanis force-pushed the azure branch from 9844027 to d23dd0d Compare April 4, 2023 15:44

This comment was marked as off-topic.

Sign in to view

teo-tsirpanis added 13 commits April 11, 2023 16:34

Clone submodules on Windows CI.

a90b100

Do not depend on libcurl if we use Azure.

b688d7c

Fix indentation in vcpkg.json.

f0c1421

Fix documentation of the vfs.azure.blob_endpoint option.

f5e9209

Avoid calling getenv many times.

806d959

Do not warn about the Azure blob endpoint's scheme when it is not spe…

1c84903

…cified.

Simplify checking for the SAS token config option and use nullptr.

e81d9c6

Use the default Azure SDK retry policy.

0b6f43e

Remove vcpkg port overlays for the old Azure SDK.

f8e5e79

Include the Azure SDK headers first.

f431d9b

Use PollUntilDone to wait until the blob copying completes.

502ebbc

Fix formatting and compile errors.

a9b9bfd

Build spdlog as a static library.

26143c7

teo-tsirpanis force-pushed the azure branch from 3301a15 to 26143c7 Compare April 11, 2023 13:35

ihnorton and others added 8 commits April 15, 2023 17:29

Merge remote-tracking branch 'origin/dev' into teo-azure

699aaa4

Install missing autoconf/make required for build

79261a2

Fixes for Dockerfile build w/ vcpkg

e2549e1

Fix typo.

6d83532

Fix formatting.

b234248

Clean-up leftovers from removed config options.

df260f3

Add config options for the Azure retry policies.

ff79e3e

They map to what the Azure SDK exposes and have its default values.

Set the default value for vfs.azure.max_retries to 5; the same with…

7fbc6c6

… S3.

ihnorton approved these changes Apr 23, 2023

View reviewed changes

ihnorton merged commit 969dd1c into TileDB-Inc:dev Apr 23, 2023
45 of 47 checks passed

teo-tsirpanis deleted the azure branch April 23, 2023 13:26

teo-tsirpanis mentioned this pull request Apr 24, 2023

Improvements to the Azure VFS. #4058

Merged

jdblischak mentioned this pull request Jun 16, 2023

Nightly libtiledb builds have been failing for over a month TileDB-Inc/conda-forge-nightly-controller#7

Closed

teo-tsirpanis mentioned this pull request Oct 12, 2023

Prepend a question mark to the Azure Storage SAS token if it does not begin with one. #4418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the new Azure SDK for C++. #3910

Use the new Azure SDK for C++. #3910

teo-tsirpanis commented Feb 17, 2023 •

edited

Loading

shortcut-integration bot commented Feb 17, 2023

teo-tsirpanis commented Feb 20, 2023

jabbera commented Mar 7, 2023

ihnorton commented Mar 7, 2023

teo-tsirpanis commented Mar 8, 2023

teo-tsirpanis commented Mar 8, 2023

teo-tsirpanis commented Mar 8, 2023

ihnorton commented Mar 8, 2023

teo-tsirpanis commented Mar 22, 2023 •

edited

Loading

shaunrd0 left a comment

shaunrd0 Mar 27, 2023

teo-tsirpanis Mar 28, 2023

Shelnutt2 Mar 28, 2023

teo-tsirpanis Apr 19, 2023

teo-tsirpanis commented Apr 4, 2023

davisp commented Apr 4, 2023

This comment was marked as off-topic.

ihnorton commented Apr 23, 2023

Use the new Azure SDK for C++. #3910

Use the new Azure SDK for C++. #3910

Conversation

teo-tsirpanis commented Feb 17, 2023 • edited Loading

shortcut-integration bot commented Feb 17, 2023

teo-tsirpanis commented Feb 20, 2023

jabbera commented Mar 7, 2023

ihnorton commented Mar 7, 2023

teo-tsirpanis commented Mar 8, 2023

teo-tsirpanis commented Mar 8, 2023

teo-tsirpanis commented Mar 8, 2023

ihnorton commented Mar 8, 2023

teo-tsirpanis commented Mar 22, 2023 • edited Loading

shaunrd0 left a comment

Choose a reason for hiding this comment

shaunrd0 Mar 27, 2023

Choose a reason for hiding this comment

teo-tsirpanis Mar 28, 2023

Choose a reason for hiding this comment

Shelnutt2 Mar 28, 2023

Choose a reason for hiding this comment

teo-tsirpanis Apr 19, 2023

Choose a reason for hiding this comment

teo-tsirpanis commented Apr 4, 2023

davisp commented Apr 4, 2023

This comment was marked as off-topic.

ihnorton commented Apr 23, 2023

teo-tsirpanis commented Feb 17, 2023 •

edited

Loading

teo-tsirpanis commented Mar 22, 2023 •

edited

Loading