Bug 1141774

Summary: CodeChange: Rename code references of the "Wipe After Delete" property
Product: [Retired] oVirt Reporter: Allon Mureinik <amureini>
Component: ovirt-engine-webadminAssignee: Idan Shaby <ishaby>
Status: CLOSED WONTFIX QA Contact: Aharon Canan <acanan>
Severity: low Docs Contact:
Priority: low    
Version: 3.5CC: acanan, amureini, bugs, derez, ecohen, gklein, iheim, ishaby, juan.hernandez, mgoldboi, ogofen, pstehlik, rbalakri, scohen, tnisan, yeylon
Target Milestone: m1Keywords: CodeChange
Target Release: 3.6.0   
Hardware: Unspecified   
OS: Linux   
Whiteboard: storage
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 1141770 Environment:
Last Closed: 2015-05-04 08:46:37 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: Storage RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1127781, 1141770    
Bug Blocks:    

Description Allon Mureinik 2014-09-15 12:29:13 UTC
+++ This bug was initially created as a clone of Bug #1141770 +++

+++ This bug was initially created as a clone of Bug #1127781 +++

+++ This bug was initially created as a clone of Bug #1122510 +++

Description of problem:
Modifying "wipe after delete" flag is available via rest-api while oVirt docs state this flag should be not available for file domain(UI greys this option as expected).

engine=# SELECT vm_names,storage_type,disk_id,storage_name,wipe_after_delete from images_storage_domain_view ;
 vm_names | storage_type |               disk_id         | storage_name | wipe_after_delete 
----------+--------------+--------------------------------------+--------------+-------------------
 vm_test  |            3 | e1a05b5b-5781-4bda-9cdf-c165359aa8a1 | ISCSI     | f
 vm_test  |            1 | 64feaf6f-1333-4517-9f75-a0038bb55cee | nfs       | t
 vm_test  |            3 | 8a422c1e-7cb4-4dba-b94e-44272c89bf40 | ISCSI     | f
 

Version-Release number of selected component (if applicable):
beta.1 && av10.3

How reproducible:
100%

Steps to Reproduce:
SomeNfsDisk.set_wipe_after_delete(True)
SomeNfsDisk.update()

Actual results:
operation successful

Expected results:
operation should be disabled

Additional info:

--- Additional comment from Juan Hernández on 2014-07-23 08:53:30 EDT ---

The RESTAPI doesn't contain logic to check the type of the the storage domain, it just passes the data given by the caller to the backend. If this should be ignored or cause an error then it should be done in the backend.

--- Additional comment from Allon Mureinik on 2014-07-23 10:18:19 EDT ---

