Tal, this can/should be handled after the disk refactoring

We currently have the information of the base allocation of disk under:
  VMs -> snapshots sub tab -> Mark a snapshot -> go to the snapshot disks sub tab

After talking with Tal, I understood that there can be 2 states after taking a snapshot:

Original state: Thin provisioning, after the snapshot is taken, the allocation is Thin provisioning.

or

Original state: Preallocated, after the snapshot is taken, the allocation is Thin provisioning.

Case num 2 is more interesting, what will a user understand from seeing 2 columns as:
Original allocation and Current allocation?
He will probably ask himself why did the allocation change?

I propose:
Change "Allocation policy" to "Original Allocation policy"
Add a column with Snapshot number after the allocation column.
We can even tooltip the allocation policy with an explanation: "Original policy: Preallocated. 3 Snapshot has been taken. Current policy: Thin provisioning".

I think that adding this column, will solve the bug and will add an important parameter that would be nice to have anyway.

Einav/Allon, does this make sense to you?

(In reply to Tal Nisan from comment #6)
> Einav/Allon, does this make sense to you?

to me, it sounds like it will make more sense for the user to see the current allocation policy of the Disk, rather than the original allocation policy. 
I.e., no matter if the Disk was originally created as 'thin provision' or if it became 'thin provision' after taking a snapshot from it - the disk is now thinly provisioned. 
I understand that the original allocation policy information is important, but it is certainly not more important than the current allocation policy; if anything, it is probably less important (Allon/Tal can correct me if I am wrong). 

another problem that I see is with the tool-tip idea: dicoveribility: I am not sure if anyone would guess that hovering on the '(original) allocation policy' value would display a tool-tip. 

my recommendation:

keeping the field as "Allocation Policy", display the *current* allocation policy (as it is today, to my understanding), and find a way to display the original allocation policy: 
It can be either with a tool-tip, as Eldan suggested, but need to design it so that the user will understand that a tool-tip is "hiding" there. Maybe add an icon next to the policy text, or mark the text with a dotted underline, etc. 
It can also be without a tool-tip, as suggested in the subject - just mention "(snapshot)" as part of the policy text or something similar - I am leaving the exact design decision to Eldan. I am more in favor of Eldan's tool-tip idea (we can use the tool-tip to explain that the allocation policy has changed due to the snapshot, for example) - again, just need to make sure that the user will understand that there is a tool-tip there. 

I agree with Eldan on having two columns (Original + Current) being a bad idea. 

Other than that - I like Eldan's idea on adding a column for number-of-snapshots per Disk, it sounds like valuable data, however I am not sure how easy it is to technically make it happen; if this requires a deeper change, which is not related to this BZ, I recommend tracking it in a separate RFE. 

Please 'needinfo' me again if you need anything else. 

Thanks.

Einav, the disk will always move to Thin Provisioning indeed after taking a snapshot, but the original policy still matter a lot, imagine you have a 500GB disk with around 50GB allocated and a snapshot on top of it, thinking that it's thinly provisioned will be wrong cause the base image still allocated 500GBs in some storages and it is a relevant info to the admin

(In reply to Tal Nisan from comment #8)
> Einav, the disk will always move to Thin Provisioning indeed after taking a
> snapshot, but the original policy still matter a lot, imagine you have a
> 500GB disk with around 50GB allocated and a snapshot on top of it, thinking
> that it's thinly provisioned will be wrong cause the base image still
> allocated 500GBs in some storages and it is a relevant info to the admin

thanks for the explanation. 

the terms "important"/"matter" are confusing here. 
I'll rephrase: what does the user expects to see by default, at the very first sight, when looking at a disk: does he expect to see the current allocation policy or does he expect to see the original allocation policy? which piece of information is more "basic"?

[a (bad) comparison, just for clarifying: let's take a VM Name and Status. Status is just as important as Name, if not more, but will you consider displaying VM data without clearly displaying the VM name? will you display the status text clearly, and then have the name in a tool-tip or something similar?]

if the more basic piece of information is the current allocation policy, my recommendation in Comment #7 stays as-is. 

[remember, the "original allocation policy" information is not hidden; when the user will see that the allocation policy is not a regular "thin provision" since it will have some visual indication (tool-tipped-icon/extra-text/... - leaving that to Eldan) that this is a "special" thin-provision]

if the more basic piece of information is the original allocation policy (doesn't seem like that to me, but you are the expert), take my recommendation from Comment #7 and switch between "allocation policy / current allocation policy" and "original allocation policy" (which is very close to Eldan's suggestion).

Einav, as long as the info is there I don't think it should be displayed "on first site", having it in a tooltip or such should be totally acceptable, Allon, do you agree?

(In reply to Tal Nisan from comment #12)
> Einav, as long as the info is there I don't think it should be displayed "on
> first site", having it in a tooltip or such should be totally acceptable,
> Allon, do you agree?
I'm OK with that, assuming the UXD experts agree.

Ok, guess we're settled then, Einav?

(In reply to Tal Nisan from comment #14)
> Ok, guess we're settled then, Einav?

sure.

I've implemented the tooltip for the Disks subtab in the Virtual Machines main tab.

But to add the tooltip to the list of disks in the Disks main tab I need information about snapshots. Currently this information is absent in the data that's fetched from the DB for this list. Fetching this information will require an additional query for every VM disk. This will have serious performance impact.

(In reply to Shmuel Melamud from comment #18)
> I've implemented the tooltip for the Disks subtab in the Virtual Machines
> main tab.
> 
> But to add the tooltip to the list of disks in the Disks main tab I need
> information about snapshots. Currently this information is absent in the
> data that's fetched from the DB for this list. Fetching this information
> will require an additional query for every VM disk. This will have serious
> performance impact.

shmuel - by serious performance impact - do you mean the *cost of the query*?

Say, if we get this feature in place - would the time be more than the following ? 

I ran the following a db_dump which had around 139 Vms and 126 snapshots:

-==-==-==-==-==-==-==-==-==-==-==-==-==-
engine=> \o /tmp/snaps
engine=> SELECT * from snapshots ;
# cat /tmp/snaps | grep RECORD | wc -l
126
engine=> EXPLAIN ANALYZE SELECT * from snapshots ;
                                               QUERY PLAN                                                
---------------------------------------------------------------------------------------------------------
 Seq Scan on snapshots  (cost=0.00..3.26 rows=126 width=642) (actual time=0.019..0.061 rows=126 loops=1)
 Total runtime: 0.184 ms
(2 rows)
-==-==-==-==-==-==-==-==-==-==-==-==-==-

Yes, the cost of reading the whole table is really low. But I don't think this is good approach - to read the whole table into Engine memory and make all the processing there. At least, such approach is not scalable.

Currently, the contents of the Disks list is taken from all_disks view that is all_disks_including_snapshots with all non-active snapshots filtered out. We can instead fetch all the snapshots and make the grouping on the Engine side. This approach also requires fetching a lot of data and a lot of effort on the Engine side.

We can create a view returning the original allocation policy for each disk and then join it with the query mentioned above. This approach looks the best for me, but need to evaluate performance of the resulting query.

Needless to say that running a separate query to get snapshots or just the policy for every VM disk is too much.

Need to decide what approach is better and what cost is justified for the feature.

The info is only relevant to VM disks anyway since floating disks or template disks cannot have snapshots and in that case you can see those disks in the relevant VM "disks" sub tab

I'm removing the pm ack until we discuss the performance implication.

Yaniv - this weekathon RFE was merged yesterday (see https://gerrit.ovirt.org/#/c/41071/), not sure why it wasn't set to MODIFIED.

Can you consider re-instating the pm-ack please?

ovirt-3.6.0-3 release

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHEA-2016-0376.html