python facade pattern implementation in ceph and ceph-deploy is bad practice?

python facade pattern implementation in ceph and ceph-deploy is bad practice?

[Owen Synge]

Dear all,

The facade pattern (or façade pattern) is a software design pattern
commonly used with object-oriented programming. The name is by analogy
to an architectural facade. (wikipedia)

I am frustrated with the desire to "standardise" on the one bad practice
implementations of the facade pattern in python that is used in
ceph-deploy even in ceph.

The current "standard" of selectively calling modules with functions has
a series of complexities to it.

Ceph example:

	https://github.com/ceph/ceph/tree/master/src/ceph-detect-init/ceph_detect_init

ceph-deploy example:

	https://github.com/ceph/ceph-deploy/tree/master/ceph_deploy/hosts

SO I guess _some_ of you dont immediately see why this is VERY bad
practice, and wonder why this makes me feel like the orange in this story:

http://www.dailymail.co.uk/news/article-2540670/The-perfect-half-time-oranges-five-football-matches-Farmers-create-pentagon-shaped-fruit.html

when I try to code with ceph-deploy in particular.

The ceph/ceph-deploy way of implementing a facade in python causes this
list the problems:

1) facade cannot be instantiated twice.

2) facade requires code layout inflexibility.

3) facade implementation can have side effects when implementation is
changed.

And probably others I have not thought of.

In consequence from points above.

From (1)

(1A) no concurrency, so you cant configure more than one ceph-node at a
time.
(1A) You have to close one facade to start anouther, eg in ceph-deploy
you have to close each connection before connecting to the next server
so making it slow to use as all state has to be gathered.

From (2)

(2A) No nesting of facades without code duplication. EG reuse of systemd
support between distributions.
(2B) inflexible / complex include paths for shared code between facade
implementations.

From (3)

(3A) Since all variables in a facade implementation are global, but
isolated to an implementation, we cannot have variables in the
implementation, any cross function variables that are not passed as
parameters or return values will return will lead to side effects that
are difficult to test.

So this set of general points will have complex side effects that make
you feel like an the pictured orange when developing that are related to
wear the facade is implemented.

About this point you will say well its open source so fix it ?

My answer to this is that when I try to do this, as in this patch:

https://github.com/osynge/ceph-deploy/commit/b82f89f35b27814ed4aba1082efd134c24ecf21f

More than once, seem to suggest I should use the much more complex multi
file implementation of a façade.

The only advantages of implementing façades in the "standard" ceph /
ceph-deploy way that I can see is:

(I) how ceph-deploy has always done this way.
(II) It allows you to continue using nearly no custom objects in python.
(III) We like our oranges in funny shapes.

It seems to me the current implementation could have been created due to
the misunderstanding that modules are like objects, when intact they are
like classes. Issues (1) and (3) can be solved simply by importing
methods as objects rather than classes, but this does nothing to solve
the bigger issue (2) which is more serious, but its a simple step
forward that might very simple to patch, but their is little point in a
POC unless people agree that issues (1) (2) and (3) are serious.

Please can we not spread this boat anchor* implementation of a facade,
further around the code base, and ideally migrate away from this bad
practice, and help us all feel like happy round oranges.

Best regards

Owen


boat anchor

https://en.wikipedia.org/wiki/Boat_anchor_%28metaphor%29
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: python facade pattern implementation in ceph and ceph-deploy is bad practice?

[John Spray]

Owen,

Please can you say what your overall goal is with recent ceph-deploy 
patches?  Whether a given structure is right or wrong is really a 
function of the overall direction.  If you are aiming to make 
ceph-deploy into an extensible framework that can do things in parallel, 
then you need to say so, and that's a bigger conversation about whether 
that's a reasonable goal for a tool which has so far made a virtue of 
maintaining modest ambitions.

Along these lines, I noticed that you recently submitted a pull request 
to ceph-deploy that added a dependency on SQLAlchemy, and several 
hundred lines of extra boilerplate code -- this kind of thing is 
unlikely to get a warm reception without a stronger justification for 
the extra weight.  While I don't know how related that is to the points 
you're making in this post, but it certainly inspires some curiosity 
about where you're going with this.

I had not seen your wip_services_abstraction branch before, I've just 
taken a quick look now.  More comments would probably have made it 
easier to read, as would following PEP8.  I don't think there's anything 
problematic about having a class that knows how to start and stop a 
service, but I don't know what comments you've received elsewhere (there 
aren't any on the PR).

John


On 09/07/15 11:08, Owen Synge wrote:
> Dear all,
>
> The facade pattern (or façade pattern) is a software design pattern
> commonly used with object-oriented programming. The name is by analogy
> to an architectural facade. (wikipedia)
>
> I am frustrated with the desire to "standardise" on the one bad practice
> implementations of the facade pattern in python that is used in
> ceph-deploy even in ceph.
>
> The current "standard" of selectively calling modules with functions has
> a series of complexities to it.
>
> Ceph example:
>
> 	https://github.com/ceph/ceph/tree/master/src/ceph-detect-init/ceph_detect_init
>
> ceph-deploy example:
>
> 	https://github.com/ceph/ceph-deploy/tree/master/ceph_deploy/hosts
>
> SO I guess _some_ of you dont immediately see why this is VERY bad
> practice, and wonder why this makes me feel like the orange in this story:
>
> http://www.dailymail.co.uk/news/article-2540670/The-perfect-half-time-oranges-five-football-matches-Farmers-create-pentagon-shaped-fruit.html
>
> when I try to code with ceph-deploy in particular.
>
> The ceph/ceph-deploy way of implementing a facade in python causes this
> list the problems:
>
> 1) facade cannot be instantiated twice.
>
> 2) facade requires code layout inflexibility.
>
> 3) facade implementation can have side effects when implementation is
> changed.
>
> And probably others I have not thought of.
>
> In consequence from points above.
>
>  From (1)
>
> (1A) no concurrency, so you cant configure more than one ceph-node at a
> time.
> (1A) You have to close one facade to start anouther, eg in ceph-deploy
> you have to close each connection before connecting to the next server
> so making it slow to use as all state has to be gathered.
>
>  From (2)
>
> (2A) No nesting of facades without code duplication. EG reuse of systemd
> support between distributions.
> (2B) inflexible / complex include paths for shared code between facade
> implementations.
>
>  From (3)
>
> (3A) Since all variables in a facade implementation are global, but
> isolated to an implementation, we cannot have variables in the
> implementation, any cross function variables that are not passed as
> parameters or return values will return will lead to side effects that
> are difficult to test.
>
> So this set of general points will have complex side effects that make
> you feel like an the pictured orange when developing that are related to
> wear the facade is implemented.
>
> About this point you will say well its open source so fix it ?
>
> My answer to this is that when I try to do this, as in this patch:
>
> https://github.com/osynge/ceph-deploy/commit/b82f89f35b27814ed4aba1082efd134c24ecf21f
>
> More than once, seem to suggest I should use the much more complex multi
> file implementation of a façade.
>
> The only advantages of implementing façades in the "standard" ceph /
> ceph-deploy way that I can see is:
>
> (I) how ceph-deploy has always done this way.
> (II) It allows you to continue using nearly no custom objects in python.
> (III) We like our oranges in funny shapes.
>
> It seems to me the current implementation could have been created due to
> the misunderstanding that modules are like objects, when intact they are
> like classes. Issues (1) and (3) can be solved simply by importing
> methods as objects rather than classes, but this does nothing to solve
> the bigger issue (2) which is more serious, but its a simple step
> forward that might very simple to patch, but their is little point in a
> POC unless people agree that issues (1) (2) and (3) are serious.
>
> Please can we not spread this boat anchor* implementation of a facade,
> further around the code base, and ideally migrate away from this bad
> practice, and help us all feel like happy round oranges.
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: python facade pattern implementation in ceph and ceph-deploy is bad practice?

[Owen Synge]

On 07/09/2015 12:46 PM, John Spray wrote:
> Owen,

Hi John,

thanks for your reasonable mail.

> Please can you say what your overall goal is with recent ceph-deploy
> patches? 

To give ceph-deploy code features we have in downstream ceph-deploy to
upstream.

All I am trying to do is reduce copious code duplication during the
process of up streaming. It seems such duplication is wanted for reasons
I am yet to understand.

I only understand that due to architectural bad practices such as this
one in how to do a basic facade class.

I made my downstream code OS preferred directory neutral, and init
system neutral using abstractions.

> Whether a given structure is right or wrong is really a
> function of the overall direction.  If you are aiming to make
> ceph-deploy into an extensible framework that can do things in parallel,
> then you need to say so, and that's a bigger conversation about whether
> that's a reasonable goal for a tool which has so far made a virtue of
> maintaining modest ambitions.

Understood and irrelevant.

As I stated in the email you replied to the issue of only one instance
existing which prevents processing more than one node at a time, or
even, connecting to two servers at the same time (so you can query
update a second node) can be worked around, by importing the modules as
objects, but issue (2) the structural "orange" issues are far more
serious, leading to structural artifacts of a badly implemented facade
pattern interfere with the complete code base.

> Along these lines, I noticed that you recently submitted a pull request
> to ceph-deploy that added a dependency on SQLAlchemy, and several
> hundred lines of extra boilerplate code -- this kind of thing is
> unlikely to get a warm reception without a stronger justification for
> the extra weight.  While I don't know how related that is to the points
> you're making in this post, but it certainly inspires some curiosity
> about where you're going with this.

It was presented as a possible way to make a MVC model.

It was only a POC, that this way of doing things could do all that is
required.

I pulled this POC as it was told to me that no matter what I said MVC or
structural changes to ceph-deploy in any way where to be opposed by
former developers of ceph-deploy.

I might have presented an alternative MVC but since the objections where
to MVC and also SQLAlchemy when ever it would strengthen arguments
against MVC, without even waiting to discuss positives or negatives.

I did not want all my "bug fix" patches for ceph-deploy not beign
reviewed due to peoples seeing my name, and fearing letting in any
patches which I did into the code base.

> I had not seen your wip_services_abstraction branch before, I've just
> taken a quick look now.  More comments would probably have made it
> easier to read, as would following PEP8.  

Good review points that would have encouraged me we where moving
forwards not backwards, and suggesting everythign has to be part of the
"boat anchor" facade we already have.

