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

Attachment: 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

Reply via email to