Refactor #3074
Updated by amacdona@redhat.com over 6 years ago
Most of the work of this move will be handled by plugins, so refactor tasks will be created to implement this change for each plugin. The scope of this refactor should be limited to Importers/sync for now, so Publish is deliberately left out for now.
The pulpcore changes are:
# Remove models.Importer.sync
# Remove the viewsets.Importer.sync detail route
Plugin refactor issues should:
# Create a celery task (probably called `sync`) in pulp_myplugin.app.tasks.py
# define a POST detail route (probably called sync) on MyTypeImporterViewset that deploys the celery task with appropriate locks. This viewset should accept `repository` as a POST body parameter.
The reasoning behind this change is documented below:
The problem was described previously in an "email":https://www.redhat.com/archives/pulp-dev/2017-September/msg00005.html:
> While working with the FileImporter, I discovered an interesting problem
> with circular imports.
>
> An Importer is both a django Model and has a "sync" method, as shown here:
>
> https://github.com/pulp/pulp_file/blob/d684f75e/pulp_file/app/models.py#L69
>
> We decided to design it this way for simplicity, so that a plugin writer
> would have a minimum of objects to subclass. It also reduces the number of
> objects for the core to discover. The Importer would have methods for sync,
> copy, upload, etc. This is very similar to Pulp 2. And just like in Pulp 2,
> we know that plugins will often want to implement much of the sync logic in
> one or more separate python modules.
>
> When I tried to move the FileImporter's sync code to a separate module, I
> quickly found a circular import problem. The FileImporter wants to
> instantiate and use a Synchronizer. The Synchronizer of course needs to use
> the data model, both the FileContent model and the corresponding Key
> namedtuple. A plugin with more types would have yet more models.
>
> Thinking of the idea that circular imports often indicate a design problem,
> there is a single and specific design choice throwing a wrench in the
> gears. We have a single class acting as both model and controller: the
> FileImporter. It both defines part of the data model, AND it is expected to
> perform an action (sync) that makes use of several different models
> (classic controller territory, and definitely not in-line with OOP).
>
> We made the choice deliberately to put the plugin API methods (sync,
> upload, etc.) on the Importer to give the plugin writer a simple
> experience. But here we see that is already causing difficulty for plugin
> writing.
>
> Perhaps we should re-consider having the Importer both act as model and
> controller. I'm using those terms loosely, not to imply that we should have
> a strict MVC pattern or anything like that, but just to describe what type
> of role any given portion of code is expected to play.
In-person discussion concluded that we should move behaviors related to sync, upload, copy, remove, publish, or anything similar off of the Model objects and into a separate area of each plugin's code. This is a very similar pattern to Pulp 2.
Two new objects will be required: one to hold the importer-related behavior, and another to hold publisher-related behavior. There was not agreement on what a good name would be, but there was a broad desire to keep the models named "Importer" and "Publisher".
The plugin API should provide base classes for each of the two new models. When a plugin implements a subclass, that subclass must be discoverable by the core. For example when a sync task is run on an Importer as known through the REST API, the core must find this other plugin-provided object that implements behaviors that go with the database representation of an Importer.
The new importer-related object could be named ImporterController, pulp_<sometype>.app.controllers.Importer, ImportDoer, or some other name.
The new objects may have references to the models to which they correspond. The models must not have references to the new objects.
.