> I don't think there's anything
> problematic about having a class that knows how to start and stop a
> service, but I don't know what comments you've received elsewhere (there
> aren't any on the PR).

Here:

https://github.com/ceph/ceph-deploy/pull/317

and this is not the first time this has occurred.

Best regards

Owen

PS I have to fork ceph-deploy rgw today as I have deadlines to get some
thing out of the door.

PPS I did not intentionally kill the branch and will investigate why its
missing.


> 
> John
> 
> 
> On 09/07/15 11:08, Owen Synge wrote:
>> Dear all,
>>
>> The facade pattern (or façade pattern) is a software design pattern
>> commonly used with object-oriented programming. The name is by analogy
>> to an architectural facade. (wikipedia)
>>
>> I am frustrated with the desire to "standardise" on the one bad practice
>> implementations of the facade pattern in python that is used in
>> ceph-deploy even in ceph.
>>
>> The current "standard" of selectively calling modules with functions has
>> a series of complexities to it.
>>
>> Ceph example:
>>
>>     https://github.com/ceph/ceph/tree/master/src/ceph-detect-init/ceph_detect_init
>>
>>
>> ceph-deploy example:
>>
>>     https://github.com/ceph/ceph-deploy/tree/master/ceph_deploy/hosts
>>
>> SO I guess _some_ of you dont immediately see why this is VERY bad
>> practice, and wonder why this makes me feel like the orange in this
>> story:
>>
>> http://www.dailymail.co.uk/news/article-2540670/The-perfect-half-time-oranges-five-football-matches-Farmers-create-pentagon-shaped-fruit.html
>>
>>
>> when I try to code with ceph-deploy in particular.
>>
>> The ceph/ceph-deploy way of implementing a facade in python causes this
>> list the problems:
>>
>> 1) facade cannot be instantiated twice.
>>
>> 2) facade requires code layout inflexibility.
>>
>> 3) facade implementation can have side effects when implementation is
>> changed.
>>
>> And probably others I have not thought of.
>>
>> In consequence from points above.
>>
>>  From (1)
>>
>> (1A) no concurrency, so you cant configure more than one ceph-node at a
>> time.
>> (1A) You have to close one facade to start anouther, eg in ceph-deploy
>> you have to close each connection before connecting to the next server
>> so making it slow to use as all state has to be gathered.
>>
>>  From (2)
>>
>> (2A) No nesting of facades without code duplication. EG reuse of systemd
>> support between distributions.
>> (2B) inflexible / complex include paths for shared code between facade
>> implementations.
>>
>>  From (3)
>>
>> (3A) Since all variables in a facade implementation are global, but
>> isolated to an implementation, we cannot have variables in the
>> implementation, any cross function variables that are not passed as
>> parameters or return values will return will lead to side effects that
>> are difficult to test.
>>
>> So this set of general points will have complex side effects that make
>> you feel like an the pictured orange when developing that are related to
>> wear the facade is implemented.
>>
>> About this point you will say well its open source so fix it ?
>>
>> My answer to this is that when I try to do this, as in this patch:
>>
>> https://github.com/osynge/ceph-deploy/commit/b82f89f35b27814ed4aba1082efd134c24ecf21f
>>
>>
>> More than once, seem to suggest I should use the much more complex multi
>> file implementation of a façade.
>>
>> The only advantages of implementing façades in the "standard" ceph /
>> ceph-deploy way that I can see is:
>>
>> (I) how ceph-deploy has always done this way.
>> (II) It allows you to continue using nearly no custom objects in python.
>> (III) We like our oranges in funny shapes.
>>
>> It seems to me the current implementation could have been created due to
>> the misunderstanding that modules are like objects, when intact they are
>> like classes. Issues (1) and (3) can be solved simply by importing
>> methods as objects rather than classes, but this does nothing to solve
>> the bigger issue (2) which is more serious, but its a simple step
>> forward that might very simple to patch, but their is little point in a
>> POC unless people agree that issues (1) (2) and (3) are serious.
>>
>> Please can we not spread this boat anchor* implementation of a facade,
>> further around the code base, and ideally migrate away from this bad
>> practice, and help us all feel like happy round oranges.
> 

-- 
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB
21284 (AG
Nürnberg)

Maxfeldstraße 5

90409 Nürnberg

Germany
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: python facade pattern implementation in ceph and ceph-deploy is bad practice?

[Owen Synge]

> PPS I did not intentionally kill the branch and will investigate why its
> missing.

Correction it is not missing and is still here.

https://github.com/ceph/ceph-deploy/pull/320

I still feel that I should close it in case people stop reviewing my
patches though.

Best regards

Owen

Re: python facade pattern implementation in ceph and ceph-deploy is bad practice?

[Travis Rhoden]

> On Jul 9, 2015, at 4:59 AM, Owen Synge <osynge@suse.com> wrote:
> 
> On 07/09/2015 12:46 PM, John Spray wrote:
>> Owen,
> 
> Hi John,
> 
> thanks for your reasonable mail.
> 
>> Please can you say what your overall goal is with recent ceph-deploy
>> patches? 
> 
> To give ceph-deploy code features we have in downstream ceph-deploy to
> upstream.
> 
> All I am trying to do is reduce copious code duplication during the
> process of up streaming. It seems such duplication is wanted for reasons
> I am yet to understand.
> 
> I only understand that due to architectural bad practices such as this
> one in how to do a basic facade class.
> 
> I made my downstream code OS preferred directory neutral, and init
> system neutral using abstractions.

I’ve added a new set of comments in https://github.com/ceph/ceph-deploy/pull/317 that hopefully clarifies my position.

I am *not* in favor of copious code duplication.

I *am* in favor of not introducing new paradigms or architectures in the stable 1.5.x series.  The 1.5.x series is packaged and supported downstream, and introducing major changes will complicate that.  ceph-deploy has been in a state where the main changes we have been taking are: (1) bug/security fixes (2) new features to keep up with Ceph features (like civetweb-based RGW) (3) fixes to make user help and execution output more clear.

A major refactoring (I would consider anything that moves away from the current facade paradigm to be major in 1.5.x) doesn’t fit into that.  However, ceph-deploy is not dead or maintenance only, so long as there is community interest.  The upstream installer developers (of which I am one) and Red Hat (also me again) are looking at alternatives for more robust installation solutions that do not involve ceph-deploy.  That is also a motivation for not making major changes, like those proposed in https://github.com/ceph/ceph-deploy/pull/320.  I am concerned that it will make long-term maintenance more complicated.

> 
>> Whether a given structure is right or wrong is really a
>> function of the overall direction.  If you are aiming to make
>> ceph-deploy into an extensible framework that can do things in parallel,
>> then you need to say so, and that's a bigger conversation about whether
>> that's a reasonable goal for a tool which has so far made a virtue of
>> maintaining modest ambitions.
> 
> Understood and irrelevant.
> 
> As I stated in the email you replied to the issue of only one instance
> existing which prevents processing more than one node at a time, or
> even, connecting to two servers at the same time (so you can query
> update a second node) can be worked around, by importing the modules as
> objects, but issue (2) the structural "orange" issues are far more
> serious, leading to structural artifacts of a badly implemented facade
> pattern interfere with the complete code base.
> 
>> Along these lines, I noticed that you recently submitted a pull request
>> to ceph-deploy that added a dependency on SQLAlchemy, and several
>> hundred lines of extra boilerplate code -- this kind of thing is
>> unlikely to get a warm reception without a stronger justification for
>> the extra weight.  While I don't know how related that is to the points
>> you're making in this post, but it certainly inspires some curiosity
>> about where you're going with this.
> 
> It was presented as a possible way to make a MVC model.
> 
> It was only a POC, that this way of doing things could do all that is
> required.
> 
> I pulled this POC as it was told to me that no matter what I said MVC or
> structural changes to ceph-deploy in any way where to be opposed by
> former developers of ceph-deploy.

We’ve been trying to only accept code into ceph-deploy that has a corresponding bugzilla or Ceph tracker bug referenced.  I mention this because it helps us answer the question “what problem does this solve”?  We haven’t enforced to all contributors in the community, but we’re trying to be more stringent about it.  The code you’ve proposed in https://github.com/ceph/ceph-deploy/pull/320 seems to mainly address creating required RGW pools.  I’d like to see a bug opened against that so that we can see if it is indeed something that needs to be fixed.  It may very well be that this is something that RGW is supposed to be doing itself (just talking out loud again).

> 
> I might have presented an alternative MVC but since the objections where
> to MVC and also SQLAlchemy when ever it would strengthen arguments
> against MVC, without even waiting to discuss positives or negatives.
> 
> I did not want all my "bug fix" patches for ceph-deploy not beign
> reviewed due to peoples seeing my name, and fearing letting in any
> patches which I did into the code base.

I can only speak for myself here, but I would not do that and I don’t think my colleagues would either.  It would not lead to a very friendly community if any of us would rather have bugs in the code than review perfectly good patches that fix real problems.

