Merge lp:~justin-fathomdb/nova/check-subprocess-exit-code into lp:~hudson-openstack/nova/trunk

Any reason why you didn't make the change in process.py as well? A lot of the shell commands are called with process.simple_execute

In fact, I'm hoping we can get rid of runthis in favor of process.execute at least. After my refactors I think it is only used in a few places, perhaps utils.execute will disappear eventually as well

Didn't make the changes in process.py because I didn't realize that processes were called from there as well... I'll make the similar change to process.py

Now checking the exit code in the twisted code also.

Also incorporated passing the --fail flag to curl so that it sets the exit code. Seems to make sense here, in that we're now checking error codes, so we want curl to set them!

looks good. I requested a review from Termie, since he wrote the process.py code. Also, i noticed a few problems with nova-volume's use of simple_execute. I had to add an error_ok or two because some of the shell calls were returning output on stderr. We might need to run through creating a few volumes with my branch lp:~vishvananda/nova/fix-volumes and see if we need check_error_code=False on any of the commands. I only noticed this stuff when actually creating volumes (i.e. not using fakes).

Yes - it definitely needs an Twisted expert's eye on it. I certainly haven't run through every code path, so odds are the error checking will surface more issues and break things, but I think that at this stage of the project that's probably the right thing to do - we at least want to know when things are failing! I found some problems with the virtualenv stuff, for example, which otherwise were going unnoticed, and probably resulting in mixed versions of python libs etc...

* Please change all variable and parameter names to match Twisted's (camelCase)

* When you write a note about something (you have a comment in an exception below) please use

NOTE(justinsb): comment text

rather than

comment text (justinsb)

* Please make the lines fit in 80 characters.

* Your comment at the beginning of BackRelay has odd spacing.

* Technically the pip change (good catch) should be a different branch, it is fine now that it is already in there but please keep changes as small and specific as possible.

In general I don't think we should change the behavior of how "errortoo" works.

Specifically I don't think the majority of the changes in the twisted code are necessary, it looks like simply making a failure state in processEnded if checkExitCode is true and the exit code is non-zero is enough. I agree that we should also always store the stderr separately in addition to what twisted is doing so that we can give it back when the exit code is non-zero, but that should be an add-on to existing behavior, not a modification.

Agree that the pip comment should be in a different branch, as probably should the --fail argument to curl; just trying to minimize the overhead involved in patch submission.

On the changes to the twisted code, there were intended to fix the problems with checking stderr & exit code, that were documented in the code itself. It also felt cleaner to have one class than two, particularly as some methods in the base class appeared to be broken, but were never called as the derived class overrides those method. I'm happy to reapply my fixes against a branch if you want to submit something more along your lines of thinking.

The other comments seem to be about formatting. Can you please point me to the formatting rules we're using? Ideally (from my point of view) we could just all agree on a vim configuration (or some other editor) and post that for Python noobs such as myself.

Not sure I agree on use of camel case because Twisted does it, given that the rest of the code seems to use underscores, and Twisted might well be ripped out anyway. But this is the sort of thing that will be covered by the project formatting rules, so we don't need to debate it :-)

If someone can point me to the formatting rules for vim that we're using, I'll auto-format the code and resubmit.

Ping!

Fixes for greater compliance with pep8/pylint style checks.

In places where nova's code style was incompatible with Twisted's code style (camelCase), I used camelCase only where we had to for interactions with Twisted (e.g. method overrides, specifying arguments), and this style is confined to the process.py module and mostly to the BackRelayWithInput class.

Cleanup here is good enough for me. Be nice if termie approved as well. There have been a few additions/changes to commands since this was written, so it may be that this will need to be updated before merging (specifically I'm thinking about volume/service.py)

I still don't agree that these changes are necessary, can you point out a use case where the existing code is not functional? I suspect there is a way to solve your issues that don't involve modifying this code.

Sure - the primary case I was trying to fix when messing with the Twisted code (which is what I think you're questioning), is the one in this comment:

# NOTE(termie): current behavior means if error_ok is True
# we won't throw an error even if the process
# exited with a non-0 status, so you can't be
# okay with stderr output and not with bad exit
# codes.

I cleaned up the logic so that we can still check the exit code, whether or not the process outputs on stderr.

Any more feedback from termie on the last comment? We should merge this if it's still possible, or reject/remove from the merge queue if not.

weird, I got Eric's message but not the message from Justin. Sorry for the late reply, I expected to get an email about it.

Justin: That is a NOTE not a TODO, I don't see any use cases where this behavior is a problem, what I am asking you for is a use case that we have right now where we can't do what we want because of this behavior. I think the change you are proposing increases the complexity of the code significantly and would rather it not happen unless there is a good reason for it.

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.

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

Attempt to merge lp:~justin-fathomdb/nova/check-subprocess-exit-code into lp:nova failed due to merge conflicts:

text conflict in bin/nova-manage
text conflict in nova/utils.py
text conflict in nova/volume/service.py
text conflict in tools/install_venv.py

Merged with trunk; changed error_ok to terminate_on_stderr=False because this is what I believe is wanted (this affects a shell to vgcreate/lvcreate, so this patch would probably have helped with Bug 620027).

Also fixed a unit test that was calling release_ip it didn't get from issue_ip. Though the unit test was wrong, we weren't detecting the failure. As Ewan noticed in the most recent mailing list thread, the network unit tests are unusually tolerant of being FUBAR, this is probably why - they weren't checking exit codes!