OpenStack LBaaS v2 Support #7012

dkalleg · 2016-06-04T02:23:12Z

Gives support for LBaaS V2. Relies on a recent PR that's been merged to GopherCloud master: rackspace/gophercloud#575

dkalleg · 2016-06-04T02:27:39Z

Can anyone give some guidance on how to bump the gophercloud version up to 67139b9485d6fd682c5314e963b0915e18f7947a ? Once that is done the integration tests should be able to build and run.

jtopjian · 2016-06-04T04:21:05Z

@dkalleg This is so awesome - thank you!

I did a quick read through the code and it looks great. There's some nits with regard to the embedded formatting in the tests, but that always happens and wouldn't block a merge. I think this should be able to get merged in time for 0.7.

With regard to the gophercloud version, Terraform just switched from godep to govender. This diff shows the difference between the two. If you'd like to give it a shot, go for it, and we can get someone to approve it. Or, if you'd like, I can try it out in a separate PR.

jtopjian · 2016-06-04T04:24:34Z

builtin/providers/openstack/provider.go

@@ -105,6 +105,11 @@ func Provider() terraform.ResourceProvider {
 			"openstack_lb_monitor_v1":                  resourceLBMonitorV1(),
 			"openstack_lb_pool_v1":                     resourceLBPoolV1(),
 			"openstack_lb_vip_v1":                      resourceLBVipV1(),
+			"openstack_lbaas_loadbalancer_v2":          resourceLoadBalancerV2(),


I get that LBaaS v1 used the /lb/ URL and LBaaS v2 uses the lbaas URL, so I understand why you chose to use lbaas in the name... but I wonder if the resources should keep lb and LB in their names for consistency... Thoughts?

I have no strong feeling either way. Unless someone else chimes in before Monday, I'll push an update for this.

dkalleg · 2016-06-04T23:02:59Z

Not sure what the conflict github is reporting is. Side effect of changing a vendor project? Thoughts?

jtopjian · 2016-06-06T14:43:39Z

@dkalleg It looks like there were 117 files pulled in from gophercloud. That seems like a very large number... I would imagine the commit should just be the lbaasv2 stuff. Maybe that's where the conflict is coming from?

dkalleg · 2016-06-06T19:17:54Z

@jtopjian My intention was to bring up gophercloud to its latest revision, which would include a lot of other changes. Are you recommending I try to pull in just the lbaasv2 gophercloud changes?

jtopjian · 2016-06-06T19:28:36Z

@dkalleg hm... my understanding from using Godep was that the PR contains the diff between what's currently in vendor and what's required to bring it up to date.

jtopjian · 2016-06-06T20:14:56Z

@dkalleg ah! @phinze pointed out that the hundred-some files all look like _test.go files. Godep was nice enough to strip those out, but it seems Govendor is adding them back in.

Can you check and see if you're using the latest Govendor or if there is a flag to not add in the test files?

dkalleg · 2016-06-06T22:34:50Z

@jtopjian I removed the _test.go files from the gophercloud update, down to 38 files changed from 117 :) . I also refactored the resource naming from lbaas to lb.

jtopjian · 2016-06-07T01:19:25Z

vendor/vendor.json

