Re: [Engine-devel] FeatureSupported and compatibility versions
On 03/18/2013 01:30 PM, Omer Frenkel wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: engine-devel@ovirt.org Sent: Monday, March 18, 2013 8:28:14 AM Subject: [Engine-devel] FeatureSupported and compatibility versions Hi all, The current mechanism in oVirt to check whether a feature is supported in a particular compatibility version is to use the FeatureSupported class. e.g. FeatureSupported.networkLinking(getVm().getVdsGroupCompatibilityVersion()) Checks whether the network linking feature is supported for the the VM's cluster compatibility version. This internally checks whether the value of the corresponding config (NetworkLinkingSupported) for the given compatibility version is true/false. I'm not sure if this is a good idea, since a feature is typically supported from a particular version. E.g. Gluster support was introduced in 3.1, and it continues to be available in all subsequent versions. So I see no point in adding configuration for every version indicating whether the feature is supported in that version or not. I suggest to use either of the following options: 1) Instead of using a boolean config for each version, use a single string config that indicates the supported from version e.g. GlusterSupportedFrom = 3.1. There could be rare cases where a feature, for some reason, is removed in some release. In such cases, we could use one additional config for the supported to version. 2) Continue with the boolean approach, but do not have entries for every version; rather make use of the default value for majority of cases, and add the explicit version mapping for the minority e.g. GlusterSupported = true by default, and false in case of 3.0 (only one config required for 3.0) i like this approach better, if one add new feature for 3.3 he should add it as 'true' in the config to be used as default, and explicitly add it as false for other unsupported versions. For the record, this was the approach that was finally approved on the patch and merged (http://gerrit.ovirt.org/12970) So I think now onwards, everyone can start using this approach for new features being added. if we do go this way, we need to make sure it works because i vaguely remember we had a bug in configuration, making us explicitly specify value for all versions. I wrote a test case on DBConfigUtils that verifies that it does indeed work fine (http://gerrit.ovirt.org/13787), which has also been approved and merged. Thoughts? Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] FeatureSupported and compatibility versions
On 04/02/2013 02:47 PM, Mike Kolesnik wrote: On 03/27/2013 05:48 PM, Mike Kolesnik wrote: - Original Message - On 03/20/2013 08:20 PM, Yair Zaslavsky wrote: - Original Message - From: Shireesh Anjalsan...@redhat.com To: Mike Kolesnikmkole...@redhat.com Cc:engine-devel@ovirt.org Sent: Wednesday, March 20, 2013 4:47:08 PM Subject: Re: [Engine-devel] FeatureSupported and compatibility versions On 03/18/2013 01:11 PM, Shireesh Anjal wrote: On 03/18/2013 12:59 PM, Mike Kolesnik wrote: - Original Message - Hi all, The current mechanism in oVirt to check whether a feature is supported in a particular compatibility version is to use the FeatureSupported class. e.g. FeatureSupported.networkLinking(getVm().getVdsGroupCompatibilityVersion()) Checks whether the network linking feature is supported for the the VM's cluster compatibility version. This internally checks whether the value of the corresponding config (NetworkLinkingSupported) for the given compatibility version is true/false. I'm not sure if this is a good idea, since a feature is typically supported from a particular version. E.g. Gluster support was introduced in 3.1, and it continues to be available in all subsequent versions. So I see no point in adding configuration for every version indicating whether the feature is supported in that version or not. I suggest to use either of the following options: You can merge the configs into a single config when older versions go out of the supported versions for the system. i.e. in 4.0 you can have upgrade script that merges all GlusterFeatureSupported to one entry instead of several. Why are we even storing this information in config? Is this something that can be configured at customer site? As previously explained (but off list :) ) , Config gives you the ability to have a cachable map of entry (i.e - feature name) per version and value. I guess it was convinient for the developers to use that. I also mentioned that customers/oVirt users should config the entries of vdc_options using engine-config tool only. Not all entries are exposed via engine-config.properties (and no, not just is feature supported entries are hidden). 1) Instead of using a boolean config for each version, use a single string config that indicates the supported from version e.g. GlusterSupportedFrom = 3.1. There could be rare cases where a feature, for some reason, is removed in some release. In such cases, we could use one additional config for the supported to version. 2) Continue with the boolean approach, but do not have entries for every version; rather make use of the default value for majority of cases, and add the explicit version mapping for the minority e.g. GlusterSupported = true by default, and false in case of 3.0 (only one config required for 3.0) I'm not sure why we would want
Re: [Engine-devel] FeatureSupported and compatibility versions
On 03/27/2013 05:48 PM, Mike Kolesnik wrote: - Original Message - On 03/20/2013 08:20 PM, Yair Zaslavsky wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: Mike Kolesnik mkole...@redhat.com Cc: engine-devel@ovirt.org Sent: Wednesday, March 20, 2013 4:47:08 PM Subject: Re: [Engine-devel] FeatureSupported and compatibility versions On 03/18/2013 01:11 PM, Shireesh Anjal wrote: On 03/18/2013 12:59 PM, Mike Kolesnik wrote: - Original Message - Hi all, The current mechanism in oVirt to check whether a feature is supported in a particular compatibility version is to use the FeatureSupported class. e.g. FeatureSupported.networkLinking(getVm().getVdsGroupCompatibilityVersion()) Checks whether the network linking feature is supported for the the VM's cluster compatibility version. This internally checks whether the value of the corresponding config (NetworkLinkingSupported) for the given compatibility version is true/false. I'm not sure if this is a good idea, since a feature is typically supported from a particular version. E.g. Gluster support was introduced in 3.1, and it continues to be available in all subsequent versions. So I see no point in adding configuration for every version indicating whether the feature is supported in that version or not. I suggest to use either of the following options: You can merge the configs into a single config when older versions go out of the supported versions for the system. i.e. in 4.0 you can have upgrade script that merges all GlusterFeatureSupported to one entry instead of several. Why are we even storing this information in config? Is this something that can be configured at customer site? As previously explained (but off list :) ) , Config gives you the ability to have a cachable map of entry (i.e - feature name) per version and value. I guess it was convinient for the developers to use that. I also mentioned that customers/oVirt users should config the entries of vdc_options using engine-config tool only. Not all entries are exposed via engine-config.properties (and no, not just is feature supported entries are hidden). 1) Instead of using a boolean config for each version, use a single string config that indicates the supported from version e.g. GlusterSupportedFrom = 3.1. There could be rare cases where a feature, for some reason, is removed in some release. In such cases, we could use one additional config for the supported to version. 2) Continue with the boolean approach, but do not have entries for every version; rather make use of the default value for majority of cases, and add the explicit version mapping for the minority e.g. GlusterSupported = true by default, and false in case of 3.0 (only one config required for 3.0) I'm not sure why we would want to complicate this simple mechanism? Is there much to gain? I think option 1 suggested above is simpler - to implement as well as to understand. Let me give you an example of why I don't like current mechanism. I introduce a version check for a feature that was introduced in 3.1. I'm being asked now to add three entries in config 3.0 - false 3.1 - true 3.2 - true It will also mean that when 3.3 goes out, someone has to make sure that another entry is added for 3.3-true. I think it is not logical as well as scalable if you have more versions. And it sounds far more complex (to maintain) than just having FeatureSupportedFrom = 3.1 So I would like to know if there are any objections to my proposal. I intend to use this for at least the gluster related features. I've sent a patch (http://gerrit.ovirt.org/12970) with following changes: 1) Introduced CompatibilityUtils that provides utility methods for checking if a given feature is supported in the config. One method to check based on boolean values (as is being done today for virt features), and nother to check based on a range (from, to) which I would like to use for gluster features. 2) Renamed FeatureSupported to VirtFeatureSupported, and made it use the first utility method from CompatibilityUtils 3) Introduced GlusterFeatureSupported for gluster features, which uses the second utility method from CompatibilityUtils Key advantage here is that - we don't have to touch any virt specifc source for adding compatibility checks for gluster features - virt features continue to use the existing boolean config check Any comments / suggestions / reviews will be highly appreciated :) I think splitting to two classes is OK, but the underlying mechanism IMO should be as Omer suggested: Use the default value from the java config file, and if in the DB there is a version specific value then use it for that version only. Review comments here are on the contrary: http://gerrit.ovirt.org/#/c/12970/5/backend/manager/dbscripts/upgrade/pre_upgrade/_config.sql I don't think From, To, etc is a good design, it's not a standard way and is very restrictive. Can you please explain in what way
Re: [Engine-devel] FeatureSupported and compatibility versions
On 03/20/2013 08:20 PM, Yair Zaslavsky wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: Mike Kolesnik mkole...@redhat.com Cc: engine-devel@ovirt.org Sent: Wednesday, March 20, 2013 4:47:08 PM Subject: Re: [Engine-devel] FeatureSupported and compatibility versions On 03/18/2013 01:11 PM, Shireesh Anjal wrote: On 03/18/2013 12:59 PM, Mike Kolesnik wrote: - Original Message - Hi all, The current mechanism in oVirt to check whether a feature is supported in a particular compatibility version is to use the FeatureSupported class. e.g. FeatureSupported.networkLinking(getVm().getVdsGroupCompatibilityVersion()) Checks whether the network linking feature is supported for the the VM's cluster compatibility version. This internally checks whether the value of the corresponding config (NetworkLinkingSupported) for the given compatibility version is true/false. I'm not sure if this is a good idea, since a feature is typically supported from a particular version. E.g. Gluster support was introduced in 3.1, and it continues to be available in all subsequent versions. So I see no point in adding configuration for every version indicating whether the feature is supported in that version or not. I suggest to use either of the following options: You can merge the configs into a single config when older versions go out of the supported versions for the system. i.e. in 4.0 you can have upgrade script that merges all GlusterFeatureSupported to one entry instead of several. Why are we even storing this information in config? Is this something that can be configured at customer site? As previously explained (but off list :) ) , Config gives you the ability to have a cachable map of entry (i.e - feature name) per version and value. I guess it was convinient for the developers to use that. I also mentioned that customers/oVirt users should config the entries of vdc_options using engine-config tool only. Not all entries are exposed via engine-config.properties (and no, not just is feature supported entries are hidden). 1) Instead of using a boolean config for each version, use a single string config that indicates the supported from version e.g. GlusterSupportedFrom = 3.1. There could be rare cases where a feature, for some reason, is removed in some release. In such cases, we could use one additional config for the supported to version. 2) Continue with the boolean approach, but do not have entries for every version; rather make use of the default value for majority of cases, and add the explicit version mapping for the minority e.g. GlusterSupported = true by default, and false in case of 3.0 (only one config required for 3.0) I'm not sure why we would want to complicate this simple mechanism? Is there much to gain? I think option 1 suggested above is simpler - to implement as well as to understand. Let me give you an example of why I don't like current mechanism. I introduce a version check for a feature that was introduced in 3.1. I'm being asked now to add three entries in config 3.0 - false 3.1 - true 3.2 - true It will also mean that when 3.3 goes out, someone has to make sure that another entry is added for 3.3-true. I think it is not logical as well as scalable if you have more versions. And it sounds far more complex (to maintain) than just having FeatureSupportedFrom = 3.1 So I would like to know if there are any objections to my proposal. I intend to use this for at least the gluster related features. I've sent a patch (http://gerrit.ovirt.org/12970) with following changes: 1) Introduced CompatibilityUtils that provides utility methods for checking if a given feature is supported in the config. One method to check based on boolean values (as is being done today for virt features), and nother to check based on a range (from, to) which I would like to use for gluster features. 2) Renamed FeatureSupported to VirtFeatureSupported, and made it use the first utility method from CompatibilityUtils 3) Introduced GlusterFeatureSupported for gluster features, which uses the second utility method from CompatibilityUtils Key advantage here is that - we don't have to touch any virt specifc source for adding compatibility checks for gluster features - virt features continue to use the existing boolean config check Any comments / suggestions / reviews will be highly appreciated :) Thoughts? Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel
Re: [Engine-devel] Best place for new interfaces used by bll
On 03/15/2013 03:49 PM, Itamar Heim wrote: On 03/06/2013 06:57 PM, Ravi Nori wrote: Hi, I am working on refactoring some of the backend code in bll and need to create interfaces so that I can eliminate the dependency between bll and the new module. Currently I created the interfaces in org.ovirt.engine.core.common.interfaces, but from what I understand this module is used by both frontend and backend. The new interfaces will only be used by the backend. What would be the best place to put these new interfaces? 1. i agree with alissa and alon on the larger refactoring. 2. not all of common today is used by gwt, specifically, interfaces are shared/common, but backendinterfaces are limited to backend. since we are trying to refactor the frontend to not use common at all by moving to the restapi, I'm not sure i'd bother we'd splitting common more by shared/not-shared with frontend. but i would focus on making things in their own packages. i.e., I'd like to see ovirt-engine for gluster-only having to deploy/build/pull only what's relevant for them, not the 'virt' stuff for example. (easier for bll. a bit more work for db/common) +1 Happy to see we're finally talking about it seriously :) Thanks, Itamar ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] FeatureSupported and compatibility versions
On 03/18/2013 01:11 PM, Shireesh Anjal wrote: On 03/18/2013 12:59 PM, Mike Kolesnik wrote: - Original Message - Hi all, The current mechanism in oVirt to check whether a feature is supported in a particular compatibility version is to use the FeatureSupported class. e.g. FeatureSupported.networkLinking(getVm().getVdsGroupCompatibilityVersion()) Checks whether the network linking feature is supported for the the VM's cluster compatibility version. This internally checks whether the value of the corresponding config (NetworkLinkingSupported) for the given compatibility version is true/false. I'm not sure if this is a good idea, since a feature is typically supported from a particular version. E.g. Gluster support was introduced in 3.1, and it continues to be available in all subsequent versions. So I see no point in adding configuration for every version indicating whether the feature is supported in that version or not. I suggest to use either of the following options: You can merge the configs into a single config when older versions go out of the supported versions for the system. i.e. in 4.0 you can have upgrade script that merges all GlusterFeatureSupported to one entry instead of several. Why are we even storing this information in config? Is this something that can be configured at customer site? 1) Instead of using a boolean config for each version, use a single string config that indicates the supported from version e.g. GlusterSupportedFrom = 3.1. There could be rare cases where a feature, for some reason, is removed in some release. In such cases, we could use one additional config for the supported to version. 2) Continue with the boolean approach, but do not have entries for every version; rather make use of the default value for majority of cases, and add the explicit version mapping for the minority e.g. GlusterSupported = true by default, and false in case of 3.0 (only one config required for 3.0) I'm not sure why we would want to complicate this simple mechanism? Is there much to gain? I think option 1 suggested above is simpler - to implement as well as to understand. Let me give you an example of why I don't like current mechanism. I introduce a version check for a feature that was introduced in 3.1. I'm being asked now to add three entries in config 3.0 - false 3.1 - true 3.2 - true It will also mean that when 3.3 goes out, someone has to make sure that another entry is added for 3.3-true. I think it is not logical as well as scalable if you have more versions. And it sounds far more complex (to maintain) than just having FeatureSupportedFrom = 3.1 So I would like to know if there are any objections to my proposal. I intend to use this for at least the gluster related features. Thoughts? Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] FeatureSupported and compatibility versions
On 03/18/2013 12:59 PM, Mike Kolesnik wrote: - Original Message - Hi all, The current mechanism in oVirt to check whether a feature is supported in a particular compatibility version is to use the FeatureSupported class. e.g. FeatureSupported.networkLinking(getVm().getVdsGroupCompatibilityVersion()) Checks whether the network linking feature is supported for the the VM's cluster compatibility version. This internally checks whether the value of the corresponding config (NetworkLinkingSupported) for the given compatibility version is true/false. I'm not sure if this is a good idea, since a feature is typically supported from a particular version. E.g. Gluster support was introduced in 3.1, and it continues to be available in all subsequent versions. So I see no point in adding configuration for every version indicating whether the feature is supported in that version or not. I suggest to use either of the following options: You can merge the configs into a single config when older versions go out of the supported versions for the system. i.e. in 4.0 you can have upgrade script that merges all GlusterFeatureSupported to one entry instead of several. 1) Instead of using a boolean config for each version, use a single string config that indicates the supported from version e.g. GlusterSupportedFrom = 3.1. There could be rare cases where a feature, for some reason, is removed in some release. In such cases, we could use one additional config for the supported to version. 2) Continue with the boolean approach, but do not have entries for every version; rather make use of the default value for majority of cases, and add the explicit version mapping for the minority e.g. GlusterSupported = true by default, and false in case of 3.0 (only one config required for 3.0) I'm not sure why we would want to complicate this simple mechanism? Is there much to gain? I think option 1 suggested above is simpler - to implement as well as to understand. Let me give you an example of why I don't like current mechanism. I introduce a version check for a feature that was introduced in 3.1. I'm being asked now to add three entries in config 3.0 - false 3.1 - true 3.2 - true It will also mean that when 3.3 goes out, someone has to make sure that another entry is added for 3.3-true. I think it is not logical as well as scalable if you have more versions. And it sounds far more complex (to maintain) than just having FeatureSupportedFrom = 3.1 So I would like to know if there are any objections to my proposal. I intend to use this for at least the gluster related features. Thoughts? Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] restapi - domains
On 03/09/2013 12:52 AM, Itamar Heim wrote: On 03/08/2013 06:04 AM, Shireesh Anjal wrote: On 03/07/2013 01:05 PM, Aravinda wrote: We can have only two fields in login screen, username and password. Username will include domain name(username@domain). Default domain name can be internal if user didn't enter the domain name as part of username then we can append the default value and validate. Note: We use username@domain as username when we connect through rest_api_url/api The idea is to *not* have the user type in the domain name, but rather let him/her choose one, just like what happens in webadmin. We should try and minimize typing as much as possible when it comes to mobile apps. I think this was done on purpose for some reason to not provide a public api for the rest api, but i could be wrong and don't remember the detail. as the concepts of multi tenancy and multiple domains grow, providing the list of domains is considered an issue, Is it an issue specific to restapi? For we *do show* the list of domains in webadmin login screen. and most systems today require user to provide their full user/domain (well, usually in the form of their email address). -- regards Aravinda On 03/07/2013 11:15 AM, Shireesh Anjal wrote: Hi, We are trying to develop a simple android app to monitor and manage gluster clusters by consuming the restapi exposed by engine. The first screen is the login screen, which is similar to the webadmin login screen. Here, we want to populate the combo box of domains by fetching it from the restapi. However, the domains api cannot be invoked without authentication! So we have a sort of a chicken-and-egg problem. Any suggestions on how to tackle this? I feel the domains api should be public, in the sense it should not expect authentication. Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] restapi - domains
On 03/07/2013 01:05 PM, Aravinda wrote: We can have only two fields in login screen, username and password. Username will include domain name(username@domain). Default domain name can be internal if user didn't enter the domain name as part of username then we can append the default value and validate. Note: We use username@domain as username when we connect through rest_api_url/api The idea is to *not* have the user type in the domain name, but rather let him/her choose one, just like what happens in webadmin. We should try and minimize typing as much as possible when it comes to mobile apps. -- regards Aravinda On 03/07/2013 11:15 AM, Shireesh Anjal wrote: Hi, We are trying to develop a simple android app to monitor and manage gluster clusters by consuming the restapi exposed by engine. The first screen is the login screen, which is similar to the webadmin login screen. Here, we want to populate the combo box of domains by fetching it from the restapi. However, the domains api cannot be invoked without authentication! So we have a sort of a chicken-and-egg problem. Any suggestions on how to tackle this? I feel the domains api should be public, in the sense it should not expect authentication. Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
[Engine-devel] restapi - domains
Hi, We are trying to develop a simple android app to monitor and manage gluster clusters by consuming the restapi exposed by engine. The first screen is the login screen, which is similar to the webadmin login screen. Here, we want to populate the combo box of domains by fetching it from the restapi. However, the domains api cannot be invoked without authentication! So we have a sort of a chicken-and-egg problem. Any suggestions on how to tackle this? I feel the domains api should be public, in the sense it should not expect authentication. Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] 3.2 features for release notes
Following gluster related features were planned, but are not yet ready to be released: * Unidirectional Gluster Geo-replication support http://www.ovirt.org/Features/Gluster_Geo_Replication * Support for asynchronous Gluster volume tasks http://www.ovirt.org/Features/Gluster_Volume_Asynchronous_Tasks_Management * Gluster Volume Performance Statistics http://www.ovirt.org/Features/Gluster_Volume_Performance_Statistics Features that are not on the wiki, but will be released are: * Bug 901443 https://bugzilla.redhat.com/show_bug.cgi?id=901443 - engine-setup should prompt for application mode * oVirt log collector enhanced to collect gluster logs if applicable http://gerrit.ovirt.org/9765 Regards, Shireesh On Thursday 24 January 2013 01:17 PM, Cheryn Tan wrote: Hi all, I'm preparing the oVirt 3.2 release notes, starting with the features listed here: http://www.ovirt.org/OVirt_3.2_release-management#Features Can you please go through the list, and reply to this thread if: a) There are features being released for 3.2 which should be documented, but are not on the list; or b) Some of the planned features on this list are not being released for 3.2; or c) Some of the features on this list are being released for 3.2, but should not be documented. For features which aren't on the wiki, please send a short description or Bugzilla numbers of each feature. Please reply no later than Jan 28. Thank you very much for your help! Cheryn ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] 3.2 features for release notes
- Original Message - From: Itamar Heim ih...@redhat.com To: Cheryn Tan cheryn...@redhat.com Cc: Juan Antonio Hernandez Fernandez jhern...@redhat.com, engine-devel@ovirt.org, Shireesh Anjal san...@redhat.com Sent: Thursday, January 24, 2013 9:00:07 PM Subject: Re: [Engine-devel] 3.2 features for release notes On 23/01/2013 23:47, Cheryn Tan wrote: Hi all, I'm preparing the oVirt 3.2 release notes, starting with the features listed here: http://www.ovirt.org/OVirt_3.2_release-management#Features Can you please go through the list, and reply to this thread if: a) There are features being released for 3.2 which should be documented, but are not on the list; or b) Some of the planned features on this list are not being released for 3.2; or c) Some of the features on this list are being released for 3.2, but should not be documented. For features which aren't on the wiki, please send a short description or Bugzilla numbers of each feature. Please reply no later than Jan 28. Thank you very much for your help! Cheryn ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel I think most of these made it to 3.2, i'll point fingers at others to give more details... (doesn't mean others weren't involved in them, just picked someone to give the details for release notes). SSIA: Add support for Opteron G5 (Seoul) Add Support for Intel Haswell CPU windows 8/12 - add os type windows 8/12 - default in user portal should be rdp (till we have spice support...) expand tree by default with list of DCs federico: storage live migration tomas: Configure Smartcard for VM including Portal support Allow e1000 to be selected as nic type for Windows VM dynamic frantisek: checkbox at vm level to prevent accidental deletion shahar: vm payload issues (with cd-rom) Arik Changing the name of the virtual machines, created from a pool through web-admin-portal Alona: network as a main tab network permissions network main tab: search for networks (and api) network main tab: show vms by logical network hot-plug network - change networks on the fly Igor: hotplug nic hook Muli: display the VM interface IP per vnic report ipv6 addresses from guest vojtech: ui plugins Greg: cluster policy on hyperthreads usage Laszlo: -cpu host Ofri: quota - add to resource tab in power user portal moran: ovirt-live daniel erez: slm: move multiple disks (one by one) unicode enablement for OVF fields disks subtab/api under storage domain Tal: delete disk on vm delete flag (for image vs. volume) Liron: move OVF update out of reconstruct master and all other flows as an independent sync process Eli: inject events bootstrap: fetch logs to engine power management proxy (DC or cluster) multiple fencing devices add ilo2/ilo4 in text Yaniv Bronhaim: collect hardware (bios) information juan: engine-vdsm ssl session caching michael kublin: auto-recovery: last host in status up Alon Bar-Lev: bootstrap: install rhev-h without communication to engine 443 bootstrap: pki: use PKCS#12 format to store keys bootstrap: rewrite (ovirt-host-deploy) bootstrap: rewrite (otopi) Yair: rhevm-manage-domains hardcodes the DC's address instead of using DNS lookups Yaniv Dary: cloud provider report Report Daily list of people logged into Spice consoles and their activity add status of storage domain to dwh/reports DC Dashboard should display Storage Domains count single vm uptime - rhevm-reports should be able to report uptime against virtual machines Storage Inventory Report - New report michael pasternak: sdk - allow simultaneous connections to multiple servers cli - disable output redirection to ease testing efforts java sdk Doron: tuned profile for host doron/adam: not sure about status of vdsm-mom in 3.2? shireesh - what about: Forced removal of a host Thanks - I had missed this one - https://bugzilla.redhat.com/show_bug.cgi?id=882807 Configuration sync with Gluster CLI Was planned, and feature page exists (http://www.ovirt.org/Features/Gluster_Sync_Configuration_With_CLI) Import of existing gluster clusters Was planned, and feature page exists (http://www.ovirt.org/Features/Gluster_Import_Existing_Cluster) Thanks, Itamar ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] RFE: Actions on tasks (jobs)
On Monday 21 January 2013 05:47 PM, Yair Zaslavsky wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: engine-devel@ovirt.org Sent: Monday, January 21, 2013 2:08:14 PM Subject: [Engine-devel] RFE: Actions on tasks (jobs) Hi, We plan to introduce support for gluster tasks in oVirt, using the current Jobs/steps framework. Which means, any gluster async task started on a cluster will be shown as a Job in the Tasks tab. While this helps in listing and monitoring the status of all gluster tasks, we have a requirement to support a set of actions on such tasks. One should be able to select a task, and then, if supported for that task, perform one or more of the following actions on it: - pause - resume - abort - commit I think this can probably be achieved by introducing a generic mechanism for performing actions on task, allowing each type of task to define what actions are allowed on it in it's current state. Requesting opinions/suggestions on possible ways to achieve this requirement. Several points - 1. By performing actions - you probably mean the entry point to engine will be BllCommand? StopTaskCommand for example? If so, we need to think about the permission of the command. who can stop a for example? What is its role? I see that Job extends BusinessEntityGuid, and hence should be possible to assign permissions on it, just like any other entity? Though I think this doesn't fit directly in the current UI, as jobs is not a main tab. In case of gluster, I would add this permission to the 'GlusterAdmin' role. 2. You mentioned task type - does this mean extending the AsyncTaskType enum? Are you going to have your own Tasks enum? I'm not sure about AsyncTaskType, as I believe it is related to the vdsm async tasks and AsyncTaskManager, and I don't think we'll be going there. In our case, the command being executed in the job itself could be synonymous to the task type. What I mean by task type specific actions is, not all actions are applicable on all types of tasks. e.g. commit is applicable only to a replace brick task, and not to rebalance volume task, whereas abort is applicable to both. So the corresponding command should dictate what action can be performed on the task. Thanks, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] RFE: Actions on tasks (jobs)
On Monday 21 January 2013 08:49 PM, Moti Asayag wrote: On 01/21/2013 02:08 PM, Shireesh Anjal wrote: Hi, We plan to introduce support for gluster tasks in oVirt, using the current Jobs/steps framework. Which means, any gluster async task started on a cluster will be shown as a Job in the Tasks tab. While this helps in listing and monitoring the status of all gluster tasks, we have a requirement to support a set of actions on such tasks. One should be able to select a task, and then, if supported for that task, perform one or more of the following actions on it: - pause - resume - abort - commit Could you explain what is the meaning of 'commit' in this context? The replace brick task in gluster migrates all data from an existing brick to the new brick which is replacing it. After all the data is migrated, user needs to perform a commit for the new brick to come into effect. http://gluster.org/community/documentation/index.php/Gluster_3.1:_Migrating_Volumes In addition, is there a need for a 'restart' action ? At present, we don't need such an action as far as gluster is concerned. Generally, a command (user-action / internal action) is mapped to a Job when it's monitored. The job may contain several steps where each step might represent an async-task (i.e task running on a node, vdsm task). There are two levels of control: controlling a specific vdsm task/step or controlling the entire job. I think that the granularity of the action should be on Job level, since a step's result (assuming cancelled step is considered failed) will determine the job's status as failed/aborted - therefore the rest of running steps should also be aborted. +1 If needed, supporting 'restart' operation could also be relatively easily support for job level (requires saving the action's parameters for a re-run). The Jobs cleanup manager should take care of cleaning cancelled jobs and to keep paused jobs. Does the suggested actions supported by the AsyncTaskManager and by VDSM ? No. These are not vdsm tasks. They'll be managed completely by glusterfs. We plan to introduce gluster specific verbs in vdsm for starting/managing/monitoring these tasks. http://gerrit.ovirt.org/10200 And then have a background periodic job in engine to fetch and update the status of the same in the Job repository. I think this can probably be achieved by introducing a generic mechanism for performing actions on task, allowing each type of task to define what actions are allowed on it in it's current state. Requesting opinions/suggestions on possible ways to achieve this requirement. Thanks, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
[Engine-devel] RFE: Health Indication on UI
Hi, Currently, oVirt is intelligent to understand the relationship hierarchies of various entities in the system, and even allows setting permissions based on this hierarchy. Can this information further be used to enhance the health monitoring of various entities in the system? Let me take an example. Consider this hierarchy (gluster): System - Cluster - Volume - Brick If one or more bricks of a volume are down, which indicates a problem, following entities should be overlaid with a warning symbol (say, a yellow exclamation mark) - Volume to which the brick(s) belong(s) - it's parent cluster - System This helps in highlighting what parts of the system are affected by failures/problems at the lowest level entities, and gives a better idea of the overall health of the entire environment to the administrator. Thoughts? Thanks, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
[Engine-devel] RFE: Actions on tasks (jobs)
Hi, We plan to introduce support for gluster tasks in oVirt, using the current Jobs/steps framework. Which means, any gluster async task started on a cluster will be shown as a Job in the Tasks tab. While this helps in listing and monitoring the status of all gluster tasks, we have a requirement to support a set of actions on such tasks. One should be able to select a task, and then, if supported for that task, perform one or more of the following actions on it: - pause - resume - abort - commit I think this can probably be achieved by introducing a generic mechanism for performing actions on task, allowing each type of task to define what actions are allowed on it in it's current state. Requesting opinions/suggestions on possible ways to achieve this requirement. Thanks, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Error on starting webadmin
On Thursday 13 December 2012 08:06 PM, Roy Golan wrote: On 12/13/2012 04:32 PM, Vojtech Szocs wrote: Hi, I've updated http://www.ovirt.org/Building_Engine_Draft - added section Engine local configuration (feel free to edit or modify as necessary) this is making the contribution process more complex. lets think of a lighter way to get a developing setup. +1 Vojtech - Original Message - From: Vojtech Szocs vsz...@redhat.com To: engine-devel@ovirt.org Sent: Thursday, December 13, 2012 2:49:39 PM Subject: Re: [Engine-devel] Error on starting webadmin Hi guys, as Moti mentioned, this issue is related to UI plugins patches that have been merged recently. However, the root cause of this issue is LocalConfig class (machine-specific Engine configuration) throwing exception in following cases: - cannot load default properties (ENGINE_DEFAULTS property if defined, otherwise /usr/share/ovirt-engine/conf/engine.conf.defaults) - cannot load custom properties (ENGINE_VARS property if defined, otherwise /etc/sysconfig/ovirt-engine) - requesting missing property value (LocalConfig.getProperty method) (Note that UI plugins use two Engine properties: ENGINE_USR and ENGINE_ETC.) As Juan suggests in his comment in http://gerrit.ovirt.org/#/c/10026/, we could include this in building Engine wiki. For example: - have ENGINE_DEFAULTS property point to OVIRT_HOME/backend/manager/conf/engine.conf.defaults (alternatively, users can copy this file to /usr/share/ovirt-engine/conf) - create custom Engine config file, e.g. ~/myengine.conf, and have ENGINE_VARS property point to this file Vojtech - Original Message - From: Daniel Erez de...@redhat.com To: Moti Asayag masa...@redhat.com Cc: engine-devel@ovirt.org Sent: Thursday, December 13, 2012 11:49:13 AM Subject: Re: [Engine-devel] Error on starting webadmin - Original Message - From: Moti Asayag masa...@redhat.com To: engine-devel@ovirt.org Sent: Thursday, December 13, 2012 12:25:07 PM Subject: [Engine-devel] Error on starting webadmin Hi, I get the following error when trying to connect to webadmin: 2012-12-13 11:57:06,689 ERROR [org.apache.catalina.core.ContainerBase.[jboss.web].[default-host].[/webadmin].[WebAdminDynamicHosting]] (http--0.0.0.0-8700-1) Servlet.service() for servlet WebAdminDynamicHosting threw exception: java.lang.IllegalArgumentException: The property ENGINE_USR doesn't have a value. It caused by the PluginDataManager instantiation which requires several variables to be defined. In order to by pass those errors do: 1. create a file /usr/share/ovirt-engine/conf/engine.conf.defaults 2. Add the following properties to the file: ENGINE_USR=username ENGINE_ETC=/etc/ovirt-engine 3. restart Jboss You can use the 'engine.conf.defaults' that resides at 'backend/manager/conf'. Regards, Moti ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Fixed -Xmx could kill your JVM
On Wednesday 12 September 2012 06:36 PM, Alon Bar-Lev wrote: - Original Message - From: Juan Hernandez jhern...@redhat.com To: Laszlo Hornyak lhorn...@redhat.com Cc: engine-devel@ovirt.org Sent: Wednesday, September 12, 2012 3:57:33 PM Subject: Re: [Engine-devel] Fixed -Xmx could kill your JVM snip Worst case we can change the defaults for ENGINE_HEAP_MIN and ENGINE_HEAP_MAX to get that behavior, for example: ENGINE_HEAP_MIN=128m ENGINE_HEAP_MAX=1g I am open to do that change, specially if you show me hard data proving that it is better for performance ;-) . We are talking about virtual memory, allocation of the heap is done as reserved, so there is no real impact on the system if memory is unused. When a reserved page is actually used it is committed. Unless there is a system with small page file, there is no real reason to use lower values in HEAP_MIN. Alon. I've seen that Java experts from Sun (now Oracle) recommend to identify a good (high enough) value for the heap size and set the same value for both min and max, during the sessions on Java performance in the Java One conference. ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
[Engine-devel] is gerrit.ovirt.org down? eom
___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Gluster IPTable configuration
On Friday 31 August 2012 12:05 AM, Alon Bar-Lev wrote: - Original Message - From: Selvasundaram sesub...@redhat.com To: engine-devel@ovirt.org Cc: Shireesh Anjal san...@redhat.com Sent: Thursday, August 30, 2012 4:30:16 PM Subject: [Engine-devel] Gluster IPTable configuration Hi, I want to add gluster specific IPTable configuration in addition to the ovirt IPTable configuration (if it is gluster node). There are two approaches, 1. Having one more gluster specific IP table config in db and merge with ovirt IPTable config (merging NOT appending) [I have the patch engine: Gluster specific firewall configurations #7244] 2. Having two different IP Table config (ovirt and ovirt+gluster) and use either one. Please provide your suggestions or improvements on this. Hello all, The mentioned patch[1], adds hard coded gluster code into the bootstrap code, manipulate the firewall configuration to be gluster specific. It hardcoded search for reject, insert before some other rules. I believe this hardcode approach is obsolete now that we have proper tools for templates. A more robust solution would be defining generic profiles, each profile as a template, each template can refer to different profiles, and assign profile to a node. This way the implementation is not gluster [or any] specific and can be reused for more setups, code is cleaner. Example: BASIC.PRE :INPUT ACCEPT [0:0] :FORWARD ACCEPT [0:0] :OUTPUT ACCEPT [0:0] BASIC.IN accept ... accept ... BASIC.POST reject ... reject ... BASIC ${BASIC.PRE} ${BASIC.IN} ${BASIC.POST} GLUSTER ${BASIC.PRE} ${BASIC.IN} accept ... ${BASIC.POST} reject ... I like the separation of PRE/IN/POST rules here. However I think it is better to keep the service specific rules in separate configurations. Currently, whole iptables rules script is kept in the vdc option IPTablesConfig. How about changing this as follows? - Split the current config into three: IPTablesConfig.PRE, IPTablesConfig.VIRT and IPTablesConfig.POST - Let services like Gluster add their own vdc options e.g. IPTablesConfig.GLUSTER - When assembling the full script in VdsInstaller, - Take IPTablesConfig.PRE - Append it with IPTablesConfig.service for every service to be enabled on the host/cluster - Append it with IPTablesConfig.POST Thoughts? Regards, Alon Bar-Lev [1] http://gerrit.ovirt.org/#/c/7244/ ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Gluster IPTable configuration
On Monday 03 September 2012 06:22 PM, Alon Bar-Lev wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: engine-devel@ovirt.org Cc: Alon Bar-Lev alo...@redhat.com, Selvasundaram sesub...@redhat.com Sent: Monday, September 3, 2012 3:42:17 PM Subject: Re: [Engine-devel] Gluster IPTable configuration On Friday 31 August 2012 12:05 AM, Alon Bar-Lev wrote: - Original Message - From: Selvasundaram sesub...@redhat.com To: engine-devel@ovirt.org Cc: Shireesh Anjal san...@redhat.com Sent: Thursday, August 30, 2012 4:30:16 PM Subject: [Engine-devel] Gluster IPTable configuration Hi, I want to add gluster specific IPTable configuration in addition to the ovirt IPTable configuration (if it is gluster node). There are two approaches, 1. Having one more gluster specific IP table config in db and merge with ovirt IPTable config (merging NOT appending) [I have the patch engine: Gluster specific firewall configurations #7244] 2. Having two different IP Table config (ovirt and ovirt+gluster) and use either one. Please provide your suggestions or improvements on this. Hello all, The mentioned patch[1], adds hard coded gluster code into the bootstrap code, manipulate the firewall configuration to be gluster specific. It hardcoded search for reject, insert before some other rules. I believe this hardcode approach is obsolete now that we have proper tools for templates. A more robust solution would be defining generic profiles, each profile as a template, each template can refer to different profiles, and assign profile to a node. This way the implementation is not gluster [or any] specific and can be reused for more setups, code is cleaner. Example: BASIC.PRE :INPUT ACCEPT [0:0] :FORWARD ACCEPT [0:0] :OUTPUT ACCEPT [0:0] BASIC.IN accept ... accept ... BASIC.POST reject ... reject ... BASIC ${BASIC.PRE} ${BASIC.IN} ${BASIC.POST} GLUSTER ${BASIC.PRE} ${BASIC.IN} accept ... ${BASIC.POST} reject ... I like the separation of PRE/IN/POST rules here. However I think it is better to keep the service specific rules in separate configurations. Currently, whole iptables rules script is kept in the vdc option IPTablesConfig. How about changing this as follows? - Split the current config into three: IPTablesConfig.PRE, IPTablesConfig.VIRT and IPTablesConfig.POST - Let services like Gluster add their own vdc options e.g. IPTablesConfig.GLUSTER - When assembling the full script in VdsInstaller, - Take IPTablesConfig.PRE - Append it with IPTablesConfig.service for every service to be enabled on the host/cluster - Append it with IPTablesConfig.POST Thoughts? This is a simple approach that will work for current implementation and configuration. However, it will effect all nodes, with or without gluster. I don't get the concern here. Could you please elaborate? I think that while we at it we should consider how to effect only appropriate subset of nodes. Alon. ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Gluster IPTable configuration
On Monday 03 September 2012 06:41 PM, Alon Bar-Lev wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: Alon Bar-Lev alo...@redhat.com Cc: Selvasundaram sesub...@redhat.com, engine-devel@ovirt.org Sent: Monday, September 3, 2012 4:00:14 PM Subject: Re: [Engine-devel] Gluster IPTable configuration On Monday 03 September 2012 06:22 PM, Alon Bar-Lev wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: engine-devel@ovirt.org Cc: Alon Bar-Lev alo...@redhat.com, Selvasundaram sesub...@redhat.com Sent: Monday, September 3, 2012 3:42:17 PM Subject: Re: [Engine-devel] Gluster IPTable configuration On Friday 31 August 2012 12:05 AM, Alon Bar-Lev wrote: - Original Message - From: Selvasundaram sesub...@redhat.com To: engine-devel@ovirt.org Cc: Shireesh Anjal san...@redhat.com Sent: Thursday, August 30, 2012 4:30:16 PM Subject: [Engine-devel] Gluster IPTable configuration Hi, I want to add gluster specific IPTable configuration in addition to the ovirt IPTable configuration (if it is gluster node). There are two approaches, 1. Having one more gluster specific IP table config in db and merge with ovirt IPTable config (merging NOT appending) [I have the patch engine: Gluster specific firewall configurations #7244] 2. Having two different IP Table config (ovirt and ovirt+gluster) and use either one. Please provide your suggestions or improvements on this. Hello all, The mentioned patch[1], adds hard coded gluster code into the bootstrap code, manipulate the firewall configuration to be gluster specific. It hardcoded search for reject, insert before some other rules. I believe this hardcode approach is obsolete now that we have proper tools for templates. A more robust solution would be defining generic profiles, each profile as a template, each template can refer to different profiles, and assign profile to a node. This way the implementation is not gluster [or any] specific and can be reused for more setups, code is cleaner. Example: BASIC.PRE :INPUT ACCEPT [0:0] :FORWARD ACCEPT [0:0] :OUTPUT ACCEPT [0:0] BASIC.IN accept ... accept ... BASIC.POST reject ... reject ... BASIC ${BASIC.PRE} ${BASIC.IN} ${BASIC.POST} GLUSTER ${BASIC.PRE} ${BASIC.IN} accept ... ${BASIC.POST} reject ... I like the separation of PRE/IN/POST rules here. However I think it is better to keep the service specific rules in separate configurations. Currently, whole iptables rules script is kept in the vdc option IPTablesConfig. How about changing this as follows? - Split the current config into three: IPTablesConfig.PRE, IPTablesConfig.VIRT and IPTablesConfig.POST - Let services like Gluster add their own vdc options e.g. IPTablesConfig.GLUSTER - When assembling the full script in VdsInstaller, - Take IPTablesConfig.PRE - Append it with IPTablesConfig.service for every service to be enabled on the host/cluster - Append it with IPTablesConfig.POST Thoughts? This is a simple approach that will work for current implementation and configuration. However, it will effect all nodes, with or without gluster. I don't get the concern here. Could you please elaborate? If we have 500 nodes, out of them 200 gluster, why do I need to distribute gluster specific rules to all 500? When I say Append it with IPTablesConfig.service for every service to be enabled on the host/cluster, I mean every service that needs to be enabled on the host. In today's scenario, where virt and gluster are the only two services, it will look something like: - If cluster supports virt, append IPTablesConfig.VIRT - If cluster supports gluster, append IPTablesConfig.GLUSTER So it will be driven by the cluster in which the host is being added. And gluster rules will be sent to only the hosts of clusters that have gluster enabled. Alon. ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Gluster IPTable configuration
On Monday 03 September 2012 07:20 PM, Alon Bar-Lev wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: Alon Bar-Lev alo...@redhat.com Cc: Selvasundaram sesub...@redhat.com, engine-devel@ovirt.org Sent: Monday, September 3, 2012 4:21:58 PM Subject: Re: [Engine-devel] Gluster IPTable configuration On Monday 03 September 2012 06:41 PM, Alon Bar-Lev wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: Alon Bar-Lev alo...@redhat.com Cc: Selvasundaram sesub...@redhat.com, engine-devel@ovirt.org Sent: Monday, September 3, 2012 4:00:14 PM Subject: Re: [Engine-devel] Gluster IPTable configuration On Monday 03 September 2012 06:22 PM, Alon Bar-Lev wrote: - Original Message - From: Shireesh Anjal san...@redhat.com To: engine-devel@ovirt.org Cc: Alon Bar-Lev alo...@redhat.com, Selvasundaram sesub...@redhat.com Sent: Monday, September 3, 2012 3:42:17 PM Subject: Re: [Engine-devel] Gluster IPTable configuration On Friday 31 August 2012 12:05 AM, Alon Bar-Lev wrote: - Original Message - From: Selvasundaram sesub...@redhat.com To: engine-devel@ovirt.org Cc: Shireesh Anjal san...@redhat.com Sent: Thursday, August 30, 2012 4:30:16 PM Subject: [Engine-devel] Gluster IPTable configuration Hi, I want to add gluster specific IPTable configuration in addition to the ovirt IPTable configuration (if it is gluster node). There are two approaches, 1. Having one more gluster specific IP table config in db and merge with ovirt IPTable config (merging NOT appending) [I have the patch engine: Gluster specific firewall configurations #7244] 2. Having two different IP Table config (ovirt and ovirt+gluster) and use either one. Please provide your suggestions or improvements on this. Hello all, The mentioned patch[1], adds hard coded gluster code into the bootstrap code, manipulate the firewall configuration to be gluster specific. It hardcoded search for reject, insert before some other rules. I believe this hardcode approach is obsolete now that we have proper tools for templates. A more robust solution would be defining generic profiles, each profile as a template, each template can refer to different profiles, and assign profile to a node. This way the implementation is not gluster [or any] specific and can be reused for more setups, code is cleaner. Example: BASIC.PRE :INPUT ACCEPT [0:0] :FORWARD ACCEPT [0:0] :OUTPUT ACCEPT [0:0] BASIC.IN accept ... accept ... BASIC.POST reject ... reject ... BASIC ${BASIC.PRE} ${BASIC.IN} ${BASIC.POST} GLUSTER ${BASIC.PRE} ${BASIC.IN} accept ... ${BASIC.POST} reject ... I like the separation of PRE/IN/POST rules here. However I think it is better to keep the service specific rules in separate configurations. Currently, whole iptables rules script is kept in the vdc option IPTablesConfig. How about changing this as follows? - Split the current config into three: IPTablesConfig.PRE, IPTablesConfig.VIRT and IPTablesConfig.POST - Let services like Gluster add their own vdc options e.g. IPTablesConfig.GLUSTER - When assembling the full script in VdsInstaller, - Take IPTablesConfig.PRE - Append it with IPTablesConfig.service for every service to be enabled on the host/cluster - Append it with IPTablesConfig.POST Thoughts? This is a simple approach that will work for current implementation and configuration. However, it will effect all nodes, with or without gluster. I don't get the concern here. Could you please elaborate? If we have 500 nodes, out of them 200 gluster, why do I need to distribute gluster specific rules to all 500? When I say Append it with IPTablesConfig.service for every service to be enabled on the host/cluster, I mean every service that needs to be enabled on the host. In today's scenario, where virt and gluster are the only two services, it will look something like: - If cluster supports virt, append IPTablesConfig.VIRT - If cluster supports gluster, append IPTablesConfig.GLUSTER So it will be driven by the cluster in which the host is being added. And gluster rules will be sent to only the hosts of clusters that have gluster enabled. OK... this is why I prefer templates. Much more simple and generic. No need to have custom application logic within application. I don't see what is so complex about the above approach, but do agree that it is not completely generic. A new service will require a new if condition in the VdsInstaller. In order to understand the templates approach better, can you please point me to some existing code in oVirt that uses such an approach? I see an example of the template text above, but I'm not sure how it can be used from the code. Alon ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
[Engine-devel] JUnit tests in 'bll' project
Hi, When I do a full build of oVirt engine (mvn clean install -Pgwtdev,gwt-admin,dep,enable-dao-tests), it seems that the JUnit tests in project 'bll' are not executed. Is this done intentionally? If yes, is there a simple way to execute them using mvn ? Thanks, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Any way to break the dao_unit_tests up?
On Monday 20 August 2012 11:16 AM, Mike Kolesnik wrote: - Original Message - The DAO unit tests take twice as long as the rest of the test to run is there any way to break them up into two pieces? It will not be easy.. The way the tests are built today is with DB-unit. DB-unit allows to have an XML file with predefined data (called fixtures) which is used to recreate the DB data each time a test-class is run. This is all fine, except that in our tests there are (at least) 2 issues: 1. The same fixtures.xml file is used in all DAO tests. 2. Some DAOs require fixtures for several tables. Now, we could fix issue #1 by splitting the fixtures file into smaller files, each relating to only one table, which would allow us to run these tests in parallel on the same DB. Issue #2 would require to figure out which tests require several fixtures, and have them run isolated from the other tests which require only a single table. A simpler solution could be to have the tests run each on it's own db schema (or it's own db) which eliminates the dependencies and allows to run all in parallel, but is a bit more complicated to maintain (we would need some script that generates these schemas/dbs for tests automatically) and keeping multiple schemas up to date would also require CPU time. This is speaking in terms of the tests themselves, without considering the build process itself. There are two issues here: 1) DB connection is created during initialization of every test case, and destroyed at the end of each test case execution 2) The fixtures data is inserted during initialization of every test case I think both of these can be resolved by - creating the test data only during initialization of the first test case, which will include creating the connection (with auto-commit = false), inserting fixtures data and committing it - rolling back any changes done to the database during test case execution in the tearDown method I just tried this in two phases. Using the same connection across all test cases brought down the dao unit tests run time from 4:42.683s to 1:07.628s, and inserting the fixtures data only once further brought it down to just 22.295s ! (on my local development machine) I've just sent a patch with these changes: http://gerrit.ovirt.org/7336 -- Thanks Robert Middleswarth @rmiddle (twitter/IRC) ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Proposed change in default port numbers
On Tuesday 17 July 2012 11:57 PM, Juan Hernandez wrote: On 07/17/2012 08:19 PM, Steve Gordon wrote: - Original Message - From: Juan Hernandez jhern...@redhat.com To: Andrew Cathrow acath...@redhat.com Cc: engine-devel@ovirt.org Sent: Monday, July 16, 2012 3:27:02 PM Subject: Re: [Engine-devel] Proposed change in default port numbers On 07/16/2012 09:21 PM, Andrew Cathrow wrote: - Original Message - From: Juan Hernandez jhern...@redhat.com To: engine-devel@ovirt.org Sent: Monday, July 16, 2012 2:44:40 PM Subject: [Engine-devel] Proposed change in default port numbers Hello all, In change http://gerrit.ovirt.org/6348 I am proposing to change the default port numbers used by the engine, in order to avoid conflicts with the default ports used by JBoss. To be clear though even if we moved to use port 6090 for http and 6091 for https we'd still have 80/443 available through the installer. Correct, 80 and 443 will continue to be the default ports when using Apache as proxy in front of JBoss: 80 - 80 (no change) 443 - 443 (no change) 8080 - 6090 8443 - 6091 This is probably a stupid question, but what are the following ports used for: 8009 - 6092 This port is used for the communication between the Apache web server and the JBoss application server using the AJP protocol. It doesn't need to be available outside of the machine. The Firewall Configuration chapter of oVirt installation guide (http://wiki.ovirt.org/wiki/File:OVirt-3.0-Installation_Guide-en-US.pdf) says that ports 8006 through 8009 are required for network communication from Administration Portal Clients to oVirt Engine. 4447 - 6093 These port is used by the remoting capability of the application server: calling EJBs from external applications. We don't use it but it is required anyhow. It doesn't need to be available outside of the machine. 4712 - 6094 4713 - 6095 These two ports are used by the transaction manager inside JBoss. They don't need to be available outside of the machine. So none of them needs a firewall rule to allow inbound traffic. I am proposing a different change to bind those ports to the loopback address so that they are not available even when the firewall is disabled: http://gerrit.ovirt.org/6349 I would disable them completely, but didn't find the way to do it yet. As far as I know we don't have them listed anywhere in the documentation as requiring a firewall rule to allow them, should we? They don't require a firewall rule to allow incoming traffic. We could explain in the documentation that they are required, but only for communications internal to the machine. ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
[Engine-devel] is gerrit.ovirt.org down?
I have been unable to access the web UI or work with the repository for around half an hour now... ~Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] restapi: 'gluster' prefix
On Tuesday 08 May 2012 10:45 AM, Itamar Heim wrote: On 05/07/2012 11:52 PM, Ayal Baron wrote: - Original Message - On 05/07/2012 07:06 PM, Shireesh Anjal wrote: On Monday 07 May 2012 02:06 AM, Ayal Baron wrote: - Original Message - i can't see any justification for the 'gluster' prefix, as this is only additional /service/ provided by the project, and Gluster now is a part of the RHT. I believe there needs to be an indication which service this is about. If we will support provisioning other storage types which also have volumes then we'd want a way to differentiate. However, isn't there a way to simply add gluster as the name space? i.e. somthing like: /api/gluster/.../volumes ? (instead of 'cluster' as it is redundant imho) A gluster volume is a cluster level entity, and hence /api/.../clusters/{cluster:id} seems like the right parent URI for the gluster volumes collection resource. that's true for all other root entities as well: - VM is DC/cluster level - template is DC level - disk is storage domain level - network is DC level - hosts are cluster level (for now) yet all of them have their own root collections as well. I think glustervolumes seems safest/most reasonable for now (either at cluster level or root level as well) does it make sense to also have gluster/bricks ? if so, I would nest it, i.e. gluster/{volumes|bricks|...} bricks are host level, afair they are not used like this at all. Though bricks reside inside a host, they are logically part of a gluster volume, and hence should be sub-resources of the volume. [/api/.../(gluster)volumes/{(gluster)volume:id}/bricks]. gluster/xxx is interesting as well, though not parallel to current virt mappings (storage_domains, disks, etc., being root collections) Separate gluster namespace does sound interesting, however it may not be feasible because we want the gluster volumes to be a collection sub-resource of the existing cluster resource. It can work if all the existing resources relevant to gluster are made available under it, which includes clusters and hosts. e.g. /api/gluster/clusters/{cluster:id}/hosts/{host:id}/... /api/gluster/clusters/{cluster:id}/volumes/{volume:id}/... It'll be interesting to see what others think about this. shireesh - any thoughts about this approach: - do you want volumes as root collection, or only under cluster Root collection for gluster volumes could be useful, though not critically important right now. We can revisit this at a later point of time. - if root, should these be glustervolumes like other root collection, or the under a gluster collection. This depends on whether we decide to go with the gluster namespace or not. ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel Thanks, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] restapi: 'gluster' prefix
On Monday 07 May 2012 02:06 AM, Ayal Baron wrote: - Original Message - i can't see any justification for the 'gluster' prefix, as this is only additional /service/ provided by the project, and Gluster now is a part of the RHT. I believe there needs to be an indication which service this is about. If we will support provisioning other storage types which also have volumes then we'd want a way to differentiate. However, isn't there a way to simply add gluster as the name space? i.e. somthing like: /api/gluster/.../volumes ? (instead of 'cluster' as it is redundant imho) A gluster volume is a cluster level entity, and hence /api/.../clusters/{cluster:id} seems like the right parent URI for the gluster volumes collection resource. On 05/06/2012 09:56 AM, Ori Liel wrote: We are introducing Gluster functionality into ovirt-engine REST-API. Gluster entities are 'Volumes' and 'Bricks'. The question is: should they be called: 'volume'/'brick' or 'gluster_volume/gluster_brick'? (with or without the 'gluster' prefix)? There was a short conversation about this in a patch in Gerrit: http://gerrit.ovirt.org/#change,3918 Since there are conflicting opinions, I'm bringing it here for a resolution. Thanks, Ori. ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel -- Michael Pasternak RedHat, ENG-Virtualization RD ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Query regarding ValidationUtils#validateInputs
On Sunday 15 April 2012 11:31 AM, Roy Golan wrote: On 04/12/2012 10:50 AM, Omer Frenkel wrote: *From: *Shireesh Anjal san...@redhat.com *To: *engine-devel@ovirt.org *Sent: *Thursday, April 12, 2012 9:35:25 AM *Subject: *[Engine-devel] Query regarding ValidationUtils#validateInputs Hi, This is regarding the following validation method we have in ValidationUtils: /public static T extends VdcActionParametersBase ArrayListString validateInputs(ListClass? validationGroupList, T parameters);/ I there any particular reason for supporting the validations only on objects of classes derived from VdcActionParametersBase? I guess this was done because this method is primarily intended to validate the action parameters passed to a BLL action, using the validation annotations on the parameter class. However I think this method can be useful for general use as well. e.g. I cannot add a @Valid annotation on a list or a map in a parameter class. So I need to iterate over the list/map, and validate each element inside the loop. The validation inside the loop can also utilize the above method if the restriction extends VdcActionParametersBase is removed. This will allow me to do the following in the canDoAction method: protected boolean canDoAction() { ... for(GlusterBrickEntity brick : getParameters().getGlusterVolume().getBricks()) { ListString errors = ValidationUtils.validateInputs(getValidationGroups(), brick); if(errors != null) { for(String error : errors) { addCanDoActionMessage(error); } } } ... } Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel i don't think there is a reason to restrict only for VdcActionParametersBase, roy what do you think? also you can use here getReturnValue().getCanDoActionMessages().addAll(errors); instead of going over on all errors. pls see SetupNetworksParameters.java for validating a list of network interfaces. hibernate validator supports trivial object graphs and members that are arrays, list and java.util.map implementations. If your code looks like that it should work. If you are using special validation groups please add them to your command with addValidationGroups. this should work: GlusterCommandParameters.java @Valid ListBricks bricks Bricks.java @NotNull Guid id Thanks! I had tried adding @Valid annotation as suggested above, which had resulted in an exception from the validation engine, without any specific information about the problem, and I assumed that @Valid may not work on Lists. Digged deeper after reading your response, and found that the @Valid annotation resulted in a call to equals method on the entity, which had a bug, resulting in the exception. It all works well after fixing the bug. ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
[Engine-devel] Query regarding ValidationUtils#validateInputs
Hi, This is regarding the following validation method we have in ValidationUtils: /public static T extends VdcActionParametersBase ArrayListString validateInputs(ListClass? validationGroupList, T parameters);/ I there any particular reason for supporting the validations only on objects of classes derived from VdcActionParametersBase? I guess this was done because this method is primarily intended to validate the action parameters passed to a BLL action, using the validation annotations on the parameter class. However I think this method can be useful for general use as well. e.g. I cannot add a @Valid annotation on a list or a map in a parameter class. So I need to iterate over the list/map, and validate each element inside the loop. The validation inside the loop can also utilize the above method if the restriction extends VdcActionParametersBase is removed. This will allow me to do the following in the canDoAction method: protected boolean canDoAction() { ... for(GlusterBrickEntity brick : getParameters().getGlusterVolume().getBricks()) { ListString errors = ValidationUtils.validateInputs(getValidationGroups(), brick); if(errors != null) { for(String error : errors) { addCanDoActionMessage(error); } } } ... } Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Re: [Engine-devel] Eclipse IDE setup
On Wednesday 08 February 2012 04:32 AM, Itamar Heim wrote: On 02/07/2012 04:26 PM, Shireesh Anjal wrote: Hi, I went through setting Eclipse IDE for engine development recently. After getting the maven build to work, and after importing all projects into eclipse, following setup is required to get rid of all compilation errors reported by eclipse: 1) at restapi-definition - project - properties - java build path - source - add source folder - target/generated sources/xjc 2) at webadmin - project - properties - java build path - source - add source folder- target/generated sources/annotations,gwt,test-annotations 3) To get rid of the error The method setCharacterEncoding(String) is undefined for the type HttpServletResponse in source frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/WebadminDynamicHostingServlet.java, I modified pom.xml at root level to change servlet API version from 2.3 to 2.4 as the concerned API is introduced in 2.4 javax.ejb.api.version3.0/javax.ejb.api.version -javax.servlet.api.version2.3/javax.servlet.api.version +javax.servlet.api.version2.4/javax.servlet.api.version jcraft.jsch.version0.1.42/jcraft.jsch.version 4) Make sure that you import the engine code formatter into eclipse _before_ starting development. Window - Preferences - Java - Code Style - Formatter - Import - ovirt-src-root/config/engine-code-format.xml 5) Above mentioned formatted doesn't work in comments. This can be resolved by adding Remove trailing whitespace in Save actions as follows: Window - Preferences - Java - Editor - Save Actions - Additional Actions - Configure - Code Organizing - Remove trailing whitespace -All lines 6) For some reason, editing a properties file in eclipse results in a lot of diff in git, making it difficult to review the code change. So I'm resorting to editing properties file in a text editor outside eclipse for the time being. I suspect this may not happen on all machines. Shireesh - great points - care to update the wiki? Have added the above points to an existing wiki page: http://www.ovirt.org/wiki/Building_Ovirt_Engine/IDE Also - I'd like to hope we can clean the pom files so #1-#3 would not be needed manually? -- Regards, Shireesh ___ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
[Engine-devel] Fwd: Re: Moving to Jboss AS7
Hi Oved, After rebasing our POC code with upstream, I tried to follow the below steps for moving to JBoss AS7. Since a newer version of JBoss (CR1) is available, I downloaded that instead of Beta1 assuming that it will be more stable and will contain mostly bug-fixes and hence should work. However it turns out that the format of standalone.xml itself has changed and that created/modified by our deploy profile doesn't work. I had to download the Beta1 tarball to get the deployment to work. This is just FYI. Please ignore in case you are already aware of this :) Thanks, Shireesh Original Message Subject:Re: [Engine-devel] Moving to Jboss AS7 Date: Sun, 15 Jan 2012 04:05:33 -0500 (EST) From: Oved Ourfalli ov...@redhat.com To: engine-devel@ovirt.org Hey all, The patches are now pushed! So please, if you fetch from now on you'll have to perform the actions below. The wiki was updated as well. Short list of steps: 1. Download jboss 7.1.0 Beta1b (wget http://download.jboss.org/jbossas/7.1/jboss-as-7.1.0.Beta1b/jboss-as-7.1.0.Beta1b.tar.gz) 2. Fetch the latest changes from our git repository 3. Change the Jboss home to the new path, both in the JBOSS_HOME environment variable, and in maven settings file (~/.m2/settings.xml) 4. Build the engine, with profiles dep and setup. This will put all the proper configuration files, postgresql driver and make all the other needed changes in Jboss in order to make it work properly mvn clean install -Psetup,dep ... 5. In order to run Jboss go to JBOSS_HOME/bin and run ./standalone.sh A more descriptive set of steps and notes can be found in my previous E-mail below. I'm here if you need any help. Thank you, Oved - Original Message - From: Oved Ourfalliov...@redhat.com To: engine-devel@ovirt.org Sent: Wednesday, January 11, 2012 2:57:19 PM Subject: Moving to Jboss AS7 Hey all, The code changes required to make the engine work on Jboss AS7 will soon be push It will, of course, require you to install it, and start working with it :-) A separate E-mail will be sent to notify you all once pushed, and then you'll have to perform the following steps: 1. Download jboss 7.1.0 Beta1b (http://download.jboss.org/jbossas/7.1/jboss-as-7.1.0.Beta1b/jboss-as-7.1.0.Beta1b.tar.gz) - There is a newer version, but it has issues in the REST-API, so we decided to work with the beta version until a proper fix will be released. 2. Fetch the latest changes from our git repository 3. Change the Jboss home to the new path, both in the JBOSS_HOME environment variable, and in maven settings file (~/.m2/settings.xml) 4. Build the engine, with profiles dep and setup. This will put all the proper configuration files, postgresql driver and make all the other needed changes in Jboss in order to make it work properly mvn clean install -Psetup,dep ... 5. In order to run Jboss go to JBOSS_HOME/bin and run ./standalone.sh 6. Look inside the JBOSS_HOME/bin/standalone.conf file in order to enable jboss debugging (just uncomment the line JAVA_OPTS=$JAVA_OPTS -Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=n) 7. If you have a krb5.conf file you are working with, put it in JBOSS_HOME/standalone/configuration directory 8. Run Jboss (and be impressed by the startup speed!) The above will be also added to the wiki page for building the engine as soon as the patches will be pushed upstream. Some facts about Jboss 7: 1. It supports both standalone deployment, and domain deployment. We use the standalone one. 2. Stuff related to the standalone mode can be found in the JBOSS_HOME/standalone directory: * configuration - contains the main configuration file called standalone.xml file, plus some other configuration files * deployments - that's where your deployments should be. When adding a new one, don't forget to create a file called deployment-name.dodeploy in order to make it deploy. * log - that's where the log files are written (unless stated differently in the standalone.xml file). 3. The different modules that come with Jboss 7 are located in JBOSS_HOME/modules directory * No more common/lib directory. * Every module has a module.xml file which contains it's dependencies in other modules, the jars that are part of the module, and etc. * In order to use a dependency from there you have to add Dependencies section to your manifest file (do git grep Dependencies to take a look at some examples done in the engine source code). 4. Useful links: * Documentation - https://docs.jboss.org/author/display/AS7/Documentation * Class loading changes - https://docs.jboss.org/author/display/AS7/Class+Loading+in+AS7 * The Jboss community - http://community.jboss.org/wiki Please send issues/feedback to this mailing list. Thank you all, Oved ___ Engine-devel mailing list Engine-devel@ovirt.org
Re: [Engine-devel] a different approach to the command classes
This is a really interesting discussion! Though I'm pretty new to oVirt, here (inline) are my two cents :) On Tuesday 17 January 2012 07:07 PM, Jon Choate wrote: - Original Message - From: Maormlipc...@redhat.com To: engine-devel@ovirt.org Sent: Tuesday, January 17, 2012 4:33:19 AM Subject: Re: [Engine-devel] a different approach to the command classes On 01/17/2012 09:24 AM, Ayal Baron wrote: - Original Message - On 17/01/12 04:58, Jon Choate wrote: The way the command classes are written has bothered me for a while. While implementing the multiple storage domain features I am presented with the opportunity to create a new command from scratch. I gave some thought to what I would like the command classes to look like while balancing that the class must still fit in with the existing structure. So here is what I came up with. I'd appreciate any feedback. The Command class encompasses only the rules of what needs to be done. It relies upon Validator classes to determine if the canDoAction conditions have been met. @Override public boolean canDoAction() { ... checkTargetDomainHasSpace(); checkTargetDomainIsValidTarget(); checkSourceDomainIsValidSource(); checkSourceAndTargetAreDifferent(); ... } ... private void checkTargetDomainHasSpace() { if(!actionAllowed) return; if(!targetDomainValidator.hasSpace(getParameters().getDiskImageId())) { addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW); actionAllowed = false; } } Each of the checks follows a similar pattern of - guard clause to see of we already failed and bail if we did - test for failure of the condition - add failure message if needed - set variable to failed if needed Storing the success flag in a variable allows us to keep the canDoAction method readable as a series of commands and to allow it to be accessed by all the private methods without them having to pass it around. The execution of the command will follow a similar pattern where the command class will only know how to describe what needs to be done and to rely on supporting objects to handle the implementation of these steps. Getting the implementation out of the command classes will allow the commands to share validation and implementation details and remove a lot of the duplication that currently exists within the commands. How do people feel about this approach? Hi Jon, The scope of your proposal is changing the implementation of the canDoAction method, I think that the title of this mail is a bit misleading. Actually I think Jon was suggesting to do the same in the command itself. Yes I am. I just haven't written that code yet! I think, using shared canDoAction validation between commands might be a good idea also, it can be seen in operations such as add/update commands. In most cases, the update validation, is already based on the add command validation, sometime it is implemented with inheritance/external class, in other cases it is just implemented with multiple code. Basically what you are suggesting is to split the canDoAction implementation into methods and then extract them from the command class to a shared class so they can be reused. In many cases we can use (are using) inheritance for reusing code, there are cases where inheritance does not do the work and we can extract to external classes. Our overuse of inheritance is one of the things I'm trying to rectify. Inheritance wasn't added to the language to facilitate reuse. It is there to represent object hierarchies. In some cases we abuse this to leverage code reuse. This is often a code smell that the code is in the wrong place to begin with. If both classes need the code and they are not logically related in an object hierarchy, the code really needs to be outside the classes. We have cases where the use of inheritance for reuse have gone too far. For instance: public class RemoveVdsSpmIdCommandT extends VdsActionParameters extends AddVdsSpmIdCommandT So this says that a RemoveVdsSmpIdCommand is a type of AddVdsSpmIdCommand? That implies that I can use a RemoveVdsSmpIdCommand anywhere that an AddVdsSpmIdCommand is expected. Otherwise we have broken one of the fundamental contracts of object oriented programming. I think such a change is welcomed but on a needed basis, I think it is overkill for the general use case and will make the code more cumbersome (if the original canDoAction was 4-5 lines long...). From what I have seen those canDoActions are in the minority. The overall goals are to increase the readability and maintainability of the code so I would suggest being pragmatic about any approach. If doing it doesn't help achieve the goal, then don't do it. One of the ideas I'm trying to convey here is that the command classes should be fairly ignorant. They should be responsible for knowing the list of