Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute network refactor #23

Closed
wants to merge 154 commits into from
Closed

Compute network refactor #23

wants to merge 154 commits into from

Conversation

jtopjian
Copy link

No description provided.

@jrperritt
Copy link
Owner

If no one else chimes in, I can try to find a solution to the Neutron issue on Monday.

@hartzell
Copy link

This seems to work, but crashes at the end of terraform apply.

Here's a gist of the crash.

Here's how I built it (the patch step just fixes the colored warnings that have evolved in the cli library):

  871  mkdir go
  872  git clone -b compute-network-refactor3 https://github.com/jtopjian/terraform $GOPATH/src/github.com/hashicorp/terraform
  873  cd $GOPATH/src/github.com/hashicorp/terraform
  874  ls
  875  make updatedeps
  877  find . -type d | grep azure
  878  rm -rf ./builtin/bins/provider-azure ./builtin/providers/azure
  879  make updatedeps
  880  make dev
  882  find . -type d | grep digitalocean
  883  find . -type d | grep digitalocean | xargs rm -rf
  884  patch -p1  < ~/tmp/diffs
  885  make dev
  886  find . -type d | grep cloudstack
  887  find . -type d | grep cloudstack | grep buil | xargs rm -rf
  888  make dev

g.

@jtopjian
Copy link
Author

@hartzell: Thanks for the report. Can you try the latest?

@hartzell
Copy link

Did a git fetch and a make dev. Binaries appear to have been rebuilt (via ls -l).

Crashed again. Gist here.

g.

@jtopjian
Copy link
Author

Hm. The crash log shows it failed on the same line again (/resource_openstack_compute_instance_v2.go:803), but with the latest commit, line 803 is not trying to do a string conversion.

Can you compare line 803 with what's on github vs your local copy? Maybe the changes weren't pulled in?

@jtopjian
Copy link
Author

@jrperritt My idea to resolve the "first neutron network" is to add a flag to the networks block that identifies the network as a floating IP enabled network.

In order to allow that, os-floating-ips needs updated in gophercloud. I discovered an extension to the os-floating-ips extension that allows a fixed ip to be specified. I have a patch ready to go, but want to first confirm that everything is working for George with this current PR before making further changes.

@hartzell
Copy link

My mistake, too much svn & not enough git in my daily diet. Forgot the git merge after the git fetch....

Worked fine.

Thanks!

@jtopjian
Copy link
Author

woohoo! Thank you for verifying!

OK, I'm going to start making the changes to handle multi-neutron networks.

@hartzell If it's OK with you, I'll need another test tomorrow or Friday to make sure nothing on your side broke.

@hartzell
Copy link

@jtopjian, fine with me. I'll try to keep on top of it (work permitting).

This commit causes the resource to manage floating IPs by way of the
os-floating-ips API.

At the moment, it works with both nova-network and Neutron environments,
but if you use multiple Neutron networks, the network that supports the
floating IP must be listed first.
This is only possible if the OpenStack cloud explicitly has a network
called "public".
This commit allows the user to specify a network by name rather than
just uuid. This is done via the os-tenant-networks api extension.
This works for both neutron and nova-network.
This commit changes how the network info is read from OpenStack.
It pulls all relevant information from server.Addresses and merges
it with the available information from the networks parameters.
The access_v4, access_v6, and floating IP information is then
determined from the result.

A MAC address parameter is also added since that information is
available in server.Addresses.
This commit enables multiple neturon networks to be specified in any order.
In order to attach a floating IP, the floating ip enabled neutron network
must either be the first in the list or be tagged with "set_floating_ip" in
the network block.

Existing nova-network support is unchanged.
@jtopjian
Copy link
Author

I just pushed 3 commits. The first two should resolve the "first neutron network" issue. As per the commit notes, if a neutron network is tagged with set_floating_ip, that network will be used to associate the floating IP. I'm happy to change the name of the flag if set_floating_ip is not suitable.

One caveat is that these commits rely on this patch to gophercloud. If anyone wants to test this, they will need to apply that patch or wait until it gets merged.