(In reply to Ori from comment #0)
> Description of problem:
> Modifying "wipe after delete" flag is available via rest-api while oVirt
> docs state this flag should be not available for file domain(UI greys this
> option as expected).

Actually, I'm not quite sure this is the way we'd like to do. Since 3.4 we allow moving disks from block storage to file storage, so perhaps we'd like to keep this parameter as a "logical" parameter and only act upon in case the underlying storage makes sense (see bug 1097820).

However, I agree the two interfaces should be aligned - either both GUI and REST should allow it, or they should both block it.

Sean - your two cents?

--- Additional comment from Sean Cohen on 2014-07-24 06:31:44 EDT ---

(In reply to Allon Mureinik from comment #2)
> (In reply to Ori from comment #0)
> > Description of problem:
> > Modifying "wipe after delete" flag is available via rest-api while oVirt
> > docs state this flag should be not available for file domain(UI greys this
> > option as expected).
> 
> Actually, I'm not quite sure this is the way we'd like to do. Since 3.4 we
> allow moving disks from block storage to file storage, so perhaps we'd like
> to keep this parameter as a "logical" parameter and only act upon in case
> the underlying storage makes sense (see bug 1097820).
> 
IMHO if the disk is moved from a block domain the flag should be set by the user

> However, I agree the two interfaces should be aligned - either both GUI and
> REST should allow it, or they should both block it.
> 
> Sean - your two cents?

Indeed the two interfaces should be aligned, to keep it simple they should both block it.
Sean

--- Additional comment from Allon Mureinik on 2014-07-27 05:04:24 EDT ---

Sean, after giving the issue some more thought, I think the better approach would be to just allow setting it in the UI and change the field name to "treat securely" or something less asinine to the same effect. 

What happens when you move a disk with WAD=true from a block domain to a file domain?
- Blocking the operation would consist of bad UX
- Clearing the WAD flag would be inconsistent with other move operations, which do not modify other properties. Additionally, consider the flow of moving a disk block1>file->block. You'd end up with a disk with different properties although you theoretically moved it back to where it came from.
- Simply allowing it will have all the problems of the current situation, with the additional inconsistency of why can I get a WAD disk on a file domain by moving it but not straight-forward by creating it.

My suggestion:
- Allow it in the GUI
- Change the property to address the functionality and not the implementation (e.g., "secure deleting")
- Add a warning when moving from a file domain to a block domain.

Sean - ack/nack

--- Additional comment from Sean Cohen on 2014-07-29 01:41:01 EDT ---

(In reply to Allon Mureinik from comment #4)
> Sean, after giving the issue some more thought, I think the better approach
> would be to just allow setting it in the UI and change the field name to
> "treat securely" or something less asinine to the same effect. 
> 
> What happens when you move a disk with WAD=true from a block domain to a
> file domain?
> - Blocking the operation would consist of bad UX
> - Clearing the WAD flag would be inconsistent with other move operations,
> which do not modify other properties. Additionally, consider the flow of
> moving a disk block1>file->block. You'd end up with a disk with different
> properties although you theoretically moved it back to where it came from.

Indeed, this raises another security consistency consideration...

> - Simply allowing it will have all the problems of the current situation,
> with the additional inconsistency of why can I get a WAD disk on a file
> domain by moving it but not straight-forward by creating it.
> 
> My suggestion:
> - Allow it in the GUI
> - Change the property to address the functionality and not the
> implementation (e.g., "secure deleting")
> - Add a warning when moving from a file domain to a block domain.
+1 on the warning,
Ack on "secure deleting" property change.
Sean


In addition to enabling "wipe after delete" for file domains, we need to add a warning when moving a disk from a file domain to a block domain.
For now, these are the scenarios where we need that warning:
1. Moving a disk.
2. Copying a disk.
3. Template creation
4. Cloning a VM from a template.
5. Cloning a VM from a snapshot.
6. Importing a VM.
7. Importing a template.

--- Additional comment from Allon Mureinik on 2014-08-27 12:30:59 EDT ---

This requires quite a bit of grunt-work, pushing out to oVirt 3.5.1

--- Additional comment from Idan Shaby on 2014-09-15 08:02:37 EDT ---

After discussing about it with Allon, Tal and Daniel, we see three different solutions:

1. Add a warning message for each case.
2. Add a warning message for each case with an option to set the relevant disks' 'wipe after delete' value to true.
3. Don't add any warning message (i.e., CLOSE WONTFIX).

================================================================================
This bug is used to track the change of the "wipe after delete" property.
"wipe after delete" is no longer an appropriate name in oVirt 3.5.0 - it is not acted upon in file domains, and in 3.6.0 it may be even less appropriate if we provide the option for custom wiping solutions (see bug 872530).

One suggested term was "secure deletion", which, upon reflection, may be ambiguous, as this flag does not promise deletion (e.g., in case the storage fails), but only asserts that if the deletion succeeds it will be done in a secure fashion.
Sean/Liz/Julie - could you suggest a better term?
================================================================================
This bug is used to track changing the code to fit the new name given to the "wipe after delete" property in bug  1141770.

Comment 1 Allon Mureinik 2015-05-04 08:46:37 UTC
Closing old bugs, not important enough.
If someone thinks this is critical, feel free to reopen it.