[katello-devel] Foreman integration model question

Thu Aug 30 10:52:38 UTC 2012

On 29/08/12 06:23 PM, Petr Chalupa wrote:
>
>
> On 29.08.12 16:36, Dmitri Dolguikh wrote:
>> On 29/08/12 12:21 PM, Petr Chalupa wrote:
>>>
>>>
>>> On 23.08.12 17:27, Dmitri Dolguikh wrote:
>>>> On 22/08/12 06:07 PM, Justin Sherrill wrote:
>>>>> On 08/22/2012 08:26 AM, Petr Chalupa wrote:
>>>>>> Lukas is right, our motivation was to break the code into smaller
>>>>>> pieces (I am big fan of SOLID and 'single responsibility principle'
>>>>>> in particular). If you write code this way, you can then move things
>>>>>> around very quickly so you can develop features and fix bugs faster.
>>>>>> No to mention it can be testes more easily.
>>>>>>
>>>>>> app/models/user.rb (User) is representation of katello user which
>>>>>> mixes (if foreman is installed) foreman orchestration
>>>>>> app/models/glue/forema/user.rb. Orchestration is working with 
>>>>>> foreman
>>>>>> through children of ForemanModel which are representation of remote
>>>>>> resources app/models/foreman/user.rb (Foreman::User). Foreman::User
>>>>>> is using foreman-api bindings which are defined as singletons in
>>>>>> lib/resources/foreman.rb.
>>>>>>
>>>>>> So there are 4 layers:
>>>>>> - katello model
>>>>>> - foreman-katello orchestration
>>>>>> - foreman model (remote resource)
>>>>>> - foreman bindings (gem foreman-api)
>>>>>>
>>>>>> I also for splitting up big parts.
>>>>> I do like this auto generated api method better (aside from there
>>>>> being two distinct User models).  To say a downside of the current
>>>>> model is a giant single file pulp.rb is kinda moot, as that was just
>>>>> how it was organized, we can organize it differently quite easily.
>>>>> What we are doing here with foreman is wayyyy more than just
>>>>> organizing it differently, its a fairly completely different way of
>>>>> orchestrating things, bypassing our lazy_accessor magic.
>>>> I will insist on using the orchestration layer for all access 
>>>> outside of
>>>> local model. Controller code shouldn't be aware of where a given model
>>>> attribute is, otherwise you are leaking orchestration outside of 
>>>> model.
>>>
>>> I am sorry I did not send an email about it before we started working
>>> on this. We had a discussion about it and we agreed that this is
>>> better approach, but we didn't feel that this is a big change. Sorry
>>> about that. I'll try to explain why I think this is better.
>>>
>>> The change is that we didn't use lazy_accessor but introduced new
>>> object representing remote resources. The reason is that katello
>>> models don't get any fatter.
>>>
>>> I agree that orchestration should be hidden from ActiveController.
>>> Because Foreman::User doesn't need to expose any of its attributes
>>> it's not clear how it would look like in KTEnvironment example.
>>>
>>> In KTEnvironment case it would look like this:
>>>
>>> There would be 4 files:
>>> - models/kt_environment.rb - class definition of katello model
>>> (KTEnvironment)
>>> - models/glue/foreman/kt_environment.rb - module defining foreman
>>> orchestration (Glue::Pulp::KTEnvironment)
>>> - models/foreman/kt_environment.rb - class definition of foreman
>>> remote resource (Foreman::KTEnvironment)
>>> - lib/resources/foreman.rb - defines singleton resource adapter using
>>> foreman_api (Resources::Foreman::KTEnvironment)
>>>
>>> Lets assume Foreman::KTEnvironment has two attributes :hidden and
>>> :exposed where :hidden should not be visible form ActiveController nad
>>> :exposed should.
>>>
>>> Then :hidden is hidden in Foreman::KTEnvironment and
>>> Foreman::KTEnvironment instance accesible from KTEnvironment instance
>>> should not be accessed directly, Foreman::KTEnvironment is private
>>> represantion managed by foreman orchestration
>>> (Glue::Pulp::KTEnvironment).
>>>
>>> To expose :exposed attribute in KTEnvironment you would define
>>> decelerators in Glue::Pulp::KTEnvironment:
>>>
>>> def exposed
>>>   foreman.exposed
>>> end
>>>
>>> def exposed=(val)
>>>   foreman.exposed = val
>>> end
>>>
>>> # or method .delegate can by used
>>> delegate :exposed, :exposed=, :to => :foreman
>>>
>>> :exposed is then accessible from ActiveController but everything else
>>> is hidden.
>>
>> Heh. I was thinking of something similar to encapsulate initialization
>> of remote attributes and possibly having a longer-living cache shared by
>> all remote attributes from a given sub-system.
>
> I would not introduce any cache until it's measured that this a real 
> bottleneck and needs to be cached. Caches are often introducing 
> hard-to-find bugs, I would avoid them as long as possible.

Optimization should be done only when needed. Katello is slow, when we 
are orchestrating two remote systems, and we are adding another. I think 
we need to discuss how to speed up the whole thing (caching of http 
responses is a straightforward route to take), not how to avoid caching.

As I said it in the private conversation - the answer is more tests and 
better tests, not avoiding useful features because of the fear of 
breaking things.

