Bug 845466 - "wipe after delete" default differs in GUI and API
Summary: "wipe after delete" default differs in GUI and API
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat Enterprise Virtualization Manager
Classification: Red Hat
Component: ovirt-engine-restapi
Version: 3.1.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
: 3.1.0
Assignee: Ravi Nori
QA Contact: Jakub Libosvar
URL:
Whiteboard: storage
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-08-03 07:21 UTC by Michal Skrivanek
Modified: 2016-02-10 17:16 UTC (History)
11 users (show)

Fixed In Version: si22
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-12-04 20:02:12 UTC
oVirt Team: Storage
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)

Description Michal Skrivanek 2012-08-03 07:21:24 UTC
"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 11:23:31 UTC
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 11:40:25 UTC
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 14:41:46 UTC
(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 14:57:59 UTC
(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 16:12:24 UTC
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 15:56:12 UTC
Link http://gerrit.ovirt.org/#/c/8628/

Commit hash : 4bc5a2565fb9d62daeeb6cbc15570f2995759898

Comment 7 Ravi Nori 2012-10-18 16:55:18 UTC
Correct Commit Hash : 5824c91db5d83540d941d220187bc7f7c32d015e

Comment 8 Haim 2012-10-22 17:01:03 UTC
what to test here (notes for QE)?

Comment 9 Ravi Nori 2012-10-22 18:15:36 UTC
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 15:26:22 UTC
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.