Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
Thanks for the comments Mandeep! Responses in-line #PCM On May 23, 2014, at 8:57 PM, Mandeep Dhami dh...@noironetworks.com wrote: My preferences: For where, I'd go with Gary's recommendation (A) for two reasons (1) Consistency and (2) I don't think it will create any boilerplate requirements since the abstract class provides the default implementation. #PCM Actually, (A) requires a lot of additional code that doesn’t currently exist today. ABC methods would be needed for each validation. In addition, to be consistent, there should be an abstract method for the apply function, and a “pass” implementation on each of the concrete classes. Overall, it is adding a lot of code, for arguably little benefit. For naming, I'd prefer to go with ML2 terminology for two reasons (1) Again Consistency, and (2) it is then clear what actions are happening within a transaction or outside of it. With a validation function no transaction fence is implied by it's name - but for any validation that depends on what currently exists in the database, these transaction semantics are important. #PCM I’m still struggling with the naming for question #2. I must admit, I like the naming of validate/apply because it makes it clear as to what is happening with the calls. With pre-commit/post-commit, it doesn’t indicate what the functions are actually doing, and in one case (for the Cisco VPN service driver) the post-commit will actually be doing a database commit in addition to applying the changes to the device driver (so both the postcommt and apply naming don’t clearly indicate the actions - an alternative would be to call the driver for the commit phase too, but this is a rare case). To muddy it up a bit more, the main functions have no indication in the name that they deal with persisting to the database. The hierarchy currently is (using create_vpnservice... class VPNPluginBase(service_base.ServicePluginBase): # ABC with Northbound API definition @abc.abstractmethod def create_vpnservice(self, context, vpnservice): pass class VPNPluginDb(vpnaas.VPNPluginBase, base_db.CommonDbMixin): # DBase concrete class def create_vpnservice(self, context, vpnservice): vpns = vpnservice['vpnservice'] tenant_id = self._get_tenant_id_for_create(context, vpns) with context.session.begin(subtransactions=True): … class VPNPlugin(vpn_db.VPNPluginDb): … class VPNDriverPlugin(VPNPlugin, vpn_db.VPNPluginRpcDbMixin): # Plugin child class def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.create_vpnservice(context, vpnservice) class VpnDriver(object): # ABC for service driver # Nothing for create_vpnservice() class IPsecVPNDriver(service_drivers.VpnDriver): # Service driver class # Nothing for create_vpnservice() In the proposals, we talking about a sequence of this (2X) at the plugin: def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) driver.create_vpnservice_precommit(vpnservice) super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.create_vpnservice_postcommit(context, vpnservice) versus this (2Y)… def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) driver.validate_create_vpnservice(vpnservice) super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.apply_create_vpnservice(context, vpnservice) The former makes is clearer that there is a database operation in-between (implied by the naming). The latter makes it clearer what the service driver is doing before and after. In both cases, the ABC for the service driver (VpnDriver) could have the default validation/precommit actions, and the service driver (IPsecVPNDriver) could optionally have any provider validation/precommit and would have the mandatory apply/post-commit actions. Maybe I could do a WIP patch out for review to give something for concrete commenting… Regards, PCM Regards, Mandeep On Fri, May 23, 2014 at 4:25 PM, Paul Michali (pcm) p...@cisco.com wrote: Thanks for the comment Carl. See @PCM inline PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 On May 23, 2014, at 6:09 PM, Carl Baldwin c...@ecbaldwin.net wrote: Paul, On Fri, May 23, 2014 at 8:24 AM, Paul Michali (pcm) p...@cisco.com wrote: Hi, I’m working on a task for a BP to separate validation from persistence logic in L3 services code (VPN currently), so that providers can override/extend the validation logic (before persistence). So I’ve separated
Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
So I hit some more complexity, when I got to the IPSec connection resource’s
update API…
I was doing code like this:
In VPN plugin:
def create_ipsec_site_connection(self, context, ipsec_site_connection):
driver = self._get_driver_for_ipsec_site_connection(
context, ipsec_site_connection)
driver.validate_create_ipsec_site_connection(context,
ipsec_site_connection)
ipsec_site_connection = super(
VPNDriverPlugin, self).create_ipsec_site_connection(
context, ipsec_site_connection)
driver.apply_create_ipsec_site_connection(context,
ipsec_site_connection)
return ipsec_site_connection
In ABC for service drvier:
def validate_create_ipsec_site_connection(self, context,
ipsec_site_connection):
ipsec_sitecon = ipsec_site_connection['ipsec_site_connection']
dpd = ipsec_sitecon.get('dpd', {})
ipsec_sitecon['dpd_action'] = dpd.get('action', 'hold')
ipsec_sitecon['dpd_interval'] = dpd.get('interval', 30)
ipsec_sitecon['dpd_timeout'] = dpd.get('timeout', 120)
self._check_dpd(ipsec_sitecon)
self._check_mtu(context,
ipsec_sitecon['mtu'],
ipsec_sitecon['vpnservice_id'])
However, when considering the update_ipsec_site_connection() API, I realized
that there could be multiple requests coming in to change the same connection’s
attributes. In this case, validating and then persisting has the issue of
incompatible changes being committed (e.g. one client changes DPD interval, and
another client changes DPD timeout, such that they fail the _check_mtu() call).
I’m wondering what the thoughts are, regarding how to handle this. One way, I
guess, may be do call the service driver validation from within the database
logic, like this:
In VPN plugin
def create_ipsec_site_connection(self, context, ipsec_site_connection):
driver = self._get_driver_for_ipsec_site_connection(
context, ipsec_site_connection)
ipsec_site_connection = super(
VPNDriverPlugin, self).create_ipsec_site_connection(
context, ipsec_site_connection)
driver.apply_create_ipsec_site_connection(context,
ipsec_site_connection)
return ipsec_site_connection
In Database (pseudo-code)
def create_ipsec_site_connection(self, context, ipsec_site_connection):
ipsec_sitecon = ipsec_site_connection['ipsec_site_connection']
tenant_id = self._get_tenant_id_for_create(context, ipsec_sitecon)
with context.session.begin(subtransactions=True):
# I guess this would actually call the child class method to
# get the driver?
driver = self._get_driver_for_ipsec_site_connection(
context, ipsec_sitecon)
driver.validate_create_ipsec_site_connection(context, ipsec_sitecon)
The database method is a base class of the VPN plugin class, so I’m thinking,
although awkward, it could work. Can’t say I’m comfortable with the call flow
here. Probably could add an abstract method for
_get_driver_for_ipsec_site_connection(), so that it cannot be called with an
instance of the database class.
I’m not seeing a really clean solution here.
Any thoughts?
PCM (Paul Michali)
MAIL …..…. p...@cisco.com
IRC ……..… pcm_ (irc.freenode.com)
TW ………... @pmichali
GPG Key … 4525ECC253E31A83
Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83
On May 27, 2014, at 7:37 AM, Paul Michali (pcm) p...@cisco.com wrote:
Thanks for the comments Mandeep! Responses in-line #PCM
On May 23, 2014, at 8:57 PM, Mandeep Dhami dh...@noironetworks.com wrote:
My preferences:
For where, I'd go with Gary's recommendation (A) for two reasons (1)
Consistency and (2) I don't think it will create any boilerplate
requirements since the abstract class provides the default implementation.
#PCM Actually, (A) requires a lot of additional code that doesn’t currently
exist today. ABC methods would be needed for each validation. In addition, to
be consistent, there should be an abstract method for the apply function, and
a “pass” implementation on each of the concrete classes. Overall, it is
adding a lot of code, for arguably little benefit.
For naming, I'd prefer to go with ML2 terminology for two reasons (1) Again
Consistency, and (2) it is then clear what actions are happening within a
transaction or outside of it. With a validation function no transaction
fence is implied by it's name - but for any validation that depends on what
currently exists in the database, these transaction semantics are important.
#PCM I’m still struggling with the naming for question #2. I must admit, I
like the naming of validate/apply because
Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
Paul, do you have enough to be able to put up a WIP in gerrit? This
discussion is getting to the point where having a discussion in gerrit
will be beneficial.
Carl
On Tue, May 27, 2014 at 12:37 PM, Paul Michali (pcm) p...@cisco.com wrote:
So I hit some more complexity, when I got to the IPSec connection resource’s
update API…
I was doing code like this:
In VPN plugin:
def create_ipsec_site_connection(self, context, ipsec_site_connection):
driver = self._get_driver_for_ipsec_site_connection(
context, ipsec_site_connection)
driver.validate_create_ipsec_site_connection(context,
ipsec_site_connection)
ipsec_site_connection = super(
VPNDriverPlugin, self).create_ipsec_site_connection(
context, ipsec_site_connection)
driver.apply_create_ipsec_site_connection(context,
ipsec_site_connection)
return ipsec_site_connection
In ABC for service drvier:
def validate_create_ipsec_site_connection(self, context,
ipsec_site_connection):
ipsec_sitecon = ipsec_site_connection['ipsec_site_connection']
dpd = ipsec_sitecon.get('dpd', {})
ipsec_sitecon['dpd_action'] = dpd.get('action', 'hold')
ipsec_sitecon['dpd_interval'] = dpd.get('interval', 30)
ipsec_sitecon['dpd_timeout'] = dpd.get('timeout', 120)
self._check_dpd(ipsec_sitecon)
self._check_mtu(context,
ipsec_sitecon['mtu'],
ipsec_sitecon['vpnservice_id'])
However, when considering the update_ipsec_site_connection() API, I realized
that there could be multiple requests coming in to change the same
connection’s attributes. In this case, validating and then persisting has
the issue of incompatible changes being committed (e.g. one client changes
DPD interval, and another client changes DPD timeout, such that they fail
the _check_mtu() call).
I’m wondering what the thoughts are, regarding how to handle this. One way,
I guess, may be do call the service driver validation from within the
database logic, like this:
In VPN plugin
def create_ipsec_site_connection(self, context, ipsec_site_connection):
driver = self._get_driver_for_ipsec_site_connection(
context, ipsec_site_connection)
ipsec_site_connection = super(
VPNDriverPlugin, self).create_ipsec_site_connection(
context, ipsec_site_connection)
driver.apply_create_ipsec_site_connection(context,
ipsec_site_connection)
return ipsec_site_connection
In Database (pseudo-code)
def create_ipsec_site_connection(self, context, ipsec_site_connection):
ipsec_sitecon = ipsec_site_connection['ipsec_site_connection']
tenant_id = self._get_tenant_id_for_create(context, ipsec_sitecon)
with context.session.begin(subtransactions=True):
# I guess this would actually call the child class method to
# get the driver?
driver = self._get_driver_for_ipsec_site_connection(
context, ipsec_sitecon)
driver.validate_create_ipsec_site_connection(context,
ipsec_sitecon)
The database method is a base class of the VPN plugin class, so I’m
thinking, although awkward, it could work. Can’t say I’m comfortable with
the call flow here. Probably could add an abstract method for
_get_driver_for_ipsec_site_connection(), so that it cannot be called with an
instance of the database class.
I’m not seeing a really clean solution here.
Any thoughts?
PCM (Paul Michali)
MAIL …..…. p...@cisco.com
IRC ……..… pcm_ (irc.freenode.com)
TW ………... @pmichali
GPG Key … 4525ECC253E31A83
Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83
On May 27, 2014, at 7:37 AM, Paul Michali (pcm) p...@cisco.com wrote:
Thanks for the comments Mandeep! Responses in-line #PCM
On May 23, 2014, at 8:57 PM, Mandeep Dhami dh...@noironetworks.com wrote:
My preferences:
For where, I'd go with Gary's recommendation (A) for two reasons (1)
Consistency and (2) I don't think it will create any boilerplate
requirements since the abstract class provides the default implementation.
#PCM Actually, (A) requires a lot of additional code that doesn’t currently
exist today. ABC methods would be needed for each validation. In addition,
to be consistent, there should be an abstract method for the apply function,
and a “pass” implementation on each of the concrete classes. Overall, it is
adding a lot of code, for arguably little benefit.
For naming, I'd prefer to go with ML2 terminology for two reasons (1) Again
Consistency, and (2) it is then clear what actions are happening within a
transaction or outside of it. With a validation function no
[openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
Hi, I’m working on a task for a BP to separate validation from persistence logic in L3 services code (VPN currently), so that providers can override/extend the validation logic (before persistence). So I’ve separated the code for one of the create APIs, placed the default validation into an ABC class (as a non-abstract method) that the service drivers inherit from, and modified the plugin to invoke the validation function in the service driver, before doing the persistence step. The flow goes like this… def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) driver.validate_create_vpnservice(context, vpnservice) super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.apply_create_vpnservice(context, vpnservice) If the service driver has a validation routine, it’ll be invoked, otherwise, the default method in the ABC for the service driver will be called and will handle the “baseline” validation. I also renamed the service driver method that is used for applying the changes to the device driver as apply_* instead of using the same name as is used for persistence (e.g. create_vpnservice - apply_create_vpnservice). The questions I have is… 1) Should I create new validation methods A) for every create (and update?) API (regardless of whether they currently have any validation logic, B) for resources that have some validation logic already, or C) only for resources where there are providers with different validation needs? I was thinking (B), but would like to hear peoples’ thoughts. 2) I’ve added validation_* and modified the other service driver call to apply_*. Should I instead, use the ML2 terminology of pre commit_* and post commit_*? I personally favor the former, as it is more descriptive of what is happening in the methods, but I understand the desire for consistency with other code. 3) Should I create validation methods for code, where defaults are being set for missing (optional) information? For example, VPN IKE Policy lifetime being set to units=seconds, value=3600, if not set. Currently, provider implementations have same defaults, but could potentially use different defaults. The alternative is to leave this in the persistence code and not allow it to be changed. This could be deferred, if 1C is chosen above. Looking forward to your thoughts... Thanks! PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 signature.asc Description: Message signed with OpenPGP using GPGMail ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
Hi, Paul, If the backend driver maintains its own database, I think the pre_commit and post_commit approach has an advantage. The typical code flow is able to keep the driver and plugin database consistent. Regarding question 1, where validation methods should be added, I am leaning towards A, but I also agree validation hooks can be added later when they are needed. It's more important to get provider and flavor logic officially supported for services. Thanks, Gary On Fri, May 23, 2014 at 7:24 AM, Paul Michali (pcm) p...@cisco.com wrote: Hi, I’m working on a task for a BP to separate validation from persistence logic in L3 services code (VPN currently), so that providers can override/extend the validation logic (before persistence). So I’ve separated the code for one of the create APIs, placed the default validation into an ABC class (as a non-abstract method) that the service drivers inherit from, and modified the plugin to invoke the validation function in the service driver, before doing the persistence step. The flow goes like this… def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) *driver.validate_create_vpnservice(context, vpnservice)* super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.apply_create_vpnservice(context, vpnservice) If the service driver has a validation routine, it’ll be invoked, otherwise, the default method in the ABC for the service driver will be called and will handle the “baseline” validation. I also renamed the service driver method that is used for applying the changes to the device driver as apply_* instead of using the same name as is used for persistence (e.g. create_vpnservice - apply_create_vpnservice). The questions I have is… 1) Should I create new validation methods A) for every create (and update?) API (regardless of whether they currently have any validation logic, B) for resources that have some validation logic already, or C) only for resources where there are providers with different validation needs? I was thinking (B), but would like to hear peoples’ thoughts. 2) I’ve added validation_* and modified the other service driver call to apply_*. Should I instead, use the ML2 terminology of pre commit_* and post commit_*? I personally favor the former, as it is more descriptive of what is happening in the methods, but I understand the desire for consistency with other code. 3) Should I create validation methods for code, where defaults are being set for missing (optional) information? For example, VPN IKE Policy lifetime being set to units=seconds, value=3600, if not set. Currently, provider implementations have same defaults, but *could* potentially use different defaults. The alternative is to leave this in the persistence code and not allow it to be changed. This could be deferred, if 1C is chosen above. Looking forward to your thoughts... Thanks! PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
Thanks for the comments Gary! Typically, the device driver (backend) and service driver, for a provider won’t have any database requirements (at least for VPN). For the Cisco VPN, the service driver has one additional table that it maintains for mapping, but even in that case, there is no modification to the built in tables for the VPN plugin. So, the action is validation, persist, apply, with the validation possibly having a provider override/extend, the apply always having a provider action, and the persistence always being the “core” persistence. It’s a question of being validate, persist/commit, apply, or pre-commit, commit/persist, post-commit, for naming. Regards, PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 On May 23, 2014, at 12:40 PM, Gary Duan garyd...@gmail.com wrote: Hi, Paul, If the backend driver maintains its own database, I think the pre_commit and post_commit approach has an advantage. The typical code flow is able to keep the driver and plugin database consistent. Regarding question 1, where validation methods should be added, I am leaning towards A, but I also agree validation hooks can be added later when they are needed. It's more important to get provider and flavor logic officially supported for services. Thanks, Gary On Fri, May 23, 2014 at 7:24 AM, Paul Michali (pcm) p...@cisco.com wrote: Hi, I’m working on a task for a BP to separate validation from persistence logic in L3 services code (VPN currently), so that providers can override/extend the validation logic (before persistence). So I’ve separated the code for one of the create APIs, placed the default validation into an ABC class (as a non-abstract method) that the service drivers inherit from, and modified the plugin to invoke the validation function in the service driver, before doing the persistence step. The flow goes like this… def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) driver.validate_create_vpnservice(context, vpnservice) super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.apply_create_vpnservice(context, vpnservice) If the service driver has a validation routine, it’ll be invoked, otherwise, the default method in the ABC for the service driver will be called and will handle the “baseline” validation. I also renamed the service driver method that is used for applying the changes to the device driver as apply_* instead of using the same name as is used for persistence (e.g. create_vpnservice - apply_create_vpnservice). The questions I have is… 1) Should I create new validation methods A) for every create (and update?) API (regardless of whether they currently have any validation logic, B) for resources that have some validation logic already, or C) only for resources where there are providers with different validation needs? I was thinking (B), but would like to hear peoples’ thoughts. 2) I’ve added validation_* and modified the other service driver call to apply_*. Should I instead, use the ML2 terminology of pre commit_* and post commit_*? I personally favor the former, as it is more descriptive of what is happening in the methods, but I understand the desire for consistency with other code. 3) Should I create validation methods for code, where defaults are being set for missing (optional) information? For example, VPN IKE Policy lifetime being set to units=seconds, value=3600, if not set. Currently, provider implementations have same defaults, but could potentially use different defaults. The alternative is to leave this in the persistence code and not allow it to be changed. This could be deferred, if 1C is chosen above. Looking forward to your thoughts... Thanks! PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev signature.asc Description: Message signed with OpenPGP using GPGMail ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
On 5/23/14, 3:07 PM, Paul Michali (pcm) wrote: Thanks for the comments Gary! Typically, the device driver (backend) and service driver, for a provider won't have any database requirements (at least for VPN). For the Cisco VPN, the service driver has one additional table that it maintains for mapping, but even in that case, there is no modification to the built in tables for the VPN plugin. If these sorts of additional tables might use foreign keys, cascaded deletes, etc., it probably make sense to go with the precommit approach to make these explicitly part of the transactions. -Bob So, the action is validation, persist, apply, with the validation possibly having a provider override/extend, the apply always having a provider action, and the persistence always being the core persistence. It's a question of being validate, persist/commit, apply, or pre-commit, commit/persist, post-commit, for naming. Regards, PCM (Paul Michali) MAIL . p...@cisco.com mailto:p...@cisco.com IRC ... pcm_ (irc.freenode.com http://irc.freenode.com) TW @pmichali GPG Key ... 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 On May 23, 2014, at 12:40 PM, Gary Duan garyd...@gmail.com mailto:garyd...@gmail.com wrote: Hi, Paul, If the backend driver maintains its own database, I think the pre_commit and post_commit approach has an advantage. The typical code flow is able to keep the driver and plugin database consistent. Regarding question 1, where validation methods should be added, I am leaning towards A, but I also agree validation hooks can be added later when they are needed. It's more important to get provider and flavor logic officially supported for services. Thanks, Gary On Fri, May 23, 2014 at 7:24 AM, Paul Michali (pcm) p...@cisco.com mailto:p...@cisco.com wrote: Hi, I'm working on a task for a BP to separate validation from persistence logic in L3 services code (VPN currently), so that providers can override/extend the validation logic (before persistence). So I've separated the code for one of the create APIs, placed the default validation into an ABC class (as a non-abstract method) that the service drivers inherit from, and modified the plugin to invoke the validation function in the service driver, before doing the persistence step. The flow goes like this... def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) *driver.validate_create_vpnservice(context, vpnservice)* super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.apply_create_vpnservice(context, vpnservice) If the service driver has a validation routine, it'll be invoked, otherwise, the default method in the ABC for the service driver will be called and will handle the baseline validation. I also renamed the service driver method that is used for applying the changes to the device driver as apply_* instead of using the same name as is used for persistence (e.g. create_vpnservice - apply_create_vpnservice). The questions I have is... 1) Should I create new validation methods A) for every create (and update?) API (regardless of whether they currently have any validation logic, B) for resources that have some validation logic already, or C) only for resources where there are providers with different validation needs? I was thinking (B), but would like to hear peoples' thoughts. 2) I've added validation_* and modified the other service driver call to apply_*. Should I instead, use the ML2 terminology of pre commit_* and post commit_*? I personally favor the former, as it is more descriptive of what is happening in the methods, but I understand the desire for consistency with other code. 3) Should I create validation methods for code, where defaults are being set for missing (optional) information? For example, VPN IKE Policy lifetime being set to units=seconds, value=3600, if not set. Currently, provider implementations have same defaults, but *could* potentially use different defaults. The alternative is to leave this in the persistence code and not allow it to be changed. This could be deferred, if 1C is chosen above. Looking forward to your thoughts... Thanks! PCM (Paul Michali) MAIL . p...@cisco.com mailto:p...@cisco.com IRC ... pcm_ (irc.freenode.com http://irc.freenode.com/) TW @pmichali GPG Key ... 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org mailto:OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
On May 23, 2014, at 3:35 PM, Robert Kukura kuk...@noironetworks.com wrote: On 5/23/14, 3:07 PM, Paul Michali (pcm) wrote: Thanks for the comments Gary! Typically, the device driver (backend) and service driver, for a provider won’t have any database requirements (at least for VPN). For the Cisco VPN, the service driver has one additional table that it maintains for mapping, but even in that case, there is no modification to the built in tables for the VPN plugin. If these sorts of additional tables might use foreign keys, cascaded deletes, etc., it probably make sense to go with the precommit approach to make these explicitly part of the transactions. Hi Bob, In the Cisco case, it is a single table that has a key that is the IPSec site-to-site connection UUID, and then two numerical ID fields that are used to map OpenStack IPSec and IKE policy IDs to Cisco CSR ID numbers (they don’t support UUIDs). It’s the only table I’ve come across so far, so, not a common thing. PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 -Bob So, the action is validation, persist, apply, with the validation possibly having a provider override/extend, the apply always having a provider action, and the persistence always being the “core” persistence. It’s a question of being validate, persist/commit, apply, or pre-commit, commit/persist, post-commit, for naming. Regards, PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 On May 23, 2014, at 12:40 PM, Gary Duan garyd...@gmail.com wrote: Hi, Paul, If the backend driver maintains its own database, I think the pre_commit and post_commit approach has an advantage. The typical code flow is able to keep the driver and plugin database consistent. Regarding question 1, where validation methods should be added, I am leaning towards A, but I also agree validation hooks can be added later when they are needed. It's more important to get provider and flavor logic officially supported for services. Thanks, Gary On Fri, May 23, 2014 at 7:24 AM, Paul Michali (pcm) p...@cisco.com wrote: Hi, I’m working on a task for a BP to separate validation from persistence logic in L3 services code (VPN currently), so that providers can override/extend the validation logic (before persistence). So I’ve separated the code for one of the create APIs, placed the default validation into an ABC class (as a non-abstract method) that the service drivers inherit from, and modified the plugin to invoke the validation function in the service driver, before doing the persistence step. The flow goes like this… def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) driver.validate_create_vpnservice(context, vpnservice) super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.apply_create_vpnservice(context, vpnservice) If the service driver has a validation routine, it’ll be invoked, otherwise, the default method in the ABC for the service driver will be called and will handle the “baseline” validation. I also renamed the service driver method that is used for applying the changes to the device driver as apply_* instead of using the same name as is used for persistence (e.g. create_vpnservice - apply_create_vpnservice). The questions I have is… 1) Should I create new validation methods A) for every create (and update?) API (regardless of whether they currently have any validation logic, B) for resources that have some validation logic already, or C) only for resources where there are providers with different validation needs? I was thinking (B), but would like to hear peoples’ thoughts. 2) I’ve added validation_* and modified the other service driver call to apply_*. Should I instead, use the ML2 terminology of pre commit_* and post commit_*? I personally favor the former, as it is more descriptive of what is happening in the methods, but I understand the desire for consistency with other code. 3) Should I create validation methods for code, where defaults are being set for missing (optional) information? For example, VPN IKE Policy lifetime being set to units=seconds, value=3600, if not set. Currently, provider implementations have same defaults, but could potentially use different defaults. The alternative is to leave this in the persistence code and not allow it to be changed. This could be deferred, if 1C is chosen above. Looking forward to your thoughts... Thanks! PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW
Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
Paul, On Fri, May 23, 2014 at 8:24 AM, Paul Michali (pcm) p...@cisco.com wrote: Hi, I’m working on a task for a BP to separate validation from persistence logic in L3 services code (VPN currently), so that providers can override/extend the validation logic (before persistence). So I’ve separated the code for one of the create APIs, placed the default validation into an ABC class (as a non-abstract method) that the service drivers inherit from, and modified the plugin to invoke the validation function in the service driver, before doing the persistence step. The flow goes like this… def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) driver.validate_create_vpnservice(context, vpnservice) super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.apply_create_vpnservice(context, vpnservice) If the service driver has a validation routine, it’ll be invoked, otherwise, the default method in the ABC for the service driver will be called and will handle the “baseline” validation. I also renamed the service driver method that is used for applying the changes to the device driver as apply_* instead of using the same name as is used for persistence (e.g. create_vpnservice - apply_create_vpnservice). The questions I have is… 1) Should I create new validation methods A) for every create (and update?) API (regardless of whether they currently have any validation logic, B) for resources that have some validation logic already, or C) only for resources where there are providers with different validation needs? I was thinking (B), but would like to hear peoples’ thoughts. I think B. C may leave a little too much inconsistency. A feels like extra boiler-plate. Would there be any benefit to creating a higher level abstraction for the create/update API calls? I'm not suggesting you do so but if you did then you could add a validation method to that interface with a default pass. Otherwise, I'd stick with B until there is a need for more. 2) I’ve added validation_* and modified the other service driver call to apply_*. Should I instead, use the ML2 terminology of pre commit_* and post commit_*? I personally favor the former, as it is more descriptive of what is happening in the methods, but I understand the desire for consistency with other code. I'm on the fence. ML2 is not where I'm most familiar and I don't know the history behind that design. Without considering ML2 and consistency, I think I like your terminology better. 3) Should I create validation methods for code, where defaults are being set for missing (optional) information? For example, VPN IKE Policy lifetime being set to units=seconds, value=3600, if not set. Currently, provider implementations have same defaults, but could potentially use different defaults. The alternative is to leave this in the persistence code and not allow it to be changed. This could be deferred, if 1C is chosen above. I'm tempted to say punt on this until there is a need for it. Carl Looking forward to your thoughts... Thanks! PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
Thanks for the comment Carl. See @PCM inline PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 On May 23, 2014, at 6:09 PM, Carl Baldwin c...@ecbaldwin.net wrote: Paul, On Fri, May 23, 2014 at 8:24 AM, Paul Michali (pcm) p...@cisco.com wrote: Hi, I’m working on a task for a BP to separate validation from persistence logic in L3 services code (VPN currently), so that providers can override/extend the validation logic (before persistence). So I’ve separated the code for one of the create APIs, placed the default validation into an ABC class (as a non-abstract method) that the service drivers inherit from, and modified the plugin to invoke the validation function in the service driver, before doing the persistence step. The flow goes like this… def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) driver.validate_create_vpnservice(context, vpnservice) super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.apply_create_vpnservice(context, vpnservice) If the service driver has a validation routine, it’ll be invoked, otherwise, the default method in the ABC for the service driver will be called and will handle the “baseline” validation. I also renamed the service driver method that is used for applying the changes to the device driver as apply_* instead of using the same name as is used for persistence (e.g. create_vpnservice - apply_create_vpnservice). The questions I have is… 1) Should I create new validation methods A) for every create (and update?) API (regardless of whether they currently have any validation logic, B) for resources that have some validation logic already, or C) only for resources where there are providers with different validation needs? I was thinking (B), but would like to hear peoples’ thoughts. I think B. C may leave a little too much inconsistency. A feels like extra boiler-plate. Would there be any benefit to creating a higher level abstraction for the create/update API calls? I'm not suggesting you do so but if you did then you could add a validation method to that interface with a default pass. Otherwise, I'd stick with B until there is a need for more. @PCM Yeah, there is somewhat of an abstraction. In VPN, for the service drivers, they all inherit from an ABC. For the apply, there are already abstract methods in the ABC, so that the service driver is forced to provide a function (of course it is inconsistent currently - they are there for VPN services, but not for IPSec connections, which are provided in the service driver). For the validation, I was creating methods in the ABC, with implementations, but not abstract, so that the service driver could provide a function, if it wanted to extend or override (and call super, if desired), or leave it out and the base class function would be used. So, I guess it’s at two points, the validate and apply methods, where we should probably be consistent with providing base class methods for all that present in the service driver. 2) I’ve added validation_* and modified the other service driver call to apply_*. Should I instead, use the ML2 terminology of pre commit_* and post commit_*? I personally favor the former, as it is more descriptive of what is happening in the methods, but I understand the desire for consistency with other code. I'm on the fence. ML2 is not where I'm most familiar and I don't know the history behind that design. Without considering ML2 and consistency, I think I like your terminology better. @PCM I’m in the same camp. I don’t know ML2 well, and I like the explicit terminology. It *seems* like ML2 is dealing with this all at the same level of abstraction (pre-commit, commit, post-commit all for the same plugin or driver), whereas with L3 services, it is at two levels (validate at service driver and then plugin, commit at plugin, apply at service driver only). I guess I can leave it and see what happens at review…I have some asbestos here somewhere. 3) Should I create validation methods for code, where defaults are being set for missing (optional) information? For example, VPN IKE Policy lifetime being set to units=seconds, value=3600, if not set. Currently, provider implementations have same defaults, but could potentially use different defaults. The alternative is to leave this in the persistence code and not allow it to be changed. This could be deferred, if 1C is chosen above. I'm tempted to say punt on this until there is a need for it. @PCM I think you’re right. It may turn out to be YAGNI. Thanks! PCM Carl Looking forward to your thoughts... Thanks! PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………...
Re: [openstack-dev] [Neutron] Seeking opinions on scope of code refactoring...
My preferences: For where, I'd go with Gary's recommendation (A) for two reasons (1) Consistency and (2) I don't think it will create any boilerplate requirements since the abstract class provides the default implementation. For naming, I'd prefer to go with ML2 terminology for two reasons (1) Again Consistency, and (2) it is then clear what actions are happening within a transaction or outside of it. With a validation function no transaction fence is implied by it's name - but for any validation that depends on what currently exists in the database, these transaction semantics are important. Regards, Mandeep On Fri, May 23, 2014 at 4:25 PM, Paul Michali (pcm) p...@cisco.com wrote: Thanks for the comment Carl. See @PCM inline PCM (Paul Michali) MAIL …..…. p...@cisco.com IRC ……..… pcm_ (irc.freenode.com) TW ………... @pmichali GPG Key … 4525ECC253E31A83 Fingerprint .. 307A 96BB 1A4C D2C7 931D 8D2D 4525 ECC2 53E3 1A83 On May 23, 2014, at 6:09 PM, Carl Baldwin c...@ecbaldwin.net wrote: Paul, On Fri, May 23, 2014 at 8:24 AM, Paul Michali (pcm) p...@cisco.com wrote: Hi, I’m working on a task for a BP to separate validation from persistence logic in L3 services code (VPN currently), so that providers can override/extend the validation logic (before persistence). So I’ve separated the code for one of the create APIs, placed the default validation into an ABC class (as a non-abstract method) that the service drivers inherit from, and modified the plugin to invoke the validation function in the service driver, before doing the persistence step. The flow goes like this… def create_vpnservice(self, context, vpnservice): driver = self._get_driver_for_vpnservice(vpnservice) driver.validate_create_vpnservice(context, vpnservice) super(VPNDriverPlugin, self).create_vpnservice(context, vpnservice) driver.apply_create_vpnservice(context, vpnservice) If the service driver has a validation routine, it’ll be invoked, otherwise, the default method in the ABC for the service driver will be called and will handle the “baseline” validation. I also renamed the service driver method that is used for applying the changes to the device driver as apply_* instead of using the same name as is used for persistence (e.g. create_vpnservice - apply_create_vpnservice). The questions I have is… 1) Should I create new validation methods A) for every create (and update?) API (regardless of whether they currently have any validation logic, B) for resources that have some validation logic already, or C) only for resources where there are providers with different validation needs? I was thinking (B), but would like to hear peoples’ thoughts. I think B. C may leave a little too much inconsistency. A feels like extra boiler-plate. Would there be any benefit to creating a higher level abstraction for the create/update API calls? I'm not suggesting you do so but if you did then you could add a validation method to that interface with a default pass. Otherwise, I'd stick with B until there is a need for more. @PCM Yeah, there is somewhat of an abstraction. In VPN, for the service drivers, they all inherit from an ABC. For the apply, there are already abstract methods in the ABC, so that the service driver is forced to provide a function (of course it is inconsistent currently - they are there for VPN services, but not for IPSec connections, which are provided in the service driver). For the validation, I was creating methods in the ABC, with implementations, but not abstract, so that the service driver could provide a function, if it wanted to extend or override (and call super, if desired), or leave it out and the base class function would be used. So, I guess it’s at two points, the validate and apply methods, where we should probably be consistent with providing base class methods for all that present in the service driver. 2) I’ve added validation_* and modified the other service driver call to apply_*. Should I instead, use the ML2 terminology of pre commit_* and post commit_*? I personally favor the former, as it is more descriptive of what is happening in the methods, but I understand the desire for consistency with other code. I'm on the fence. ML2 is not where I'm most familiar and I don't know the history behind that design. Without considering ML2 and consistency, I think I like your terminology better. @PCM I’m in the same camp. I don’t know ML2 well, and I like the explicit terminology. It *seems* like ML2 is dealing with this all at the same level of abstraction (pre-commit, commit, post-commit all for the same plugin or driver), whereas with L3 services, it is at two levels (validate at service driver and then plugin, commit at plugin, apply at service driver only). I guess I can leave it and see what happens at review…I have some asbestos here somewhere. 3) Should I create validation methods
