From 6868481c1a0eeab06d1f18330e7fd7dc533d8007 Mon Sep 17 00:00:00 2001 From: Daniel Alley Date: Wed, 27 Jul 2016 08:46:17 -0400 Subject: [PATCH] Add generic checks for whether a full sync should be forced to the repo controller. closes #1983 --- .../integration/rest-api/repo/retrieval.rst | 2 - docs/user-guide/release-notes/2.11.x.rst | 12 ++ server/pulp/plugins/conduits/mixins.py | 4 +- server/pulp/server/controllers/repository.py | 122 ++++++++++++ .../migrations/0025_importer_schema_change.py | 23 +++ server/pulp/server/db/model/__init__.py | 15 ++ .../test/unit/plugins/conduits/test_mixins.py | 5 +- .../server/controllers/test_repository.py | 181 +++++++++++++++++- .../test_0025_importer_schema_change.py | 45 +++++ 9 files changed, 397 insertions(+), 12 deletions(-) create mode 100644 docs/user-guide/release-notes/2.11.x.rst create mode 100644 server/pulp/server/db/migrations/0025_importer_schema_change.py create mode 100644 server/test/unit/server/db/migrations/test_0025_importer_schema_change.py diff --git a/docs/dev-guide/integration/rest-api/repo/retrieval.rst b/docs/dev-guide/integration/rest-api/repo/retrieval.rst index 73f78f1f90..6556981fe9 100644 --- a/docs/dev-guide/integration/rest-api/repo/retrieval.rst +++ b/docs/dev-guide/integration/rest-api/repo/retrieval.rst @@ -324,7 +324,6 @@ will either be empty (no importer configured) or contain a single entry. "last_sync": "2015-11-06T10:38:23Z", "repo_id": "zoo", "scratchpad": { - "previous_skip_list": [], "repomd_revision": 1331832478 } } @@ -362,7 +361,6 @@ Retrieves the given :term:`importer` (if any) associated with a repository. "last_sync": "2015-11-06T10:38:23Z", "repo_id": "zoo", "scratchpad": { - "previous_skip_list": [], "repomd_revision": 1331832478 } } diff --git a/docs/user-guide/release-notes/2.11.x.rst b/docs/user-guide/release-notes/2.11.x.rst new file mode 100644 index 0000000000..766be4e39a --- /dev/null +++ b/docs/user-guide/release-notes/2.11.x.rst @@ -0,0 +1,12 @@ +======================= +Pulp 2.11 Release Notes +======================= + +Pulp 2.11.0 +=========== + +New Features +------------ + +* For RPM content, a full sync will be forced if the sync configuration has been changed or content + has been removed since the last sync. diff --git a/server/pulp/plugins/conduits/mixins.py b/server/pulp/plugins/conduits/mixins.py index 91a8e2f183..08791634f2 100644 --- a/server/pulp/plugins/conduits/mixins.py +++ b/server/pulp/plugins/conduits/mixins.py @@ -289,9 +289,7 @@ def set_scratchpad(self, value): :raises ImporterConduitException: wraps any exception that may occur in the Pulp server """ try: - importer = model.Importer.objects.get(repo_id=self.repo_id) - importer.scratchpad = value - importer.save() + model.Importer.objects.get(repo_id=self.repo_id).update(set__scratchpad=value) except Exception, e: _logger.exception(_('Error setting scratchpad for repo [%(r)s]') % {'r': self.repo_id}) raise ImporterConduitException(e), None, sys.exc_info()[2] diff --git a/server/pulp/server/controllers/repository.py b/server/pulp/server/controllers/repository.py index 9b76e1acf7..d77c2d3414 100644 --- a/server/pulp/server/controllers/repository.py +++ b/server/pulp/server/controllers/repository.py @@ -798,6 +798,10 @@ def sync(repo_id, sync_config_override=None, scheduled_call_id=None): sync_start_timestamp, summary, details, result_code, before_sync_unit_count) + # Update the override config if it has changed + if check_override_config_change(repo_id, call_config): + model.Importer.objects(repo_id=repo_id).\ + update(set__last_override_config=call_config.override_config) # Do an update instead of a save in case the importer has changed the scratchpad model.Importer.objects(repo_id=repo_obj.repo_id).update(set__last_sync=sync_end_timestamp) # Add a sync history entry for this run @@ -816,6 +820,124 @@ def sync(repo_id, sync_config_override=None, scheduled_call_id=None): return TaskResult(sync_result, spawned_tasks=spawned_tasks) +def check_unit_removed_since_last_sync(conduit, repo_id): + """ + Checks whether a content unit has been removed since the last_sync timestamp. + + :param conduit: allows the plugin to interact with core pulp + :type conduit: pulp.plugins.conduits.repo_sync.RepoSyncConduit + :param repo_id: identifies the repo to sync + :type repo_id: str + + :return: Whether a content unit has been removed since the last_sync timestamp + :rtype: bool + """ + last_sync = conduit.last_sync() + + if last_sync is None: + return False + + # convert the iso8601 datetime string to a python datetime object + last_sync = dateutils.parse_iso8601_datetime(last_sync) + repo_obj = model.Repository.objects.get_repo_or_missing_resource(repo_id=repo_id) + last_removed = repo_obj.last_unit_removed + + # check if a unit has been removed since the past sync + if last_removed is not None: + if last_removed > last_sync: + return True + + return False + + +def check_config_updated_since_last_sync(conduit, repo_id): + """ + Checks whether the config has been changed since the last sync occurred. + + :param conduit: allows the plugin to interact with core pulp + :type conduit: pulp.plugins.conduits.repo_sync.RepoSyncConduit + :param repo_id: identifies the repo to sync + :type repo_id: str + + :return: Whether the config has been changed since the last sync occurred. + :rtype: bool + """ + last_sync = conduit.last_sync() + + if last_sync is None: + return False + + # convert the iso8601 datetime string to a python datetime object + last_sync = dateutils.parse_iso8601_datetime(last_sync) + repo_importer = model.Importer.objects.get_or_404(repo_id=repo_id) + # the timestamp of the last configuration change + last_updated = repo_importer.last_updated + + # check if a configuration change occurred after the most recent sync + if last_updated is not None: + if last_sync < last_updated: + return True + + return False + + +def check_override_config_change(repo_id, call_config): + """ + Checks whether the override config is different from the override config on + the previous sync. + + :param repo_id: identifies the repo to sync + :type repo_id: str + :param call_config: Plugin Call Configuration + :type call_config: pulp.plugins.config.PluginCallConfiguration + + :return: Whether the override config is different from the override config on + the previous sync. + :rtype: bool + """ + repo_importer = model.Importer.objects.get_or_404(repo_id=repo_id) + + # check if the override config is different from the last override config, + # excluding the 'force_full' key. otherwise using the --force-full flag + # would always trigger a full sync on the next sync too + prev_config = repo_importer.last_override_config.copy() + current_config = call_config.override_config.copy() + + prev_config.pop('force_full', False) + current_config.pop('force_full', False) + + return prev_config != current_config + + +def check_perform_full_sync(repo_id, conduit, call_config): + """ + Performs generic checks to determine if the sync should be a "full" sync. + Checks if the "force full" flag has been set, if content has been removed + since the last sync, and whether the configuration has changed since the + last sync (including override configs). + + Plugins may want to perform additional checks beyond the ones appearing here. + + :param repo_id: identifies the repo to sync + :type repo_id: str + :param conduit: allows the plugin to interact with core pulp + :type conduit: pulp.plugins.conduits.repo_sync.RepoSyncConduit + :param call_config: Plugin Call Configuration + :type call_config: pulp.plugins.config.PluginCallConfiguration + + :return: Whether a full sync needs to be performed + :rtype: bool + """ + force_full = call_config.get('force_full', False) + first_sync = conduit.last_sync() is None + + content_removed = check_unit_removed_since_last_sync(conduit, repo_id) + config_changed = check_config_updated_since_last_sync(conduit, repo_id) + override_config_changed = check_override_config_change(repo_id, call_config) + + return force_full or first_sync or content_removed or config_changed or override_config_changed + + def _reposync_result(repo, importer, sync_start, summary, details, result_code, initial_unit_count): """ Creates repo sync result. diff --git a/server/pulp/server/db/migrations/0025_importer_schema_change.py b/server/pulp/server/db/migrations/0025_importer_schema_change.py new file mode 100644 index 0000000000..3de6284432 --- /dev/null +++ b/server/pulp/server/db/migrations/0025_importer_schema_change.py @@ -0,0 +1,23 @@ +from pulp.common import dateutils +from pulp.server.db.connection import get_collection + + +def migrate(*args, **kwargs): + """ + Add last_updated and last_override_config to the importer collection. + """ + updated_key = 'last_updated' + config_key = 'last_override_config' + collection = get_collection('repo_importers') + for importer in collection.find(): + if config_key not in importer.keys(): + importer[config_key] = {} + + if updated_key in importer.keys(): + continue + elif 'last_sync' in importer.keys(): + importer[updated_key] = dateutils.parse_iso8601_datetime(importer['last_sync']) + else: + importer[updated_key] = dateutils.now_utc_datetime_with_tzinfo() + + collection.save(importer) diff --git a/server/pulp/server/db/model/__init__.py b/server/pulp/server/db/model/__init__.py index d0b404c3db..a9c0b9b295 100644 --- a/server/pulp/server/db/model/__init__.py +++ b/server/pulp/server/db/model/__init__.py @@ -235,6 +235,8 @@ class Importer(AutoRetryDocument): config = DictField() scratchpad = DictField(default=None) last_sync = ISO8601StringField() + last_updated = UTCDateTimeField() + last_override_config = DictField() # For backward compatibility _ns = StringField(default='repo_importers') @@ -260,6 +262,18 @@ def pre_delete(cls, sender, document, **kwargs): repo=document.repo_id)) query_set.delete() + @classmethod + def pre_save(cls, sender, document, **kwargs): + """ + The signal that is triggered before importer is saved. + + :param sender: class of sender (unused) + :type sender: object + :param document: mongoengne document being saved + :type document: pulp.server.db.model.Importer + """ + document.last_updated = dateutils.now_utc_datetime_with_tzinfo() + def delete(self): """ Delete the Importer. Remove any documents it has stored. @@ -348,6 +362,7 @@ def _write_pem_file(self, config_key, path): signals.pre_delete.connect(Importer.pre_delete, sender=Importer) +signals.pre_save.connect(Importer.pre_save, sender=Importer) class ReservedResource(AutoRetryDocument): diff --git a/server/test/unit/plugins/conduits/test_mixins.py b/server/test/unit/plugins/conduits/test_mixins.py index 962f366e7c..a5921f8c26 100644 --- a/server/test/unit/plugins/conduits/test_mixins.py +++ b/server/test/unit/plugins/conduits/test_mixins.py @@ -383,8 +383,9 @@ def test_get_scratchpad(self, mock_importer_qs): def test_set_scratchpad(self, mock_importer_qs): mock_importer = mock_importer_qs.get.return_value self.mixin.set_scratchpad('foo') - mock_importer.save.assert_called_once_with() - self.assertEqual(mock_importer.scratchpad, 'foo') + mock_importer.update.assert_called_once_with(set__scratchpad='foo') + sp = self.mixin.get_scratchpad() + self.assertTrue(sp is mock_importer_qs.get.return_value.scratchpad) def test_set_scratchpad_server_error(self, mock_importer_qs): mock_importer_qs.get.side_effect = Exception() diff --git a/server/test/unit/server/controllers/test_repository.py b/server/test/unit/server/controllers/test_repository.py index 0bdeddd67b..029858f31f 100644 --- a/server/test/unit/server/controllers/test_repository.py +++ b/server/test/unit/server/controllers/test_repository.py @@ -1,3 +1,4 @@ +import datetime import inspect from bson.objectid import InvalidId @@ -5,7 +6,7 @@ import mock import mongoengine -from pulp.common import error_codes +from pulp.common import dateutils, error_codes from pulp.common.compat import unittest from pulp.plugins.loader import exceptions as plugin_exceptions from pulp.plugins.model import PublishReport @@ -803,6 +804,176 @@ def test_update_last_unit_removed(self, mock_date, m_repo_qs): m_repo.save.assert_called_once_with() +class TestCheckPerformFullSync(unittest.TestCase): + """ + Test the series of checks Pulp does to determine if a full sync is necessary. + """ + + def setUp(self): + self.after_ts = dateutils.now_utc_datetime_with_tzinfo() + self.before_ts = self.after_ts - datetime.timedelta(minutes=1) + + self.after_ts_str = dateutils.format_iso8601_datetime(self.after_ts) + self.before_ts_str = dateutils.format_iso8601_datetime(self.before_ts) + + @mock.patch('pulp.server.controllers.repository.model.Repository.objects') + @mock.patch('pulp.server.controllers.repository.RepoSyncConduit') + def test_check_unit_removed_since_last_sync(self, mock_conduit, mock_repo_qs): + """ + Tests that "True" is returned when a unit has been removed since the last sync, + otherwise "False" is returned. + """ + m_repo = mock_repo_qs.get_repo_or_missing_resource.return_value + m_repo_id = m_repo.repo_id + + # Test return value when there is no previous sync + mock_conduit.last_sync.return_value = None + retval = repo_controller.check_unit_removed_since_last_sync(mock_conduit, m_repo_id) + self.assertFalse(retval) + + # Test return value when there is no unit removed + mock_conduit.last_sync.return_value = self.after_ts_str + m_repo.last_unit_removed = None + retval = repo_controller.check_unit_removed_since_last_sync(mock_conduit, m_repo_id) + self.assertFalse(retval) + + # Test return value when the last unit removed occurred before the last sync + mock_conduit.last_sync.return_value = self.after_ts_str + m_repo.last_unit_removed = self.before_ts + retval = repo_controller.check_unit_removed_since_last_sync(mock_conduit, m_repo_id) + self.assertFalse(retval) + + # Test return value when the last unit removed occurred since than the last sync + mock_conduit.last_sync.return_value = self.before_ts_str + m_repo.last_unit_removed = self.after_ts + retval = repo_controller.check_unit_removed_since_last_sync(mock_conduit, m_repo_id) + self.assertTrue(retval) + + @mock.patch('pulp.server.controllers.repository.model.Importer.objects') + @mock.patch('pulp.server.controllers.repository.model.Repository.objects') + @mock.patch('pulp.server.controllers.repository.RepoSyncConduit') + def test_check_config_updated_since_last_sync(self, mock_conduit, mock_repo_qs, mock_imp_qs): + """ + Tests that "True" is returned when the config file has been changed since the last sync. + """ + m_repo = mock_repo_qs.get_repo_or_missing_resource.return_value + m_repo_id = m_repo.repo_id + mock_imp = mock_imp_qs.get_or_404.return_value + mock_imp.last_updated.return_value = None + + # Test return value when there is no last sync + mock_conduit.last_sync.return_value = None + retval = repo_controller.check_config_updated_since_last_sync(mock_conduit, m_repo_id) + self.assertFalse(retval) + + # Test return value when last_updated is None + mock_conduit.last_sync.return_value = self.after_ts_str + mock_imp.last_updated = None + retval = repo_controller.check_config_updated_since_last_sync(mock_conduit, m_repo_id) + self.assertFalse(retval) + + # Test return value when the last config update occurred before the last sync + mock_conduit.last_sync.return_value = self.after_ts_str + mock_imp.last_updated = self.before_ts + retval = repo_controller.check_config_updated_since_last_sync(mock_conduit, m_repo_id) + self.assertFalse(retval) + + # Test return value when the last config update occurred after the last sync + mock_conduit.last_sync.return_value = self.before_ts_str + mock_imp.last_updated = self.after_ts + retval = repo_controller.check_config_updated_since_last_sync(mock_conduit, m_repo_id) + self.assertTrue(retval) + + @mock.patch('pulp.server.controllers.repository.PluginCallConfiguration') + @mock.patch('pulp.server.controllers.repository.model.Importer.objects') + @mock.patch('pulp.server.controllers.repository.RepoSyncConduit') + def test_check_override_config_change(self, mock_conduit, mock_imp_qs, mock_call_config): + """ + Tests that "True" is returned when the config file has been changed since the last sync. + """ + mock_imp = mock_imp_qs.get_or_404.return_value + + # Test return value when there is no change in the override configuration + mock_imp.last_override_config = {'bg': True} + mock_call_config.override_config = {'bg': True} + retval = repo_controller.check_override_config_change(mock_conduit, mock_call_config) + self.assertFalse(retval) + + # Test return value when the only difference is a "force_full" flag + mock_imp.last_override_config = {'force_full': True} + mock_call_config.override_config = {} + retval = repo_controller.check_override_config_change(mock_conduit, mock_call_config) + self.assertFalse(retval) + + # Test return value when the only difference is a "force_full" flag + mock_imp.last_override_config = {} + mock_call_config.override_config = {'force_full': True} + retval = repo_controller.check_override_config_change(mock_conduit, mock_call_config) + self.assertFalse(retval) + + # Test return value when the override configs are different + mock_imp.last_override_config = {'bg': True} + mock_call_config.override_config = {'bg': False} + retval = repo_controller.check_override_config_change(mock_conduit, mock_call_config) + self.assertTrue(retval) + + @mock.patch('pulp.server.controllers.repository.check_override_config_change') + @mock.patch('pulp.server.controllers.repository.check_config_updated_since_last_sync') + @mock.patch('pulp.server.controllers.repository.check_unit_removed_since_last_sync') + @mock.patch('pulp.server.controllers.repository.PluginCallConfiguration') + @mock.patch('pulp.server.controllers.repository.RepoSyncConduit') + def test_check_perform_full_sync(self, mock_conduit, mock_call_config, mock_removed, + mock_updated, mock_override): + """ + Tests that if any of the constitutent checks fail, the functon will fail as expected + """ + # Test return value when all constituent checks evaluate to False + mock_call_config.get.return_value = False + mock_conduit.last_sync.return_value = self.after_ts_str + mock_removed.return_value = False + mock_updated.return_value = False + mock_override.return_value = False + + retval = repo_controller.check_perform_full_sync('mock_id', + mock_conduit, + mock_call_config) + self.assertFalse(retval) + + # Test return value when all constituent checks evaluate to True + mock_call_config.get.return_value = True + mock_conduit.last_sync.return_value = None + mock_removed.return_value = True + mock_updated.return_value = True + mock_override.return_value = True + + retval = repo_controller.check_perform_full_sync('mock_id', + mock_conduit, + mock_call_config) + self.assertTrue(retval) + + # Test return True when only the "force_full" flag check evaluates to True + mock_call_config.get.return_value = True + mock_conduit.last_sync.return_value = self.after_ts_str + mock_removed.return_value = False + mock_updated.return_value = False + mock_override.return_value = False + retval = repo_controller.check_perform_full_sync('mock_id', + mock_conduit, + mock_call_config) + self.assertTrue(retval) + + # Test return True when only the last sync check evaluates to True + mock_call_config.get.return_value = False + mock_conduit.last_sync.return_value = None + mock_removed.return_value = False + mock_updated.return_value = False + mock_override.return_value = False + retval = repo_controller.check_perform_full_sync('mock_id', + mock_conduit, + mock_call_config) + self.assertTrue(retval) + + @mock.patch('pulp.server.controllers.repository.rebuild_content_unit_counts') @mock.patch('pulp.server.controllers.repository.sys') @mock.patch('pulp.server.controllers.repository.register_sigterm_handler') @@ -887,7 +1058,7 @@ def test_sync_canceled(self, m_task_result, mock_spawn_auto_pub, m_model, mock_now(), mock_now(), m_added, m_updated, m_removed, m_sync_result.summary, m_sync_result.details, 'canceled' ) - m_model.Importer.objects().update.assert_called_once_with(set__last_sync=mock_now()) + m_model.Importer.objects().update.assert_called_with(set__last_sync=mock_now()) mock_result.get_collection().save.assert_called_once_with(mock_result.expected_result()) mock_fire_man.fire_repo_sync_finished.assert_called_once_with(mock_result.expected_result()) self.assertTrue(actual_result is m_task_result.return_value) @@ -933,7 +1104,7 @@ def test_sync_success(self, m_task_result, mock_spawn_auto_pub, m_model, mock_now(), mock_now(), m_added, m_updated, m_removed, m_sync_result.summary, m_sync_result.details, 'success' ) - m_model.Importer.objects().update.assert_called_once_with(set__last_sync=mock_now()) + m_model.Importer.objects().update.assert_called_with(set__last_sync=mock_now()) mock_result.get_collection().save.assert_called_once_with(mock_result.expected_result()) mock_fire_man.fire_repo_sync_finished.assert_called_once_with(mock_result.expected_result()) self.assertEqual(mock_imp_inst.id, mock_conduit.call_args_list[0][0][2]) @@ -978,7 +1149,7 @@ def test_sync_failed(self, m_task_result, m_model, mock_plugin_api, mock_plug_co mock_now(), mock_now(), m_added, m_updated, m_removed, m_sync_result.summary, m_sync_result.details, 'failed' ) - m_model.Importer.objects().update.assert_called_once_with(set__last_sync=mock_now()) + m_model.Importer.objects().update.assert_called_with(set__last_sync=mock_now()) mock_result.get_collection().save.assert_called_once_with(mock_result.expected_result()) mock_fire_man.fire_repo_sync_finished.assert_called_once_with(mock_result.expected_result()) @@ -1012,7 +1183,7 @@ def test_sync_invalid_sync_report(self, m_task_result, mock_logger, mock_gettext mock_now(), mock_now(), -1, -1, -1, mock_gettext(), mock_gettext(), 'error' ) - m_model.Importer.objects().update.assert_called_once_with(set__last_sync=mock_now()) + m_model.Importer.objects().update.assert_called_with(set__last_sync=mock_now()) mock_result.get_collection().save.assert_called_once_with(mock_result.expected_result()) mock_fire_man.fire_repo_sync_finished.assert_called_once_with(mock_result.expected_result()) self.assertTrue(result is m_task_result.return_value) diff --git a/server/test/unit/server/db/migrations/test_0025_importer_schema_change.py b/server/test/unit/server/db/migrations/test_0025_importer_schema_change.py new file mode 100644 index 0000000000..36b9f1c9ce --- /dev/null +++ b/server/test/unit/server/db/migrations/test_0025_importer_schema_change.py @@ -0,0 +1,45 @@ +from copy import deepcopy +from unittest import TestCase + +from mock import Mock, patch + +from pulp.server.db.migrate.models import MigrationModule + +LAST_SYNC = 'last_sync' +LAST_UPDATED = 'last_updated' +LAST_OVERRIDE_CONFIG = 'last_override_config' +MIGRATION = 'pulp.server.db.migrations.0025_importer_schema_change' + + +class TestMigration(TestCase): + """ + Test the migration. + """ + + @patch('.'.join((MIGRATION, 'dateutils.now_utc_datetime_with_tzinfo'))) + @patch('.'.join((MIGRATION, 'get_collection'))) + def test_migrate(self, m_get_collection, now_utc_datetime): + """ + Test last_updated and last_override_config fields added. + """ + collection = Mock() + found = [ + {LAST_SYNC: '2016-05-04T18:19:01Z', LAST_UPDATED: '2016-05-03T18:19:01Z'}, + {LAST_SYNC: '2016-05-04T18:20:01Z'}, + {}, + ] + collection.find.return_value = deepcopy(found) + m_get_collection.return_value = collection + + # test + module = MigrationModule(MIGRATION)._module + module.migrate() + + # validation + m_get_collection.assert_called_once_with('repo_importers') + collection.find.assert_called_once_with() + now_utc_datetime.assert_called_once_with() + self.assertTrue(LAST_UPDATED in dist for dist in collection.save.call_args_list) + self.assertTrue(LAST_OVERRIDE_CONFIG in dist for dist in collection.save.call_args_list) + self.assertEqual( + len(collection.save.call_args_list), 2)