Code review comment for lp:~rackspace-titan/nova/instance_states

Revision history for this message
Brian Lamar (blamar) wrote :

> > > This is absolutely incredible. You've done a great job, here. One general
> > > comment before the line-by-line stuff:
> > >
> > > Can we align the states with respect to tense? I don't think all of our
> > states
> > > need to end in ING or ED. What do you think?
> >
> > The original design had all vm_states and task_states in the same tenses. No
> > INGs or EDs.
> >
> > task_states.SCHEDULE just didn't have the same effect on me as
> > task_states.SCHEDULING but really it's not a huge difference in my mind.
> We're
> > just a few seds away from changing everything so it's not difficult. Some of
> > them get more confusing IMO without ING or ED:
> >
> > NETWORK vs NETWORKING
> > PAUSE vs PAUSED
> > STOP vs STOPPED
> >
> > Would you recommend unification of tenses or just removal of all
> ING/ED/tense?
>
> Actually, I think it makes a lot more sense now. Tasks make sense to end in
> -ING and vm_states in -ED (or no suffix at all). There are a few specific
> states I want to point out:
>
> task_states.SPAWN -> task_states.SPAWNING

Updated.

>
> vm_states.VERIFY_RESIZE just seems weird. How does RESIZE_VERIFICATION sound,
> or WAITING?

I actually updated that state to be a task, because it's not really a VM state. The VM is active, but the task is "waiting for input to see if I should revert or not". It's now task_states.RESIZE_VERIFY

>
> Can task_states.UNPAUSING go away in favor of RESUMING? This is more of a
> question than a suggestion, I can see why we might want to leave it.

Technically unpausing is the opposite of pausing and resuming is the opposite of suspending. It's a little silly I admit, but they have subtle differences so I'd like to keep them if that's not a deal-breaker.

>
> What about these for images:
> task_states.SNAPSHOTTING -> IMAGE_SNAPSHOT
> task_states.BACKING_UP -> IMAGE_BACKUP

Good suggestion. Updated.

>
> task_states.HARD_REBOOTING should go away until we support it. If you want to
> leave it, can you rename it to REBOOTING_HARD?

I've removed it, it's not supported at all and it seems silly to have in there.

>
> task_states.PASSWORD -> UPDATING_PASSWORD

Updated.

>
> task_states.STARTING -> BOOTING
> 'starting' can easily mean a million different things

I very much like the task pairs. STOPPING and STARTING correspond well and when thought about in a compute/instance sense I'm not sure the word is quite so ambiguous?

>
> I don't see any 'shutdown' states.

STOPPED means the VM is stopped/shutoff. STOPPING mean the VM is in the process of being shut down. I'm open to suggestions on making this clearer.

« Back to merge proposal