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 "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 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 ………... @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 >> >> >> >> _______________________________________________ >> 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 > > > _______________________________________________ > 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 > _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev