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

[Ravi Nori]

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)