----- Original Message ----- > From: "Ravi Nori" <[email protected]> > To: "Itamar Heim" <[email protected]> > Cc: "Daniel Erez" <[email protected]>, [email protected] > 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" <[email protected]> > >>> To: "Daniel Erez" <[email protected]> > >>> Cc: [email protected] > >>> 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" <[email protected]> > >>>>> To: [email protected] > >>>>> 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 > >>>>> [email protected] > >>>>> 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(). _______________________________________________ Engine-devel mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-devel
