exception handling makes bad assumptions

[John Griffith]

The brick code does things like try/except putils.ProcessExecutionError all over the place. This is fine if the executor that's set up is in fact common/processutils, however if it's a wrapped version such as cinder/utils or nova/utils these exceptions are caught.

There are other places in the Cinder code where this can be an issue as well since we allow a set executor on the driver. The reality is that only time we really do this is for unit tests, so it may be worth considering not allowing the passing in of executors in the future and just hard code which one to use instead, in the case of unit tests this will create a bit of a mess to work through, we would need to replace passing in the fake-executor via init with a mock.

[Walt Boring (walter-boring)]

I can change the external brick lib to always use putils in the executor.

I'll look at doing this after the brick lib has been removed from cinder.

[Sean McGinnis (sean-mcginnis)]

Took a little time looking at this. Every consumer I looked at sets this to putils, so I think it is safe to change the init to not accept a different executor.

There are unit tests though that use the set_executor to a fake just for testing purposes. Removing that method would also introduce a backward incompatibility.

So I think maybe the safest thing to do here is update nova and cinder consumers to not pass in an executor on init, but leave set_executor for testing purposes. To help ensure nothing bad gets introduced, it would probably also be good to update the docstring for set_executor to include a warning that it is only there as a test hook, and add a LOG.warning statement in that method call with a similar type of warning.

That should hopefully ensure users only ever are using processutils executors for "real" usage.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Sean McGinnis (<email address hidden>) on branch: master
Review: https://review.openstack.org/206549
Reason: This review is > 4 weeks without comment, and failed Jenkins the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

[Patrick East (patrick-east)]

Looks like this is still a potential issue: os_brick/executor.py