> 
>> I had not seen your wip_services_abstraction branch before, I've just
>> taken a quick look now.  More comments would probably have made it
>> easier to read, as would following PEP8.  
> 
> Good review points that would have encouraged me we where moving
> forwards not backwards, and suggesting everythign has to be part of the
> "boat anchor" facade we already have.
> 
>> I don't think there's anything
>> problematic about having a class that knows how to start and stop a
>> service, but I don't know what comments you've received elsewhere (there
>> aren't any on the PR).

All the comments relevant to the class abstraction are in that PR.  Other conversations have taken place on IRC, but that’s been around the proposal for more of an MVC style that incorporates SQL Alchemy, and that is more related to PR #320.  I’ve clarified my review on #317 to say that I like the classes for the init systems as well, but want to see them used a bit differently and to not use the higher level abstraction above them.

> 
> Here:
> 
> https://github.com/ceph/ceph-deploy/pull/317
> 
> and this is not the first time this has occurred.
> 
> Best regards
> 
> Owen
> 
> PS I have to fork ceph-deploy rgw today as I have deadlines to get some
> thing out of the door.
> 
> PPS I did not intentionally kill the branch and will investigate why its
> missing.
> 
> 
>> 
>> John
>> 
>> 
>> On 09/07/15 11:08, Owen Synge wrote:
>>> Dear all,
>>> 
>>> The facade pattern (or façade pattern) is a software design pattern
>>> commonly used with object-oriented programming. The name is by analogy
>>> to an architectural facade. (wikipedia)
>>> 
>>> I am frustrated with the desire to "standardise" on the one bad practice
>>> implementations of the facade pattern in python that is used in
>>> ceph-deploy even in ceph.
>>> 
>>> The current "standard" of selectively calling modules with functions has
>>> a series of complexities to it.
>>> 
>>> Ceph example:
>>> 
>>>    https://github.com/ceph/ceph/tree/master/src/ceph-detect-init/ceph_detect_init
>>> 
>>> 
>>> ceph-deploy example:
>>> 
>>>    https://github.com/ceph/ceph-deploy/tree/master/ceph_deploy/hosts
>>> 
>>> SO I guess _some_ of you dont immediately see why this is VERY bad
>>> practice, and wonder why this makes me feel like the orange in this
>>> story:
>>> 
>>> http://www.dailymail.co.uk/news/article-2540670/The-perfect-half-time-oranges-five-football-matches-Farmers-create-pentagon-shaped-fruit.html
>>> 
>>> 
>>> when I try to code with ceph-deploy in particular.
>>> 
>>> The ceph/ceph-deploy way of implementing a facade in python causes this
>>> list the problems:
>>> 
>>> 1) facade cannot be instantiated twice.
>>> 
>>> 2) facade requires code layout inflexibility.
>>> 
>>> 3) facade implementation can have side effects when implementation is
>>> changed.
>>> 
>>> And probably others I have not thought of.
>>> 
>>> In consequence from points above.
>>> 
>>> From (1)
>>> 
>>> (1A) no concurrency, so you cant configure more than one ceph-node at a
>>> time.
>>> (1A) You have to close one facade to start anouther, eg in ceph-deploy
>>> you have to close each connection before connecting to the next server
>>> so making it slow to use as all state has to be gathered.
>>> 
>>> From (2)
>>> 
>>> (2A) No nesting of facades without code duplication. EG reuse of systemd
>>> support between distributions.
>>> (2B) inflexible / complex include paths for shared code between facade
>>> implementations.
>>> 
>>> From (3)
>>> 
>>> (3A) Since all variables in a facade implementation are global, but
>>> isolated to an implementation, we cannot have variables in the
>>> implementation, any cross function variables that are not passed as
>>> parameters or return values will return will lead to side effects that
>>> are difficult to test.
>>> 
>>> So this set of general points will have complex side effects that make
>>> you feel like an the pictured orange when developing that are related to
>>> wear the facade is implemented.
>>> 
>>> About this point you will say well its open source so fix it ?
>>> 
>>> My answer to this is that when I try to do this, as in this patch:
>>> 
>>> https://github.com/osynge/ceph-deploy/commit/b82f89f35b27814ed4aba1082efd134c24ecf21f
>>> 
>>> 
>>> More than once, seem to suggest I should use the much more complex multi
>>> file implementation of a façade.
>>> 
>>> The only advantages of implementing façades in the "standard" ceph /
>>> ceph-deploy way that I can see is:
>>> 
>>> (I) how ceph-deploy has always done this way.
>>> (II) It allows you to continue using nearly no custom objects in python.
>>> (III) We like our oranges in funny shapes.
>>> 
>>> It seems to me the current implementation could have been created due to
>>> the misunderstanding that modules are like objects, when intact they are
>>> like classes. Issues (1) and (3) can be solved simply by importing
>>> methods as objects rather than classes, but this does nothing to solve
>>> the bigger issue (2) which is more serious, but its a simple step
>>> forward that might very simple to patch, but their is little point in a
>>> POC unless people agree that issues (1) (2) and (3) are serious.
>>> 
>>> Please can we not spread this boat anchor* implementation of a facade,
>>> further around the code base, and ideally migrate away from this bad
>>> practice, and help us all feel like happy round oranges.
>> 
> 
> -- 
> SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB
> 21284 (AG
> Nürnberg)
> 
> Maxfeldstraße 5
> 
> 90409 Nürnberg
> 
> Germany

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: python facade pattern implementation in ceph and ceph-deploy is bad practice?

[Owen Synge]

On 07/09/2015 09:58 PM, Travis Rhoden wrote:
> 
>> On Jul 9, 2015, at 4:59 AM, Owen Synge <osynge@suse.com> wrote:
>>
>> On 07/09/2015 12:46 PM, John Spray wrote:
>>> Owen,
>>
>> Hi John,
>>
>> thanks for your reasonable mail.
>>
>>> Please can you say what your overall goal is with recent ceph-deploy
>>> patches? 
>>
>> To give ceph-deploy code features we have in downstream ceph-deploy to
>> upstream.
>>
>> All I am trying to do is reduce copious code duplication during the
>> process of up streaming. It seems such duplication is wanted for reasons
>> I am yet to understand.
>>
>> I only understand that due to architectural bad practices such as this
>> one in how to do a basic facade class.
>>
>> I made my downstream code OS preferred directory neutral, and init
>> system neutral using abstractions.
> 
> I’ve added a new set of comments in https://github.com/ceph/ceph-deploy/pull/317 that hopefully clarifies my position.

A lot thank you.

> I am *not* in favor of copious code duplication.

Good. I am glad you clarified this in both the pull request and in this
thread.

> I *am* in favor of not introducing new paradigms or architectures in the stable 1.5.x series.  The 1.5.x series is packaged and supported downstream, and introducing major changes will complicate that.  ceph-deploy has been in a state where the main changes we have been taking are: (1) bug/security fixes (2) new features to keep up with Ceph features (like civetweb-based RGW) (3) fixes to make user help and execution output more clear.

Good. This was my major issue, that led to this thread.

> A major refactoring (I would consider anything that moves away from the current facade paradigm to be major in 1.5.x) doesn’t fit into that.  However, ceph-deploy is not dead or maintenance only, so long as there is community interest.  The upstream installer developers (of which I am one) and Red Hat (also me again) are looking at alternatives for more robust installation solutions that do not involve ceph-deploy.  That is also a motivation for not making major changes, like those proposed in https://github.com/ceph/ceph-deploy/pull/320.  I am concerned that it will make long-term maintenance more complicated.

I quiet understand redhat and upstreams perspective that ceph-deploy is
NOT a good plan going forward as a "robust installation solutions" for
large clusters. I also favour using more conventional cluster management
solutions on large and production clusters.

To me, when working on new functionality, such as rgw with fastcgi or
civetweb is not re factoring.

My only remaining fear is that you consider a major re-factoring is for
me minor changes for new development.

This probably only reflects our different attitudes to the compromise
between incremental improvement Vs consistency.

>>> Whether a given structure is right or wrong is really a
>>> function of the overall direction.  If you are aiming to make
>>> ceph-deploy into an extensible framework that can do things in parallel,
>>> then you need to say so, and that's a bigger conversation about whether
>>> that's a reasonable goal for a tool which has so far made a virtue of
>>> maintaining modest ambitions.
>>
>> Understood and irrelevant.
>>
>> As I stated in the email you replied to the issue of only one instance
>> existing which prevents processing more than one node at a time, or
>> even, connecting to two servers at the same time (so you can query
>> update a second node) can be worked around, by importing the modules as
>> objects, but issue (2) the structural "orange" issues are far more
>> serious, leading to structural artifacts of a badly implemented facade
>> pattern interfere with the complete code base.
>>
>>> Along these lines, I noticed that you recently submitted a pull request
>>> to ceph-deploy that added a dependency on SQLAlchemy, and several
>>> hundred lines of extra boilerplate code -- this kind of thing is
>>> unlikely to get a warm reception without a stronger justification for
>>> the extra weight.  While I don't know how related that is to the points
>>> you're making in this post, but it certainly inspires some curiosity
>>> about where you're going with this.
>>
>> It was presented as a possible way to make a MVC model.
>>
>> It was only a POC, that this way of doing things could do all that is
>> required.
>>
>> I pulled this POC as it was told to me that no matter what I said MVC or
>> structural changes to ceph-deploy in any way where to be opposed by
>> former developers of ceph-deploy.
> 
> We’ve been trying to only accept code into ceph-deploy that has a corresponding bugzilla or Ceph tracker bug referenced.  I mention this because it helps us answer the question “what problem does this solve”?  We haven’t enforced to all contributors in the community, but we’re trying to be more stringent about it.  The code you’ve proposed in https://github.com/ceph/ceph-deploy/pull/320 seems to mainly address creating required RGW pools.  I’d like to see a bug opened against that so that we can see if it is indeed something that needs to be fixed.  It may very well be that this is something that RGW is supposed to be doing itself (just talking out loud again).

I think here we see the reason why I pulled my discussion point.

You considered it a bug fix / contribution to the code.

I considered it a discussion point on code quality and best practice.

If you are happy to consider this as a discussion point on code quality
and best practice (as I thought I had explained the day before in the
meeting), then I am happy to start discussion in a meeting again.

>> I might have presented an alternative MVC but since the objections where
>> to MVC and also SQLAlchemy when ever it would strengthen arguments
>> against MVC, without even waiting to discuss positives or negatives.
>>
>> I did not want all my "bug fix" patches for ceph-deploy not beign
>> reviewed due to peoples seeing my name, and fearing letting in any
>> patches which I did into the code base.
> 
> I can only speak for myself here, but I would not do that and I don’t think my colleagues would either.  It would not lead to a very friendly community if any of us would rather have bugs in the code than review perfectly good patches that fix real problems.

Now we are clear that I wanted to discuss improving the code style,
(particularly for new functionality in rgw)

As I understand it I should not in future:

1) Provide demo code on discussion points. Slides are a better idea.
2) Discuss implementation, best practice and design at the same time.
3) Make discussion points when I have any open pull requests for fear
that others may mix these independent issues.
4) Ever (even if explicitly stated as was the case) make a pull request
for any other purposes than intending to merge with the code base.

I should in future:

1) Raise all proposals in written form.
2) Raise all proposals in written form that is public such as this
mailing list first and never in a private meeting if I expect any
discussion.
3) Always assume the reader is going to conflate every concept in the
discussion unless explicitly written down in public.
4) Contact the author of "unhelpful" comments for clarification and give
them at least 2 working days to answer as they may have wrote and I may
have read different things from what is intended by the author or
interpreted by me the reader of "unhelpful" comments.


Your suggestions for how to avoid such friction and misunderstanding in
future are very welcome.

>>> I had not seen your wip_services_abstraction branch before, I've just
>>> taken a quick look now.  More comments would probably have made it
>>> easier to read, as would following PEP8.  
>>
>> Good review points that would have encouraged me we where moving
>> forwards not backwards, and suggesting everythign has to be part of the
>> "boat anchor" facade we already have.
>>
>>> I don't think there's anything
>>> problematic about having a class that knows how to start and stop a
>>> service, but I don't know what comments you've received elsewhere (there
>>> aren't any on the PR).
> 
> All the comments relevant to the class abstraction are in that PR.  Other conversations have taken place on IRC, but that’s been around the proposal for more of an MVC style that incorporates SQL Alchemy, and that is more related to PR #320.  I’ve clarified my review on #317 to say that I like the classes for the init systems as well, but want to see them used a bit differently and to not use the higher level abstraction above them.

You have explicitly stated that you are open to such suggestions as
introducing classes and objects to the code base of ceph-deploy.

I hope you now understand that I misunderstood some statements you have
since withdrawn that led me to believe that you and others in the team
opposed any "change" in the code style ever.

I now understand I misunderstood the rules and practices you used for
communication.

Following this very helpful thread I will start a new thread on MVC. If
that goes well I will then start a discussion thread on implementation.

Best regards

Owen
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: python facade pattern implementation in ceph and ceph-deploy is bad practice?

[Travis Rhoden]

> On Jul 14, 2015, at 1:47 AM, Owen Synge <osynge@suse.com> wrote:
> 
> On 07/09/2015 09:58 PM, Travis Rhoden wrote:
>> 
>>> On Jul 9, 2015, at 4:59 AM, Owen Synge <osynge@suse.com> wrote:
>>> 
>>> On 07/09/2015 12:46 PM, John Spray wrote:
>>>> Owen,
>>> 
>>> Hi John,
>>> 
>>> thanks for your reasonable mail.
>>> 
>>>> Please can you say what your overall goal is with recent ceph-deploy
>>>> patches? 
>>> 
>>> To give ceph-deploy code features we have in downstream ceph-deploy to
>>> upstream.
>>> 
>>> All I am trying to do is reduce copious code duplication during the
>>> process of up streaming. It seems such duplication is wanted for reasons
>>> I am yet to understand.
>>> 
>>> I only understand that due to architectural bad practices such as this
>>> one in how to do a basic facade class.
>>> 
>>> I made my downstream code OS preferred directory neutral, and init
>>> system neutral using abstractions.
>> 
>> I’ve added a new set of comments in https://github.com/ceph/ceph-deploy/pull/317 that hopefully clarifies my position.
> 
> A lot thank you.
> 
>> I am *not* in favor of copious code duplication.
> 
> Good. I am glad you clarified this in both the pull request and in this
> thread.
> 
>> I *am* in favor of not introducing new paradigms or architectures in the stable 1.5.x series.  The 1.5.x series is packaged and supported downstream, and introducing major changes will complicate that.  ceph-deploy has been in a state where the main changes we have been taking are: (1) bug/security fixes (2) new features to keep up with Ceph features (like civetweb-based RGW) (3) fixes to make user help and execution output more clear.
> 
> Good. This was my major issue, that led to this thread.
> 
>> A major refactoring (I would consider anything that moves away from the current facade paradigm to be major in 1.5.x) doesn’t fit into that.  However, ceph-deploy is not dead or maintenance only, so long as there is community interest.  The upstream installer developers (of which I am one) and Red Hat (also me again) are looking at alternatives for more robust installation solutions that do not involve ceph-deploy.  That is also a motivation for not making major changes, like those proposed in https://github.com/ceph/ceph-deploy/pull/320.  I am concerned that it will make long-term maintenance more complicated.
> 
> I quiet understand redhat and upstreams perspective that ceph-deploy is
> NOT a good plan going forward as a "robust installation solutions" for
> large clusters. I also favour using more conventional cluster management
> solutions on large and production clusters.
> 
> To me, when working on new functionality, such as rgw with fastcgi or
> civetweb is not re factoring.
> 
> My only remaining fear is that you consider a major re-factoring is for
> me minor changes for new development.
> 
> This probably only reflects our different attitudes to the compromise
> between incremental improvement Vs consistency.

Probably a very true observation.

> 
>>>> Whether a given structure is right or wrong is really a
>>>> function of the overall direction.  If you are aiming to make
>>>> ceph-deploy into an extensible framework that can do things in parallel,
>>>> then you need to say so, and that's a bigger conversation about whether
>>>> that's a reasonable goal for a tool which has so far made a virtue of
>>>> maintaining modest ambitions.
>>> 
>>> Understood and irrelevant.
>>> 
>>> As I stated in the email you replied to the issue of only one instance
>>> existing which prevents processing more than one node at a time, or
>>> even, connecting to two servers at the same time (so you can query
>>> update a second node) can be worked around, by importing the modules as
>>> objects, but issue (2) the structural "orange" issues are far more
>>> serious, leading to structural artifacts of a badly implemented facade
>>> pattern interfere with the complete code base.
>>> 
>>>> Along these lines, I noticed that you recently submitted a pull request
>>>> to ceph-deploy that added a dependency on SQLAlchemy, and several
>>>> hundred lines of extra boilerplate code -- this kind of thing is
>>>> unlikely to get a warm reception without a stronger justification for
>>>> the extra weight.  While I don't know how related that is to the points
>>>> you're making in this post, but it certainly inspires some curiosity
>>>> about where you're going with this.
>>> 
>>> It was presented as a possible way to make a MVC model.
>>> 
>>> It was only a POC, that this way of doing things could do all that is
>>> required.
>>> 
>>> I pulled this POC as it was told to me that no matter what I said MVC or
>>> structural changes to ceph-deploy in any way where to be opposed by
>>> former developers of ceph-deploy.
>> 
>> We’ve been trying to only accept code into ceph-deploy that has a corresponding bugzilla or Ceph tracker bug referenced. I mention this because it helps us answer the question “what problem does this solve”?  We haven’t enforced to all contributors in the community, but we’re trying to be more stringent about it.  The code you’ve proposed in https://github.com/ceph/ceph-deploy/pull/320 seems to mainly address creating required RGW pools.  I’d like to see a bug opened against that so that we can see if it is indeed something that needs to be fixed.  It may very well be that this is something that RGW is supposed to be doing itself (just talking out loud again).
> 
> I think here we see the reason why I pulled my discussion point.
> 
> You considered it a bug fix / contribution to the code.
> 
> I considered it a discussion point on code quality and best practice.
> 
> If you are happy to consider this as a discussion point on code quality
> and best practice (as I thought I had explained the day before in the
> meeting), then I am happy to start discussion in a meeting again.

I do think the waters got muddied.  I take some blame for that.  It is much easier to evaluate the code in light of the exact problem it is trying to fix rather than seeing it as a model for something different.  You did make it clear that it was for discussion only.  We do this all the time, actually.  Usually by prefixing Pull Request titles with “DNM” — “Do Not Merge”.  Though sometimes that can also be “I want you to review this early, even though it’s not ready”.

> 
>>> I might have presented an alternative MVC but since the objections where
>>> to MVC and also SQLAlchemy when ever it would strengthen arguments
>>> against MVC, without even waiting to discuss positives or negatives.
>>> 
>>> I did not want all my "bug fix" patches for ceph-deploy not beign
>>> reviewed due to peoples seeing my name, and fearing letting in any
>>> patches which I did into the code base.
>> 
>> I can only speak for myself here, but I would not do that and I don’t think my colleagues would either.  It would not lead to a very friendly community if any of us would rather have bugs in the code than review perfectly good patches that fix real problems.
> 
> Now we are clear that I wanted to discuss improving the code style,
> (particularly for new functionality in rgw)
> 
> As I understand it I should not in future:
> 
> 1) Provide demo code on discussion points. Slides are a better idea.

I don’t think any of us want slides. :)  DNM pull requests are fine.

> 2) Discuss implementation, best practice and design at the same time.
> 3) Make discussion points when I have any open pull requests for fear
> that others may mix these independent issues.

This is hard to say.  I certainly don’t think that it is bad to have pull request open when trying to talk about major architectural changes.  Each PR should stand on its own merit, and be reviewed individually.  What is probably helpful is if we make the intent of each PR clear — as in this is a simple bug fix, this is a discussion point / idea, etc.

> 4) Ever (even if explicitly stated as was the case) make a pull request
> for any other purposes than intending to merge with the code base.

Again, I wouldn’t go that far.  I think most of speak code better than slides.  When proposing something major, it’s always a risk.  You have to fully flesh out the idea to make it work and convince others that it’s good, but that takes time and effort that you are guaranteed to see pay off.  If enough discussion can happen early on to see if the idea will be well received, that’s better for everyone.  I think you did try to do that, and the change in design pattern was not well received.  However in this context, I know I evaluated it in terms of *could* I merge this right now, not “would this be worthwhile down the road”.

> 
> I should in future:
> 
> 1) Raise all proposals in written form.
> 2) Raise all proposals in written form that is public such as this
> mailing list first and never in a private meeting if I expect any
> discussion.

Open discussion is best, for sure.  The more eyes the better.  When we chatted in our standup, it was a difficult forum as it was not the correct audience.  Those daily standups pretty much never will be right forum, as they are much more of the form “what did you do yesterday, what are you going to do today, and is there anything blocking your progress?”.  They are meant to be fast paced and keep the group up to date with everyone else’s activities.

> 3) Always assume the reader is going to conflate every concept in the
> discussion unless explicitly written down in public.
> 4) Contact the author of "unhelpful" comments for clarification and give
> them at least 2 working days to answer as they may have wrote and I may
> have read different things from what is intended by the author or
> interpreted by me the reader of "unhelpful" comments.
> 
> 
> Your suggestions for how to avoid such friction and misunderstanding in
> future are very welcome.

I think we are on the right track right here — sticking to ceph-devel the mailing list, and IRC.  As for comments you deem unhelpful, asking for clarification does seem like the way to go.

> 
>>>> I had not seen your wip_services_abstraction branch before, I've just
>>>> taken a quick look now.  More comments would probably have made it
>>>> easier to read, as would following PEP8.  
>>> 
>>> Good review points that would have encouraged me we where moving
>>> forwards not backwards, and suggesting everythign has to be part of the
>>> "boat anchor" facade we already have.
>>> 
>>>> I don't think there's anything
>>>> problematic about having a class that knows how to start and stop a
>>>> service, but I don't know what comments you've received elsewhere (there
>>>> aren't any on the PR).
>> 
>> All the comments relevant to the class abstraction are in that PR.  Other conversations have taken place on IRC, but that’s been around the proposal for more of an MVC style that incorporates SQL Alchemy, and that is more related to PR #320. I’ve clarified my review on #317 to say that I like the classes for the init systems as well, but want to see them used a bit differently and to not use the higher level abstraction above them.
> 
> You have explicitly stated that you are open to such suggestions as
> introducing classes and objects to the code base of ceph-deploy.
> 
> I hope you now understand that I misunderstood some statements you have
> since withdrawn that led me to believe that you and others in the team
> opposed any "change" in the code style ever.
> 
> I now understand I misunderstood the rules and practices you used for
> communication.
> 
> Following this very helpful thread I will start a new thread on MVC. If
> that goes well I will then start a discussion thread on implementation.

 I think that sounds like a prudent plan.

> 
> Best regards
> 
> Owen

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: python facade pattern implementation in ceph and ceph-deploy is bad practice?

[Owen Synge]

Dear all,

Lets put a positive spin on this thread and set all misunderstandings on
my side :)

I propose that John clarified and I misunderstood the others in upstream
ceph-deploy's position, the style guide includes:

(0) Opposes duplication of code.
(1) Opposes duplication of code for each platform making up more than a
minimum of code.
(2) Allowing the use of objects in python.
(3) Allowing the use of objects with properties.
(4) Allowing the use of some standard design patterns not already in
ceph-deploy.
(5) Allowing that a change in style (such as use of 3 objects in a
facade) has to be propagated in all parts of ceph and tested for all
platforms before they can reach master even if the migration can be staged.

