[ovirt-devel] VM status checks - request for comments

[Oved Ourfali]

Hi

Nice initiative!
I reviewed some suspect infra-related flows, and added comments.
I must say that it is not very clear what you're looking for.
In one of the cases I reviewed, for example, there was a comment explaining the logic.
And, not all cases require a helper method, as some should be determined only in another helper class (like the case I reviewed, which eventually is SLA-related).

It seems like you want people to do is:
1. Go over conditions that are relevant to you, and make sure they are complete with regards to the possible VM statuses (do you really care about a comment explaining why is the logic that way?)
2. Write down a name of a method that might help your code look better, if any (if there isn't a clear one, like in the case above, then I guess it should either move to another helper that is related to the domain in question, or left as is with comparison).

Am I correct?

Also, I'd want to see these helper methods in the VM class rather than in some helper class.
myVM.isRunning() is more readable than than VMStatusHelper.isRunning(myVm), but that's just my opinion.

Thanks!
Oved

----- Original Message -----
> From: "Shmuel Melamud" <smelamud at redhat.com>
> To: devel at ovirt.org
> Sent: Tuesday, July 14, 2015 12:12:52 PM
> Subject: [ovirt-devel] VM status checks - request for comments
> 
> Hi!
> 
> I'm currently working on creation of new VMStatus helper functions that may
> be used to check VM status when needed being all-purpose, robust and
> universal.
> 
> For this purpose I've created a table of all places where VM status is
> checked, using current VMStatus functions, long lines of comparisons or some
> indirect way like validators. (Sure, I didn't include some cases. VmAnalyzer
> logic doesn't fit here. Mappings, rendering, message selection, changing
> states in migration process - it is not the case when helper functions may
> help.)
> 
> Here is the table:
> https://docs.google.com/a/redhat.com/spreadsheets/d/1yb-JdTAGh_4bOC6tL7KLz1Q2VR-UkF3eMSo1o_kU3MM/edit?usp=sharing
> 
> Many lines are raising questions - why these states are allowed and these are
> not? What to do in Unknown state, in migration, hibernation,
> WaitingForLaunch?
> 
> That's why I'm asking those of you who're familiar with some places to review
> them and write down your comments. What logic is behind the selection of the
> states? What states should be here, or shouldn't be here? This will give
> invaluable help to create helper functions that are really helpful ;)
> 
> --
> Shmuel
> _______________________________________________
> Devel mailing list
> Devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/devel
> 
> 
>