[Engine-devel] "wipe after delete" Matching defaults from GUI to Backend
Daniel Erez
derez at redhat.com
Sun Oct 14 07:14:24 UTC 2012
----- Original Message -----
> From: "Ravi Nori" <rnori at redhat.com>
> To: "Itamar Heim" <iheim at redhat.com>
> Cc: "Daniel Erez" <derez at redhat.com>, engine-devel at ovirt.org
> Sent: Friday, October 12, 2012 9:25:33 PM
> Subject: Re: [Engine-devel] "wipe after delete" Matching defaults from GUI to Backend
>
> On 10/11/2012 07:50 PM, Itamar Heim wrote:
> > 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)
> Yes it is going to be a problem with the UI, I will work on an
> alternative. Since this method will be called from both backend and
> frontend code, I think it is best not to have the logic to extract
> the
> default value in the BaseDisk class. I am leaning towards the
> following
> implementation, please let me know your thoughts. I also took a look
> at
> ConfigurationValues but I thinks the below works best
>
> The only change in the BaseDisk class is changing the reference type
> for
> wipeAfterDelete flag 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;
> }
> }
>
>
Wouldn't it be simpler to get SANWipeAfterDelete config value from REST (and keep the 'default' logic there..)?
E.g. BackendCapabilitiesResource.java -> localStorageEnabled().
More information about the Engine-devel
mailing list