Hopefully this can be agreed.

On 07/09/2015 12:46 PM, John Spray wrote:

> I had not seen your wip_services_abstraction branch before, I've just
> taken a quick look now.  More comments would probably have made it
> easier to read, as would following PEP8.  I don't think there's anything
> problematic about having a class that knows how to start and stop a
> service, but I don't know what comments you've received elsewhere (there
> aren't any on the PR).

If this is not agreeable to all, please speak up.

Best regards

Owen

Re: python facade pattern implementation in ceph and ceph-deploy is bad practice?

[Owen Synge]

Small correction due to not proof reading enough.

On 07/09/2015 06:28 PM, Owen Synge wrote:
> Dear all,
> 
> Lets put a positive spin on this thread and set all misunderstandings on
> my side :)
> 
> I propose that John clarified and I misunderstood the others in upstream
> ceph-deploy's position, the style guide includes:
> 
> (0) Opposes duplication of code.
> (1) Opposes duplication of code for each platform making up more than a
> minimum of code.
> (2) Allowing the use of objects in python.
> (3) Allowing the use of objects with properties.
> (4) Allowing the use of some standard design patterns not already in
> ceph-deploy.

(5) Allow changes to be made in one part of ceph-deploy without having
to change all parts of ceph-deploy at the same time, when making an
implementation of some thing "similar" but not the "same" in a better
way. (such as use of 3 objects in a facade, and not binding it to the
"single façade" that can exist in ceph-deploy)

> Hopefully this can be agreed.
> 
> On 07/09/2015 12:46 PM, John Spray wrote:
> 
>> I had not seen your wip_services_abstraction branch before, I've just
>> taken a quick look now.  More comments would probably have made it
>> easier to read, as would following PEP8.  I don't think there's anything
>> problematic about having a class that knows how to start and stop a
>> service, but I don't know what comments you've received elsewhere (there
>> aren't any on the PR).
> 
> If this is not agreeable to all, please speak up.
> 
> Best regards
> 
> Owen
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB
21284 (AG
Nürnberg)

Maxfeldstraße 5

90409 Nürnberg

Germany
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: python facade pattern implementation in ceph and ceph-deploy is bad practice?

[Travis Rhoden]

Hi Owen,

There are quite a few emails in this thread already, but there are points in each of them I would like to address.

Up front let me say I hear and recognize your frustration.  If you’ve felt ignored, I apologize.  I think you’ve turned around a lot of patches, PR comments, and emails faster then I’ve been able to look at them all.  It’s completely been “I haven’t gotten there yet” and none of “I’m ignoring you”.

> On Jul 9, 2015, at 3:08 AM, Owen Synge <osynge@suse.com> wrote:
> 
> Dear all,
> 
> The facade pattern (or façade pattern) is a software design pattern
> commonly used with object-oriented programming. The name is by analogy
> to an architectural facade. (wikipedia)
> 
> I am frustrated with the desire to "standardise" on the one bad practice
> implementations of the facade pattern in python that is used in
> ceph-deploy even in ceph.
> 
> The current "standard" of selectively calling modules with functions has
> a series of complexities to it.
> 
> Ceph example:
> 
> 	https://github.com/ceph/ceph/tree/master/src/ceph-detect-init/ceph_detect_init
> 
> ceph-deploy example:
> 
> 	https://github.com/ceph/ceph-deploy/tree/master/ceph_deploy/hosts
> 
> SO I guess _some_ of you dont immediately see why this is VERY bad
> practice, and wonder why this makes me feel like the orange in this story:
> 
> http://www.dailymail.co.uk/news/article-2540670/The-perfect-half-time-oranges-five-football-matches-Farmers-create-pentagon-shaped-fruit.html
> 
> when I try to code with ceph-deploy in particular.

I’ve never heard the facade name attached to it.  For our modest goals with ceph-deploy we have found it be quite flexilble, as it allows us to do have distro specific code only where it is needed, and to pull all common code into common places, all behind a consistent abstracted interface.  I think this actually leads to less code duplication, but there are plenty of places in ceph-deploy where this is not the case and the code could be cleaned up to bring more code into common places.

Let me also say that specific to one of the comments I made in your PR, when I brought up this facade idea, I was just talking out loud.  The comment I was going to leave today (and still will) is that my point there was unrelated to your PR.  It sent us off in a wrong direction, and was not relevant to the code under discussion.  It was more a note to myself that this was something that was still on our plate, but it did not/does not need to change what you were looking at specifically.

> 
> The ceph/ceph-deploy way of implementing a facade in python causes this
> list the problems:
> 
> 1) facade cannot be instantiated twice.
> 
> 2) facade requires code layout inflexibility.
> 
> 3) facade implementation can have side effects when implementation is
> changed.
> 
> And probably others I have not thought of.
> 
> In consequence from points above.
> 
> From (1)
> 
> (1A) no concurrency, so you cant configure more than one ceph-node at a
> time.
> (1A) You have to close one facade to start anouther, eg in ceph-deploy
> you have to close each connection before connecting to the next server
> so making it slow to use as all state has to be gathered.

concurrency has come up before in ceph-deploy.  It has been our explicit goal to make ceph-deploy as simple and *clear* as possible for users, with one of the main purposes to be extremely verbose and essentially *teach* a user how to deploy a Ceph cluster.  That’s why it prints everything it does by default, shows every remote command, and prints the output back in order.  Concurrency would muddy those waters, though we do all want things to go faster.

It is not necessarily the facade pattern that is the limitation there — it is the implementation within ceph-deploy.  We simply do a “for host is hostnames…” loop everywhere — it doesn’t matter what we are using underneath, we are doing one SSH connection at a time.

Furthermore, I don’t think it’s the facade paradigm that would limit you to one host at a time at all.  It’s one host module (facade) instantiated per host.  You could do many of these at once — I don’t see any reason why you couldn’t.  I’ve never tried it out, and I don’t know if python-remoto handles concurrency, but I don’t think the facade paradigm prevents it.

> 
> From (2)
> 
> (2A) No nesting of facades without code duplication. EG reuse of systemd
> support between distributions.
> (2B) inflexible / complex include paths for shared code between facade
> implementations.
> 

I disagree here.  There are plenty of places to put code that is common and does not have to be duplicated.  I definitely don’t want code in each host module that knows how to deal with systemd. The code that does have to be in each host module is how to detect/assign what init system is being used, and how to call into the appropriate (common) module to do what it needs.

As for complex include paths, I’m not sure I have much of an argument here other than to say that all frameworks have their conventions.  I don’t see what we are doing as anything different than what is enforced/recommended by many frameworks/scaffolds popular in Web app and MVC paradigms like RoR, Pyramid, Django, etc.  It’s just convention to learn.

> From (3)
> 
> (3A) Since all variables in a facade implementation are global, but
> isolated to an implementation, we cannot have variables in the
> implementation, any cross function variables that are not passed as
> parameters or return values will return will lead to side effects that
> are difficult to test.

I admit I don’t follow this one completely.

> 
> So this set of general points will have complex side effects that make
> you feel like an the pictured orange when developing that are related to
> wear the facade is implemented.
> 
> About this point you will say well its open source so fix it ?
> 
> My answer to this is that when I try to do this, as in this patch:
> 
> https://github.com/osynge/ceph-deploy/commit/b82f89f35b27814ed4aba1082efd134c24ecf21f
> 
> More than once, seem to suggest I should use the much more complex multi
> file implementation of a façade.
> 
> The only advantages of implementing façades in the "standard" ceph /
> ceph-deploy way that I can see is:
> 
> (I) how ceph-deploy has always done this way.
> (II) It allows you to continue using nearly no custom objects in python.
> (III) We like our oranges in funny shapes.

Again, I have more to say here in the actual PR, but do give me some time.  I can tell you it will be at least 1.5 hrs from now before I will get to go read it all again.
In short, yes I would like to continue to use what we have, but I think the classes you’ve created to handle systemd and sysv can fit into that very nicely.

> 
> It seems to me the current implementation could have been created due to
> the misunderstanding that modules are like objects, when intact they are
> like classes. Issues (1) and (3) can be solved simply by importing
> methods as objects rather than classes, but this does nothing to solve
> the bigger issue (2) which is more serious, but its a simple step
> forward that might very simple to patch, but their is little point in a
> POC unless people agree that issues (1) (2) and (3) are serious.
> 
> Please can we not spread this boat anchor* implementation of a facade,
> further around the code base, and ideally migrate away from this bad
> practice, and help us all feel like happy round oranges.
> 

John touched on this a bit already, about the real question here being about ultimate goals and intentions.  I’ll come back to this in another email, but I have a couple of other scheduled events to attend to first.  Again, do please give me the time to reply properly.

> Best regards
> 
> Owen
> 
> 
> boat anchor
> 
> https://en.wikipedia.org/wiki/Boat_anchor_%28metaphor%29

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pentagon Orange redefined in ceph-deploy

[Owen Synge]

Dear all,

Their are other details to be discussed, and hopefully lead to
agreement, but lets get to issue #1. The style issues still apply to
ceph and ceph-deploy.

From what you said, in my opinion the "boat anchor" in ceph-deploy is
redefined, as coupling of facade pattern, where all data is available,
to the ssh loop in a connection. This is probably the biggest single
architectural issue in ceph-deploy.

Travis Rhoden stated that the modules are imported as objects as they
are "instantiated", I should check this, this is very good news and
removes many objections to the outcome.

The discussion of point (3) is still worth continuing though in a
separate thread as it is still important enough to require discussion,
but it is of a style and good practice discussion rather than Boat
Anchor problem level.

Many other topics are unaffected.

On 07/09/2015 07:00 PM, Travis Rhoden wrote:
>> (1A) You have to close one facade to start anouther, eg in ceph-deploy
>> > you have to close each connection before connecting to the next server
>> > so making it slow to use as all state has to be gathered.
> concurrency has come up before in ceph-deploy.  It has been our explicit goal to make ceph-deploy as simple and *clear* as possible for users, with one of the main purposes to be extremely verbose and essentially *teach* a user how to deploy a Ceph cluster.  That’s why it prints everything it does by default, shows every remote command, and prints the output back in order.  Concurrency would muddy those waters, though we do all want things to go faster.
> 
> It is not necessarily the facade pattern that is the limitation there — it is the implementation within ceph-deploy.  We simply do a “for host is hostnames…” loop everywhere — it doesn’t matter what we are using underneath, we are doing one SSH connection at a time.

Best regards

Owen
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Pentagon Orange redefined in ceph-deploy

[Owen Synge]

Typo:

