Code review comment for lp://qastaging/~justin-fathomdb/nova/restart-instance

Revision history for this message
termie (termie) wrote :

- seems like your flag should be DEFINE_boolean rather than integer and instead of persistence mode it would be compute_persist and applicable to both APIs. Alternatively if you really think it is going to mave more than two settings, use an enum so that we don't have magic numbers... but I think persist true/false is probably enough, we can either add another axis later on if there is more granulatiry or switch to an enum at that point
- where you pass persistence mode in compute.api it should default to None and use the flag value if it is None, rather than 0
- you can remove the note in the migrate file, that is how it works
- please format the docstring for init host correctly
- ditto for _init_instance, get_info and spawn
- lots of magic numbers in the PersistenceModel class
- for your todo around line 419 of the diff, please don't generate multiple levels of indentation, same with most of your comments that start with '#'
- for your note in _create_domain please make that into a docstring instead, it explains what is going on in the method more than is explaining an unexpected situation.
- why did you comment out the entire init_host at the end? seems like it is already explaining what it does and is commented out
- in vmutils you may as well remove the extra config option rather than make the note about it
- at like 527 you ask how to raise and keep the stack trace, assuming you mean the original error you can just do 'raise' instead of 'raise e' to reraise the original exception
- in the notes around 545 you don't really need the NOTE(justinsb) stuff, it is just explaining what is going on, not explaining an unexpected condition

I'm not sure I agree with how you're doing PersistenceMode as part of driver and then having it talk to the db, either it is an option on the model or it is a driver specific thing and it doesn't look like it is a driver-specific thing to determine whether persist is enabled. The part that is driver specific is how to deal with persisting something.

I know you prefer object-based data types but the standard at the moment is simple data types and your creation of PersistenceMode goes against that quite a bit and is generally a rather new pattern in the system as a whole, I'd rather simply have "persist" be a property on option on instance and that be the end of it, the rest is driver implementation of what to do based on what that value is equal to.

review: Needs Fixing

« Back to merge proposal