Add multiple mirror sync with extensive include/exclude pattern matching #639

YYYasin19 · 2023-06-01T21:26:59Z

Hi,

this is a draft PR to check out how much effort it would be to

allow quetz to mirror packages from multiple channels
define more elaborate include/exclude list patterns on mirror channels

We've had the need for these features for a long time and, until now, worked around them with internal tooling.

Reasoning

Mirror Servers are not only used for load distribution but also access management to packages.
There are multiple common cases where it might be important to have more control over the sync feature:

Company provides access to a mirrored server that has package versions that are permitted (e.g. because of outstanding security checks or licensing issues)
A software company distributes IP (aka code) as packages to customers and needs to share this IP

Changes

The needed changes to allow all these are:

A more fine-grained filter on packages (e.g. my_cool_package=1.5.*)
Sync should also remove packages when they're not in the includelist anymore
Allow a channel to mirror multiple channels

I'm happy to discuss other (maybe even easier) ways of implementing this! 😊

codecov-commenter · 2023-06-01T21:45:25Z

Codecov Report

Patch coverage: 85.71% and project coverage change: -0.05 ⚠️

Comparison is base (282f8ab) 83.14% compared to head (77ddf40) 83.09%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #639      +/-   ##
==========================================
- Coverage   83.14%   83.09%   -0.05%     
==========================================
  Files          78       78              
  Lines        6153     6153              
==========================================
- Hits         5116     5113       -3     
- Misses       1037     1040       +3

