In order to solve this bug we'll need to periodically query for the snapshots when the grid of the VMs is displayed. That's different than what we're used to do - which is to query periodically only the data that is presented in the grid.

The data that is not presented in the grid is queried only when it is needed - so in this case, only when the user press the 'create snapshot' button (and if there's already snapshot creation operation in the background the user will receive an error).

In the snapshots sub-tab the snapshots, as the data that is presented in the grid, is queried periodically so we can disable the 'create' button if one of them is lock (which means snapshot is made in the background).

Einav, can I close this one as 'not a bug' since that's the way our UI works or do you think the snapshots should be periodically queried as well when the VMs grid is displayed?

without bigger ux changes I don't think we can solve this...

(In reply to Arik from comment #1)
> In order to solve this bug we'll need to periodically query for the
> snapshots when the grid of the VMs is displayed. That's different than what
> we're used to do - which is to query periodically only the data that is
> presented in the grid.

to be accurate: we will need to periodically query for the snapshots of the selected VM in the VMs main grid (probably via the UpdateActionAvailability() method in the VmListModel); in case there are no VMs selected or there are two or more selected VMs in the grid, or in case the VMStatus doesn't allow snapshot creation at the moment - we won't need to query anything, since the action won't be available by definition [1]

> 
> The data that is not presented in the grid is queried only when it is needed
> - so in this case, only when the user press the 'create snapshot' button
> (and if there's already snapshot creation operation in the background the
> user will receive an error).
> 
> In the snapshots sub-tab the snapshots, as the data that is presented in the
> grid, is queried periodically so we can disable the 'create' button if one
> of them is lock (which means snapshot is made in the background).
> 
> Einav, can I close this one as 'not a bug' since that's the way our UI works
> or do you think the snapshots should be periodically queried as well when
> the VMs grid is displayed?

Again: not constantly while the VMs grid is displayed, and not on all VMs in the grid; it should theoretically be done only when there is (exactly) one item selected in the grid and when the VMStatus allows snapshot creation at the moment, and only on the selected VM in the grid [1]

Having said that: AFAIK - we have never retrieved information from the server in any  UpdateActionAvailability() context. Not sure if it is such a good idea to do so, but it is theoretically possible.

a couple of notes:

- The "Create Snapshot" button in the main-tab seems a bit strange to me; I found out that it was added in the context of Bug 909930, but I am not sure that it is really imperative/correct to have it there, just like we don't have "Add NIC", "Add Disk", etc. as main-tab actions (although theoretically possible); in general, when we have a one-to-many relation between business entities, relevant actions are done via the relevant sub-tab. 

- I think it is important to keep the behavior consistent for the main-tab button and the sub-tab button (i.e. if the main-tab button is disabled, the sub-tab button should be disabled as well - their states should be constantly identical); it can be quite confusing for a user to select a VM, navigate to its Snapshots tab, and see the "Create Snapshot" button enabled in the main tab while the "Create Snapshot" button is disabled in the sub-tab.

These are the possible solutions in my view, ordered by my preference (maybe you can shoot an e-mail to engine-devel/users to poll for additional opinions):

(a) Remove the "Create Snapshot" button from the VMs main tab (keep it only in the Snapshots sub-tab), and disable the "Create Snapshot" button in the sub-tab according to the snapshots data that we are already retrieving periodically anyway.

(b) do nothing (i.e. button stays enabled in both main-tab and sub-tab, even if a snapshot is being created in the background).

(c) add a read-only Boolean field to the VM business entity / VM view in the data-base ("AreSnapshotsBeingCreatedNow") which will be 'true' if at least one snapshot is locked and 'false' otherwise, and disable the "Create Snapshot" button according to this new field's value (similar to (d) below, only the desired data is incorporated into the data that we are already retrieving periodically anyway, i.e. the VMs list).

(d) query snapshots status periodically when necessary for both VMs main tab (see [1]) and Snapshots sub-tab (which is done anyway), and disable the "Create Snapshot" buttons accordingly.

(e) like (a), without removing the "Create Snapshot" main-tab button (my least favorite, since will result in inconsistency between main-tab button state and sub-tab button state).

----

[1] in order to disable the "create snapshot button" while there are other snapshots for this VM being created in the background, the relevant section in the UpdateActionAvailability() method of the VmListModel would need to look something like:

...
getCreateSnapshotCommand().setIsExecutionAllowed(
   items.size() == 1  // ** condition 1 **
   && VdcActionUtils.CanExecute(...)  // ** condition 2 **
   && allSnapshotsAreNotLocked-CheckInServer(items[0]));
...

so practically, the call to the server to check the snapshots status ("allSnapshotsAreNotLocked-CheckInServer(items[0])") will be called only when "condition 1" and "condition 2" have passed (i.e. exactly one VM selected in the grid and current VMStatus is legal for snapshot creation), and only on a single VM (again, see "condition 1").

Adding a "Create Snapshot" button to VMs main-tab has been highly requested on Captain KVM blog [1]: "...Easier would be to make “snapshot” a menu option when you ‘right click’ a VM...".

I understand it appears inconsistent having this action in a main-tab, however, in case of creating a VM snapshot, I tend to agree it makes sense and useful (after all, it's a VM action just like Export/Migrate/etc).

Regarding graying-out the button, there's already a check for number of selected items (i.e. the action is enabled only when one VM is selected). There's also a verification that the VM's status is applicable for snapshot creation (e.g. VMStatus.ImageLocked/VMStatus.MigratingFrom/etc).

I prefer not to query the backend on UpdateActionAvailability, especially on a main-tab, as we refrain any queries other than retrieving the items (which is probably already too much - as it happens every 5 seconds by default...).

So, although we must rely on the business entities, I'm really not sure we want to add a flag such as "AreSnapshotsBeingCreatedNow"; it would indeed be an optimal solution to have it but I'm afraid the performance penalty might be too harsh.

In short, consequent upon the infrastructural limitations, I'm in favor of keeping the inconsistency for now - looks to me like the best of a bad lot.

[1]
http://captainkvm.com/2013/02/use-cases-for-both-rhev-snapshots-and-netapp-snapshot-copies/

(In reply to Daniel Erez from comment #4)
+1

Thanks for the detailed explanation, Derez, I trust your opinion on this.

From your feedback, I understand that we should probably rule out solutions (a), (c) and (d), so we are left with (b) [which means that main-tab button and sub-tab button will be consistent with each other, but won't be disabled in case there is a snapshot created in the background], or (e) [which means that only the sub-tab button behavior will be fixed, which will cause the main-tab button and sub-tab button behavior to be inconsistent with each other].

as I mentioned in comment #3 - my suggested solutions were ordered by my preference, so I am in favor of (b) over (e) (i.e. doing nothing and closing this bug, as Arik suggested in Comment #1).

[one last small note - in case we decide to close this bug, it shouldn't be closed on NOT-A-BUG, but on WONT-FIX or CANT-FIX]

going with (b)…