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

[Michal Skrivanek]

On Jul 15, 2015, at 08:13 , Oved Ourfali <ovedo at redhat.com> wrote:

> 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?)

indeed the main point is to review that the condition is complete. By initial brief look it's apparent that the corner/non-ususal states are often not considered properly, and the helper functions in some cases are misleading.
There are several points of view of what does "is running"  mean. 
E.g. questions like do I really want to do AddVmToPoolCommand in VM state Unknown and MigratingTo? 

> 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.

+1

> 
> 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
>> 
>> 
>> 
> _______________________________________________
> Devel mailing list
> Devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/devel
> 
>