On 07/09/2015 09:37 PM, Owen Synge wrote:
> Dear all,
> 
> Their are other details to be discussed, and hopefully lead to
> agreement, but lets get to issue #1. The style issues still apply to
> ceph and ceph-deploy.
> 
> From what you said, in my opinion the "boat anchor" in ceph-deploy is
> redefined, as coupling of facade pattern, where all data is available,
> to the ssh loop in a connection. This is probably the biggest single
> architectural issue in ceph-deploy.
> 
> Travis Rhoden stated that the modules are imported as objects as they
> are "instantiated", I should check this, this is very good news and
> removes many objections to the outcome.
> 
> The discussion of point 

	2) façade requires code layout inflexibility.

> is still worth continuing though in a
> separate thread as it is still important enough to require discussion,
> but it is of a style and good practice discussion rather than Boat
> Anchor problem level.
> 
> Many other topics are unaffected.
> 
> On 07/09/2015 07:00 PM, Travis Rhoden wrote:
>>> (1A) You have to close one facade to start anouther, eg in ceph-deploy
>>>> you have to close each connection before connecting to the next server
>>>> so making it slow to use as all state has to be gathered.
>> concurrency has come up before in ceph-deploy.  It has been our explicit goal to make ceph-deploy as simple and *clear* as possible for users, with one of the main purposes to be extremely verbose and essentially *teach* a user how to deploy a Ceph cluster.  That’s why it prints everything it does by default, shows every remote command, and prints the output back in order.  Concurrency would muddy those waters, though we do all want things to go faster.
>>
>> It is not necessarily the facade pattern that is the limitation there — it is the implementation within ceph-deploy.  We simply do a “for host is hostnames…” loop everywhere — it doesn’t matter what we are using underneath, we are doing one SSH connection at a time.
> 
> Best regards
> 
> Owen
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB
21284 (AG
Nürnberg)

Maxfeldstraße 5

90409 Nürnberg

Germany
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Pentagon Orange redefined in ceph-deploy

[Travis Rhoden]

> On Jul 9, 2015, at 12:45 PM, Owen Synge <osynge@suse.com> wrote:
> 
> Typo:
> 
> On 07/09/2015 09:37 PM, Owen Synge wrote:
>> Dear all,
>> 
>> Their are other details to be discussed, and hopefully lead to
>> agreement, but lets get to issue #1. The style issues still apply to
>> ceph and ceph-deploy.
>> 
>> From what you said, in my opinion the "boat anchor" in ceph-deploy is
>> redefined, as coupling of facade pattern, where all data is available,
>> to the ssh loop in a connection. This is probably the biggest single
>> architectural issue in ceph-deploy.
>> 
>> Travis Rhoden stated that the modules are imported as objects as they
>> are "instantiated", I should check this, this is very good news and
>> removes many objections to the outcome.

I went back through my previous messages to make sure I knew what I said.  What I found was:

> Furthermore, I don’t think it’s the facade paradigm that would limit you to one host at a time at all.  It’s one host module (facade) instantiated per host.  You could do many of these at once — I don’t see any reason why you couldn’t.  I’ve never tried it out, and I don’t know if python-remoto handles concurrency, but I don’t think the facade paradigm prevents it.

As you say, it would be very good news if things were indeed instantiated, but I am afraid it is not good news and that I was wrong. As I’ve started to re-familiarize myself with this bit of the code base (it has been a while), I’m starting to better understand your points.  Each remote connection is indeed handled by a direct assignment of the needed module (be it CentOS, Debian, etc), and the module is used directly.  It is not an instance of a class.

This does have the drawbacks that you’ve mentioned — that you can only do one at a time, that variables would not be thread safe, etc.  This has not been an issue thusfar since adding concurrency to ceph-deploy hasn’t really been on the road map.  As far as being an “architectural issue”, it depends on where ceph-deploy is going.  When the current host module paradigm was put in place, it was a *vast* improvement of what was there before, but that does not mean it now always has to be that way.

I would be against making any major changes in the current 1.5.x series, but we could start to talk about an improved 1.6.x series.  There are other non-backwards compatible changes that I’ve been considering as well, in addition to wanting to remove some deprecated code.

>> 
>> The discussion of point 
> 
> 	2) façade requires code layout inflexibility.
> 
>> is still worth continuing though in a
>> separate thread as it is still important enough to require discussion,
>> but it is of a style and good practice discussion rather than Boat
>> Anchor problem level.
>> 
>> Many other topics are unaffected.

I think the biggest challenge that you and I may run into is an impedance mismatch on priority for getting architectural changes into ceph-deploy. It’s pretty low priority for me, job-wise, and would be something that would happen over the course of a few months, with a few changes here and there.  Getting bug fixes in, keeping pace with new features in upstream Ceph, and improving usability in the 1.5.x series does get much more attention from me since we have community users (and downstream products) using it every day.

>> 
>> On 07/09/2015 07:00 PM, Travis Rhoden wrote:
>>>> (1A) You have to close one facade to start anouther, eg in ceph-deploy
>>>>> you have to close each connection before connecting to the next server
>>>>> so making it slow to use as all state has to be gathered.
>>> concurrency has come up before in ceph-deploy.  It has been our explicit goal to make ceph-deploy as simple and *clear* as possible for users, with one of the main purposes to be extremely verbose and essentially *teach* a user how to deploy a Ceph cluster.  That’s why it prints everything it does by default, shows every remote command, and prints the output back in order.  Concurrency would muddy those waters, though we do all want things to go faster.
>>> 
>>> It is not necessarily the facade pattern that is the limitation there — it is the implementation within ceph-deploy.  We simply do a “for host is hostnames…” loop everywhere — it doesn’t matter what we are using underneath, we are doing one SSH connection at a time.
>> 
>> Best regards
>> 
>> Owen
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
> 
> -- 
> SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB
> 21284 (AG
> Nürnberg)
> 
> Maxfeldstraße 5
> 
> 90409 Nürnberg
> 
> Germany

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Pentagon Orange redefined in ceph-deploy

[Owen Synge]

Dear Travis,

I think we are building up a ground work upon where we can find much
agreement.

I think their is a lot of misunderstanding and we are beginning to
resolve which I believe is essential that we complete before I return to
up streaming work.

I want to leave time for people who strongly object to the compromises
and what looks like eventual agreement to have their time to speak.

More importantly to give a fair answer that wont potentially lead to
more misunderstanding I simply cant schedule time today.

(I am late with my back port (as I have already forward ported) of our
"ceph-deploy rgw" product suitable to go out the door. I want to make
one further change after the last mkdir fix, which is add a flag to
disable install and configuring apache for fastcgi and use just civetweb.)

When I get back to this thread I strongly suspect we can close at least
4 major subjects covered in this thread.

I would love to continue chat but its work time (releasing).

Hopefully then you can look at my solution and see it work for your self
on opensuse (its not working on rhel atm for reasons I explained before
as well as systemd being used for ceph in SUSE and not in redhat)

Best regards

Owen






On 07/10/2015 07:03 AM, Travis Rhoden wrote:
> 
>> On Jul 9, 2015, at 12:45 PM, Owen Synge <osynge@suse.com> wrote:
>>
>> Typo:
>>
>> On 07/09/2015 09:37 PM, Owen Synge wrote:
>>> Dear all,
>>>
>>> Their are other details to be discussed, and hopefully lead to
>>> agreement, but lets get to issue #1. The style issues still apply to
>>> ceph and ceph-deploy.
>>>
>>> From what you said, in my opinion the "boat anchor" in ceph-deploy is
>>> redefined, as coupling of facade pattern, where all data is available,
>>> to the ssh loop in a connection. This is probably the biggest single
>>> architectural issue in ceph-deploy.
>>>
>>> Travis Rhoden stated that the modules are imported as objects as they
>>> are "instantiated", I should check this, this is very good news and
>>> removes many objections to the outcome.
> 
> I went back through my previous messages to make sure I knew what I said.  What I found was:
> 
>> Furthermore, I don’t think it’s the facade paradigm that would limit you to one host at a time at all.  It’s one host module (facade) instantiated per host.  You could do many of these at once — I don’t see any reason why you couldn’t.  I’ve never tried it out, and I don’t know if python-remoto handles concurrency, but I don’t think the facade paradigm prevents it.
> 
> As you say, it would be very good news if things were indeed instantiated, but I am afraid it is not good news and that I was wrong. As I’ve started to re-familiarize myself with this bit of the code base (it has been a while), I’m starting to better understand your points.  Each remote connection is indeed handled by a direct assignment of the needed module (be it CentOS, Debian, etc), and the module is used directly.  It is not an instance of a class.
> 
> This does have the drawbacks that you’ve mentioned — that you can only do one at a time, that variables would not be thread safe, etc.  This has not been an issue thusfar since adding concurrency to ceph-deploy hasn’t really been on the road map.  As far as being an “architectural issue”, it depends on where ceph-deploy is going.  When the current host module paradigm was put in place, it was a *vast* improvement of what was there before, but that does not mean it now always has to be that way.
> 
> I would be against making any major changes in the current 1.5.x series, but we could start to talk about an improved 1.6.x series.  There are other non-backwards compatible changes that I’ve been considering as well, in addition to wanting to remove some deprecated code.
> 
>>>
>>> The discussion of point 
>>
>> 	2) façade requires code layout inflexibility.
>>
>>> is still worth continuing though in a
>>> separate thread as it is still important enough to require discussion,
>>> but it is of a style and good practice discussion rather than Boat
>>> Anchor problem level.
>>>
>>> Many other topics are unaffected.
> 
> I think the biggest challenge that you and I may run into is an impedance mismatch on priority for getting architectural changes into ceph-deploy. It’s pretty low priority for me, job-wise, and would be something that would happen over the course of a few months, with a few changes here and there.  Getting bug fixes in, keeping pace with new features in upstream Ceph, and improving usability in the 1.5.x series does get much more attention from me since we have community users (and downstream products) using it every day.
> 
>>>
>>> On 07/09/2015 07:00 PM, Travis Rhoden wrote:
>>>>> (1A) You have to close one facade to start anouther, eg in ceph-deploy
>>>>>> you have to close each connection before connecting to the next server
>>>>>> so making it slow to use as all state has to be gathered.
>>>> concurrency has come up before in ceph-deploy.  It has been our explicit goal to make ceph-deploy as simple and *clear* as possible for users, with one of the main purposes to be extremely verbose and essentially *teach* a user how to deploy a Ceph cluster.  That’s why it prints everything it does by default, shows every remote command, and prints the output back in order.  Concurrency would muddy those waters, though we do all want things to go faster.
>>>>
>>>> It is not necessarily the facade pattern that is the limitation there — it is the implementation within ceph-deploy.  We simply do a “for host is hostnames…” loop everywhere — it doesn’t matter what we are using underneath, we are doing one SSH connection at a time.
>>>
>>> Best regards
>>>
>>> Owen
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>> -- 
>> SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB
>> 21284 (AG
>> Nürnberg)
>>
>> Maxfeldstraße 5
>>
>> 90409 Nürnberg
>>
>> Germany
> 
> 

