Add support for storing the "local" pool on Azure #1074

refi64 · 2022-05-19T13:29:44Z

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

This adds support for storing the "local" package pool on Azure. It includes:

Misc. test fixes from the Python 3 migration when passing --capture.
Adding some functional tests for Azure publishing based on the S3 ones, as a foundation for these changes.
Changes to the Azure endpoint syntax to match the official Azure connection string format.
Some refactoring of Azure publishing to move any code reused by the package pool to a shared file.
Tangentially related change to clean up temporary files when mirroring.

I described some of these in further detail in the individual commits.

This does not include changing all references to the "local" package pool, because I'm honestly not sure if that's worthwhile...

Checklist

unit-test added (if change is algorithm)
functional test added/updated (if change is functional)
man page updated (if applicable)
~~bash completion updated (if applicable)~~ (CLI was not touched)
~~documentation updated~~ (I don't believe the changes here would result in changes to the website content)
author name in AUTHORS

chuan

Nice work! Great to see the new functionality to store the package pool in the remote storage. I will take a more detailed pass of the change later today.

context/context.go

azure/package_pool.go

refi64 · 2022-05-25T15:53:22Z

Hmm, the Azure logs seem to have some 500 internal server errors from Azurite, I'll look into it locally.

refi64 · 2022-05-25T21:26:28Z

I can't reproduce the CI failure at all locally, so I added a step to upload Azurite's logs if the tests fail, which should hopefully help clear things up a bit...

refi64 · 2022-06-02T13:12:51Z

Can't use 'tar -xzf' extract archive file: /home/runner/work/_actions/_temp_4c5950e6-f316-4a7c-aa09-7b03239b3a59/3b979214-6f9f-46a7-976a-ed55121cf5e1.tar.gz. return code: 2.

...GitHub Actions issue maybe?

chuan · 2022-06-02T15:19:51Z

Can't use 'tar -xzf' extract archive file: /home/runner/work/_actions/_temp_4c5950e6-f316-4a7c-aa09-7b03239b3a59/3b979214-6f9f-46a7-976a-ed55121cf5e1.tar.gz. return code: 2.

...GitHub Actions issue maybe?

Where did you see this error?

Since in the CI #137 run only the long test for v1.17 failed, I suspect it is an intermittent issue with Azurite or the runtime environment.

refi64 · 2022-06-02T20:14:12Z

@chuan apologies for the confusion, that was from an earlier run, but I think @randombenj restarted it since then. There was a bug in the way config files were written that I missed, just pushed up a fix.

chuan · 2022-06-07T20:47:15Z

@refi64 I figured out that the Azurite 500 errors are due to its use of /tmp. The Azure tests in your change should pass the CI if you pull the change here: #1078.

refi64 · 2022-06-08T02:06:05Z

@chuan thanks! I just pushed a new version including that commit to see how it goes

.github/workflows/ci.yml

aptly/interfaces.go

system/t02_config/ConfigShowTest_gold

chuan · 2022-06-09T16:53:29Z

The latest change looks good to me and glad to see all CI jobs passed!

refi64 · 2022-06-13T18:54:40Z

Just rebased on top of the merged master incl. the Azurite fixes PR.

randombenj · 2022-06-20T08:14:11Z

As this is quite a big change and I am not too familiar with the source yet, this will probably take some time to review ...

randombenj · 2022-06-23T09:17:38Z

I am not sure if I understand all the changes yet ...
However, I am wondering a bit why the changes are necessary in the first place?
Couldn't you just mount the azure blob storage to the local disk and then run aptly as is to this directory?
This would be much simpler and aptly wouldn't have to implement all the read write logic?

chuan · 2022-06-23T14:13:02Z

I am not sure if I understand all the changes yet ... However, I am wondering a bit why the changes are necessary in the first place? Couldn't you just mount the azure blob storage to the local disk and then run aptly as is to this directory? This would be much simpler and aptly wouldn't have to implement all the read write logic?

Mounting Azure blob storage to local disk can usually be done via either Azure Blob NFS support or via FUSE driver, both have their own limitations.

Azure blob NFS protocol only supports limited features of blob storage. For example, users can only use it with a storage account that enables the hierarchical namespace and cannot enable NFS on an existing storage account. More limitations can be found here: https://docs.microsoft.com/en-us/azure/storage/blobs/network-file-system-protocol-known-issues

FUSE implementation (https://github.com/Azure/azure-storage-fuse) uses the same underlying REST APIs as the implementation here. It is targeting more generic use cases, has different requirements, and its implementation is much more complicated because it needs to support the complete set of filesystem APIs. Having our own storage and publish implementation directly based on REST APIs cuts through the middle layer and makes it possible to optimize for our own use cases. For example, the file moving operation is made atomic in the Azure publish support, which is not supported in FUSE.

Similar arguments also apply as to why we need to support publishing to S3 and OpenStack Swift.

refi64 · 2022-06-24T01:29:02Z

As an aside, in addition to the other reasons already detailed above, using FUSE filesystems for stuff like this tends to introduce a decent number of new and exciting bugs, simply because the ways that the filesystems deviate from standard POSIX behavior usually aren't accounted for in various tools. Quoting Benj Fassbind (2022-06-23 04:17:49)

…

I am not sure if I understand all the changes yet ... However, I am wondering a bit why the changes are necessary in the first place? Couldn't you just mount the azure blob storage to the local disk and then run aptly as is to this directory? This would be much simpler and aptly wouldn't have to implement all the read write logic? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.*Message ID: <aptly-dev/aptly ***@***.***>

randombenj

Thanks for this high quality patch! I think it also allows for future implementations in different storage backends.

One thing I was wondering however, what is your main use case for implementing this changes?

azure/package_pool_test.go

randombenj · 2022-06-27T15:53:10Z

system/azure_lib.py

+        if path.startswith('public/'):
+            path = path[7:]


Suggested change

if path.startswith('public/'):

path = path[7:]

path.removeprefix("public/")

system/azure_lib.py

codecov · 2022-06-27T16:09:56Z

Codecov Report

Attention: Patch coverage is 61.22449% with 133 lines in your changes are missing coverage. Please review.

Project coverage is 52.10%. Comparing base (b3d9055) to head (5307c1a).
Report is 88 commits behind head on master.

❗ Current head 5307c1a differs from pull request most recent head da0c525. Consider uploading reports for the commit da0c525 to get more accurate results

Files	Patch %	Lines
azure/package_pool.go	64.96%	33 Missing and 15 partials ⚠️
utils/config.go	34.37%	19 Missing and 2 partials ⚠️
azure/azure.go	76.82%	13 Missing and 6 partials ⚠️
context/context.go	0.00%	18 Missing ⚠️
api/mirror.go	0.00%	16 Missing ⚠️
files/public.go	47.61%	8 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1074       +/-   ##
===========================================
- Coverage   66.30%   52.10%   -14.21%     
===========================================
  Files         141       75       -66     
  Lines       15908    11491     -4417     
===========================================
- Hits        10548     5987     -4561     
- Misses       4610     4920      +310     
+ Partials      750      584      -166

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

read_path() can read in binary, which the S3 tests don't support (simply because they don't need it)...but it needs to be able to take the `mode` argument anyway. Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

None of the commands' output is ever treated as binary, so we can just always decode it as text. Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Before, a "partial" URL (either "localhost:port" or an endpoint URL *without* the account name as the subdomain) would be specified, and the full one would automatically be inferred. Although this is somewhat nice, it means that the endpoint string doesn't match the official Azure syntax: https://docs.microsoft.com/en-us/azure/storage/common/storage-configure-connection-string This also raises issues for the creation of functional tests for Azure, as the code to determine the endpoint string needs to be duplicated there as well. Instead, it's just easiest to follow Azure's own standard, and then sidestep the need for any custom logic in the functional tests. Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

The contents of `os.Stat` are rather fitted towards local package pools, but the method is in the generic PackagePool interface. This moves it to LocalPackagePool, and the use case of simply finding a file's size is delegated to a new, more generic PackagePool.Size() method. Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Several sections of the code *required* a LocalPackagePool, but they could still perform their operations with a standard PackagePool. Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

This adds support for storing packages directly on Azure, with no truly "local" (on-disk) repo used. The existing Azure PublishedStorage implementation was refactored to move the shared code to a separate context struct, which can then be re-used by the new PackagePool. In addition, the files package's mockChecksumStorage was made public so that it could be used in the Azure PackagePool tests as well. Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

neolynx · 2024-04-21T12:38:11Z

could you check if #1283 works for you ?

especially since a prefix was introduced in azure/public.go ...

chuan reviewed May 23, 2022

View reviewed changes

chuan reviewed May 24, 2022

View reviewed changes

context/context.go Outdated Show resolved Hide resolved

refi64 force-pushed the cloud-storage branch from 336ac82 to a36addc Compare May 24, 2022 13:25

chuan reviewed May 24, 2022

View reviewed changes

azure/package_pool.go Outdated Show resolved Hide resolved

chuan reviewed May 24, 2022

View reviewed changes

azure/package_pool.go Show resolved Hide resolved

randombenj self-requested a review May 25, 2022 08:24

refi64 force-pushed the cloud-storage branch from a36addc to 122af68 Compare May 25, 2022 21:25

chuan approved these changes May 25, 2022

View reviewed changes

refi64 force-pushed the cloud-storage branch from 326fc9a to 646035c Compare May 26, 2022 01:49

refi64 force-pushed the cloud-storage branch from 646035c to 36398a8 Compare June 2, 2022 20:13

refi64 force-pushed the cloud-storage branch from 36398a8 to ecb4b1d Compare June 8, 2022 02:05

chuan reviewed Jun 8, 2022

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

adalessandro reviewed Jun 8, 2022

View reviewed changes

aptly/interfaces.go Outdated Show resolved Hide resolved

chuan reviewed Jun 8, 2022

View reviewed changes

system/t02_config/ConfigShowTest_gold Show resolved Hide resolved

refi64 force-pushed the cloud-storage branch 2 times, most recently from 405bf8f to f158b3f Compare June 8, 2022 21:31

chuan approved these changes Jun 9, 2022

View reviewed changes

refi64 force-pushed the cloud-storage branch from f158b3f to 68f3691 Compare June 13, 2022 18:53

randombenj approved these changes Jun 27, 2022

View reviewed changes

randombenj force-pushed the cloud-storage branch from 68f3691 to 5307c1a Compare June 27, 2022 16:06

refi64 added 11 commits August 31, 2022 11:14

Fix S3 tests on Python 3

2a5b829

read_path() can read in binary, which the S3 tests don't support (simply because they don't need it)...but it needs to be able to take the `mode` argument anyway. Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Fix functional tests' '--capture' on Python 3

