Merge pull request #902 from ATIX-AG/optimize_improvements

Improve optimize sync conditions

CHANGES/903.bugfix

@@ -0,0 +1 @@
+Optimize mode can now take effect, when switching from mirrored to not mirrored mode between syncs.

pulp_deb/app/tasks/synchronizing.py

@@ -176,12 +176,6 @@ def synchronize(remote_pk, repository_pk, mirror, optimize):
     repository = AptRepository.objects.get(pk=repository_pk)
     previous_repo_version = repository.latest_version()
 
-    # TODO: The optimize feature currently does not work in combination with mirror=True! We fall
-    # back to full sync for now, until a proper fix is ready:
-    if mirror:
-        log.info(_("Falling back to optimize=False behaviour since mirror=True is set!"))
-        optimize = False
-
     if not remote.url:
         raise ValueError(_("A remote must have a url specified to synchronize."))
 
@@ -546,19 +540,30 @@ def __init__(self, remote, optimize, mirror, previous_repo_version, *args, **kwa
         self.remote = remote
         self.optimize = optimize
         self.previous_repo_version = previous_repo_version
-        self.previous_sync_info = defaultdict(dict, previous_repo_version.info)
         self.sync_info = defaultdict()
         self.sync_info["remote_options"] = self._gen_remote_options()
         self.sync_info["sync_options"] = {
             "optimize": optimize,
             "mirror": mirror,
         }
         self.parsed_url = urlparse(remote.url)
-        self.sync_options_unchanged = (
-            self.previous_sync_info["remote_options"] == self.sync_info["remote_options"]
-            and self.previous_sync_info["sync_options"]["mirror"]
-            == self.sync_info["sync_options"]["mirror"]
-        )
+        if self.optimize:
+            previous_sync_info = defaultdict(dict, self.previous_repo_version.info)
+            if not previous_sync_info:
+                log.info(_("Setting optimize=False since there is no previous_sync_info."))
+                self.optimize = False
+            elif not previous_sync_info["remote_options"] == self.sync_info["remote_options"]:
+                log.info(_("Setting optimize=False since the remote options have changed."))
+                self.optimize = False
+            elif mirror and not previous_sync_info["sync_options"]["mirror"]:
+                log.info(_("Setting optimize=False since this sync switches to mirror=True."))
+                self.optimize = False
+            # TODO: https://github.com/pulp/pulp_deb/issues/631
+            if mirror:
+                log.info(_("Falling back to optimize=False behaviour since mirror=True is set!"))
+                log.info(_("See https://github.com/pulp/pulp_deb/issues/631 for more information."))
+                self.optimize = False
+                self.sync_info["sync_options"]["optimize"] = False
 
     async def run(self):
         """
@@ -618,7 +623,7 @@ async def _handle_distribution(self, distribution):
         release_file = await self._create_unit(release_file_dc)
         if release_file is None:
             return
-        if self.optimize and self.sync_options_unchanged:
+        if self.optimize:
             previous_release_file = await _get_previous_release_file(
                 self.previous_repo_version, distribution
             )
@@ -899,7 +904,7 @@ async def _handle_package_index(
             else:
                 raise NoPackageIndexFile(relative_dir=package_index_dir)
 
-        if self.optimize and self.sync_options_unchanged:
+        if self.optimize:
             previous_package_index = await _get_previous_package_index(
                 self.previous_repo_version, relative_path
             )

pulp_deb/tests/functional/api/test_sync.py

@@ -303,6 +303,27 @@ def test_sync_optimize_skip_unchanged_package_index(
     assert apt_release_component_api.list(package=f"{package_href},{repo.pulp_href}").count == 1
 
 
+@pytest.mark.parallel
+def test_sync_optimize_switch_to_no_mirror(
+    deb_init_and_sync,
+):
+    """
+    Test that when syncing a repo with mirror=True, and then re-syncing that repo with
+    mirror=False, optimize=True, the releases will be skipped by optimize mode.
+    """
+
+    sync_args = {"mirror": True}
+    repo, remote, task = deb_init_and_sync(sync_args=sync_args, return_task=True)
+    assert repo.latest_version_href.endswith("/1/")
+    assert not is_sync_skipped(task, DEB_REPORT_CODE_SKIP_RELEASE)
+    sync_args = {"optimize": True, "mirror": False}
+    repo, _, task = deb_init_and_sync(
+        repository=repo, remote=remote, sync_args=sync_args, return_task=True
+    )
+    assert repo.latest_version_href.endswith("/1/")
+    assert is_sync_skipped(task, DEB_REPORT_CODE_SKIP_RELEASE)
+
+
 def test_sync_orphan_cleanup_fail(
     deb_init_and_sync,
     orphans_cleanup_api_client,

pulp_deb/tests/functional/conftest.py

@@ -447,14 +447,18 @@ def deb_sync_repository(apt_repository_api, monitor_task):
     and returns the monitored task.
     """
 
-    def _deb_sync_repository(remote, repo):
+    def _deb_sync_repository(remote, repo, mirror=False, optimize=True):
         """Sync a given remote and repository.
 
         :param remote: The remote where to sync from.
         :param repo: The repository that needs syncing.
+        :param mirror: Whether the sync should use mirror mode. Default False.
+        :param optimize: Whether the sync should use optimize mode. Default True.
         :returns: The task of the sync operation.
         """
-        repository_sync_data = AptRepositorySyncURL(remote=remote.pulp_href)
+        repository_sync_data = AptRepositorySyncURL(
+            remote=remote.pulp_href, mirror=mirror, optimize=optimize
+        )
         sync_response = apt_repository_api.sync(repo.pulp_href, repository_sync_data)
         return monitor_task(sync_response.task)
 
@@ -580,7 +584,13 @@ def deb_init_and_sync(
     """Initialize a new repository and remote and sync the content from the passed URL."""
 
     def _deb_init_and_sync(
-        repository=None, remote=None, url=None, remote_args={}, repo_args={}, return_task=False
+        repository=None,
+        remote=None,
+        url=None,
+        remote_args={},
+        repo_args={},
+        sync_args={},
+        return_task=False,
     ):
         """Initializes and syncs a repository and remote.
 
@@ -589,6 +599,7 @@ def _deb_init_and_sync(
         :param url: The name of the data repository. Default: None -> /debian/.
         :param remote_args: Parameters for the remote creation. Default {}.
         :param repo_args: Parameters for the repository creation. Default {}.
+        :param sync_args: Parameters for the sync API call. Default {}.
         :param return_task: Whether to include the sync task to the return value. Default: False.
         :returns: A tuple containing the repository and remote and optionally the sync task.
         """
@@ -598,7 +609,7 @@ def _deb_init_and_sync(
         if remote is None:
             remote = deb_remote_factory(url=url, **remote_args)
 
-        task = deb_sync_repository(remote, repository)
+        task = deb_sync_repository(remote, repository, **sync_args)
 
         repository = apt_repository_api.read(repository.pulp_href)
         return (repository, remote) if not return_task else (repository, remote, task)