Re: [openstack-dev] [Cinder][Glance] OSLO update

[Monty Taylor]

On 11/20/2013 07:04 AM, Roman Bogorodskiy wrote:
> I know it was brought up on the list a number of times, but...
> 
> If we're talking about storing commit ids for each module and writing
> some shell scripts for that, isn't it a chance to reconsider using git
> submodules?

No. They're too complex. We don't allow merge commits because they're
too easy to mess up. Even seasoned (and I mean SEASONED git devs) shy
away from submodules because the semantics are tricky.

We have 1600 devs - advanced git features lead us to death. (I say this
as the person who fields questions about basic git features)

> On Wed, Nov 20, 2013 at 12:37 PM, Elena Ezhova  > wrote:
> 
> 
> 20.11.2013, 06:18, "John Griffith"  >:
> 
> 
> On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin  > wrote:
> 
> >  On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote:
> >>  Random OSLO updates with no list of what changed, what got fixed etc
> >>  are unlikely to get review attention - doing such a review is
> >>  extremely difficult. I was -2ing them and asking for more info, but
> >>  they keep popping up. I'm really not sure what the best way of
> >>  updating from OSLO is, but this isn't it.
> >  Best practice is to include a list of changes being synced, for
> example:
> >
> >https://review.openstack.org/54660
> >
> >  Every so often, we throw around ideas for automating the
> generation of
> >  this changes list - e.g. cinder would have the oslo-incubator
> commit ID
> >  for each module stored in a file in git to tell us when it was last
> >  synced.
> >
> >  Mark.
> >
> >  _
> 
> __
> >  OpenStack-dev mailing list
> >  OpenStack-dev@lists.openstack.org
> 
> >  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
> Been away on vacation so I'm afraid I'm a bit late on this... but;
> 
> I think the point Duncan is bringing up here is that there are some
> VERY large and significant patches coming from OSLO pulls.  The DB
> patch in particular being over 1K lines of code to a critical
> portion
> of the code is a bit unnerving to try and do a review on.  I realize
> that there's a level of trust that goes with the work that's done in
> OSLO and synchronizing those changes across the projects, but I
> think
> a few key concerns here are:
> 
> 1. Doing huge pulls from OSLO like the DB patch here are nearly
> impossible to thoroughly review and test.  Over time we learn a lot
> about real usage scenarios and the database and tweak things as
> we go,
> so seeing a patch set like this show up is always a bit
> unnerving and
> frankly nobody is overly excited to review it.
> 
> 2. Given a certain level of *trust* for the work that folks do
> on the
> OSLO side in submitting these patches and new additions, I think
> some
> of the responsibility on the review of the code falls on the OSLO
> team.  That being said there is still the issue of how these changes
> will impact projects *other* than Nova which I think is sometimes
> neglected.  There have been a number of OSLO synchs pushed to Cinder
> that fail gating jobs, some get fixed, some get abandoned, but in
> either case it shows that there wasn't any testing done with
> projects
> other than Nova (PLEASE note, I'm not referring to this particular
> round of patches or calling any patch set out, just stating a
> historical fact).
> 
> 3. We need better documentation in commit messages explaining
> why the
> changes are necessary and what they do for us.  I'm sorry but in my
> opinion the answer "it's the latest in OSLO and Nova already has it"
> is not enough of an answer in my opinion.  The patches mentioned in
> this thread in my opinion met the minimum requirements because
> they at
> least reference the OSLO commit which is great.  In addition I'd
> like
> to see something to address any discovered issues or testing
> done with
> the specific projects these changes are being synced to.
> 
> I'm in no way saying I don't want Cinder to play nice with the
> common
> code or to get in line with the way other projects do things but
> I am
> saying that I think we have a ways to go in terms of better
> communication here and in terms of OSLO code actually keeping in
> mind
> the entire OpenStack eco-system as opposed to just changes that were
> needed/

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Doug Hellmann]

On Fri, Nov 22, 2013 at 11:21 AM, Duncan Thomas wrote:

> On 22 November 2013 12:27, Elena Ezhova  wrote:
> > But what if I want to update some module that consists of ten or even
> more
> > files (like rpc or db) and each of these files has quite a long change
> log?
> > In that case the commit message may turn out to be really long even if
> only
> > commit ids and names are included.
>
>
> A message that is too long is definitely a better problem to have than
> a message that is missing important details.
>

If we are talking about merging only part of oslo into a consuming project,
then we can't just keep track of the "last" revision that was merged,
because that won't necessarily include all of the changes. Elena, were you
planning to keep a separate revision for each entry under openstack/common
(not every file, just the files and directories at that level)?


>
> To turn the question round, how can a reviewer review a change that
> includes ten or even more files without any information on what
> changed and why? rpc and db are extremely difficult imports to review,
> and I've found problems in the last two I looked at.
>

Problems in the code, or in the way the code was merged?

Doug



>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Duncan Thomas]

