[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. Thanks Ravi
----- Original Message -----
From: "Ravi Nori" <rnori@redhat.com> To: engine-devel@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
On 10/10/2012 01:17 AM, Daniel Erez wrote:
From: "Ravi Nori" <rnori@redhat.com> To: engine-devel@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
----- Original Message ----- 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@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)
----- Original Message -----
From: "Ravi Nori" <rnori@redhat.com> To: "Daniel Erez" <derez@redhat.com> Cc: engine-devel@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:
From: "Ravi Nori" <rnori@redhat.com> To: engine-devel@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
----- Original Message ----- 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@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)].
On 10/11/2012 05:28 PM, Daniel Erez wrote:
----- Original Message -----
From: "Ravi Nori" <rnori@redhat.com> To: "Daniel Erez" <derez@redhat.com> Cc: engine-devel@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:
From: "Ravi Nori" <rnori@redhat.com> To: engine-devel@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
----- Original Message ----- 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@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)
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@redhat.com> To: "Daniel Erez" <derez@redhat.com> Cc: engine-devel@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:
From: "Ravi Nori" <rnori@redhat.com> To: engine-devel@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
----- Original Message ----- 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@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; } }
Please send a WIP patch to gerrit for easier review. ----- Original Message -----
On 10/11/2012 05:28 PM, Daniel Erez wrote:
----- Original Message -----
From: "Ravi Nori" <rnori@redhat.com> To: "Daniel Erez" <derez@redhat.com> Cc: engine-devel@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:
From: "Ravi Nori" <rnori@redhat.com> To: engine-devel@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
----- Original Message ----- 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@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
On 10/11/2012 07:50 PM, Itamar Heim wrote: 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; } }
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
----- Original Message -----
From: "Ravi Nori" <rnori@redhat.com> To: "Itamar Heim" <iheim@redhat.com> Cc: "Daniel Erez" <derez@redhat.com>, engine-devel@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 05:28 PM, Daniel Erez wrote:
----- Original Message -----
From: "Ravi Nori" <rnori@redhat.com> To: "Daniel Erez" <derez@redhat.com> Cc: engine-devel@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:
From: "Ravi Nori" <rnori@redhat.com> To: engine-devel@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
----- Original Message ----- 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@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
On 10/11/2012 07:50 PM, Itamar Heim wrote: 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().
On 10/14/2012 09:14 AM, Daniel Erez wrote:
----- Original Message -----
From: "Ravi Nori" <rnori@redhat.com> To: "Itamar Heim" <iheim@redhat.com> Cc: "Daniel Erez" <derez@redhat.com>, engine-devel@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 05:28 PM, Daniel Erez wrote:
----- Original Message -----
From: "Ravi Nori" <rnori@redhat.com> To: "Daniel Erez" <derez@redhat.com> Cc: engine-devel@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@redhat.com> > To: engine-devel@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@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
On 10/11/2012 07:50 PM, Itamar Heim wrote: 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().
no, 1. BackendCapabilitiesResource not used to keep any logic, but for capabilities representation 2. api is stateless 3. defaults should be assigned in object .ctr, (user can override it in parameters .ctr or via setters) this way both api/UI will benefit from it.
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
-- Michael Pasternak RedHat, ENG-Virtualization R&D
On 10/11/2012 03:21 PM, Ravi Nori wrote:
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; } }
Ravi, Is there any reason for not doing all this ^ in .ctr? i.e iiuc wipeAfterDelete became an reference type, so you can see if it was not set even in parameters .ctr and apply your logic there. (this way setter/s won't have to carry any logic)
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; }
-- Michael Pasternak RedHat, ENG-Virtualization R&D
participants (5)
-
Ayal Baron -
Daniel Erez -
Itamar Heim -
Michael Pasternak -
Ravi Nori