Code review comment for lp:~mdragon/nova/xs-console

Revision history for this message
Sandy Walsh (sandy-walsh) wrote :

This is a just a source code review. I haven't tried to run the branch tests yet:

Overall, awesome work! Very impressive first commit!

nova/console/driver.py -> ConsoleProxy? Perhaps nova/console/proxy.py (since the class is truly a proxy and not a driver)

574 ... should there be debug code in here? Can't this be stubbed in the tests via stubout?

I think ConsolePool should be ConsoleProxyPool. It wasn't clear to me what a "console" was (since I always just viewed a console as an endpoint). A ConsoleProxy, however, makes more sense that it's a running entity.

I must be missing something, but I don't see where the ConsoleProxyManager is used? Other than via the tests. Yet, this is the only way to get a ConsoleProxy instantiated, afaik. Do you have any examples of the curl or python-cloudserver calls to add/remove a console?

I assume the proxies run on the console application server?

I have not reviewed the breadth of the tests yet. I'm not marking it Approved or Needs Fixing until you've had a chance to give feedback.

« Back to merge proposal