You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Zack Shoylev <no...@github.com> on 2014/03/11 21:52:24 UTC
[jclouds] JCLOUDS-486 This will allow booting up nova servers with
fixed IPs and p... (#313)
...orts.
You can merge this Pull Request by running:
git pull https://github.com/rackspace/jclouds backport-add-nova-neutron-networks
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/313
-- Commit Summary --
* JCLOUDS-486 This will allow booting up nova servers with fixed IPs and ports.
-- File Changes --
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/NovaComputeServiceAdapter.java (7)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/options/NovaTemplateOptions.java (48)
A apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/domain/Network.java (173)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/options/CreateServerOptions.java (103)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/compute/NovaComputeServiceAdapterExpectTest.java (49)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/features/ServerApiLiveTest.java (288)
A apis/openstack-nova/src/test/resources/new_server_nova_networks.json (41)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/313.patch
https://github.com/jclouds/jclouds/pull/313.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> @@ -364,6 +386,16 @@ public CreateServerOptions availabilityZone(String availabilityZone) {
> public Set<String> getNetworks() {
> return networks;
> }
> +
> + /**
> + * Get custom networks specified for the server.
[minor] Gets
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826455
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Zack Shoylev <no...@github.com>.
Closed #313.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Zack Shoylev <no...@github.com>.
merged
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313#issuecomment-37553242
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> - }
> + }
> + }
> + }
> +
> + @Test
> + public void testCreateInWrongAvailabilityZone() {
> + String serverId = null;
> + for (String zoneId : zones) {
> + ServerApi serverApi = api.getServerApiForZone(zoneId);
> + try {
> + serverId = createServer(zoneId, "err", Server.Status.ERROR).getId();
> + Server server = serverApi.get(serverId);
> + assertEquals(server.getStatus(), Server.Status.ERROR);
> + } finally {
> + serverApi.delete(serverId);
> I don't want to modify this because it already got merged
Fair point. Separate PR works for me!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10841344
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> + serverApi.delete(serverId);
> + }
> + }
> + }
> +
> + private Server createServer(String regionId, Server.Status serverStatus) {
> + ServerApi serverApi = api.getServerApiForZone(regionId);
> + CreateServerOptions options = new CreateServerOptions();
> + ServerCreated server = serverApi.create(hostName, imageIdForZone(regionId), flavorRefForZone(regionId), options);
> +
> + blockUntilServerInState(server.getId(), serverApi, serverStatus);
> +
> + return serverApi.get(server.getId());
> + }
> +
> + private Server createServer(String regionId, String availabilityZoneId, Server.Status serverStatus) {
[minor] Rather than duplicate almost the whole method, add a null switch for availabilityZone?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826604
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #654](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/654/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313#issuecomment-37352983
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Zack Shoylev <no...@github.com>.
Note: backporting
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313#issuecomment-37348415
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Zack Shoylev <no...@github.com>.
> - }
> - }
> - }
> -
> - @Test
> - public void testCreateInAvailabilityZone() {
> - String serverId = null;
> - for (String zoneId : zones) {
> - ServerApi serverApi = api.getServerApiForZone(zoneId);
> - try {
> - serverId = createServer(zoneId, "nova", Server.Status.ACTIVE).getId();
> - Server server = serverApi.get(serverId);
> - assertEquals(server.getStatus(), Server.Status.ACTIVE);
> - } finally {
> - serverApi.delete(serverId);
> + @Test(description = "GET /v${apiVersion}/{tenantId}/servers")
I think this is also just identation fixes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10840424
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> - }
> + }
> + }
> + }
> +
> + @Test
> + public void testCreateInWrongAvailabilityZone() {
> + String serverId = null;
> + for (String zoneId : zones) {
> + ServerApi serverApi = api.getServerApiForZone(zoneId);
> + try {
> + serverId = createServer(zoneId, "err", Server.Status.ERROR).getId();
> + Server server = serverApi.get(serverId);
> + assertEquals(server.getStatus(), Server.Status.ERROR);
> + } finally {
> + serverApi.delete(serverId);
What if serverId is still null at this point?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826577
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> + */
> +package org.jclouds.openstack.nova.v2_0.domain;
> +
> +import java.beans.ConstructorProperties;
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Objects.ToStringHelper;
> +
> +import static com.google.common.base.Preconditions.checkArgument;
> +
> +/**
> + * Nova (or Neutron) network definition
> + * Used to provide support for network, port, and fixed_ip when booting Nova servers.
> + * OpenStack will support either a Nova Network or Neutron, but not both at the same time.
> + * Specifying a port is only possible with Neutron.
> + * @author Zack Shoylev
[minor] Blank line before `@author`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826266
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> + return this;
> + }
> +
> + /**
> + * @return A new Network object.
> + */
> + public Network build() {
> + return new Network(networkUuid, portUuid, fixedIp);
> + }
> +
> + /**
> + * @param in The target Network
> + * @return A Builder from the provided Network
> + */
> + public Builder fromNetwork(Network in) {
> + return this
[minor] Bring `.networkUuid`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826357
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> @@ -545,6 +567,15 @@ public NovaTemplateOptions networks(Iterable<String> networks) {
> }
>
> /**
> + * <br>Ensures NovaTemplateOptions can work with networks specified as Strings.
What's the `<br>` for here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826218
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> + /**
> + * This needs to be supported by the provider, and is usually not supported.
> + * However this can be tested on devstack:
> + * In apis/openstack-nova:
> + * mvn -Plive clean install "-Dtest.openstack-nova.endpoint=http://localhost:5000/v2.0" "-Dtest.openstack-nova.identity=demo:demo" "-Dtest.openstack-nova.credential=devstack" "-Dtest=org.jclouds.openstack.nova.v2_0.features.ServerApiLiveTest#testCreateWithNetworkOptions"
> + */
> + @Test(enabled = false)
> + public void testCreateWithNetworkOptions() {
> + String serverId = null;
> + for (String zoneId : zones) {
> + ServerApi serverApi = api.getServerApiForZone(zoneId);
> + try {
> + CreateServerOptions options = CreateServerOptions.Builder.novaNetworks(
> + // This network UUID must match an existing network.
> + ImmutableSet.of(Network.builder().networkUuid("bc4cfa2b-2b27-4671-8e8f-73009623def0").fixedIp("192.168.55.56").build())
> + );
Formatting? Something like:
```
// This network UUID must match an existing network.
CreateServerOptions options = CreateServerOptions.Builder.novaNetworks(ImmutableSet.of(
Network.builder().networkUuid("bc4cfa2b-2b27-4671-8e8f-73009623def0").fixedIp("192.168.55.56").build()));
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826543
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Zack Shoylev <no...@github.com>.
> - }
> + }
> + }
> + }
> +
> + @Test
> + public void testCreateInWrongAvailabilityZone() {
> + String serverId = null;
> + for (String zoneId : zones) {
> + ServerApi serverApi = api.getServerApiForZone(zoneId);
> + try {
> + serverId = createServer(zoneId, "err", Server.Status.ERROR).getId();
> + Server server = serverApi.get(serverId);
> + assertEquals(server.getStatus(), Server.Status.ERROR);
> + } finally {
> + serverApi.delete(serverId);
I actually don't know about this... this is mostly reformatting old code for style.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10840288
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> + withImage(server.getImage().getId()).
> + name("newName").
> + adminPass("password").
> + ipv4Address("1.1.1.1").
> + ipv6Address("fe80::100");
> +
> + serverApi.rebuild(serverId, options);
> +
> + Server rebuiltServer = serverApi.get(serverId);
> +
> + assertEquals("newName", rebuiltServer.getName());
> + assertEquals("1.1.1.1", rebuiltServer.getAccessIPv4());
> + assertEquals("fe80::100", rebuiltServer.getAccessIPv6());
> +
> + } finally {
> + serverApi.delete(serverId);
What if serverId is still null at this point?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826578
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> - }
> - }
> - }
> -
> - @Test
> - public void testCreateInAvailabilityZone() {
> - String serverId = null;
> - for (String zoneId : zones) {
> - ServerApi serverApi = api.getServerApiForZone(zoneId);
> - try {
> - serverId = createServer(zoneId, "nova", Server.Status.ACTIVE).getId();
> - Server server = serverApi.get(serverId);
> - assertEquals(server.getStatus(), Server.Status.ACTIVE);
> - } finally {
> - serverApi.delete(serverId);
> + @Test(description = "GET /v${apiVersion}/{tenantId}/servers")
I like this (the description), but is it something we do anywhere else..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826467
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> @@ -575,4 +606,15 @@ public NovaTemplateOptions configDrive(boolean configDrive) {
> this.configDrive = configDrive;
> return this;
> }
> +
> + /**
> + * @param novaNetworks The list of network declarations.
> + * Nova-specific network declarations allow for specifying network UUIDs, port UUIDs, and fixed IPs.
> + * Unline {@link #networks(Iterable)} this supports setting additional network parameters and not just network UUIDs.
> + * @see CreateServerOptions#getNetworks()
> + */
> + public NovaTemplateOptions novaNetworks(Set<Network> novaNetworks) {
For consistency, should this be `Iterable<Network>`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826256
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.openstack.nova.v2_0.domain;
> +
> +import java.beans.ConstructorProperties;
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Objects.ToStringHelper;
> +
> +import static com.google.common.base.Preconditions.checkArgument;
> +
> +/**
> + * Nova (or Neutron) network definition
> + * Used to provide support for network, port, and fixed_ip when booting Nova servers.
> + * OpenStack will support either a Nova Network or Neutron, but not both at the same time.
[minor] Is this "network"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826287
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> @@ -537,7 +558,8 @@ public NovaTemplateOptions nodeNames(Iterable<String> nodeNames) {
> }
>
> /**
> - * {@inheritDoc}
> + * <br>Ensures NovaTemplateOptions can work with networks specified as Strings.
What's the `<br>` for here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826214
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> - }
> + }
> + }
> + }
> +
> + @Test
> + public void testCreateInWrongAvailabilityZone() {
> + String serverId = null;
> + for (String zoneId : zones) {
> + ServerApi serverApi = api.getServerApiForZone(zoneId);
> + try {
> + serverId = createServer(zoneId, "err", Server.Status.ERROR).getId();
> + Server server = serverApi.get(serverId);
> + assertEquals(server.getStatus(), Server.Status.ERROR);
> + } finally {
> + serverApi.delete(serverId);
> this is mostly reformatting old code for style
Perhaps we can improve it (or at least iron out a possible bug) at the same time? ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10840353
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Zack Shoylev <no...@github.com>.
> + serverId = createServer(zoneId, "nova", Server.Status.ACTIVE).getId();
> + Server server = serverApi.get(serverId);
> + assertEquals(server.getStatus(), Server.Status.ACTIVE);
> + } finally {
> + serverApi.delete(serverId);
> + }
> + }
> + }
> +
> + /**
> + * This needs to be supported by the provider, and is usually not supported.
> + * However this can be tested on devstack:
> + * In apis/openstack-nova:
> + * mvn -Plive clean install "-Dtest.openstack-nova.endpoint=http://localhost:5000/v2.0" "-Dtest.openstack-nova.identity=demo:demo" "-Dtest.openstack-nova.credential=devstack" "-Dtest=org.jclouds.openstack.nova.v2_0.features.ServerApiLiveTest#testCreateWithNetworkOptions"
> + */
> + @Test(enabled = false)
Actually the plan is to enable this at some point soon. I will look into detection options.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10840156
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.openstack.nova.v2_0.domain;
> +
> +import java.beans.ConstructorProperties;
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Objects.ToStringHelper;
> +
> +import static com.google.common.base.Preconditions.checkArgument;
> +
> +/**
> + * Nova (or Neutron) network definition
> + * Used to provide support for network, port, and fixed_ip when booting Nova servers.
> + * OpenStack will support either a Nova Network or Neutron, but not both at the same time.
Thanks for clarifying...lazy me, sorry! ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10840818
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1124](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1124/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313#issuecomment-37353113
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Zack Shoylev <no...@github.com>.
> - }
> + }
> + }
> + }
> +
> + @Test
> + public void testCreateInWrongAvailabilityZone() {
> + String serverId = null;
> + for (String zoneId : zones) {
> + ServerApi serverApi = api.getServerApiForZone(zoneId);
> + try {
> + serverId = createServer(zoneId, "err", Server.Status.ERROR).getId();
> + Server server = serverApi.get(serverId);
> + assertEquals(server.getStatus(), Server.Status.ERROR);
> + } finally {
> + serverApi.delete(serverId);
I think all of this code will benefit from a separate cleanup PR.
Also: the live tests have been very fragile for me, so they should be ... refactored.
I don't want to modify this because it already got merged, and will probably have to be fixed in master first and then backported (which is why separate PR).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10840920
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Zack Shoylev <no...@github.com>.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.openstack.nova.v2_0.domain;
> +
> +import java.beans.ConstructorProperties;
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Objects.ToStringHelper;
> +
> +import static com.google.common.base.Preconditions.checkArgument;
> +
> +/**
> + * Nova (or Neutron) network definition
> + * Used to provide support for network, port, and fixed_ip when booting Nova servers.
> + * OpenStack will support either a Nova Network or Neutron, but not both at the same time.
OpenStack documentation has is capitalized:
http://docs.openstack.org/trunk/openstack-ops/content/nova-network-deprecation.html
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10840577
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> + public int hashCode() {
> + return Objects.hashCode(networkUuid, portUuid, fixedIp);
> + }
> +
> + @Override
> + public boolean equals(Object obj) {
> + if (this == obj) return true;
> + if (obj == null || getClass() != obj.getClass()) return false;
> + Network that = Network.class.cast(obj);
> + return Objects.equal(this.networkUuid, that.networkUuid) &&
> + Objects.equal(this.portUuid, that.portUuid) &&
> + Objects.equal(this.fixedIp, that.fixedIp);
> + }
> +
> + protected ToStringHelper string() {
> + return Objects.toStringHelper(this)
Add `ignoreNullValues`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826312
Re: [jclouds] JCLOUDS-486 This will allow booting up nova servers
with fixed IPs and p... (#313)
Posted by Andrew Phillips <no...@github.com>.
> + serverId = createServer(zoneId, "nova", Server.Status.ACTIVE).getId();
> + Server server = serverApi.get(serverId);
> + assertEquals(server.getStatus(), Server.Status.ACTIVE);
> + } finally {
> + serverApi.delete(serverId);
> + }
> + }
> + }
> +
> + /**
> + * This needs to be supported by the provider, and is usually not supported.
> + * However this can be tested on devstack:
> + * In apis/openstack-nova:
> + * mvn -Plive clean install "-Dtest.openstack-nova.endpoint=http://localhost:5000/v2.0" "-Dtest.openstack-nova.identity=demo:demo" "-Dtest.openstack-nova.credential=devstack" "-Dtest=org.jclouds.openstack.nova.v2_0.features.ServerApiLiveTest#testCreateWithNetworkOptions"
> + */
> + @Test(enabled = false)
Hm...is there some way we can programmatically test whether to run this test or not, rather than adding a disabled test to the code base, that is liable to rot?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/313/files#r10826500