[Pulp-dev] Revisit: sync modes

Tue Aug 28 21:05:52 UTC 2018

I couldn't come up with any more use cases that would motivate keeping
sync_mode over mirror. As pointed out, even retention would likely be its
own option. I'm now +1 on making the API and DeclarativeVersion use
mirror=False.

I think I was the main dissenting opinion on this change, so I believe this
represents consensus. If there are any -1's please raise them on-list.
Otherwise I would say that PR and any follow-up work can go ahead and
happen.

On Tue, Aug 21, 2018 at 12:48 PM, David Davis <daviddavis at redhat.com> wrote:

> I wanted to bump this thread. There’s a PR waiting on a resolution. I see
> some agreement but not sure what the next steps are?
>
> David
>
>
> On Thu, Aug 9, 2018 at 5:34 PM Jeff Ortel <jortel at redhat.com> wrote:
>
>>
>>
>> On 08/09/2018 01:29 PM, Daniel Alley wrote:
>>
>> It's possible we could want additional sync_modes in the future. To me,
>>> sync mode deals with the contents of the repo during the sync. There are
>>> other ways you would want to have a sync associate content with a
>>> repository. Consider a retention behavior that retains 5 versions of each
>>> unit, e.g. rpms, ansible modules, etc; that behavior is somewhere in
>>> between mirror and additive. If we make mirror a boolean then to introduce
>>> this retention feature we would have to add it as an additional option.
>>> This creates the downside I hope to avoid which is that interaction between
>>> options becomes complicated.
>>>
>>> For example, a call with both (mirror=False, retention=True) now becomes
>>> more complicated to think about. Is it mirroring or using the retention
>>> policy? How do these interact? At that point, it seems more complicated
>>> than what we have now. The way to avoid this is by keeping them together as
>>> one option, but that can only be done if it stays as a string.
>>>
>>
>> These are all good points but I think "retention" would likely need to be
>> a configurable parameter, probably one that you would have to pass in.  The
>> default value could mean "unlimited retention", i.e.  "additive".
>>
>> So what you could do is:
>>
>> (mirror=False)                       # this is normal additive mode,
>>> retain everything.  let's say that default retention=0, which is
>>> nonsensical and would map to this behavior instead
>>>
>> (mirror=False, retention=5)     # retain at most 5 versions of any given
>>> unit
>>>
>> (mirror=False, retention=1)     # this is *almost* like mirror mode,
>>> except that you would still keep one historical copy of units that are no
>>> longer present in the upstream repository
>>>
>>
>> Maybe it even makes sense to have retention be able to modify "mirror"
>> mode, although this would make the concept of "mirror" more difficult to
>> understand as you point out.  Maybe we could find a name that would be less
>> misleading.
>>
>> (mirror=True, retention=5)       # retain at most 5 versions of any given
>>> unit, *but purge units that that are no longer present in the upstream
>>> repo entirely*
>>>
>>
>> This ^^ matches what I was thinking as well.
>>
>>
>> I don't have a specific use case in mind for that one, but maybe someone
>> can think of one?
>>
>>
>> On Thu, Aug 9, 2018 at 12:53 PM, Brian Bouterse <bbouters at redhat.com>
>> wrote:
>>
>>> It's possible we could want additional sync_modes in the future. To me,
>>> sync mode deals with the contents of the repo during the sync. There are
>>> other ways you would want to have a sync associate content with a
>>> repository. Consider a retention behavior that retains 5 versions of each
>>> unit, e.g. rpms, ansible modules, etc; that behavior is somewhere in
>>> between mirror and additive. If we make mirror a boolean then to introduce
>>> this retention feature we would have to add it as an additional option.
>>> This creates the downside I hope to avoid which is that interaction between
>>> options becomes complicated.
>>>
>>> For example, a call with both (mirror=False, retention=True) now becomes
>>> more complicated to think about. Is it mirroring or using the retention
>>> policy? How do these interact? At that point, it seems more complicated
>>> than what we have now. The way to avoid this is by keeping them together as
>>> one option, but that can only be done if it stays as a string.
>>>
>>> On Thu, Aug 9, 2018 at 9:04 AM, Milan Kovacik <mkovacik at redhat.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Wed, Aug 8, 2018 at 7:54 PM, Jeff Ortel <jortel at redhat.com> wrote:
>>>>
>>>>> I'm not convinced that *named* sync mode is a good approach.  I doubt
>>>>> it will ever be anything besides (additive|mirror) which really boils down
>>>>> to mirror (or not).  Perhaps the reasoning behind a *named* mode is
>>>>> that it is potentially more extensible in that the API won't be impacted
>>>>> when a new mode is needed.  The main problem with this approach is that the
>>>>> mode names are validated and interpreted in multiple places. Adding another
>>>>> mode will require coordinated changes in both the core and most plugins.
>>>>> Generally, I'm an advocate of named things like *modes* and *policies*
>>>>> but given the orthogonal nature of the two modes we currently support
>>>>> *and* that no *real* or anticipated use cases for additional modes
>>>>> are known, I'm not convinced it's a good fit.  Are there any *real*
>>>>> or anticipated use cases I'm missing?
>>>>>
>>>>
>>>> Looking at the code[1] we're actually talking about almost a (pipeline)
>>>> factory that has exactly 2 modes of operation with a limited possibilities
>>>> of extending, unsure that the possibility to extend was a goal though.
>>>> Moreover it turns out current implementation prevents using
>>>> (class-level) constants instead of custom strings due to plugin--platform
>>>> import issues: core serializer can't refer to DeclarativeVersion.defaul_sync_mode
>>>> --- at least I wasn't able to make this work as part of the sync_mode
>>>> docstring PR[2] review suggestion.
>>>>
>>>>
>>>>>
>>>>> I propose we replace the (str)sync_mode="" with (bool)mirror=False
>>>>> anywhere stored or passed.
>>>>>
>>>>> Are there any *real* or anticipated use cases I'm missing?
>>>>>
>>>>> Thoughts?
>>>>>
>>>>
>>>> I'm afraid replacing custom strings with True/False won't make the
>>>> situation much better.
>>>> I'd vote for some refactor besides other things, it might better be
>>>> part of remote (or repository) creation endpoint.
>>>>
>>>> Cheers,
>>>> milan
>>>>
>>>> [1] https://github.com/pulp/pulp/blob/master/plugin/pulpcore/
>>>> plugin/stages/declarative_version.py#L100#L114
>>>> [2] https://github.com/pulp/pulp/pull/3583#discussion_r208869824
>>>>
>>>>
>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Pulp-dev mailing list
>>>>> Pulp-dev at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>
>>>>>
>>>>
>>>> _______________________________________________
>>>> Pulp-dev mailing list
>>>> Pulp-dev at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>
>>>>
>>>
>>> _______________________________________________
>>> Pulp-dev mailing list
>>> Pulp-dev at redhat.com
>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>
>>>
>>
>>
>> _______________________________________________
>> Pulp-dev mailing listPulp-dev at redhat.comhttps://www.redhat.com/mailman/listinfo/pulp-dev
>>
>>
>> _______________________________________________
>> Pulp-dev mailing list
>> Pulp-dev at redhat.com
>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>
>
> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20180828/b723dfa7/attachment.htm>