@@ -1,5 +1,6 @@
 {
 	"comment": "",
+<<<<<<< 8c3ff933951a5ab0111c0ef6b3553e42e4ce14af


This looks leftover from a merge conflict?

Yep, sorry. Fixed.

Ping on this one. Looks like it still exists?

jtopjian · 2016-06-07T01:20:23Z

@dkalleg Thank you for your patience with this one! It's never fun to be caught in between a core refactor like the current Godep to Govendor one right now...

dkalleg · 2016-06-07T17:32:36Z

@jtopjian No worries! Was mostly a painless process and taught me about govendor.

jtopjian · 2016-06-08T03:09:57Z

@dkalleg I'm in the process of setting up the OpenStack acceptance test environment for LBaaS v2. Once that's done, I'll run these acceptance tests through it.

In the meantime, I'm going to do a more thorough review of the code and make some inline comments.

jtopjian · 2016-06-08T03:12:59Z

builtin/providers/openstack/resource_openstack_lbaas_listener_v2.go

+	d.Set("default_pool_id", listener.DefaultPoolID)
+	d.Set("connection_limit", listener.ConnLimit)
+	d.Set("sni_container_refs", listener.SniContainerRefs)
+	d.Set("default_tls_container_ref", listener.DefaultTlsContainerRef)


For d.Set to work correctly, the arguments must have Computed: true.

Can you explain what doesn't work correctly if Computed: false? I ask because in testing I:

Create a listener w/ TF.

Note the "name" field (just calling out this attr arbitrarily) was properly populated.

Manually change the listeners name in OpenStack.

Call terraform plan with no changes to the .tf script

Note the plan output wants to change the listeners name from what I manually set to what is in the .tf script: name: "foo" => "tf_test_listener" (forces new resource)

I do notice that the terraform.tfstate does not show the name as "foo" after the plan. I tried adding Computed: true to the name attr and re-ran through this process with the same results.

My current understanding of the Computed attribute is that its for schema elements that the user may not provide, but if that info can be captured during a Read(), you can then write it back to the tfstate via d.Set() BUT it will not cause a terraform plan to consider this field as a "user change". The id schema element is the prime example. Please do correct me if this is wrong or incomplete.

Also, fyi, I realized I don't need a ForceNew: true on name :) I'll fix that and go review all the other attrs.

jtopjian · 2016-06-08T03:26:58Z

@dkalleg Looks like there are three areas to work on:

The FINDME debug lines, though this is more of a nitpick
If you're using d.Set() with any argument, it must be configured with Computed: true.
Terraform won't evaluate the HasChange portion for aguments that are configured with ForceNew: true. There seems to be a lot of arguments in these resources with ForceNew set -- maybe a review of what needs this?

Once devstack is set up, I'll run everything through and see what the result is.

By far, though, this is a great PR! I think the above items are minor compared to the rest of the work you put into this (docs, great tests, etc).

dkalleg · 2016-06-08T22:15:51Z

builtin/providers/openstack/resource_openstack_lbaas_listener_v2.go

+	return &schema.Resource{
+		Create: resourceListenerV2Create,
+		Read:   resourceListenerV2Read,
+		Delete: resourceListenerV2Delete,


@jtopjian I just realized, I missed adding the Update: entry here! But.. how could the test cases have passed if I missed this? Fixing this in my next patch.

dkalleg · 2016-06-08T23:02:21Z

@jtopjian I fixed # 1 and 3. I posted a question above about your #2 comment. Thanks for reviewing!

jtopjian · 2016-06-09T03:07:01Z

@dkalleg Re #2: I ran some tests and fiddled with some code and surprised myself - it looks like d.Set works just fine on attributes that aren't computed. I was always under the impression that the two had to go together. Sorry about that.

I was able to get a working devstack installation with LBaaS. I found that I had to increase the timeout of the resource_openstack_lb_loadbalancer_v2 resource because it takes a while for the vm to start and provision. IMO, raising it from 2 minutes to 20 minutes is probably a safe thing to do.

Once I made that change, all tests passed!

But I also noticed that the files were still called resource_openstack_lbaas_*.go - any chance you can rename these to the lb version?

Once that's done, I'd say squash everything into two commits: one for the Terraform resources and one for the Govendor dependencies. Then I'll see if someone else can sign off on Govendor stuff just to make sure the commit doesn't mess anything up.

Then we should be good to go. 😄

dkalleg · 2016-06-09T18:08:01Z

@jtopjian Done

jtopjian · 2016-06-09T19:53:40Z

@dkalleg Looks like that one line is still floating around vendor.json:

https://github.com/hpcloud/terraform/blob/6aa655fde4aa85ac90eff9efa6e2b8f30a79c35d/vendor/vendor.json#L3

CRUD, tests and Docs for managing a LoadBalancer, Listener, Pool, Member, and Monitor resources.

dkalleg · 2016-06-09T20:37:03Z

@jtopjian Ah, sorry. Should be fixed now.

phinze · 2016-06-10T21:07:06Z

Great work on this @dkalleg! And thanks as always @jtopjian for the review. 👍

dkalleg · 2016-06-10T21:53:19Z

@jtopjian @phinze Thanks!

ghost · 2020-04-25T02:07:22Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

jtopjian added provider/openstack labels Jun 4, 2016

jtopjian reviewed Jun 4, 2016
View reviewed changes

dkalleg force-pushed the lbaas-v2 branch 2 times, most recently from f76112f to d0c39f2 Compare June 4, 2016 19:01

dkalleg force-pushed the lbaas-v2 branch from d0c39f2 to 97d5fc0 Compare June 6, 2016 19:30

dkalleg force-pushed the lbaas-v2 branch from 97d5fc0 to f73e37a Compare June 6, 2016 22:24

jtopjian reviewed Jun 7, 2016
View reviewed changes

dkalleg force-pushed the lbaas-v2 branch from f73e37a to e2a850a Compare June 7, 2016 17:30

jtopjian reviewed Jun 8, 2016
View reviewed changes

dkalleg reviewed Jun 8, 2016
View reviewed changes

dkalleg force-pushed the lbaas-v2 branch from 28b5d98 to bce127b Compare June 9, 2016 18:06

dkalleg added 2 commits June 9, 2016 13:19

Openstack - Updating gophercloud version

95731f2

OpenStack LBaaS v2 Support

8467f52

CRUD, tests and Docs for managing a LoadBalancer, Listener, Pool, Member, and Monitor resources.

dkalleg force-pushed the lbaas-v2 branch from bce127b to 8467f52 Compare June 9, 2016 20:20

phinze merged commit 757aae5 into hashicorp:master Jun 10, 2016

ghost locked and limited conversation to collaborators Apr 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenStack LBaaS v2 Support #7012

OpenStack LBaaS v2 Support #7012

dkalleg commented Jun 4, 2016

dkalleg commented Jun 4, 2016

jtopjian commented Jun 4, 2016

jtopjian Jun 4, 2016

dkalleg Jun 4, 2016

dkalleg commented Jun 4, 2016

jtopjian commented Jun 6, 2016 •

edited

Loading

dkalleg commented Jun 6, 2016

jtopjian commented Jun 6, 2016

jtopjian commented Jun 6, 2016

dkalleg commented Jun 6, 2016

jtopjian Jun 7, 2016

dkalleg Jun 7, 2016

jtopjian Jun 9, 2016

jtopjian commented Jun 7, 2016

dkalleg commented Jun 7, 2016

jtopjian commented Jun 8, 2016

jtopjian Jun 8, 2016

dkalleg Jun 8, 2016 •

edited

Loading

dkalleg Jun 8, 2016

jtopjian commented Jun 8, 2016

dkalleg Jun 8, 2016

dkalleg commented Jun 8, 2016 •

edited

Loading

jtopjian commented Jun 9, 2016

dkalleg commented Jun 9, 2016

jtopjian commented Jun 9, 2016

dkalleg commented Jun 9, 2016

phinze commented Jun 10, 2016

dkalleg commented Jun 10, 2016

ghost commented Apr 25, 2020

OpenStack LBaaS v2 Support #7012

OpenStack LBaaS v2 Support #7012

Conversation

dkalleg commented Jun 4, 2016

dkalleg commented Jun 4, 2016

jtopjian commented Jun 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkalleg commented Jun 4, 2016

jtopjian commented Jun 6, 2016 • edited Loading

dkalleg commented Jun 6, 2016

jtopjian commented Jun 6, 2016

jtopjian commented Jun 6, 2016

dkalleg commented Jun 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtopjian commented Jun 7, 2016

dkalleg commented Jun 7, 2016

jtopjian commented Jun 8, 2016

Choose a reason for hiding this comment

dkalleg Jun 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtopjian commented Jun 8, 2016

Choose a reason for hiding this comment

dkalleg commented Jun 8, 2016 • edited Loading

jtopjian commented Jun 9, 2016

dkalleg commented Jun 9, 2016

jtopjian commented Jun 9, 2016

dkalleg commented Jun 9, 2016

phinze commented Jun 10, 2016

dkalleg commented Jun 10, 2016

ghost commented Apr 25, 2020

jtopjian commented Jun 6, 2016 •

edited

Loading

dkalleg Jun 8, 2016 •

edited

Loading

dkalleg commented Jun 8, 2016 •

edited

Loading