On 22 November 2013 12:27, Elena Ezhova  wrote:
> But what if I want to update some module that consists of ten or even more
> files (like rpc or db) and each of these files has quite a long change log?
> In that case the commit message may turn out to be really long even if only
> commit ids and names are included.


A message that is too long is definitely a better problem to have than
a message that is missing important details.

To turn the question round, how can a reviewer review a change that
includes ten or even more files without any information on what
changed and why? rpc and db are extremely difficult imports to review,
and I've found problems in the last two I looked at.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Elena Ezhova]

But what if I want to update some module that consists of ten or even more
files (like rpc or db) and each of these files has quite a long change log?
In that case the commit message may turn out to be really long even if only
commit ids and names are included.


On Wed, Nov 20, 2013 at 12:37 PM, Elena Ezhova  wrote:

>
> 20.11.2013, 06:18, "John Griffith" :
>
> On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin 
> wrote:
>
> >  On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote:
> >>  Random OSLO updates with no list of what changed, what got fixed etc
> >>  are unlikely to get review attention - doing such a review is
> >>  extremely difficult. I was -2ing them and asking for more info, but
> >>  they keep popping up. I'm really not sure what the best way of
> >>  updating from OSLO is, but this isn't it.
> >  Best practice is to include a list of changes being synced, for example:
> >
> >https://review.openstack.org/54660
> >
> >  Every so often, we throw around ideas for automating the generation of
> >  this changes list - e.g. cinder would have the oslo-incubator commit ID
> >  for each module stored in a file in git to tell us when it was last
> >  synced.
> >
> >  Mark.
> >
> >  _
>>
>> __
>> >  OpenStack-dev mailing list
>> >  OpenStack-dev@lists.openstack.org
>> >  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>> Been away on vacation so I'm afraid I'm a bit late on this... but;
>>
>> I think the point Duncan is bringing up here is that there are some
>> VERY large and significant patches coming from OSLO pulls.  The DB
>> patch in particular being over 1K lines of code to a critical portion
>> of the code is a bit unnerving to try and do a review on.  I realize
>> that there's a level of trust that goes with the work that's done in
>> OSLO and synchronizing those changes across the projects, but I think
>> a few key concerns here are:
>>
>> 1. Doing huge pulls from OSLO like the DB patch here are nearly
>> impossible to thoroughly review and test.  Over time we learn a lot
>> about real usage scenarios and the database and tweak things as we go,
>> so seeing a patch set like this show up is always a bit unnerving and
>> frankly nobody is overly excited to review it.
>>
>> 2. Given a certain level of *trust* for the work that folks do on the
>> OSLO side in submitting these patches and new additions, I think some
>> of the responsibility on the review of the code falls on the OSLO
>> team.  That being said there is still the issue of how these changes
>> will impact projects *other* than Nova which I think is sometimes
>> neglected.  There have been a number of OSLO synchs pushed to Cinder
>> that fail gating jobs, some get fixed, some get abandoned, but in
>> either case it shows that there wasn't any testing done with projects
>> other than Nova (PLEASE note, I'm not referring to this particular
>> round of patches or calling any patch set out, just stating a
>> historical fact).
>>
>> 3. We need better documentation in commit messages explaining why the
>> changes are necessary and what they do for us.  I'm sorry but in my
>> opinion the answer "it's the latest in OSLO and Nova already has it"
>> is not enough of an answer in my opinion.  The patches mentioned in
>> this thread in my opinion met the minimum requirements because they at
>> least reference the OSLO commit which is great.  In addition I'd like
>> to see something to address any discovered issues or testing done with
>> the specific projects these changes are being synced to.
>>
>> I'm in no way saying I don't want Cinder to play nice with the common
>> code or to get in line with the way other projects do things but I am
>> saying that I think we have a ways to go in terms of better
>> communication here and in terms of OSLO code actually keeping in mind
>> the entire OpenStack eco-system as opposed to just changes that were
>> needed/updated in Nova.  Cinder in particular went through some pretty
>> massive DB re-factoring and changes during Havana and there was a lot
>> of really good work there but it didn't come without a cost and the
>> benefits were examined and weighed pretty heavily.  I also think that
>> some times the indirection introduced by adding some of the
>> openstack.common code is unnecessary and in some cases makes things
>> more difficult than they should be.
>>
>> I'm just not sure that we always do a very good ROI investigation or
>> risk assessment on changes, and that opinion applies to ALL changes in
>> OpenStack projects, not OSLO specific or anything else.
>>
>> All of that being said, a couple of those syncs on the list are
>> outdated.  We should start by doing a fresh pull for these and if
>> possible add some better documentation in the commit messages as to
>> the justification for the patches that would be great.  We can take a
>> closer look at the changes and the history behind them and try to get
>> some review progress m

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Flavio Percoco]

On 21/11/13 16:14 -0500, Doug Hellmann wrote:

   So I would really appreciate any comments or pieces of advice.