-- 
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB
21284 (AG
Nürnberg)

Maxfeldstraße 5

90409 Nürnberg

Germany
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Difference between convention and enforcement.

[Owen Synge]

Dear Travis,

We clearly disagree in this area.

I hope me explaining my perspective is not seen as unhelpful.

On 07/09/2015 07:00 PM, Travis Rhoden wrote:
>> (2B) inflexible / complex include paths for shared code between facade
>> > implementations.
>> > 
> I disagree here.  There are plenty of places to put code that is common and does not have to be duplicated.  I definitely don’t want code in each host module that knows how to deal with systemd. The code that does have to be in each host module is how to detect/assign what init system is being used, and how to call into the appropriate (common) module to do what it needs.
> 
> As for complex include paths, I’m not sure I have much of an argument here other than to say that all frameworks have their conventions.  I don’t see what we are doing as anything different than what is enforced/recommended by many frameworks/scaffolds popular in Web app and MVC paradigms like RoR, Pyramid, Django, etc.  It’s just convention to learn.

I don't understand why you chose to defend this. Please expand your
argument.

I will not go into the negative side effects of importing modules as
objects in the include paths, as a work around for potential side
effects of our current façade model, as this topic is more complex than
the points I do raise in this email, but I will expand them should these
arguments not be valid, as I would like to understand where we intend to
go, and how we minimise long term maintenance and maximise extensibility.

Here are the simpler reasons why I don't agree:

I hate the enforced code structures used my many "popular" frameworks
especially common in web frameworks but I accept that these enforce very
clear standard design patterns that are known to scale and be extensible
and you acknowledge this difference to the ceph-deploy code base simply
by calling them MVC. *

Having enforced relative paths between code files defined by the
implementation of the code is a great limitation.

While one can place code modules in the correct location if only one
thing defines the correct location, one can easily get into difficulties
2 obvious limitations in tolerating this are:

(1) by having two parts of the code base enforcing relative paths
between modules.

(2) by having relationships between modules become needlessly complex
because the enforced relationship says the code must go in a place that
is not correct to solve the problem. (this is analogous to the death of
hierarchical databases and their replacement with relational databases)

So since the issue is most serious if you have 2 components in the code
base that "enforce" module layout, and we only have one, we are only
preventing further components that "enforce" module layout.

Admittedly neither of these issues has _prevented_ development on
ceph-deploy so far, or led to duplication now you withdrew your comments
about all façades being the same.

Due to the façade defining module location we so far have, some things
have become confused in where the module can be placed that are clearly
OS specific.

Examples include:

	ceph_deploy/util/constants.py which is clearly includes constants that
are OS specific, such as package names.

	ceph_deploy/util/pkg_managers.py which clearly is a set of OS specific
implementations.

Both of these lead to unnecessary complications, when they could be
façades in their own right and should have the modules moved if we
continue to have this enforced module location in the façade implementation.

This inconsistency will not effect customers but does effect the clarity
of the code, and leads to more conditionals than we might have if we
took in my opinion a cleaner implementation to our model.

I will _not_ provide sample code of improvements, as this may lead to
people discussing irrelevant issues, until this point is resolved as my
mistake, or an area that is agreed can be improved.

Best regards

Owen


* It is one the many reasons I abandoned django in favour of Pyramid for
my personal projects, even though django only enforced separation of
components Model View and controller, pyramid to my recollection (its
been 3 years at least) only recommended a module relationship path  .
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Difference between convention and enforcement.

[Gregory Farnum]

Hey Owen,
I haven't followed any of the conversations you've had in ceph-deploy
land, but I've been trying to keep track of the ones on ceph-devel et
al. I can't comment on very much of it because I suck at Python — I
can write C in any language, and do so! ;)

I interject this comment because there are a lot of C developers in
the community who are doing their best in Python, and it has
historically been important (and, I think, still is?) that they be
able to contribute meaningful change to our python codebases. I think
that has biased a lot of the architecture decisions our python
developers made over the years and I'm not sure if everybody's on the
same page about it.

As an example, I can follow the include chains in ceph-deploy without
any problem and make appropriate changes to them. The in-memory
SQLAlchemy code you posted just left me scratching my head trying to
figure out how any of it worked. That doesn't make it *bad*, but
there's definitely a higher assumed knowledge base/learning curve
which we'd need to decide we want to adopt. In ceph-deploy
particularly that's a bit of a problem since it's (at least nominally,
although that mission has been slowly eroded) supposed to be a model
for people building their configuration management or other deployment
systems.

Anyway, I hope this outside observation is helpful to everybody; if
not please ignore. :)
-Greg

On Tue, Jul 14, 2015 at 11:41 AM, Owen Synge <osynge@suse.com> wrote:
> Dear Travis,
>
> We clearly disagree in this area.
>
> I hope me explaining my perspective is not seen as unhelpful.
>
> On 07/09/2015 07:00 PM, Travis Rhoden wrote:
>>> (2B) inflexible / complex include paths for shared code between facade
>>> > implementations.
>>> >
>> I disagree here.  There are plenty of places to put code that is common and does not have to be duplicated.  I definitely don’t want code in each host module that knows how to deal with systemd. The code that does have to be in each host module is how to detect/assign what init system is being used, and how to call into the appropriate (common) module to do what it needs.
>>
>> As for complex include paths, I’m not sure I have much of an argument here other than to say that all frameworks have their conventions.  I don’t see what we are doing as anything different than what is enforced/recommended by many frameworks/scaffolds popular in Web app and MVC paradigms like RoR, Pyramid, Django, etc.  It’s just convention to learn.
>
> I don't understand why you chose to defend this. Please expand your
> argument.
>
> I will not go into the negative side effects of importing modules as
> objects in the include paths, as a work around for potential side
> effects of our current façade model, as this topic is more complex than
> the points I do raise in this email, but I will expand them should these
> arguments not be valid, as I would like to understand where we intend to
> go, and how we minimise long term maintenance and maximise extensibility.
>
> Here are the simpler reasons why I don't agree:
>
> I hate the enforced code structures used my many "popular" frameworks
> especially common in web frameworks but I accept that these enforce very
> clear standard design patterns that are known to scale and be extensible
> and you acknowledge this difference to the ceph-deploy code base simply
> by calling them MVC. *
>
> Having enforced relative paths between code files defined by the
> implementation of the code is a great limitation.
>
> While one can place code modules in the correct location if only one
> thing defines the correct location, one can easily get into difficulties
> 2 obvious limitations in tolerating this are:
>
> (1) by having two parts of the code base enforcing relative paths
> between modules.
>
> (2) by having relationships between modules become needlessly complex
> because the enforced relationship says the code must go in a place that
> is not correct to solve the problem. (this is analogous to the death of
> hierarchical databases and their replacement with relational databases)
>
> So since the issue is most serious if you have 2 components in the code
> base that "enforce" module layout, and we only have one, we are only
> preventing further components that "enforce" module layout.
>
> Admittedly neither of these issues has _prevented_ development on
> ceph-deploy so far, or led to duplication now you withdrew your comments
> about all façades being the same.
>
> Due to the façade defining module location we so far have, some things
> have become confused in where the module can be placed that are clearly
> OS specific.
>
> Examples include:
>
>         ceph_deploy/util/constants.py which is clearly includes constants that
> are OS specific, such as package names.
>
>         ceph_deploy/util/pkg_managers.py which clearly is a set of OS specific
> implementations.
>
> Both of these lead to unnecessary complications, when they could be
> façades in their own right and should have the modules moved if we
> continue to have this enforced module location in the façade implementation.
>
> This inconsistency will not effect customers but does effect the clarity
> of the code, and leads to more conditionals than we might have if we
> took in my opinion a cleaner implementation to our model.
>
> I will _not_ provide sample code of improvements, as this may lead to
> people discussing irrelevant issues, until this point is resolved as my
> mistake, or an area that is agreed can be improved.
>
> Best regards
>
> Owen
>
>
> * It is one the many reasons I abandoned django in favour of Pyramid for
> my personal projects, even though django only enforced separation of
> components Model View and controller, pyramid to my recollection (its
> been 3 years at least) only recommended a module relationship path  .
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Difference between convention and enforcement.

[Owen Synge]

Hi Gregory,

On 07/14/2015 01:03 PM, Gregory Farnum wrote:
> Hey Owen,
> I haven't followed any of the conversations you've had in ceph-deploy
> land, but I've been trying to keep track of the ones on ceph-devel et
> al. I can't comment on very much of it because I suck at Python — I
> can write C in any language, and do so! ;)

/me giggles

Much much better than Fortran in any language.

On this subject:

I have to brush up my C++ myself and look forward to helping when I get
the opportunity and totally understand where you are coming from. I look
forward to getting back to C++, so when I get to this area I will need
lot of help from you guys who will probably pull your hair out and say I
am programming C++ like its 2005 even when it all comes back.

> I interject this comment because there are a lot of C developers in
> the community who are doing their best in Python, and it has
> historically been important (and, I think, still is?) that they be
> able to contribute meaningful change to our python codebases. I think
> that has biased a lot of the architecture decisions our python
> developers made over the years and I'm not sure if everybody's on the
> same page about it.

Of this I am sure.

> As an example, I can follow the include chains in ceph-deploy without
> any problem and make appropriate changes to them. The in-memory
> SQLAlchemy code you posted just left me scratching my head trying to
> figure out how any of it worked. That doesn't make it *bad*, but
> there's definitely a higher assumed knowledge base/learning curve
> which we'd need to decide we want to adopt. In ceph-deploy
> particularly that's a bit of a problem since it's (at least nominally,
> although that mission has been slowly eroded) supposed to be a model
> for people building their configuration management or other deployment
> systems.

For me this thread is about the façade pattern, which for me is a
separate discussion.

Both these discussions got intertwined, and I wish they could be unlinked.

I wish I had never even talked of or mentioned SQLAlchemy, as this is a
very tiny detail that is almost unimportant compared to the use of
design patterns and in that discussion MVC in particular. It is one of
many ways we could implement a model and my ideal of how _I_ would do
things, and am still happy if people are keen on MVC but not SQLAlchemy.

I hope this thread can be limited to the discussion of the façade, and
we move all MVC discussion to the MVC thread I started.

> Anyway, I hope this outside observation is helpful to everybody; if
> not please ignore. :)

It is helpful, I have been programming python for 12 years, and forget
that this has had an impact, and led me to not make issues cross
language enough.

Thanks