b23f40a

None of the commands' output is ever treated as binary, so we can just always decode it as text. Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Document Azure configuration

f6e77d8

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Add functional tests for Azure publishing

078ff79

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Add support for custom package pool locations

d802bfd

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Reduce required usage of LocalPackagePool

92678e5

Several sections of the code *required* a LocalPackagePool, but they could still perform their operations with a standard PackagePool. Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Clean up temporary files when mirroring

3ee9049

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Add myself to AUTHORS

da0c525

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

refi64 force-pushed the cloud-storage branch from 5307c1a to da0c525 Compare August 31, 2022 17:55

neolynx added the needs rebase The PR needs to be rebased on master label Apr 20, 2024

neolynx self-assigned this Apr 21, 2024

neolynx mentioned this pull request Apr 21, 2024

Add support for storing the "local" pool on Azure #1283

Open

6 tasks

neolynx added PR superseded and removed needs rebase The PR needs to be rebased on master labels Apr 21, 2024

lubo mentioned this pull request May 3, 2024

Store working data directly in S3 #843

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for storing the "local" pool on Azure #1074

Add support for storing the "local" pool on Azure #1074

refi64 commented May 19, 2022

chuan left a comment

refi64 commented May 25, 2022

refi64 commented May 25, 2022

refi64 commented Jun 2, 2022

