Bug 845466 - "wipe after delete" default differs in GUI and API
"wipe after delete" default differs in GUI and API
Status: CLOSED CURRENTRELEASE
Product: Red Hat Enterprise Virtualization Manager
Classification: Red Hat
Component: ovirt-engine-restapi (Show other bugs)
3.1.0
Unspecified Unspecified
unspecified Severity unspecified
: ---
: 3.1.0
Assigned To: Ravi Nori
Jakub Libosvar
storage
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-03 03:21 EDT by Michal Skrivanek
Modified: 2016-02-10 12:16 EST (History)
11 users (show)

See Also:
Fixed In Version: si22
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-12-04 15:02:12 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: Storage
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Michal Skrivanek 2012-08-03 03:21:24 EDT
"Wipe after delete" defaults to true in GUI, but false in REST API. They should be the same to avoid issues.
There are more instances of this problem, so please think about some way how to keep defaults in sync in all places (e.g. a bug 842344 for bootable disk default).
Comment 1 Itamar Heim 2012-08-03 07:23:31 EDT
wipe after delete in gui default to true based on type of storage iirc.
need to decide if default behavior should be the same by backend doing it, rather than client (problem is the need to expose it to the UI, which means UI needs to rely on backend logic to display the field)

I'm not sure API should not require user to be more specific on such details.
Comment 2 Oded Ramraz 2012-08-03 07:40:25 EDT
Maybe a good solution which can prevent mistakes such as this one , or bootable field is to set those fields to be mandatory . 
 

(In reply to comment #1)
> wipe after delete in gui default to true based on type of storage iirc.
> need to decide if default behavior should be the same by backend doing it,
> rather than client (problem is the need to expose it to the UI, which means
> UI needs to rely on backend logic to display the field)
> 
> I'm not sure API should not require user to be more specific on such details.
Comment 3 Simon Grinberg 2012-08-05 10:41:46 EDT
(In reply to comment #1)
> wipe after delete in gui default to true based on type of storage iirc.
> need to decide if default behavior should be the same by backend doing it,
> rather than client (problem is the need to expose it to the UI, which means
> UI needs to rely on backend logic to display the field)
> 
> I'm not sure API should not require user to be more specific on such details.

According to bug 845463 we think it's wrong thus blocking in the GUI.
If we think it's totally wrong then we should block at the BE as well.

Block can be - default is True, but ignore this for FS based file system 
               default is according to the storage type, and return error if trying to override for FS types.
Comment 4 Michael Pasternak 2012-08-05 10:57:59 EDT
(In reply to comment #3)
> (In reply to comment #1)
> > wipe after delete in gui default to true based on type of storage iirc.
> > need to decide if default behavior should be the same by backend doing it,
> > rather than client (problem is the need to expose it to the UI, which means
> > UI needs to rely on backend logic to display the field)
> > 
> > I'm not sure API should not require user to be more specific on such details.
> 
> According to bug 845463 we think it's wrong thus blocking in the GUI.
> If we think it's totally wrong then we should block at the BE as well.
> 
> Block can be - default is True, but ignore this for FS based file system 
>                default is according to the storage type, and return error if
> trying to override for FS types.

the question is not 'what', but 'where',
+1 for keeping defaults in the BE, UI can duplicate/reuse BE logic/rule for displaying purposes, other client should pass NULL and let BE set appropriate
value.
Comment 5 Ravi Nori 2012-10-15 12:12:24 EDT
The change in the BaseDisk class is changing the type of wipeAfterDelete flag ro refercne type Boolean and the addition of a new method

     public boolean isWipeAfterDeleteSet() {
         return wipeAfterDelete != null;
     }

and change in isWipeAfterDelete method

     public boolean isWipeAfterDelete() {
         if (isWipeAfterDeleteSet()) {
             return wipeAfterDelete;
         }
         return false;
     }

The backend handles the logic when wipeAfterDelete flag is not set.
So in AddDiskCommand executeVmCommand method we check if the flag is
set, otherwise we get the storage type and get the default value using the
new Utility class WipeAfterDeleteUtils

        if(!getParameters().getDiskInfo().isWipeAfterDeleteSet()) {
             StorageType storageType =
             getStorageDomain().getstorage_type();
getParameters().getDiskInfo().setWipeAfterDelete(WipeAfterDeleteUtils.getDefaultWipeAfterDeleteFlag(storageType));
         }

and the Utility class is a very simple class that extracts the
default value based on the storagetype

public class WipeAfterDeleteUtils {

     static Boolean wipeAfterDeleteBlockStorageDomain;

     static boolean wipeAfterDeleteFileStorageDomain = false;

     public static synchronized boolean
getDefaultWipeAfterDeleteFlag(final StorageType storageType) {
         if(storageType.isBlockDomain()) {
             if(wipeAfterDeleteBlockStorageDomain == null) {
                     wipeAfterDeleteBlockStorageDomain =
Config.<Boolean>GetValue(ConfigValues.SANWipeAfterDelete);
             }
             return wipeAfterDeleteBlockStorageDomain;
         }
         if(storageType.isFileDomain()) {
             return wipeAfterDeleteFileStorageDomain;
         }
         return false;
     }
}
Comment 6 Ravi Nori 2012-10-18 11:56:12 EDT
Link http://gerrit.ovirt.org/#/c/8628/

Commit hash : 4bc5a2565fb9d62daeeb6cbc15570f2995759898
Comment 7 Ravi Nori 2012-10-18 12:55:18 EDT
Correct Commit Hash : 5824c91db5d83540d941d220187bc7f7c32d015e
Comment 8 Haim 2012-10-22 13:01:03 EDT
what to test here (notes for QE)?
Comment 9 Ravi Nori 2012-10-22 14:15:36 EDT
For File Storage Domains

1. Create a VM on a File Storage Domain (NFS) 
2. Configure a Virtual Disk
   Wipe After Delete check box should be disabled
3. After the virtual disk has been created, the database table "base_disks" should have a new row with wipe_after_delete flag set to FALSE


For Block Storage Domains

1. Create a VM on a Block Storage Domain (SAN) 
2. Configure a Virtual Disk
   Wipe After Delete check box should be enabled and can be changed by the user
3. After the virtual disk has been created, the database table base_disks should have a new row with wipe_after_delete flag set to TRUE if the user checked the check box.
4. If the user did not check the wipe after delete box the value in the database should correspond to the value for 'SANWipeAfterDelete' flag in the database table "vdc_options"
Comment 10 Jakub Libosvar 2012-10-25 11:26:22 EDT
Verified rhevm-3.1.0-22.el6ev.noarch

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