rackspace / gophercloud

A Go SDK for OpenStack. IN FEATURE FREEZE. See Issue #592
http://gophercloud.io
Other
456 stars 180 forks source link

Add CreateOptsBuilder for routers #526

Closed Fodoj closed 8 years ago

Fodoj commented 8 years ago

There is a "hidden" parameter - router type. Either exclusive or shared (so exclusive will get a separate VM for sure, while shared will be shared with other routers on same tenants). The only actual reference (except testing it on live system) I could find is this blog post: https://blogs.vmware.com/openstack/openstack-networking-with-vmware-nsx-part-2/ It could be that this feature is VIO only.

jtopjian commented 8 years ago

If this feature is found only in VMWare drivers/plugins/environments, I wonder if it would be best to create a vmware-specific router...

Fodoj commented 8 years ago

@jtopjian I don't think we should do that, because only underlying drive changes, not an API for Neutron itself. Unless there are already provider-specific entities inside gophercloud?..

jtopjian commented 8 years ago

The only reference to router_type that I can find is in a file specific to VMWare:

https://github.com/openstack/neutron/blob/60c1f99655fc096e6bfaf4d45e13737e18a73fc2/neutron/db/migration/alembic_migrations/nsxv_initial_opts.py

This makes me feel as if the underlying API is changing specifically for this driver. However, I could be completely wrong.

I don't think this is handled anywhere else in Gophercloud, so this might be setting precedent. My intention is definitely not to complicate or create more work, I just want to make sure this is implemented correctly so the Gophercloud Neutron interaction doesn't spin out of control.

Fodoj commented 8 years ago

@jtopjian do you think router creation could fail on none-vmware stacks because of this change?

jtopjian commented 8 years ago

I don't think so, no. I think it's a matter of cleanliness. Sure, this is just one parameter, but let's say 5 more driver-specific parameters are added down the road.

Speaking as someone not affiliated with Gophercloud, my impression has been that this project tries to be as clean and mindful of the future as possible.

Maybe a compromise would be to have a struct called DriverOps that would hold the driver-specific options, keeping the core Opts with only the native Neutron options.

This is all me thinking out loud -- I could be totally wrong about all of this :smile:

Fodoj commented 8 years ago

I actually like this idea with DriverOpts that will be merged into core opts during api request. And probably we should allow this DriverOpts to hold any number of custom properties, what do you think?

jtopjian commented 8 years ago

So you're saying to make DriverOpts a generic map[string]interface{} so that it can hold any arbitrary key/value?

That's the approach I would take. The downside of a defined structure is that these options seem to be completely undocumented. So a generic key/value would help more people use the options rather than get them merged into Gophercloud first.

Fodoj commented 8 years ago

Yes, that's what I would like to do too. Okay, then I mark this PR as WIP and ping you once I have an update.

Fodoj commented 8 years ago

@jtopjian ping

Fodoj commented 8 years ago

@jtopjian so I checked, even if DriverOps are ommited everything works well still

Fodoj commented 8 years ago

@jtopjian I resolved conflicts with master, can we get this one merged? :)

jtopjian commented 8 years ago

Did you mean @jrperritt? I don't have commit rights for Gophercloud :)

Fodoj commented 8 years ago

Yes, sorry. @jrperritt

On 16 Feb 2016 04:36 +0100, Joe Topjiannotifications@github.com, wrote:

Did you mean@jrperritt(https://github.com/jrperritt)? I don't have commit rights for Gophercloud :)

— Reply to this email directly orview it on GitHub(https://github.com/rackspace/gophercloud/pull/526#issuecomment-184500094).

Fodoj commented 8 years ago

@jrperritt any update? Can we get it merged?

jrperritt commented 8 years ago

If this feature is found only in VMWare drivers/plugins/environments, I wonder if it would be best to create a vmware-specific router...

Yes.

Unless there are already provider-specific entities inside gophercloud?..

Yes. See here: https://github.com/rackspace/gophercloud/tree/master/rackspace

Fodoj commented 8 years ago

@jrperritt(https://github.com/jrperritt)does it mean I need to rewrite PR again and remove DriverOpts?

On 23 Feb 2016 03:44 +0100, jrperrittnotifications@github.com, wrote:

If this feature is found only in VMWare drivers/plugins/environments, I wonder if it would be best to create a vmware-specific router...

Yes.

Unless there are already provider-specific entities inside gophercloud?..

Yes. See here:https://github.com/rackspace/gophercloud/tree/master/rackspace

— Reply to this email directly orview it on GitHub(https://github.com/rackspace/gophercloud/pull/526#issuecomment-187492357).

jrperritt commented 8 years ago

There are a few things going on here, one directly related to this and one indirectly:

  1. (indirectly related) Though you can add the VMware-specific directory and router within it, I'm actively working on removing the rackspace directory in the new version of gophercloud. This repo will stay as-is for legacy reasons, but future versions will only be OpenStack proper, and other OpenStack providers can import from that repo in their own repos.
  2. (directly related) I'd suggest changing the CreateOpts parameter to be a CreateOptsBuilder like the other packages have. Then force CreateOpts to implement the CreateOptsBuilder interface, putting the logic for creating the request body in the ToRouterCreateMap. Then, in your app code, pass in your own CreateOptsBuilder that has the additional request field. Similarly, after calling Create, you get back a CreateResult which has a Body field and you can extract the response body as you like (namely, into a structure that has your required new field).
VMTrooper commented 8 years ago

Hey folks. VMware environments could benefit from a generic implementation like with Heat's value_specs router attribute.

Example: acme_router: type: OS::Neutron::Router properties: external_gateway_info: { network: EXTNET } value_specs: { "router_type": "exclusive" }

That way, something specific to VMware-only isn't being added to the codebase.

If anyone wants assistance with testing the implementation, I have an environment with the VMware components. This support would be good for any environment running OpenStack on VMware. Not just VIO.

mkdev-mentor-1 commented 8 years ago

I already tested my PR on VIO ☺️ Just need to refactor it now with changes asked above

On 25 Feb 2016 16:57 +0100, Trevor Roberts Jrnotifications@github.com, wrote:

Hey folks. VMware environments could benefit from a generic implementation like with Heat's value_specs router attribute.

Example: acme_router: type: OS::Neutron::Router properties: external_gateway_info: { network: EXTNET } value_specs: { "router_type": "exclusive" }

That way, something specific to VMware-only isn't being added to the codebase.

If anyone wants assistance with testing the implementation, I have an environment with the VMware components. This support would be good for any environment running OpenStack on VMware. Not just VIO.

— Reply to this email directly orview it on GitHub(https://github.com/rackspace/gophercloud/pull/526#issuecomment-188852202).

Fodoj commented 8 years ago

@jrperritt I implemented your proposal with CreateOptsBuilder, please review again :)

coveralls commented 8 years ago

Coverage Status

Coverage increased (+4.9%) to 79.966% when pulling 8aeb1d9a30f9aef20506e2e2619bda57bac5ba8c on Fodoj:add-router-type into f3d053460f7c37970af6733bf370a3256e3648fb on rackspace:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+4.8%) to 79.934% when pulling 8aeb1d9a30f9aef20506e2e2619bda57bac5ba8c on Fodoj:add-router-type into f3d053460f7c37970af6733bf370a3256e3648fb on rackspace:master.

jrperritt commented 8 years ago

Yes, exactly that :) +2

VMTrooper commented 8 years ago

Awesome news, all! Thank you!