OpenStack Compute (nova)

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

check-subprocess-exit-code
Merge into trunk

Revision history for this message

justinsb (justin-fathomdb) wrote on 2010-08-17:

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

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?) 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.

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

In other words, there was crummy stderr/exit code checking code (I think it was grabbed out of Twisted?)  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.

« Back to merge proposal