chuan commented Jun 2, 2022 •

edited

refi64 commented Jun 2, 2022

chuan commented Jun 7, 2022 •

edited

refi64 commented Jun 8, 2022 •

edited

chuan commented Jun 9, 2022

refi64 commented Jun 13, 2022

randombenj commented Jun 20, 2022

randombenj commented Jun 23, 2022

chuan commented Jun 23, 2022

refi64 commented Jun 24, 2022 via email

randombenj left a comment

randombenj Jun 27, 2022

codecov bot commented Jun 27, 2022 •

edited

neolynx commented Apr 21, 2024

	if path.startswith('public/'):
	path = path[7:]
	path.removeprefix("public/")

Add support for storing the "local" pool on Azure #1074

Are you sure you want to change the base?

Add support for storing the "local" pool on Azure #1074

Conversation

refi64 commented May 19, 2022

Requirements

Description of the Change

Checklist

chuan left a comment

Choose a reason for hiding this comment

refi64 commented May 25, 2022

refi64 commented May 25, 2022

refi64 commented Jun 2, 2022

chuan commented Jun 2, 2022 • edited

refi64 commented Jun 2, 2022

chuan commented Jun 7, 2022 • edited

refi64 commented Jun 8, 2022 • edited

chuan commented Jun 9, 2022

refi64 commented Jun 13, 2022

randombenj commented Jun 20, 2022

randombenj commented Jun 23, 2022

chuan commented Jun 23, 2022

refi64 commented Jun 24, 2022 via email

randombenj left a comment

Choose a reason for hiding this comment

randombenj Jun 27, 2022

Choose a reason for hiding this comment

codecov bot commented Jun 27, 2022 • edited

Codecov Report

neolynx commented Apr 21, 2024

chuan commented Jun 2, 2022 •

edited

chuan commented Jun 7, 2022 •

edited

refi64 commented Jun 8, 2022 •

edited

codecov bot commented Jun 27, 2022 •

edited