Code review comment for lp:~rconradharris/nova/server_progress

Revision history for this message
Rick Harris (rconradharris) wrote :

TL;DR: Agree that instance progress is a bad idea, we should merge this anyway and reevaluate our options at the Essex Design Summit.

This patch is catching a lot of heat, and for good reason. The current implementation, while simple, is not all that accurate; and creating accurate progress at scale may turn out to be either far too complex or outright impossible.

To those points, I'm in complete agreement and at the Essex design summit I will argue against including any kind of instance progress.

All that said, I still think we should merge this patch in the interim. The reasons are:

1. REQUIREMENT: We committed to supporting the Openstack API and progress is a required part of the spec. If we're going to abandon part of the spec for technical reasons, that's something we should hash out in a formal setting where we can have some good back-and-forth on this.

Until we decide to change course, though, I think we should continue our march towards 100% compatibility.

2. EASY-TO-REMOVE: When and if we decide to drop support for instance progress, removing the code should be a dead-simple; it's just a matter of removing the update_progress method and its callers, dropping the column, and having the OSAPI return to its original behavior of stubbing out progress to be 0 or 100% depending on status.

3. INCLUDES OTHER USEFUL FIXES/REFACTORINGS: This patch includes additional bug-fixes and code cleanups which would be nice to have merged in. They could be extracted into a separate patch (in hindsight that would have been the right approach), but since point 1) is (to me) a hard-requirement, the effort of separating out these fixes is probably not worth it.

« Back to merge proposal