Owen

PS

Realising it is actually 12 years since some a collaborator at CERN gave
me python test scripts to maintain when they left CERN, for the C++
project I was working on, is really scary.


> -Greg
> 
> On Tue, Jul 14, 2015 at 11:41 AM, Owen Synge <osynge@suse.com> wrote:
>> Dear Travis,
>>
>> We clearly disagree in this area.
>>
>> I hope me explaining my perspective is not seen as unhelpful.
>>
>> On 07/09/2015 07:00 PM, Travis Rhoden wrote:
>>>> (2B) inflexible / complex include paths for shared code between facade
>>>>> implementations.
>>>>>
>>> I disagree here.  There are plenty of places to put code that is common and does not have to be duplicated.  I definitely don’t want code in each host module that knows how to deal with systemd. The code that does have to be in each host module is how to detect/assign what init system is being used, and how to call into the appropriate (common) module to do what it needs.
>>>
>>> As for complex include paths, I’m not sure I have much of an argument here other than to say that all frameworks have their conventions.  I don’t see what we are doing as anything different than what is enforced/recommended by many frameworks/scaffolds popular in Web app and MVC paradigms like RoR, Pyramid, Django, etc.  It’s just convention to learn.
>>
>> I don't understand why you chose to defend this. Please expand your
>> argument.
>>
>> I will not go into the negative side effects of importing modules as
>> objects in the include paths, as a work around for potential side
>> effects of our current façade model, as this topic is more complex than
>> the points I do raise in this email, but I will expand them should these
>> arguments not be valid, as I would like to understand where we intend to
>> go, and how we minimise long term maintenance and maximise extensibility.
>>
>> Here are the simpler reasons why I don't agree:
>>
>> I hate the enforced code structures used my many "popular" frameworks
>> especially common in web frameworks but I accept that these enforce very
>> clear standard design patterns that are known to scale and be extensible
>> and you acknowledge this difference to the ceph-deploy code base simply
>> by calling them MVC. *
>>
>> Having enforced relative paths between code files defined by the
>> implementation of the code is a great limitation.
>>
>> While one can place code modules in the correct location if only one
>> thing defines the correct location, one can easily get into difficulties
>> 2 obvious limitations in tolerating this are:
>>
>> (1) by having two parts of the code base enforcing relative paths
>> between modules.
>>
>> (2) by having relationships between modules become needlessly complex
>> because the enforced relationship says the code must go in a place that
>> is not correct to solve the problem. (this is analogous to the death of
>> hierarchical databases and their replacement with relational databases)
>>
>> So since the issue is most serious if you have 2 components in the code
>> base that "enforce" module layout, and we only have one, we are only
>> preventing further components that "enforce" module layout.
>>
>> Admittedly neither of these issues has _prevented_ development on
>> ceph-deploy so far, or led to duplication now you withdrew your comments
>> about all façades being the same.
>>
>> Due to the façade defining module location we so far have, some things
>> have become confused in where the module can be placed that are clearly
>> OS specific.
>>
>> Examples include:
>>
>>         ceph_deploy/util/constants.py which is clearly includes constants that
>> are OS specific, such as package names.
>>
>>         ceph_deploy/util/pkg_managers.py which clearly is a set of OS specific
>> implementations.
>>
>> Both of these lead to unnecessary complications, when they could be
>> façades in their own right and should have the modules moved if we
>> continue to have this enforced module location in the façade implementation.
>>
>> This inconsistency will not effect customers but does effect the clarity
>> of the code, and leads to more conditionals than we might have if we
>> took in my opinion a cleaner implementation to our model.
>>
>> I will _not_ provide sample code of improvements, as this may lead to
>> people discussing irrelevant issues, until this point is resolved as my
>> mistake, or an area that is agreed can be improved.
>>
>> Best regards
>>
>> Owen
>>
>>
>> * It is one the many reasons I abandoned django in favour of Pyramid for
>> my personal projects, even though django only enforced separation of
>> components Model View and controller, pyramid to my recollection (its
>> been 3 years at least) only recommended a module relationship path  .
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB
21284 (AG
Nürnberg)

Maxfeldstraße 5

90409 Nürnberg

Germany
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Difference between convention and enforcement.

[Travis Rhoden]

> On Jul 14, 2015, at 3:41 AM, Owen Synge <osynge@suse.com> wrote:
> 
> Dear Travis,
> 
> We clearly disagree in this area.
> 
> I hope me explaining my perspective is not seen as unhelpful.
> 
> On 07/09/2015 07:00 PM, Travis Rhoden wrote:
>>> (2B) inflexible / complex include paths for shared code between facade
>>>> implementations.
>>>> 
>> I disagree here.  There are plenty of places to put code that is common and does not have to be duplicated.  I definitely don’t want code in each host module that knows how to deal with systemd. The code that does have to be in each host module is how to detect/assign what init system is being used, and how to call into the appropriate (common) module to do what it needs.
>> 
>> As for complex include paths, I’m not sure I have much of an argument here other than to say that all frameworks have their conventions.  I don’t see what we are doing as anything different than what is enforced/recommended by many frameworks/scaffolds popular in Web app and MVC paradigms like RoR, Pyramid, Django, etc.  It’s just convention to learn.
> 
> I don't understand why you chose to defend this. Please expand your
> argument.

Truthfully I’m not sure how to, because I’m not sure I agree that code location is enforced at all.  I don’t think that it is.  There is nothing “enforcing” where code lives.  By that I mean that as long is once class/function/module is importable into another module, it works. There is a general style that has fallen out as development has happened over time, but like most projects there are bits and pieces scattered throughout in places that probably made logical sense at the time, but no longer is the best fit.

I say nothing is “enforcing” locations because it’s not like the code won’t work if the code is some place else.  As long you can import what you need, things should be good.  There used to be relative imports in ceph_deploy (e.g. “import ..utils; import .helpers”), but I believe all of that has been gone for a while.

For the most part it’s really up to the author of new code to put it where they think is best, and for the reviewers to agree or be indifferent about it.  :)  Along those lines, if code were to get moved in a PR with a description of “this makes a lot more sense”, that would be seen as a positive contribution.

Most of the “rigid” layout is in the hosts modules.  It makes sense that we have hosts/centos, hosts/debian, etc.  But then even inside to those top levels, they import things from all over the place.  One example I can think of is ceph_deploy.hosts.remotes, the contents of which magically (except, not magic) appear as distro.conn.remote_module.  This is using a capability of python-remoto, and is an area of code I’ve spent very little time with it.  I’m not sure if this is perhaps what you are concerned about.

> 
> I will not go into the negative side effects of importing modules as
> objects in the include paths, as a work around for potential side
> effects of our current façade model, as this topic is more complex than
> the points I do raise in this email, but I will expand them should these
> arguments not be valid, as I would like to understand where we intend to
> go, and how we minimise long term maintenance and maximise extensibility.
> 
> Here are the simpler reasons why I don't agree:
> 
> I hate the enforced code structures used my many "popular" frameworks
> especially common in web frameworks but I accept that these enforce very
> clear standard design patterns that are known to scale and be extensible
> and you acknowledge this difference to the ceph-deploy code base simply
> by calling them MVC. *
> 
> Having enforced relative paths between code files defined by the
> implementation of the code is a great limitation.
> 
> While one can place code modules in the correct location if only one
> thing defines the correct location, one can easily get into difficulties
> 2 obvious limitations in tolerating this are:
> 
> (1) by having two parts of the code base enforcing relative paths
> between modules.

I hope that I’ve made it clear that I don’t think anything is actually enforcing any relative paths between modules.  That doesn’t mean I’m not missing something, so do please point it out if I am.

> 
> (2) by having relationships between modules become needlessly complex
> because the enforced relationship says the code must go in a place that
> is not correct to solve the problem. (this is analogous to the death of
> hierarchical databases and their replacement with relational databases)
> 
> So since the issue is most serious if you have 2 components in the code
> base that "enforce" module layout, and we only have one, we are only
> preventing further components that "enforce" module layout.
> 
> Admittedly neither of these issues has _prevented_ development on
> ceph-deploy so far, or led to duplication now you withdrew your comments
> about all façades being the same.
> 
> Due to the façade defining module location we so far have, some things
> have become confused in where the module can be placed that are clearly
> OS specific.
> 
> Examples include:
> 
> 	ceph_deploy/util/constants.py which is clearly includes constants that
> are OS specific, such as package names.

I get your point, but really these could be anywhere.  For all the OS’s we’ve been supporting, most of this is *not* OS specific, really.  They are indeed constants across all Linux distros.  There’s been new additions to deal with downstream package splits, and there very well be better ways of handling that.  Those pieces are relatively new (just a few months), and I highly suspect that will be improved more over time.  I also fully accept that while perhaps these were simple/universal contstants in the past, the variety of packaging we support, distros, and init systems may not make that the case (systemd comes to mind).  However it is important to keep in mind that ceph-deploy is only intended to support installing Ceph from upstream Ceph release/testing packages, or downstream packages that follow the same locations about where to install things to, not homegrown or locally compiled binaries.  ceph-deploy is purposely a highly opinionated tool.

> 
> 	ceph_deploy/util/pkg_managers.py which clearly is a set of OS specific
> implementations.

Not so much OS specific, as packager specific (yum, apt, etc.)  This area is ripe for improvement, and is actually something I’ve started on already in order to support DNF on Fedora.  I have a branch I can point you at if interested.  But I want to make sure I truly understand your concern here.  If there were classes in ceph_deploy.util.pkg_managers, one for each type of of manager we use (dnf, yum, apt, zypper), and these were imported as needed by host modules in ceph_deploy.hosts.*, would you see that as a problem?  It doesn’t matter to me where the code is.  It’s more important to me that the module names are clear, so I don’t have to find package manager code in a file named “decorators” or something.

> 
> Both of these lead to unnecessary complications, when they could be
> façades in their own right and should have the modules moved if we
> continue to have this enforced module location in the façade implementation.

Totally agree that it can be improved.  ceph-deploy is growing organically and has had a lot of different developers on as you would expect from any project of this nature.

> 
> This inconsistency will not effect customers but does effect the clarity
> of the code, and leads to more conditionals than we might have if we
> took in my opinion a cleaner implementation to our model.
> 
> I will _not_ provide sample code of improvements, as this may lead to
> people discussing irrelevant issues, until this point is resolved as my
> mistake, or an area that is agreed can be improved.
> 
> Best regards
> 
> Owen
> 
> 
> * It is one the many reasons I abandoned django in favour of Pyramid for
> my personal projects, even though django only enforced separation of
> components Model View and controller, pyramid to my recollection (its
> been 3 years at least) only recommended a module relationship path  .

I rather enjoyed working with Pyramid as well, and it’s precursor pylons.--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html