[Engine-devel] "wipe after delete" Matching defaults from GUI to Backend

[Itamar Heim]

On 10/11/2012 05:28 PM, Daniel Erez wrote:
>
>
> ----- Original Message -----
>> From: "Ravi Nori" <rnori at redhat.com>
>> To: "Daniel Erez" <derez at redhat.com>
>> Cc: engine-devel at ovirt.org
>> Sent: Thursday, October 11, 2012 3:21:05 PM
>> Subject: Re: [Engine-devel] "wipe after delete" Matching defaults from GUI to	Backend
>>
>> On 10/10/2012 01:17 AM, Daniel Erez wrote:
>>>
>>> ----- Original Message -----
>>>> From: "Ravi Nori" <rnori at redhat.com>
>>>> To: engine-devel at ovirt.org
>>>> Sent: Tuesday, October 9, 2012 6:40:56 PM
>>>> Subject: [Engine-devel] "wipe after delete" Matching defaults from
>>>> GUI to	Backend
>>>>
>>>> I am working on the bug where the defaults values in the GUI for
>>>> disk
>>>> actions don't correspond to the default values in the API/Backend.
>>>> For
>>>> example the "wipe after delete" flag defaults to true in the GUI
>>>> but
>>>> false in the backend code. Default values are not sent from the
>>>> frontend
>>>> to the backend. Please refer to the bugzilla link
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=845466
>>>>
>>>> After discussing with Michael as to how to approach fixing this,
>>>> we
>>>> think the best way to do it would be to fix it in the Backend
>>>> class
>>>> businessentities.Disk by replacing primitives with reference types
>>>> and
>>>> forcing business logic in the class to set the default values to
>>>> match
>>>> the default values in the client.
>>>>
>>>> I would appreciate some feedback on this and also on what the
>>>> default
>>>> values are for Disk operations on the GUI.
>>> GUI behavior is as follows:
>>> For File storage domains - WipeAfterDelete check-box is
>>> unchangeable and unchecked (false).
>>> For Block storage domains - WipeAfterDelete check-box is changeable
>>> and
>>> the default value is according to 'SANWipeAfterDelete'
>>> configuration value.
>>>
>>> I think you should simply utilize the same configuration value in
>>> REST or backend when creating a new disk.
>>>
>>>> Thanks
>>>>
>>>> Ravi
>>>> _______________________________________________
>>>> Engine-devel mailing list
>>>> Engine-devel at ovirt.org
>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>>>>
>> Thanks for the reply.
>>
>> Here is my current implementation
>>
>> I modified businessentities.BaseDisk class to change wipeAfterDelete
>> to
>> wrapper class (reference class) for boolean
>>
>>       private Boolean wipeAfterDelete;
>>
>> I also added default values for File and block storages
>>       private boolean wipeAfterDeleteFileStorageDomain = false;
>>       private boolean wipeAfterDeleteBlockStorageDomain =
>> Config.<Boolean>GetValue(ConfigValues.SANWipeAfterDelete);
>>
>> The storage type determines which of the two default values is
>> returned
>> by BaseDisk when wipeAfterDelete is not set (is null)
>>       StorageType storageType = StorageType.UNKNOWN;
>>
>> So isWipeAfterDeleteMethodLooks like this
>>
>>       public boolean isWipeAfterDelete() {
>>           if (wipeAfterDelete == null) {
>>               return getDefaultIsWipeAfterDelete();
>>           }
>>           return wipeAfterDelete;
>>       }
>>
>>       private boolean getDefaultIsWipeAfterDelete() {
>>           if(storageType.equals(StorageType.UNKNOWN)) {
>>               return false;
>>           }
>>           switch (storageType) {
>>               case ISCSI:
>>               case FCP:
>>                   return wipeAfterDeleteBlockStorageDomain;
>>               default:
>>                   return wipeAfterDeleteFileStorageDomain;
>>           }
>>       }
>>
>> There is a new method isWipeAfterDeleteSet that will be used by
>> commands
>> to set the storage type to get the default wipeAfterDeleteFlag if
>> wipeAfterDelete was not set.
>>
>>       public boolean isWipeAfterDeleteSet() {
>>           return wipeAfterDelete != null;
>>       }
>>
>> Now in bll.AddDiskCommand I modified executeVmCommand to add the
>> following
>>
>>           if(!getParameters().getDiskInfo().isWipeAfterDeleteSet()) {
>>               StorageType storageType =
>> getStorageDomain().getstorage_type();
>> getParameters().getDiskInfo().setStorageType(storageType);
>>           }
>>
>> This approach minimizes the impact of changes. So all commands
>> interested in wipeAfterDelete flag based on storage type if
>> wipeAfterDelete has not been set need to set the storageType before
>> calling isWipeAfterDelete.
>>
>> Please let me know if this is the solution conflicts with something
>> you
>> have been working on. All the existing code will work as it has been
>> working, if wipeAfterDelete has not been set and storageType has not
>> been set the method isWipeAfterDelete return false (like it was
>> earlier)
>>
>
>
> Calling 'Config.<?> GetValue' in a business entity could be problematic in the UI since
> only the backend can invoke it (business entities are common for the backend and the UI
> which means that both may instantiate/manipulate them).
> We use GWT RPC for serializing/deserializing these business entities,
> thus we usually can't reference non plain java objects (or use some tweaks/tricks if applicable).
> Try to check the UI behavior after the new changes and let me know...
> [as an alternative, you can probably get 'SANWipeAfterDelete' config value from REST code (or backend)].

clients (UI/API) can only call the ConfigurationValues enum, and should 
use the query for it (gui has this already of course, just look it up)