Is it sufficient to include just the short form of the original commit message,
along with the commit id in the oslo-incubator repository for reference?



I've done this and alse seen it being done by others. In most of the
cases, using the commit message title is enough. However, if the sync
is intended to fix a bug or introduces more relevant changes, it is
definitely useful to have that expressed in the commit message.


FF

--
@flaper87
Flavio Percoco


pgpLu2NRP0IXZ.pgp
Description: PGP signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Doug Hellmann]

On Wed, Nov 20, 2013 at 3:37 AM, Elena Ezhova  wrote:

>
> 20.11.2013, 06:18, "John Griffith" :
>
>
> On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin 
> wrote:
>
> >  On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote:
> >>  Random OSLO updates with no list of what changed, what got fixed etc
> >>  are unlikely to get review attention - doing such a review is
> >>  extremely difficult. I was -2ing them and asking for more info, but
> >>  they keep popping up. I'm really not sure what the best way of
> >>  updating from OSLO is, but this isn't it.
> >  Best practice is to include a list of changes being synced, for example:
> >
> >https://review.openstack.org/54660
> >
> >  Every so often, we throw around ideas for automating the generation of
> >  this changes list - e.g. cinder would have the oslo-incubator commit ID
> >  for each module stored in a file in git to tell us when it was last
> >  synced.
> >
> >  Mark.
> >
> >  _
>>
>> __
>> >  OpenStack-dev mailing list
>> >  OpenStack-dev@lists.openstack.org
>> >  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>> Been away on vacation so I'm afraid I'm a bit late on this... but;
>>
>> I think the point Duncan is bringing up here is that there are some
>> VERY large and significant patches coming from OSLO pulls.  The DB
>> patch in particular being over 1K lines of code to a critical portion
>> of the code is a bit unnerving to try and do a review on.  I realize
>> that there's a level of trust that goes with the work that's done in
>> OSLO and synchronizing those changes across the projects, but I think
>> a few key concerns here are:
>>
>> 1. Doing huge pulls from OSLO like the DB patch here are nearly
>> impossible to thoroughly review and test.  Over time we learn a lot
>> about real usage scenarios and the database and tweak things as we go,
>> so seeing a patch set like this show up is always a bit unnerving and
>> frankly nobody is overly excited to review it.
>>
>> 2. Given a certain level of *trust* for the work that folks do on the
>> OSLO side in submitting these patches and new additions, I think some
>> of the responsibility on the review of the code falls on the OSLO
>> team.  That being said there is still the issue of how these changes
>> will impact projects *other* than Nova which I think is sometimes
>> neglected.  There have been a number of OSLO synchs pushed to Cinder
>> that fail gating jobs, some get fixed, some get abandoned, but in
>> either case it shows that there wasn't any testing done with projects
>> other than Nova (PLEASE note, I'm not referring to this particular
>> round of patches or calling any patch set out, just stating a
>> historical fact).
>>
>> 3. We need better documentation in commit messages explaining why the
>> changes are necessary and what they do for us.  I'm sorry but in my
>> opinion the answer "it's the latest in OSLO and Nova already has it"
>> is not enough of an answer in my opinion.  The patches mentioned in
>> this thread in my opinion met the minimum requirements because they at
>> least reference the OSLO commit which is great.  In addition I'd like
>> to see something to address any discovered issues or testing done with
>> the specific projects these changes are being synced to.
>>
>> I'm in no way saying I don't want Cinder to play nice with the common
>> code or to get in line with the way other projects do things but I am
>> saying that I think we have a ways to go in terms of better
>> communication here and in terms of OSLO code actually keeping in mind
>> the entire OpenStack eco-system as opposed to just changes that were
>> needed/updated in Nova.  Cinder in particular went through some pretty
>> massive DB re-factoring and changes during Havana and there was a lot
>> of really good work there but it didn't come without a cost and the
>> benefits were examined and weighed pretty heavily.  I also think that
>> some times the indirection introduced by adding some of the
>> openstack.common code is unnecessary and in some cases makes things
>> more difficult than they should be.
>>
>> I'm just not sure that we always do a very good ROI investigation or
>> risk assessment on changes, and that opinion applies to ALL changes in
>> OpenStack projects, not OSLO specific or anything else.
>>
>> All of that being said, a couple of those syncs on the list are
>> outdated.  We should start by doing a fresh pull for these and if
>> possible add some better documentation in the commit messages as to
>> the justification for the patches that would be great.  We can take a
>> closer look at the changes and the history behind them and try to get
>> some review progress made here.  Mark mentioned some good ideas
>> regarding capturing commit ID's from synchronization pulls and I'd
>> like to look into that a bit as well.
>>
>> Thanks,
>> John
>>
>> ___
>> OpenStack-dev mailing list
>> O

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Doug Hellmann]

On Tue, Nov 19, 2013 at 9:12 PM, John Griffith
wrote:

> On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin 
> wrote:
> > On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote:
> >> Random OSLO updates with no list of what changed, what got fixed etc
> >> are unlikely to get review attention - doing such a review is
> >> extremely difficult. I was -2ing them and asking for more info, but
> >> they keep popping up. I'm really not sure what the best way of
> >> updating from OSLO is, but this isn't it.
> >
> > Best practice is to include a list of changes being synced, for example:
> >
> >   https://review.openstack.org/54660
> >
> > Every so often, we throw around ideas for automating the generation of
> > this changes list - e.g. cinder would have the oslo-incubator commit ID
> > for each module stored in a file in git to tell us when it was last
> > synced.
> >
> > Mark.
> >
> >
> > ___
> > OpenStack-dev mailing list
> > OpenStack-dev@lists.openstack.org
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> Been away on vacation so I'm afraid I'm a bit late on this... but;
>
> I think the point Duncan is bringing up here is that there are some
> VERY large and significant patches coming from OSLO pulls.  The DB
> patch in particular being over 1K lines of code to a critical portion
> of the code is a bit unnerving to try and do a review on.  I realize
> that there's a level of trust that goes with the work that's done in
> OSLO and synchronizing those changes across the projects, but I think
> a few key concerns here are:
>
> 1. Doing huge pulls from OSLO like the DB patch here are nearly
> impossible to thoroughly review and test.  Over time we learn a lot
> about real usage scenarios and the database and tweak things as we go,
> so seeing a patch set like this show up is always a bit unnerving and
> frankly nobody is overly excited to review it.
>
> 2. Given a certain level of *trust* for the work that folks do on the
> OSLO side in submitting these patches and new additions, I think some
> of the responsibility on the review of the code falls on the OSLO
> team.  That being said there is still the issue of how these changes
> will impact projects *other* than Nova which I think is sometimes
> neglected.  There have been a number of OSLO synchs pushed to Cinder
> that fail gating jobs, some get fixed, some get abandoned, but in
> either case it shows that there wasn't any testing done with projects
> other than Nova (PLEASE note, I'm not referring to this particular
> round of patches or calling any patch set out, just stating a
> historical fact).
>
> 3. We need better documentation in commit messages explaining why the
> changes are necessary and what they do for us.  I'm sorry but in my
> opinion the answer "it's the latest in OSLO and Nova already has it"
> is not enough of an answer in my opinion.  The patches mentioned in
> this thread in my opinion met the minimum requirements because they at
> least reference the OSLO commit which is great.  In addition I'd like
> to see something to address any discovered issues or testing done with
> the specific projects these changes are being synced to.
>
> I'm in no way saying I don't want Cinder to play nice with the common
> code or to get in line with the way other projects do things but I am
> saying that I think we have a ways to go in terms of better
> communication here and in terms of OSLO code actually keeping in mind
> the entire OpenStack eco-system as opposed to just changes that were
> needed/updated in Nova.  Cinder in particular went through some pretty
> massive DB re-factoring and changes during Havana and there was a lot
> of really good work there but it didn't come without a cost and the
> benefits were examined and weighed pretty heavily.  I also think that
> some times the indirection introduced by adding some of the
> openstack.common code is unnecessary and in some cases makes things
> more difficult than they should be.
>
> I'm just not sure that we always do a very good ROI investigation or
> risk assessment on changes, and that opinion applies to ALL changes in
> OpenStack projects, not OSLO specific or anything else.
>
> All of that being said, a couple of those syncs on the list are
> outdated.  We should start by doing a fresh pull for these and if
> possible add some better documentation in the commit messages as to
> the justification for the patches that would be great.  We can take a
> closer look at the changes and the history behind them and try to get
> some review progress made here.  Mark mentioned some good ideas
> regarding capturing commit ID's from synchronization pulls and I'd
> like to look into that a bit as well.
>

+1 to all of this. We'll work on improving the documentation in commit
messages.

At the same time, it would be nice to have some of the tweaks and
improvements you've made pushed back into Oslo to be shared. The db code in
particular is 

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Ben Nemec]

 

I don't recall the full discussion from before, but I know one of the
big problems with doing that is it actually makes it more difficult to
review these syncs. Instead of having 1000 lines of copied changes to
review, you have a one-line commit hash to look at and you then have to
try to figure out what changed since the previous hash, whether that's
one line or 1000. 

I think there were some other issues too, but I don't remember what they
were so maybe someone else can chime in. 

-Ben 

On 2013-11-20 06:04, Roman Bogorodskiy wrote: 

> I know it was brought up on the list a number of times, but...
> 
> If we're talking about storing commit ids for each module and writing
> some shell scripts for that, isn't it a chance to reconsider using git 
> submodules?
> 
> On Wed, Nov 20, 2013 at 12:37 PM, Elena Ezhova  wrote:
> 
> 20.11.2013, 06:18, "John Griffith" : 
> 
> On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin  wrote:
> 
>> On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote:
>>> Random OSLO updates with no list of what changed, what got fixed etc
>>> are unlikely to get review attention - doing such a review is
>>> extremely difficult. I was -2ing them and asking for more info, but
>>> they keep popping up. I'm really not sure what the best way of
>>> updating from OSLO is, but this isn't it.
>> Best practice is to include a list of changes being synced, for example:
>>
>> https://review.openstack.org/54660 [1]
>>
>> Every so often, we throw around ideas for automating the generation of
>> this changes list - e.g. cinder would have the oslo-incubator commit ID
>> for each module stored in a file in git to tell us when it was last
>> synced.
>>
>> Mark.
>>
>> _ __
>> OpenStack-dev mailing list
>> OpenStack-dev@lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev [2]
> 
> Been away on vacation so I'm afraid I'm a bit late on this... but;
> 
> I think the point Duncan is bringing up here is that there are some
> VERY large and significant patches coming from OSLO pulls. The DB
> patch in particular being over 1K lines of code to a critical portion
> of the code is a bit unnerving to try and do a review on. I realize
> that there's a level of trust that goes with the work that's done in
> OSLO and synchronizing those changes across the projects, but I think
> a few key concerns here are:
> 
> 1. Doing huge pulls from OSLO like the DB patch here are nearly
> impossible to thoroughly review and test. Over time we learn a lot
> about real usage scenarios and the database and tweak things as we go,
> so seeing a patch set like this show up is always a bit unnerving and
> frankly nobody is overly excited to review it.
> 
> 2. Given a certain level of *trust* for the work that folks do on the
> OSLO side in submitting these patches and new additions, I think some
> of the responsibility on the review of the code falls on the OSLO
> team. That being said there is still the issue of how these changes
> will impact projects *other* than Nova which I think is sometimes
> neglected. There have been a number of OSLO synchs pushed to Cinder
> that fail gating jobs, some get fixed, some get abandoned, but in
> either case it shows that there wasn't any testing done with projects
> other than Nova (PLEASE note, I'm not referring to this particular
> round of patches or calling any patch set out, just stating a
> historical fact).
> 
> 3. We need better documentation in commit messages explaining why the
> changes are necessary and what they do for us. I'm sorry but in my
> opinion the answer "it's the latest in OSLO and Nova already has it"
> is not enough of an answer in my opinion. The patches mentioned in
> this thread in my opinion met the minimum requirements because they at
> least reference the OSLO commit which is great. In addition I'd like
> to see something to address any discovered issues or testing done with
> the specific projects these changes are being synced to.
> 
> I'm in no way saying I don't want Cinder to play nice with the common
> code or to get in line with the way other projects do things but I am
> saying that I think we have a ways to go in terms of better
> communication here and in terms of OSLO code actually keeping in mind
> the entire OpenStack eco-system as opposed to just changes that were
> needed/updated in Nova. Cinder in particular went through some pretty
> massive DB re-factoring and changes during Havana and there was a lot
> of really good work there but it didn't come without a cost and the
> benefits were examined and weighed pretty heavily. I also think that
> some times the indirection introduced by adding some of the
> openstack.common code is unnecessary and in some cases makes things
> more difficult than they should be.
> 
> I'm just not sure that we always do a very good ROI investigation or
> risk assessment on changes, and that opinion applies to ALL changes in
> OpenStack project

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Roman Bogorodskiy]

I know it was brought up on the list a number of times, but...

If we're talking about storing commit ids for each module and writing
some shell scripts for that, isn't it a chance to reconsider using git
submodules?




On Wed, Nov 20, 2013 at 12:37 PM, Elena Ezhova  wrote:

>
> 20.11.2013, 06:18, "John Griffith" :
>
>
> On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin 
> wrote:
>
> >  On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote:
> >>  Random OSLO updates with no list of what changed, what got fixed etc
> >>  are unlikely to get review attention - doing such a review is
> >>  extremely difficult. I was -2ing them and asking for more info, but
> >>  they keep popping up. I'm really not sure what the best way of
> >>  updating from OSLO is, but this isn't it.
> >  Best practice is to include a list of changes being synced, for example:
> >
> >https://review.openstack.org/54660
> >
> >  Every so often, we throw around ideas for automating the generation of
> >  this changes list - e.g. cinder would have the oslo-incubator commit ID
> >  for each module stored in a file in git to tell us when it was last
> >  synced.
> >
> >  Mark.
> >
> >  _
>>
>> __
>> >  OpenStack-dev mailing list
>> >  OpenStack-dev@lists.openstack.org
>> >  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>> Been away on vacation so I'm afraid I'm a bit late on this... but;
>>
>> I think the point Duncan is bringing up here is that there are some
>> VERY large and significant patches coming from OSLO pulls.  The DB
>> patch in particular being over 1K lines of code to a critical portion
>> of the code is a bit unnerving to try and do a review on.  I realize
>> that there's a level of trust that goes with the work that's done in
>> OSLO and synchronizing those changes across the projects, but I think
>> a few key concerns here are:
>>
>> 1. Doing huge pulls from OSLO like the DB patch here are nearly
>> impossible to thoroughly review and test.  Over time we learn a lot
>> about real usage scenarios and the database and tweak things as we go,
>> so seeing a patch set like this show up is always a bit unnerving and
>> frankly nobody is overly excited to review it.
>>
>> 2. Given a certain level of *trust* for the work that folks do on the
>> OSLO side in submitting these patches and new additions, I think some
>> of the responsibility on the review of the code falls on the OSLO
>> team.  That being said there is still the issue of how these changes
>> will impact projects *other* than Nova which I think is sometimes
>> neglected.  There have been a number of OSLO synchs pushed to Cinder
>> that fail gating jobs, some get fixed, some get abandoned, but in
>> either case it shows that there wasn't any testing done with projects
>> other than Nova (PLEASE note, I'm not referring to this particular
>> round of patches or calling any patch set out, just stating a
>> historical fact).
>>
>> 3. We need better documentation in commit messages explaining why the
>> changes are necessary and what they do for us.  I'm sorry but in my
>> opinion the answer "it's the latest in OSLO and Nova already has it"
>> is not enough of an answer in my opinion.  The patches mentioned in
>> this thread in my opinion met the minimum requirements because they at
>> least reference the OSLO commit which is great.  In addition I'd like
>> to see something to address any discovered issues or testing done with
>> the specific projects these changes are being synced to.
>>
>> I'm in no way saying I don't want Cinder to play nice with the common
>> code or to get in line with the way other projects do things but I am
>> saying that I think we have a ways to go in terms of better
>> communication here and in terms of OSLO code actually keeping in mind
>> the entire OpenStack eco-system as opposed to just changes that were
>> needed/updated in Nova.  Cinder in particular went through some pretty
>> massive DB re-factoring and changes during Havana and there was a lot
>> of really good work there but it didn't come without a cost and the
>> benefits were examined and weighed pretty heavily.  I also think that
>> some times the indirection introduced by adding some of the
>> openstack.common code is unnecessary and in some cases makes things
>> more difficult than they should be.
>>
>> I'm just not sure that we always do a very good ROI investigation or
>> risk assessment on changes, and that opinion applies to ALL changes in
>> OpenStack projects, not OSLO specific or anything else.
>>
>> All of that being said, a couple of those syncs on the list are
>> outdated.  We should start by doing a fresh pull for these and if
>> possible add some better documentation in the commit messages as to
>> the justification for the patches that would be great.  We can take a
>> closer look at the changes and the history behind them and try to get
>> some review progress made here.  Mark mentioned some good ideas

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Elena Ezhova]

20.11.2013, 06:18, "John Griffith" :

On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin  wrote:

>  On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote:
>>  Random OSLO updates with no list of what changed, what got fixed etc
>>  are unlikely to get review attention - doing such a review is
>>  extremely difficult. I was -2ing them and asking for more info, but
>>  they keep popping up. I'm really not sure what the best way of
>>  updating from OSLO is, but this isn't it.
>  Best practice is to include a list of changes being synced, for example:
>
>https://review.openstack.org/54660
>
>  Every so often, we throw around ideas for automating the generation of
>  this changes list - e.g. cinder would have the oslo-incubator commit ID
>  for each module stored in a file in git to tell us when it was last
>  synced.
>
>  Mark.
>
>  _
>
> __
> >  OpenStack-dev mailing list
> >  OpenStack-dev@lists.openstack.org
> >  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
> Been away on vacation so I'm afraid I'm a bit late on this... but;
>
> I think the point Duncan is bringing up here is that there are some
> VERY large and significant patches coming from OSLO pulls.  The DB
> patch in particular being over 1K lines of code to a critical portion
> of the code is a bit unnerving to try and do a review on.  I realize
> that there's a level of trust that goes with the work that's done in
> OSLO and synchronizing those changes across the projects, but I think
> a few key concerns here are:
>
> 1. Doing huge pulls from OSLO like the DB patch here are nearly
> impossible to thoroughly review and test.  Over time we learn a lot
> about real usage scenarios and the database and tweak things as we go,
> so seeing a patch set like this show up is always a bit unnerving and
> frankly nobody is overly excited to review it.
>
> 2. Given a certain level of *trust* for the work that folks do on the
> OSLO side in submitting these patches and new additions, I think some
> of the responsibility on the review of the code falls on the OSLO
> team.  That being said there is still the issue of how these changes
> will impact projects *other* than Nova which I think is sometimes
> neglected.  There have been a number of OSLO synchs pushed to Cinder
> that fail gating jobs, some get fixed, some get abandoned, but in
> either case it shows that there wasn't any testing done with projects
> other than Nova (PLEASE note, I'm not referring to this particular
> round of patches or calling any patch set out, just stating a
> historical fact).
>
> 3. We need better documentation in commit messages explaining why the
> changes are necessary and what they do for us.  I'm sorry but in my
> opinion the answer "it's the latest in OSLO and Nova already has it"
> is not enough of an answer in my opinion.  The patches mentioned in
> this thread in my opinion met the minimum requirements because they at
> least reference the OSLO commit which is great.  In addition I'd like
> to see something to address any discovered issues or testing done with
> the specific projects these changes are being synced to.
>
> I'm in no way saying I don't want Cinder to play nice with the common
> code or to get in line with the way other projects do things but I am
> saying that I think we have a ways to go in terms of better
> communication here and in terms of OSLO code actually keeping in mind
> the entire OpenStack eco-system as opposed to just changes that were
> needed/updated in Nova.  Cinder in particular went through some pretty
> massive DB re-factoring and changes during Havana and there was a lot
> of really good work there but it didn't come without a cost and the
> benefits were examined and weighed pretty heavily.  I also think that
> some times the indirection introduced by adding some of the
> openstack.common code is unnecessary and in some cases makes things
> more difficult than they should be.
>
> I'm just not sure that we always do a very good ROI investigation or
> risk assessment on changes, and that opinion applies to ALL changes in
> OpenStack projects, not OSLO specific or anything else.
>
> All of that being said, a couple of those syncs on the list are
> outdated.  We should start by doing a fresh pull for these and if
> possible add some better documentation in the commit messages as to
> the justification for the patches that would be great.  We can take a
> closer look at the changes and the history behind them and try to get
> some review progress made here.  Mark mentioned some good ideas
> regarding capturing commit ID's from synchronization pulls and I'd
> like to look into that a bit as well.
>
> Thanks,
> John
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



I see now that updating OSLO is a far more complex issue than it may seem

Re: [openstack-dev] [Cinder][Glance] OSLO update

[John Griffith]

On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin  wrote:
> On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote:
>> Random OSLO updates with no list of what changed, what got fixed etc
>> are unlikely to get review attention - doing such a review is
>> extremely difficult. I was -2ing them and asking for more info, but
>> they keep popping up. I'm really not sure what the best way of
>> updating from OSLO is, but this isn't it.
>
> Best practice is to include a list of changes being synced, for example:
>
>   https://review.openstack.org/54660
>
> Every so often, we throw around ideas for automating the generation of
> this changes list - e.g. cinder would have the oslo-incubator commit ID
> for each module stored in a file in git to tell us when it was last
> synced.
>
> Mark.
>
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Been away on vacation so I'm afraid I'm a bit late on this... but;

I think the point Duncan is bringing up here is that there are some
VERY large and significant patches coming from OSLO pulls.  The DB
patch in particular being over 1K lines of code to a critical portion
of the code is a bit unnerving to try and do a review on.  I realize
that there's a level of trust that goes with the work that's done in
OSLO and synchronizing those changes across the projects, but I think
a few key concerns here are:

1. Doing huge pulls from OSLO like the DB patch here are nearly
impossible to thoroughly review and test.  Over time we learn a lot
about real usage scenarios and the database and tweak things as we go,
so seeing a patch set like this show up is always a bit unnerving and
frankly nobody is overly excited to review it.

2. Given a certain level of *trust* for the work that folks do on the
OSLO side in submitting these patches and new additions, I think some
of the responsibility on the review of the code falls on the OSLO
team.  That being said there is still the issue of how these changes
will impact projects *other* than Nova which I think is sometimes
neglected.  There have been a number of OSLO synchs pushed to Cinder
that fail gating jobs, some get fixed, some get abandoned, but in
either case it shows that there wasn't any testing done with projects
other than Nova (PLEASE note, I'm not referring to this particular
round of patches or calling any patch set out, just stating a
historical fact).

3. We need better documentation in commit messages explaining why the
changes are necessary and what they do for us.  I'm sorry but in my
opinion the answer "it's the latest in OSLO and Nova already has it"
is not enough of an answer in my opinion.  The patches mentioned in
this thread in my opinion met the minimum requirements because they at
least reference the OSLO commit which is great.  In addition I'd like
to see something to address any discovered issues or testing done with
the specific projects these changes are being synced to.

I'm in no way saying I don't want Cinder to play nice with the common
code or to get in line with the way other projects do things but I am
saying that I think we have a ways to go in terms of better
communication here and in terms of OSLO code actually keeping in mind
the entire OpenStack eco-system as opposed to just changes that were
needed/updated in Nova.  Cinder in particular went through some pretty
massive DB re-factoring and changes during Havana and there was a lot
of really good work there but it didn't come without a cost and the
benefits were examined and weighed pretty heavily.  I also think that
some times the indirection introduced by adding some of the
openstack.common code is unnecessary and in some cases makes things
more difficult than they should be.

I'm just not sure that we always do a very good ROI investigation or
risk assessment on changes, and that opinion applies to ALL changes in
OpenStack projects, not OSLO specific or anything else.

