-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
provider/openstack Compute Network Refactor #1347
Conversation
@phinze It does, yes. #1342 did help with regard to the existing Neutron only floating IP support, but this PR here would indeed end up removing all of that. I'm don't mind rebasing this PR against #1342. #1342 will help a few people during the period of time when this PR is reviewed and possibly (hopefully) merged. |
Ok, lets plan on rebasing this. My http://docs.openstack.org/developer/devstack/guides/single-vm.html just finished bootstrapping so I'll take this for a spin! 👍 |
@phinze rebased and tested in my clouds. Do let me know if you run into issues. |
aw, crap. commit log got messed up. I'll fix that. |
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.
OK, better. |
This allows the obtained network information to be successfully stored for environments that do not require a network resource to be specified.
@phinze just a heads up: one additional commit. |
By way of testing, this config file
works using "my" nebula cloud with jtopjian's compute-network-refactor branch cloned a couple of minutes ago:
|
Whoops. Found a rough spot. I'm running against my Nebula system (nova networking) with jtopjian's compute-network-refactor branch cloned earlier this morning.
Given this config file:
I can apply and show and things look good in the GUI. Unfortunately,
|
ps.
|
@hartzell Ah - I know where that's coming from. Nebula-spcific item and I should be able to easily fix it. I'll let you know. |
Okay @jtopjian - I'm making progress! I get all the tests to run, but I'm seeing most of the floating IP tests fail. https://gist.github.com/phinze/e2aa7cc286ade0379abc I'm sure it's probably a config issue with the devstack setup I'm using. Any chance you can tell what I'm missing based on the errors? |
This commit resolves an issue where the tenant-network api extension does not exist. The caveat is that the user must either specify no networks (single network environment) or can only specify UUIDs for network configurations.
@hartzell Let me know if that latest commit helps you. |
@jtopjian, looks good. Thanks! |
Changes the test to require a network UUID rather than a name.
@phinze Ah! I think two of the failures (the ones related to the floating IP resource) are similar to the discussion on #1342 -- they require a network to be specified. The failure of TestAccComputeV2Instance_floatingIPAttach is, ironically enough, similar to @hartzell's problem. Instead of specifying OS_NETWORK_NAME for the test, specify OS_NETWORK_ID with the UUID of the network and let me know if the Instance_floatingIPAttach test now passes. If so, I can work on fixing the other tests. As a side note, I'm curious if you could post:
|
It does! Though I had to leave I'm running through the rest of the tests with that set to get the full results.
Sure! I had success getting https://github.com/lorin/devstack-vm set up with the changes from lorin/devstack-vm#6 applied locally. (I also manually bumped the number of vcpus in the Both the commands work - here's what the output looks like:
|
All great news! And I'm glad the output of the commands worked... I'm curious why the tests did not work with OS_NETWORK_NAME -- I will research that. It's also odd that you were still required to set OS_NETWORK_NAME. The string no longer exists in the code. I will work on fixing up the other two acceptance tests. Do let me know if you run into any more errors. The errors you're seeing about Firewall and such are most likely due to them not being set up in the devstack environment. |
@phinze Also, if you have time, can you do a
Then do a |
Okay here's latest test run: https://gist.github.com/phinze/d7e7db4bd8f91709c196 I realized that I hadn't pulled latest earlier, which probably accounts for the I'll try that config now! 👍 |
FWIW here's the devstack config I got by default from that project:
What environment are you using to for testing, @jtopjian? |
I'm running my tests against a few production clouds I maintain. One is nova-network and one is Neutron, but the Neutron does not have the FWaaS and LBaaS services enabled, either. Just to clarify with your above config: |
@phinze lol - thanks OK, definitely great news that you can define networks in instances by both either UUID or Name and Terraform is obtaining the omitted one. This is confirming my theory that the What's strange is that the acc tests were not working until you used an ID. I can look into that in more detail... but at least it looks like it's local to the acc tests. I just pushed some changes to the floating IP acc tests. I think some further work needs done to the networking_floatingip resource (as mentioned in the comments), but that perhaps can wait? I've been trying to focus on the compute side here. |
We release 0.4 tomorrow, and it seems like it'd be a shame if these improvements had to wait until 0.5 - so it seems like a good plan to limit the scope so we can land it sooner. Given that - how do you feel about the current state of this? Do all |
A change was made to account for clouds with multiple networks.
I agree it'd be a shame. Thank you for giving this so much attention today -- I really do appreciate it. I just ran through the Compute acceptance tests and made a modification that prevented the Instance test from passing on a Neutron cloud. They all now work for me in two different clouds. One thing to keep in mind is that only the Compute Instance resource has been modified. Of course, that's the biggest part of this whole provider, but any issues that are apparent in other resources (such as the At least with these changes, things like I never want to say something is 100% perfect, but I have confidence with these changes. I'm also very interested to see the initial reception of this provider, and if there are any issues, especially if directly related to these changes, I'll want to find a fix. |
Great! This all sounds good - I'll tentatively plan on getting this merged first thing tomorrow. |
Awesome 😄 |
As an eager consumer: Awesome^2. Love to see it in 0.4. Thanks to you both for all the work! |
Here's something I threw together that uses Terraform to deploy DevStack and run the Compute acceptance tests: https://gist.github.com/jtopjian/4ffc82bfcbbcc78d07e4 All passed. :) If I had more time tonight, I'd create a few variations that test against different versions of OpenStack and maybe run some of the DevStack "exercises". Also, perhaps a minor note, but I just wanted to clarify: In Neutron environments, when creating a "private" and "public" (floating IP) network, this still counts as only a single-network environment. It's not until the tenant creates more than one "private" network that multi-network issues come in. This escaped me today and I apologize. The good news is that the acceptance tests will run just fine in either type of environment (which I have tested), given an |
Amazing work @jtopjian. Major thanks! 🙇 |
provider/openstack Compute Network Refactor
@phinze Yay! Thank you! |
Due to the nuances of nova-networks and Neutron, this was probably the hardest and most time-consuming aspect of the OpenStack provider to implement. Great work, @jtopjian |
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. |
The following are a series of changes that I was working on during the OpenStack provider development. Unfortunately these changes did not make it for the initial merge.
I have a lot of notes and comments scattered across previous PRs, but in an attempt to start fresh, I'll try to lay everything out here.
First, to make sure everyone is on the same page, I want to define some areas:
Neutron, nova-network, and Nova:
nova-network is sometimes labelled as "legacy". While it's true it was previously deprecated, the deprecation status was removed some time ago. As recently as last week there are still no plans to deprecate nova-network. The recent user survey shows that 30% of OpenStack clouds use nova-network.
The reason I'm placing so much emphasis on this is because if Terraform is going to officially support OpenStack, nova-network should be considered a peer to Neutron for the foreseeable future. Calling it "legacy" is the same as saying Neutron is still in development.
Supporting two network services can definitely cause headaches, but fortunately, the areas where Nova and Neutron cross paths are taken care of naturally through the Nova API. A lot of this was already done with this provider, but the one area that was left untouched was Floating IPs.
During the development of this provider, Floating IP support was initially added using the Neutron API. This was because gophercloud did not support Floating IPs through the Nova API. While this worked for Neutron-based clouds, it prevented anyone running nova-network from using Floating IPs.
gophercloud was patched to support floating IPs via the Nova API and I set out to remedy Terraform. Along the way I learned that the Nova API can also handle the allocation, deallocation, association, and disassociation of Neutron-based Floating IPs.
There are areas in OpenStack where it makes sense to strictly use the Neutron API with Floating IPs. This is why two Floating IP resources exist (a networking and compute one). However, when it comes to working solely with Compute, the Compute API can be used regardless of the underlying network service. This holds true for Neutron Security Groups as well which has always used the Nova API.
References:
os-floating-ips
)So, there's the history.
Now, for the commits included, here's what each one is doing:
os-floating-ips
. As per the commit message, it works with both network systems but has one drawback (will discuss later). At a minimum, this is the only commit that needs merged and a lot of nova-network users will be very happy. But not just that, it gets rid of a lot of Neutron code that is already handled by Nova. I'd be happy to simply submit this single commit as a PR.terraform show
with this patch._NAME
)The Drawback
I mentioned a drawback. With Neutron environments, when specifying multiple networks, the network that will receive the floating IP attachment must be the first network specified in the
instance
resource. #1342 has a similar drawback, but it looks like it would be the last network?I have a patch into gophercloud to remedy this, and I also have the corresponding terraform code ready, too, but I want to wait until gophercloud is patched.
Other References
So that's my case! I apologize for being overly verbose. It seems a little silly for a single feature of Floating IPs, but since this is a new audience, I wanted to lay everything out.
I'd love to get some feedback -- especially if there are mistakes. I'm in no way saying this is all perfect and without error, but I've been working with this issue, trying different ways of solving it, digging into code, and testing on multiple clouds for six weeks now.