The third commit fixed a typo for cases where no network was specified (usually nova-network environments). After fixing this typo, doing a terraform show should now print the obtainable network information.

At this point, I think this branch is ready for others to test and to give feedback.

@jtopjian jtopjian mentioned this pull request Mar 28, 2015
@hartzell
Copy link

I just noticed that it's still possible to shoot yourself in the foot if you use a floating IP address that's in use by another instance.

Here's an example:

resource "openstack_compute_floatingip_v2" "test" {
  pool = "nebula"
}

resource "openstack_compute_instance_v2" "test" {
  name = "tf-test"
  image_id = "62896feb-0c93-49bd-93a3-23f597d3f9ec"
  flavor_id = "n1.small"
  key_pair = "alanturing-nebula-keypair"
  floating_ip = "${openstack_compute_floatingip_v2.test.address}"
  security_groups = ["default"]
}

resource "openstack_compute_instance_v2" "test-the-other" {
  name = "tf-test-2"
  image_id = "62896feb-0c93-49bd-93a3-23f597d3f9ec"
  flavor_id = "n1.small"
  key_pair = "alanturing-nebula-keypair"
  floating_ip = "${openstack_compute_floatingip_v2.test.address}"
  security_groups = ["default"]
}

I'd like to see this generate an error. Instead, one instance ends up with the floating ip, the other looses, but everything appears to have worked.

I believe that Openstack allows this type of foot shooting. Is it possible to at least have terraform keep track of what it is doing and not let shoot myself in the foot? I could still step on something allocated via other tools, but it seems better than nothing.

@jtopjian
Copy link
Author

@hartzell That's a really interesting scenario and I think it deserves some thought and more discussion. I think that can wait until we get the fundamentals sorted out, though, since there might be other areas where preventative foot-shooting would be nice.

If you're able to launch instances and attach floating IPs, I think that's a huge win at this point. 😄

@julienvey
Copy link

@hartzell Yes, it's a common scenario unfortunately, to stole a floating_ip. But I'm not really sure it is terraform responsibilty to manage that.

This "Bug" or "Feature" would find its use-case with Terraform. For instance you could always use the same floating IP to provision mutliple servers sequentially

resource "openstack_compute_floatingip_v2" "test" {
  pool = "nebula"
}

resource "openstack_compute_instance_v2" "test" {
  name = "tf-test"
  image_id = "62896feb-0c93-49bd-93a3-23f597d3f9ec"
  flavor_id = "n1.small"
  key_pair = "alanturing-nebula-keypair"
  floating_ip = "${openstack_compute_floatingip_v2.test.address}"
  security_groups = ["default"]
  provisioner "remote-exec" {
    ...
    connection {
      user = "root"
      key_file = "${var.ssh_key_file}"
    }
  }
}

resource "openstack_compute_instance_v2" "test-the-other" {
  name = "tf-test-2"
  image_id = "62896feb-0c93-49bd-93a3-23f597d3f9ec"
  flavor_id = "n1.small"
  key_pair = "alanturing-nebula-keypair"
  floating_ip = "${openstack_compute_floatingip_v2.test.address}"
  security_groups = ["default"]
  provisioner "remote-exec" {
    ...
    connection {
      user = "root"
      key_file = "${var.ssh_key_file}"
    }
  }
}

@jrperritt Any chance you can review and merge this PR. Seems stable enough to me with @hartzell testing.

@hartzell
Copy link

@jtopjian, yes, this work is a huge win and I'm quite pleased with it.

@julianenvy, the problem with your scenario is that it doesn't work. You end up with two instances, one of which has a floating ip and one of which doesn't. Which one does and which one doesn't is unpredictable. The only reason that I think that terraform might lend a hand here is that Openstack does not protect itself. Having allocated a floating ip address to one system, there is no requirement that you release it before you allocate it to another. I don't know if that is true of other components, e.g. can you assign a volume to one system and then assign it again to another w/out an error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants