Code review comment for lp:~justin-fathomdb/nova/check-subprocess-exit-code

On Wed, Aug 18, 2010 at 12:24 AM, justinsb <email address hidden> wrote:

> I don't believe a use-case should be needed for merging in a patch that
> adds correct error checking, but anyway...
>

Correct is what does what we need, without a use case it is just an opinion.

>
> >From nova/volume/service.py:
> # NOTE(vish): these commands sometimes sends output to stderr for
> warnings
> yield process.simple_execute("sudo vblade-persist auto all",
> error_ok=1)
> yield process.simple_execute("sudo vblade-persist start all",
> error_ok=1)

In other words, there was crummy stderr/exit code checking code (I think it
> was grabbed out of Twisted?)

Thanks this was what I was asking for. I'd also add that it is bad form to
call code 'crummy' but I sense that you are frustrated at the moment, so I
hope that if whoever wrote it sees this they won't take personal offense.

> Though this was known, there was a multi-line note instead of a (shorter)
> fix for it. As inevitably happens, the bug eventually caused problems - in
> this case, for vish in the storage service, but the only way to work around
> it was to mark stderr as being OK, or to fix the problem and risk a conflict
> with this merge-in-progress. Which means we now probably have to fix
> nova/volume/service.py as well (to state that stderr is OK but a non-zero
> exit code is not).
>
> When I see technical debt in code I'm working on, I clean it up.
>
> I believe the new code is much cleaner, so I don't know what you can point
> to that makes you believe it to be more complex. The old code used dynamic
> class modification (errReceivedIsBad, errReceivedIsGood), two classes where
> only the derived class was actually used, had some probable syntax errors
> flagged by pylint (but the methods were overridden in the derived class so
> were never actually called), and did a questionable dynamic import of
> twisted.internet.reactor if the reactor parameter to getProcessOutput was
> None. I don't believe there are any 'tricks' at all in the new code - it
> should be surprise-free. I think I've also reduced lines of code and have
> much greater pylint compliance.
>
> Finally, given that it seems unlikely that Twisted will survive for long in
> Nova, why are we nit-picking? Isn't proper error checking more important,
> so we can start getting nova working solidly and get more features working?
> I have a huge backlog of patches (not least a mostly working abstraction of
> the Redis data store and a SQL alternative implementation), but everything
> depends on this branch because - frankly - I find it really difficult to
> make progress when there's no error checking when spawning processes.
>

No need to get upset with me Justin. I apologize that I was slow on the
review, I haven't been receiving emails from any of your updates until this
one and Launchpad's workflow has yet to sink in with me.

In summary what I've understood from your comment:

1. We have a specific use case where an external tool is outputting on
stderr even though it has succeeded and therefore there is need to change
the behavior of this code.

2. You feel that the change is a general improvement in the quality of our
code despite severing most ties with the existing Twisted implementation.

Thanks to your explanations I am happy to approve the change. There is some
stylistically invalid spacing in there in a few places, but I think this
patch has sat in the queue long enough and that stuff can be cleaned up
later.

> --
>
> https://code.launchpad.net/~justin-fathomdb/nova/check-subprocess-exit-code/+merge/30707
> You are reviewing the proposed merge of
> lp:~justin-fathomdb/nova/check-subprocess-exit-code into lp:nova.
>