[Pulp-dev] Plugin relationship to tasks

Tue Mar 27 07:20:04 UTC 2018

On Tue, Mar 27, 2018 at 12:02 AM, Austin Macdonald <austin at redhat.com> wrote:
>
>
> On Mon, Mar 26, 2018 at 4:43 PM, Dennis Kliban <dkliban at redhat.com> wrote:
>>
>> On Mon, Mar 26, 2018 at 3:38 PM, Austin Macdonald <austin at redhat.com>
>> wrote:
>>>
>>>
>>>
>>> On Mon, Mar 26, 2018 at 2:30 PM, Dennis Kliban <dkliban at redhat.com>
>>> wrote:
>>>>
>>>> This proposal does not make the plugin writer's job any easier. This
>>>> simply changes where the plugin writer is providing validation logic, in a
>>>> serializer vs. in a view.
>>>
>>>
>>> It does make the plugin writer's job easier *because* it changes how the
>>> validation is provided, Plugin writers already have to understand how to
>>> create models/serializers (and it is very simple) for all the other objects.
>>> Seriallizer validation is much simpler than viewset validation because
>>> serializers are literally designed to do this.
>>
>>
>> If we don't change anything, a plugin writer is responsible for creating a
>> task that creates a Repository Version and a view which will perform
>> validation and then dispatches a task. If we do make the proposed change,
>> the plugin writer will still have to write the task code and then also a
>> serializer that performs validation. The proposed change only removes the
>> responsibility of dispatching a task which is 1 line of code.
>
>
> The difference is not in the number of lines, it is in the complexity.
> Validation with serializers is simpler, and more similar to other plugin
> code.
>
> New Way:
> https://github.com/asmacdo/pulp_file/blob/8a8cbf81b6f0f1e382be7884f2b2406a06ea230b/pulp_file/app/serializers.py#L15-L21
> Old
> way:https://github.com/asmacdo/pulp_file/blob/8a8cbf81b6f0f1e382be7884f2b2406a06ea230b/pulp_file/app/serializers.py#L13-L30
   oh, and let's not forget the boilerplate needed to make the old way
autodoc working for the custom action endpoints:
https://github.com/pulp/pulp_file/blob/master/pulp_file/app/viewsets.py#L31,#L61
   I dare to say majority of the sync objects (serializer) boilerplate
will be generic enough to be shared between the plugins; there's not
much one can do about a sync request but creating it...

>
> That "1 line of code" puts the plugin writer in contact with a very complex
> part of the pulpcore internals. `apply_async_with_reservation`:
> https://github.com/pulp/pulp/blob/e8f804dc6595732dad38ec478877d919d7d97063/pulpcore/pulpcore/tasking/tasks.py#L180
>
>>>>
>>>> The other problem I see is that complexity for users is increased.
>>>> Instead of having 1 resource for tracking task progress, there would be an
>>>> infinite number of resources for tracking progress of tasks.
>>>
>>>
>>> In the proposal, Tasks are Master/Detail. The user doesn't have to
>>> "track" it at all. They can still use v3/tasks/ to see all the tasks.
>>> Retrieving tasks will either be the same (v3/tasks/) or easier if they know
>>> the task they are looking for (v3/tasks/file/syncs/)
>>
>>
>> My understanding of the master/detail pattern we are using for our other
>> resources is that each task's URI will look something like this:
>> /api/v3/tasks/file/syncs/<uuid>/
>>
>> If the above is true, that means the client has to be aware of
>> '/api/v3/tasks/file/syncs/<uuid>/' resource. The user would need to know how
>> to interpret the response from each task type. There could be an infinite
>> number of these tasks with many different arguments. So writing client code
>> handling all those responses could become a nightmare. Right now the user
>> just needs to know how to interpret the response from '/api/v3/tasks/<uuid>'
>> for all tasks.
>
>
> I might be missing your point on this one. If you are talking about polling
> tasks, the href of a new Task is returned when the Task is created.The
> client should use the href that is returned, never string formatting urls.
> Even so, the Tasks in this proposal will be part of the API schema, so
> clients (and docs users) know where the endpoints are and what the arguments
> should be. This would actually be a big advantage over the way things are
> now. AFAIK, v3/importers/1234/sync/ is invisible to clients, so we replace
> one invisible endpoint with one visible endpoint. No complexity added.
>
> If the client doesn't know how to interpret extra fields (the Task
> parameters), they don't need to. All tasks share the same base fields, and
> those are the fields that need to be interpreted when the client is polling
> a task.
+1 all tasks will share common ancestor and path in the url; it's very
easy to explore and follow, see also the autodoc screen cast

>
>>
>> Value #1: We have to do something to address the problem that
>> adding/removing content to a repository without plugin input is incorrect.
>> This proposal is one possibility, but it isn't valid to compare the value
>> against doing nothing. Instead, if you don't like this option, we need to
>> compare it to other proposals for how to involve plugins in tasks.
>>
>>
>> We don't have a problem. I agree that a user needs to know a whole lot of
>> information to correctly create a working repository version for a complex
>> content type such as a Docker manifest. Without all this information on
>> hand, the user could easily create a broken repository version. However, a
>> rich client could solve that problem. If the plugin writer wants to support
>> simpler clients, she can provide an additional URL for handling the
>> validation and additional logic for creating a repository version. We should
>> probably have a recommended convention for plugin authors to use when adding
>> additional URLs.
>
>
> What I find troubling is that the most basic action (add/remove) suddenly
> becomes complex. If using the file plugin POST to
> v3/repository/1234/versions/. If using docker, POST to
> v3/plugins/docker/add/. This is especially irritating because the first way
> will still work for docker, but will break corrupt the repositories. So the
> responsibility falls to users to know that this endpoint "works" but should
> be avoided (for certain plugins). It will be surprising that one is a "noun"
> and the other is an "action".
+1 this is a legit UX issue

>
> Also, doing correctness enforcement in the client instead of the REST API
> seems very wrong.
+1

>
>> Value #5: Task history becomes much more useful. Currently, the only data
>> on a task that connects it to user-facing objects is "created_resource".
>> This proposal will allow users to filter tasks by parameters. For example,
>> they can filter sync tasks by "repository" or "importer". They can also use
>> the detail endpoints (v3/tasks/file/ or v3/tasks/file/syncs/) to list tasks
>> based on the plugin or action.
>>
>
>> I don't think tracking history is one of the use cases we decided to
>> support in the MVP. We should have a separate discussion on how we could
>> accomplish this.
>
>
> Fair enough, but just because it isn't on the MVP doesn't mean this it
> should be completely ignored.
+1

Cheers,
milan