>
>> We could potentially leave the proxy object in place (instead of hiding
>> it behind a delegate) - it would serve as a notice of different call
>> semantic.
>>
>> -d
>>>
>>> We felt that foreman vs candlepin/pulp orchestration is clear enough
>>> border not to rise confusion. As I see it the drawback in consistency
>>> is much more less important than advantages of this approach:
>>> - no huge objects
>>> - easier testing
>>> - clearer distribution of responsibilities between objects
>>>
>>> +1 to keep the current approach
>>>
>>> I hope this is explaining motivation behind this. If you have more
>>> questions please ask.
>>>
>>> Petr
>>>
>>>> That aside, but why do we need to *generate* code in *ruby*? Does that
>>>> mean we have a lot of dumb code with very little logic in it? We 
>>>> can and
>>>> should do better.
>>>>
>>>> -d
>>>>>
>>>>> For example, lets say we do the same with environment, where there is
>>>>> a katello environment and a foreman environment.  And lets say there
>>>>> is some special piece of information only stored in foreman (this is
>>>>> not a far-fetched scenario).
>>>>>
>>>>> With our current orchestration you would do something like:
>>>>>
>>>>> a = KTEnvironment.find(3)
>>>>> print a.remote_attribute
>>>>> a.remote_attribute = "foo"
>>>>> a.save!
>>>>>
>>>>> whereas the way i see it now, we'd have to do:
>>>>>
>>>>> a = KTEnvironment.find(3)
>>>>> a.foreman_user.remote_attribute = "foo"
>>>>> a.save!
>>>>>
>>>>> I don't really like this better as you'd have to remember which
>>>>> backend engine a particular attribute is.  Today you don't.  I don't
>>>>> really expect any of these arguments to change the way we are doing
>>>>> anything, its just annoying to change our entire way of working 
>>>>> with a
>>>>> backend engine without any discussion :)
>>>>>
>>>>> We need to be aware that doing it this way is creating a 2nd way with
>>>>> working with fully (or partially) remote objects and that needs to be
>>>>> clear and apparent.  It also needs to be justified :)
>>>>>
>>>>> -Justin
>>>>>
>>>>>>
>>>>>> Petr
>>>>>>
>>>>>> On 22.08.12 10:19, Lukas Zapletal wrote:
>>>>>>> On Tue, Aug 21, 2012 at 02:48:56PM -0400, Justin Sherrill wrote:
>>>>>>>> Taking user for example we would have:
>>>>>>>>
>>>>>>>> user.rb
>>>>>>>> glue/pulp/user.rb
>>>>>>>
>>>>>>> But you are missing one piece of the puzzle:
>>>>>>>
>>>>>>> lib/resources/pulp.rb (class User)
>>>>>>>
>>>>>>> And that is the piece which you can find in foreman/user.rb.
>>>>>>>
>>>>>>>> What I see with foreman is:
>>>>>>>>
>>>>>>>> user.rb
>>>>>>>> glue/foreman/user.rb
>>>>>>>> foreman/user.rb
>>>>>>>
>>>>>>> So instead having all the resources in a single class
>>>>>>> (lib/resources/foreman.rb), we are going to have (or better -
>>>>>>> generate)
>>>>>>> all resource (read also as "dumb") classes. Each in separate file.
>>>>>>>
>>>>>>>> Where foreman/user is a second class that can be instantiated and
>>>>>>>> used on its own, while glue/foreman/user.rb is simply the
>>>>>>>> orchestration to create that object.  I see the use of
>>>>>>>> Resources::ForemanModel, but in this user instance the foreman 
>>>>>>>> user
>>>>>>>> could be manipulated completely apart from the katello user (which
>>>>>>>> is bad IMHO).  This also is completely different from our existing
>>>>>>>> orchestration methods.
>>>>>>>
>>>>>>> Yes, it is different. I like this approach more. Creating a one
>>>>>>> instance
>>>>>>> per request is nothing, while the code is definitely better than
>>>>>>> eight
>>>>>>> hundred lines long pulp.rb with all the resources there as "class"
>>>>>>> methods.
>>>>>>>
>>>>>>> I think this is the correct approach and we may discuss splitting
>>>>>>> those
>>>>>>> big resource files in future into smaller pieces for other backend
>>>>>>> engines too. If we write a similar generator, we wont longer 
>>>>>>> need to
>>>>>>> write them manually (and we will be able to track API changes too).
>>>>>>>
>>>>>>> I was not writing the code, but I believe those instances are 
>>>>>>> treated
>>>>>>> like singletons, so you do not need to create instances by 
>>>>>>> yourself:
>>>>>>>
>>>>>>> https://github.com/Katello/katello/blob/foreman_architectures/src/lib/resources/foreman.rb 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Guys please elaborate what the intentions are.
>>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> katello-devel mailing list
>>>>>> katello-devel at redhat.com
>>>>>> https://www.redhat.com/mailman/listinfo/katello-devel
>>>>>
>>>>> _______________________________________________
>>>>> katello-devel mailing list
>>>>> katello-devel at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/katello-devel
>>>>
>>>>
>>>> _______________________________________________
>>>> katello-devel mailing list
>>>> katello-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/katello-devel
>>>
>>> _______________________________________________
>>> katello-devel mailing list
>>> katello-devel at redhat.com
>>> https://www.redhat.com/mailman/listinfo/katello-devel
>>
>>
>> _______________________________________________
>> katello-devel mailing list
>> katello-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/katello-devel
>
> _______________________________________________
> katello-devel mailing list
> katello-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/katello-devel