Impacted Files	Coverage Δ
quetz/frontend.py	`37.03% <0.00%> (ø)`
quetz/tasks/mirror.py	`88.27% <75.00%> (-1.04%)`	⬇️
quetz/cli.py	`74.68% <80.00%> (ø)`
quetz/utils.py	`81.86% <81.48%> (ø)`
quetz/dao.py	`88.54% <100.00%> (ø)`
quetz/db_models.py	`95.18% <100.00%> (ø)`
quetz/main.py	`86.46% <100.00%> (ø)`
quetz/rest_models.py	`99.41% <100.00%> (ø)`
quetz/tasks/assertions.py	`75.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

wolfv · 2023-06-02T07:22:40Z

quetz/utils.py

@@ -39,6 +39,47 @@ def check_package_membership(package_name, includelist, excludelist):
    return True


+def _include_pattern_match(name, version, build, pattern):


This looks a lot like a MatchSpec - I think we should just use that :)

That means we should either depend on conda or vendor the MatchSpec part from conda into quetz. I think there were also discussions on making MatchSpec standalone but not sure if they ever got anywhere.

Nice catch!
We actually have conda as a dependency already because of conda-build and mamba apparently.

I'm now using MatchSpec and since it also works for just the package name, we might be able to get rid of the additional fields include_pattern_list and just use the include_list again, making the transition even simpler.

…pattern matching

… complicated patterns

…includelist/excludelist

YYYasin19 · 2023-08-23T12:43:25Z

rebased ✅

AndreasAlbertQC

@YYYasin19 thank you for this PR, great stuff! I reviewed ~half of the PR, feel free to already answer whatever makes sense. I hope to be able to pick up reviewing the rest of the PR later today.

docs/source/using/mirroring.rst

AndreasAlbertQC · 2023-09-05T08:41:30Z

pyproject.toml

@@ -20,7 +20,7 @@ check-imports = ["quetz"]
 ignore = ["W004"]

 [tool.tbump.version]
-current = "0.9.2"
+current = "0.10.0"


my intuition here would be that the version bump should not be part of this PR, but determined once the release is made. @janjagusch opinions?

Yes, I just used it for deployment reasons on the dev environment so we can revert this before merging

AndreasAlbertQC · 2023-09-05T08:41:40Z

quetz/_version.py

@@ -1,2 +1,2 @@
-version_info = (0, 9, 2, "", "")
+version_info = (0, 10, 0, "", "")


same q as above

quetz/dao.py

AndreasAlbertQC · 2023-09-05T09:13:55Z

quetz/tasks/mirror.py

@@ -235,38 +261,24 @@ def handle_repodata_package(
        total_size += size
        file.file.seek(0)

-    dao.assert_size_limits(channel_name, total_size)
+    # create package in database


Should this be uncommented?

The problem is that if we create packages already create packages from the channel metadata here (which seems to be some kind of.. shortcut?) than we have problems filtering them out later.

some solutions

call this code if no includelist/excludelist is set -> then we already sync everything

do the filtering based on includelist/excludelist here as well -> meh

I've implemented no. 1 for now

quetz/tasks/mirror.py

quetz/utils.py

AndreasAlbertQC · 2023-09-05T09:28:36Z

quetz/rest_models.py

+
+    """
+    examples:
+    - includelist: ["numpy", "pandas"]


The list mode is just syntactic sugar so users with a single mirror channel don't need to type a dict, right? I feel like the extra complication might not be worth it in this case, and I would prefer to just have a single way of doing this rather than two every so slightly different ones. What do you thikn?

I could also use the list when I mirror multiple channels but still have one list of packages I want to let through or exclude.
Also, it makes this stay backward-compatible 😬

AndreasAlbertQC

here's the second half -> let's meet up and talk some time

quetz/tasks/mirror.py

AndreasAlbertQC · 2023-09-05T10:17:27Z

quetz/tasks/mirror.py

                is_uptodate = None
                for _check in version_checks:
-                    is_uptodate = _check(package_name, metadata)
+                    is_uptodate = _check(repo_package_name, metadata)


Why repo_package_name instead of package_name?

I think the latter is a bit clearer about that it's something like numpy-1.23.4-py39hefdcf20_0.tar.bz2 and not just numpy

quetz/tasks/mirror.py

AndreasAlbertQC · 2023-09-05T10:26:19Z

quetz/tasks/mirror.py

+    """
+    logger.debug(f"Removing {len(remove_batch)} packages: {remove_batch}")
+    removal_performed = False
+    package_specs_remove = set([p[1].split("-")[0] for p in remove_batch])


Does this work if the package name contains hyphens? E.g. abseil-cpp?

It actually doesn't!
Are you aware of any tooling that might support this and prevent me from writing some weird unreadable regex that no one wants to touch again?

Alright, I've used Dist from conda.
-> We'll need to have a discussion about if we want to include conda as a dependency (which it already is in our dev environment) or vendor

AndreasAlbertQC · 2023-09-05T10:29:57Z

quetz/tasks/mirror.py

-    remote_repo = RemoteRepository(new_channel.mirror_channel_url, session)
-
-    user_id = auth.assert_user()
+        auth.assert_user()


Why doe we not need the user ID anymore here?

Because it was needed for create_packages_from_channeldata (will comment on that below)

Co-authored-by: Andreas Albert <[email protected]>

YYYasin19 · 2023-09-08T16:08:06Z

idea: We could, based on the "register yourself as a mirror on the source channel feature" also implement a trigger that sends a sync action to each "listening" mirror to update itself with the new package, i.e.: mirror1 mirrors the channel channel1. We add a package to channel1 which then sends a synchronize_packages action to all its mirrors.

ivergara · 2023-09-08T21:45:09Z

quetz/rest_models.py


 T = TypeVar('T')
-URLType = Annotated[str, Field(pattern="^(http|https)://.+")]
+URLType = NewType('URLType', constr(pattern="^(http|https)://.+|None|^(\\[.*\\])+$"))


Why do you have a URL type and define a pattern? https://docs.pydantic.dev/latest/usage/types/urls/

Hm, my thought was that due to implementation of AnyHttpUrl in Pydantic

AnyHttpUrl = Annotated[Url, UrlConstraints(allowed_schemes=['http', 'https'])]

this won't work as well.
But afaik, they only require Python 3.7 which we do as well.

wolfv reviewed Jun 2, 2023

View reviewed changes

YYYasin19 force-pushed the mirror-sync branch from b708632 to d050205 Compare June 29, 2023 13:41

janjagusch added the enhancement New feature or request label Jun 29, 2023

janjagusch changed the title ~~multiple mirror sync with extensive include/exclude pattern matching~~ Add multiple mirror sync with extensive include/exclude pattern matching Jun 29, 2023

YYYasin19 added 15 commits August 23, 2023 14:40

start: allow multiple mirrors per package, extensive include/exclude …

cb4f948

…pattern matching

use condas MatchSpec utility

267b94a

package spec relies on = while our repo package names use '-'

557cec0

validate that package was therefore before delete attempt is issued

df8a217

update: include condas matchspec

5815902

refactor: extract repodata retrieval || add: package matching on more…

de27969

… complicated patterns

add: support for excludelist

dbd3f75

add: new database model for accessing channel urls

1d4af06

fix: not set mirror_channel_url

0f07838

fix: mirror_channel_urlS

a5e3847

add: new tests for validating pattern checking

8217368

refactor: channel membership is completely refactored

c3c5467

add + refactor: option to remove local packages that would _not_ fit …

b2af339

…includelist/excludelist

refactoring: clean up code, extract methods, document some others

76e7c29

debug: update version to validate deployment

1f6be80

YYYasin19 force-pushed the mirror-sync branch from a1703e0 to 1f6be80 Compare August 23, 2023 12:42

YYYasin19 added 2 commits August 29, 2023 09:08

add custom URLType for validating urls

8b4b965

fix rest models

223e29a

AndreasAlbertQC requested changes Sep 5, 2023

View reviewed changes

YYYasin19 and others added 4 commits September 8, 2023 10:20

Update docs/source/using/mirroring.rst

3ca604c

Co-authored-by: Andreas Albert <[email protected]>

Update quetz/dao.py

854d471

Co-authored-by: Andreas Albert <[email protected]>

fix

cf5cc7a

validate that proxy mode only allows a single mirror url

f6480b7

YYYasin19 added 9 commits September 8, 2023 11:19

add more commments

e0f3bf1

add docs

ce554b6

add comment

299d89b

rename method

1afd522

add type hints

0c83779

use dist for parsing

a3b39db

sooner package creation based on cond

fc5c4bf

simplify package removal from database

ff90938

update comments

5f951d0

YYYasin19 added 6 commits September 8, 2023 18:29

replaced Pydantic Field Validation with Annotated by constr

4108791

use enum instead of concrete values for mirror mode

2ed0e86

use enum instead of concrete values for mirror mode

183f482

add docstring

b51ef67

add docstring

5ded7dd

fix import

c0acdbc

ivergara reviewed Sep 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multiple mirror sync with extensive include/exclude pattern matching #639

Add multiple mirror sync with extensive include/exclude pattern matching #639

YYYasin19 commented Jun 1, 2023

codecov-commenter commented Jun 1, 2023

wolfv Jun 2, 2023

YYYasin19 Jun 9, 2023

YYYasin19 commented Aug 23, 2023

AndreasAlbertQC left a comment

AndreasAlbertQC Sep 5, 2023

YYYasin19 Sep 8, 2023

AndreasAlbertQC Sep 5, 2023

AndreasAlbertQC Sep 5, 2023

YYYasin19 Sep 8, 2023

YYYasin19 Sep 8, 2023

AndreasAlbertQC Sep 5, 2023

YYYasin19 Sep 8, 2023

AndreasAlbertQC left a comment

AndreasAlbertQC Sep 5, 2023

YYYasin19 Sep 8, 2023

AndreasAlbertQC Sep 5, 2023

YYYasin19 Sep 8, 2023

YYYasin19 Sep 8, 2023

AndreasAlbertQC Sep 5, 2023

YYYasin19 Sep 8, 2023

YYYasin19 commented Sep 8, 2023

ivergara Sep 8, 2023

YYYasin19 Sep 9, 2023

		@@ -39,6 +39,47 @@ def check_package_membership(package_name, includelist, excludelist):
		return True


		def _include_pattern_match(name, version, build, pattern):

		@@ -1,2 +1,2 @@
		version_info = (0, 9, 2, "", "")
		version_info = (0, 10, 0, "", "")

Add multiple mirror sync with extensive include/exclude pattern matching #639

Are you sure you want to change the base?

Add multiple mirror sync with extensive include/exclude pattern matching #639

Conversation

YYYasin19 commented Jun 1, 2023

Reasoning

Changes

codecov-commenter commented Jun 1, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YYYasin19 commented Aug 23, 2023

AndreasAlbertQC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasAlbertQC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YYYasin19 commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment