LGTM, thanks.
On Tue, Mar 4, 2014 at 10:04 AM, Michele Tartara <[email protected]>wrote: > On Tue, Mar 4, 2014 at 9:54 AM, Michele Tartara <[email protected]> > wrote: > > On Tue, Mar 4, 2014 at 9:12 AM, Thomas Thrainer <[email protected]> > wrote: > >> I think that's not needed. The return value of the method (lu_result) is > >> correctly updated if there are results from nodes. The case which > happened > >> in issue 742 was that an offline node was incorrectly contacted. > However, > >> the result of this attempt didn't make it in the return value of > >> HooksCallBack(...), because in line 3252 the result of offline nodes is > >> discarded. So while fixing issue 742, this instances of the bug should > go > >> away. > >> > >> However, the result of the node is also discarded if it gave an error > >> message (also checked in line 3252). This sounds fishy to me, as the > return > >> value should be False as well if the RPC call resulted in an error... So > >> maybe something like this would make more sense: > >> > >> if test: # test is defined in line 3249 > >> lu_result = False > >> continue > >> if res.offline: > >> continue > >> > >> What do you think? > >> > >> Cheers, > >> Thomas > > > > I think that both approaches can work. In either cases, when an error > > is detected, self._ErrorIf is invoked, and that function sets > > self.bad. So, using "not self.bad" should be enough. > > My favorite approach would be to not use lu_result at all in the > > function, and just use it in the return statement to propagate the > > previous Exec result, but your approach is ok as well, so I'll send > > and interdiff to implement it. > > Here it is: > diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py > index fa45eb6..f6e44e8 100644 > --- a/lib/cmdlib/cluster.py > +++ b/lib/cmdlib/cluster.py > @@ -3249,9 +3249,11 @@ class LUClusterVerifyGroup(LogicalUnit, > _VerifyErrors): > test = msg and not res.offline > self._ErrorIf(test, constants.CV_ENODEHOOKS, node_name, > "Communication failure in hooks execution: %s", msg) > - if res.offline or msg: > - # No need to investigate payload if node is offline or gave > - # an error. > + if test: > + lu_result = False > + continue > + if res.offline: > + # No need to investigate payload if node is offline > continue > for script, hkr, output in res.payload: > test = hkr == constants.HKR_FAIL > @@ -3262,7 +3264,7 @@ class LUClusterVerifyGroup(LogicalUnit, > _VerifyErrors): > feedback_fn("%s" % output) > lu_result = False > > - return lu_result and not self.bad > + return lu_result > > > class LUClusterVerifyDisks(NoHooksLU): > > > > Cheers, > Michele > -- > Google Germany GmbH > Dienerstr. 12 > 80331 München > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
