Bug 1117003 - [RFE] Add "(snapshot)" besides "Thin provision" type disks if snapshots are taken on these disks which were previously pre-allocated
Summary: [RFE] Add "(snapshot)" besides "Thin provision" type disks if snapshots are t...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Virtualization Manager
Classification: Red Hat
Component: RFEs
Version: unspecified
Hardware: All
OS: Linux
low
medium
Target Milestone: ovirt-3.6.0-rc
: 3.6.0
Assignee: Shmuel Melamud
QA Contact: Kevin Alon Goldblatt
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-07-07 19:24 UTC by Anand Nande
Modified: 2019-07-16 11:57 UTC (History)
15 users (show)

Fixed In Version:
Doc Type: Enhancement
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-03-09 20:48:35 UTC
oVirt Team: Storage
sherold: Triaged+


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHEA-2016:0376 normal SHIPPED_LIVE Red Hat Enterprise Virtualization Manager 3.6.0 2016-03-10 01:20:52 UTC
oVirt gerrit 41071 master MERGED webadmin: Tooltip in Disks subtab shows Original Allocation Policy Never

Comment 3 Allon Mureinik 2014-11-28 15:30:21 UTC
Tal, this can/should be handled after the disk refactoring

Comment 4 Maor 2015-01-19 09:47:29 UTC
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

Comment 5 Eldan Hildesheim 2015-03-31 09:40:07 UTC
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.

Comment 6 Tal Nisan 2015-04-01 09:59:21 UTC
Einav/Allon, does this make sense to you?

Comment 7 Einav Cohen 2015-04-01 15:55:58 UTC
(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.

Comment 8 Tal Nisan 2015-04-02 11:47:00 UTC
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

Comment 9 Einav Cohen 2015-04-02 15:01:49 UTC
(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).

Comment 12 Tal Nisan 2015-04-05 12:13:08 UTC
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?

Comment 13 Allon Mureinik 2015-04-08 11:08:21 UTC
(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.

Comment 14 Tal Nisan 2015-04-08 11:42:52 UTC
Ok, guess we're settled then, Einav?

Comment 15 Einav Cohen 2015-04-08 12:18:14 UTC
(In reply to Tal Nisan from comment #14)
> Ok, guess we're settled then, Einav?

sure.

Comment 18 Shmuel Melamud 2015-05-18 15:07:19 UTC
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.

Comment 19 Anand Nande 2015-05-18 21:08:27 UTC
(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)
-==-==-==-==-==-==-==-==-==-==-==-==-==-

Comment 20 Shmuel Melamud 2015-05-19 16:17:24 UTC
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.

Comment 21 Tal Nisan 2015-05-19 16:27:54 UTC
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

Comment 22 Yaniv Lavi 2015-05-20 07:48:04 UTC
I'm removing the pm ack until we discuss the performance implication.

Comment 23 Allon Mureinik 2015-06-15 08:11:41 UTC
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?

Comment 25 Max Kovgan 2015-06-28 14:12:11 UTC
ovirt-3.6.0-3 release

Comment 28 errata-xmlrpc 2016-03-09 20:48:35 UTC
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


Note You need to log in before you can comment on or make changes to this bug.