Open Stack

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) <pcm at 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 …..…. pcm at 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) <pcm at cisco.com> wrote:
>
> Thanks for the comments Mandeep!  Responses in-line #PCM
>
> On May 23, 2014, at 8:57 PM, Mandeep Dhami <dhami at 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) <pcm at cisco.com> wrote:
>>
>> Thanks for the comment Carl. See @PCM inline
>>
>>
>> PCM (Paul Michali)
>>
>> MAIL …..…. pcm at 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 <carl at ecbaldwin.net> wrote:
>>
>> Paul,
>>
>> On Fri, May 23, 2014 at 8:24 AM, Paul Michali (pcm) <pcm at 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 …..…. pcm at 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 at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>