All of that being said, a couple of those syncs on the list are
outdated.  We should start by doing a fresh pull for these and if
possible add some better documentation in the commit messages as to
the justification for the patches that would be great.  We can take a
closer look at the changes and the history behind them and try to get
some review progress made here.  Mark mentioned some good ideas
regarding capturing commit ID's from synchronization pulls and I'd
like to look into that a bit as well.

Thanks,
John

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Mark McLoughlin]

On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote:
> Random OSLO updates with no list of what changed, what got fixed etc
> are unlikely to get review attention - doing such a review is
> extremely difficult. I was -2ing them and asking for more info, but
> they keep popping up. I'm really not sure what the best way of
> updating from OSLO is, but this isn't it.

Best practice is to include a list of changes being synced, for example:

  https://review.openstack.org/54660

Every so often, we throw around ideas for automating the generation of
this changes list - e.g. cinder would have the oslo-incubator commit ID
for each module stored in a file in git to tell us when it was last
synced.

Mark.


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Duncan Thomas]

Random OSLO updates with no list of what changed, what got fixed etc
are unlikely to get review attention - doing such a review is
extremely difficult. I was -2ing them and asking for more info, but
they keep popping up. I'm really not sure what the best way of
updating from OSLO is, but this isn't it.

As an example, the DB pull modifies 1134 lines of code. I see no
evidence that the submitter has gone through the ramifications of each
line of changed code .v. the rest of the cinder code base, which is
what a reviewer needs to do. Just because it changed in OSLO doesn't
necessarily mean it will drop straight into cinder.

On 14 November 2013 12:21, Elena Ezhova  wrote:
> Hello all,
>
> I have made several patches that update modules in cinder/openstack/common
> from oslo which have not been reviewed for more than a month already. My
> colleague has the same problem with her patches in Glance.
>
> Probably it's not a top priority issue, but if oslo is not updated
> periodically in small bits it may become a problem in the future. What's
> more, it is much easier for a developer if oslo code is consistent in all
> projects.
>
> So, I would be grateful if someone reviewed these patches:
> https://review.openstack.org/#/c/48272/
> https://review.openstack.org/#/c/48273/
> https://review.openstack.org/#/c/52099/
> https://review.openstack.org/#/c/52101/
> https://review.openstack.org/#/c/53114/
> https://review.openstack.org/#/c/47581/
>
> Thanks,
>
> Elena
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>



-- 
Duncan Thomas

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Joe Gordon]

This ML is not for review requests.

Please read
http://lists.openstack.org/pipermail/openstack-dev/2013-September/015264.html

best,
Joe

sent on the go
On Nov 14, 2013 4:26 AM, "Elena Ezhova"  wrote:

> Hello all,
>
> I have made several patches that update modules in cinder/openstack/common
> from oslo which have not been reviewed for more than a month already. My
> colleague has the same problem with her patches in Glance.
>
> Probably it's not a top priority issue, but if oslo is not updated
> periodically in small bits it may become a problem in the future. What's
> more, it is much easier for a developer if oslo code is consistent in all
> projects.
>
> So, I would be grateful if someone reviewed these patches:
> https://review.openstack.org/#/c/48272/
> https://review.openstack.org/#/c/48273/
> https://review.openstack.org/#/c/52099/
> https://review.openstack.org/#/c/52101/
> https://review.openstack.org/#/c/53114/
> https://review.openstack.org/#/c/47581/
>
> Thanks,
>
> Elena
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Cinder][Glance] OSLO update

[Julia Varlamova]

Hello!


 I have made the same work in Glance — updating glance/openstack from Oslo.
I'd appreciate your reviews and comments very much.


 https://review.openstack.org/#/c/46088/

https://review.openstack.org/#/c/46955/

https://review.openstack.org/#/c/48195/


 Also I've faced a strange problem while updating glance/common/policy.py
and glance/api/policy.py — Jenkins fails due to test

glance/tests/unit/test_cache_middleware.py


 but this fail cannot be reproduced. I've reported a bug on Launchpad:


 https://bugs.launchpad.net/glance/+bug/1247111


 I wonder if anybody knows a reason of this fail? Could you share your
assumptions with me?


 Thanks,


 Julia.
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

[openstack-dev] [Cinder][Glance] OSLO update

[Elena Ezhova]

Hello all,

I have made several patches that update modules in cinder/openstack/common
from oslo which have not been reviewed for more than a month already. My
colleague has the same problem with her patches in Glance.

Probably it's not a top priority issue, but if oslo is not updated
periodically in small bits it may become a problem in the future. What's
more, it is much easier for a developer if oslo code is consistent in all
projects.

So, I would be grateful if someone reviewed these patches:
https://review.openstack.org/#/c/48272/
https://review.openstack.org/#/c/48273/
https://review.openstack.org/#/c/52099/
https://review.openstack.org/#/c/52101/
https://review.openstack.org/#/c/53114/
https://review.openstack.org/#/c/47581/

Thanks,

Elena
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev