fix(az): correctly remove nested file structures #449
Conversation
|
@M0dEx generally this looks good on initial review. Is there a test case you can add that fails against the live server without this fix and succeeds with it? |
Since the root cause might be specific container-level settings (retention, deletion policies, etc.), it has been difficult for us to replicate the issue reliably, especially using Azurite, which only provides a subset of the Azure Blob API features. I might be asking you to take a lot of faith here, but this change has fixed the behaviour of |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #449 +/- ##
========================================
- Coverage 93.7% 93.4% -0.4%
========================================
Files 23 23
Lines 1654 1658 +4
========================================
- Hits 1551 1549 -2
- Misses 103 109 +6
|
|
|
||
| # folders need to be deleted from the deepest to the shallowest | ||
| folders = sorted( | ||
| (blob.blob for blob, is_dir in blobs if is_dir and blob.exists()), reverse=True |
There was a problem hiding this comment.
I'm a little worried about perf since .exists adds a network call. This means that for large file trees (broad or deep) we potentially add a lot of calls for "fake" folders that don't actually exist on storage but _list_dir returns to act like a file system.
I believe that only explicitly created folders on blob storage (or maybe with certain parameters set) would still stick around without specific removal. To create these, you need to be using hierarchical namespaces/a Data Lake Gen2 Storage Account.
I think the fix may instead to be to use an azure-SDK API that will list all explicit blobs (files or folders) rather than _list_dir in determining what to remove. I'm not sure if list_blobs does that under accounts with the hierarchical namespaces, so I think we'd need to test that first to see if it works for your use cases.
Do you think you could dig in and see if those settings/accounts can repro the issue for you?
There was a problem hiding this comment.
I'm a little worried about perf since .exists adds a network call. This means that for large file trees (broad or deep) we potentially add a lot of calls for "fake" folders that don't actually exist on storage but _list_dir returns to act like a file system.
I don't think the call to exists here is strictly necessary, but I might be misremembering.
I believe that only explicitly created folders on blob storage (or maybe with certain parameters set) would still stick around without specific removal. To create these, you need to be using hierarchical namespaces/a Data Lake Gen2 Storage Account.
Hmm, on further investigation, it seems the "problematic" accounts are using hierarchical namespaces. The directories seems to be "sticky" even without being explicitly created, which is a bit surprising.
There was a problem hiding this comment.
That's very helpful to know, thanks. I'd love to support that feature of blob storage—I'll set one up on our test account and see how it works. We may need more fixes than this to make cloudpathlib play nice with hierarchical namespace enabled storage accounts.
As described in #448, the current
AzureBlobPath.rmtree()can sometimes fail and must be called again to remove all of the file/directory structure.This PR fixes this issue by first deleting all blobs, then deleting all folders, from the most nested to the least nested ones, and finally removing the root folder. This guarantees that the operation will always succeed.
Closes #448.