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/08 01:08:58 UTC
[jclouds] Add nova neutron support (#311)
This will allow specifying ports and fixed IPs when creating nova servers.
You can merge this Pull Request by running:
git pull https://github.com/rackspace/jclouds add-nova-neutron-support
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/311
-- Commit Summary --
* JCLOUDS-486 This will allow booting up nova servers with fixed IPs and ports.
* Adds fixed expect test.
-- File Changes --
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/NovaComputeServiceAdapter.java (6)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/options/NovaTemplateOptions.java (35)
A apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/domain/Network.java (170)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/options/CreateServerOptions.java (60)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/compute/NovaComputeServiceAdapterExpectTest.java (48)
A apis/openstack-nova/src/test/resources/new_server_nova_networks.json (41)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/311.patch
https://github.com/jclouds/jclouds/pull/311.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
merged
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37345909
Re: [jclouds] Add nova neutron support (#311)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1122](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1122/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37349632
Re: [jclouds] Add nova neutron support (#311)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #894](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/894/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37083605
Re: [jclouds] Add nova neutron support (#311)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #902](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/902/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37347193
Re: [jclouds] Add nova neutron support (#311)
Posted by Andrew Phillips <no...@github.com>.
> +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.
> + * @author Zack Shoylev
> + */
> +public class Network implements Comparable<Network> {
> + private String networkUuid;
> + private String portUuid;
> + private String fixedIp;
Can these three be `final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10410830
Re: [jclouds] Add nova neutron support (#311)
Posted by Andrew Phillips <no...@github.com>.
> + * Used to provide support for network, port, and fixed_ip when booting Nova servers.
> + * @author Zack Shoylev
> + */
> +public class Network implements Comparable<Network> {
> + private String networkUuid;
> + private String portUuid;
> + private String fixedIp;
> +
> + @ConstructorProperties({
> + "networkUuid", "portUuid", "fixedIp"
> + })
> + protected Network(String networkUuid, String portUuid, String fixedIp) {
> + checkArgument(networkUuid!=null || portUuid!=null, "At least one of networkUuid or portUuid should be specified");
> + this.networkUuid = networkUuid;
> + this.portUuid = portUuid;
> + this.fixedIp = fixedIp;
Is this nullable?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10410836
Re: [jclouds] Add nova neutron support (#311)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #644](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/644/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37084619
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
https://issues.apache.org/jira/browse/JCLOUDS-486
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37081635
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
I just added that in the last commit. Basically keep networks and novaNetworks tracked separately, but combine in the request. Thus one does not replace the other. The test should demo that as well.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37237032
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
> +
> + /**
> + * @param in The target Network
> + * @return A Builder from the provided Network
> + */
> + public Builder fromNetwork(Network in) {
> + return this
> + .networkUuid(in.getNetworkUuid())
> + .portUuid(in.getPortUuid())
> + .fixedIp(in.getFixedIp());
> + }
> + }
> +
> + @Override
> + public int compareTo(Network that) {
> + return this.toString().compareTo(that.toString());
I think so. For example, ordering is based on the string() method defined here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10442541
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
Going to rebase/merge now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37341952
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
Would it be less confusing if both were supported simultaneously? Perhaps the either-or behavior is not needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37216685
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
Closed #311.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311
Re: [jclouds] Add nova neutron support (#311)
Posted by Everett Toews <no...@github.com>.
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * 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
Include more comments about how OpenStack can either have Nova Network or Neutron but not both. Make sure it's really really clear for each method in the class which is applicable to the method.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10478655
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
> @@ -396,6 +409,14 @@ public static NovaTemplateOptions configDrive(boolean configDrive) {
> NovaTemplateOptions options = new NovaTemplateOptions();
> return NovaTemplateOptions.class.cast(options.configDrive(configDrive));
> }
> +
> + /**
> + * @see org.jclouds.openstack.nova.v2_0.options.CreateServerOptions#getNetworks()
> + */
> + public static NovaTemplateOptions novaNetworks(Set<Network> novaNetworks) {
Well the erasure problems here just pointed out an issue of design. Even if overloading was possible here, it would have been confusing, I think.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10441940
Re: [jclouds] Add nova neutron support (#311)
Posted by Andrew Phillips <no...@github.com>.
> +
> +/**
> + * Nova (or Neutron) network definition
> + * Used to provide support for network, port, and fixed_ip when booting Nova servers.
> + * @author Zack Shoylev
> + */
> +public class Network implements Comparable<Network> {
> + private String networkUuid;
> + private String portUuid;
> + private String fixedIp;
> +
> + @ConstructorProperties({
> + "networkUuid", "portUuid", "fixedIp"
> + })
> + protected Network(String networkUuid, String portUuid, String fixedIp) {
> + checkArgument(networkUuid!=null || portUuid!=null, "At least one of networkUuid or portUuid should be specified");
[minor] spaces around `!=`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10410823
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
I also added the live test. It seems to work. The instance boots... except in an error state. Seem I have some network config issues on my devstack.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37321824
Re: [jclouds] Add nova neutron support (#311)
Posted by Everett Toews <no...@github.com>.
> @@ -575,4 +604,14 @@ public NovaTemplateOptions configDrive(boolean configDrive) {
> this.configDrive = configDrive;
> return this;
> }
> +
> + /**
> + * @param novaNetworks The list of network declarations.
Comment on the difference between this and networks().
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10478836
Re: [jclouds] Add nova neutron support (#311)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #650](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/650/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37327628
Re: [jclouds] Add nova neutron support (#311)
Posted by Everett Toews <no...@github.com>.
> + this.networkUuid = networkUuid;
> + return this;
> + }
> +
> + /**
> + * @param portUuid The port UUID for this neutron Network.
> + * @return The builder object.
> + * @see Network#getPortUuid()
> + */
> + public Builder portUuid(String portUuid) {
> + this.portUuid = portUuid;
> + return this;
> + }
> +
> + /**
> + * @param fixedIp The fixed IP address for this Network (if any).
Mention that this is applicable to both Nova Network and Neutron.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10478514
Re: [jclouds] Add nova neutron support (#311)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1114](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1114/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37084663
Re: [jclouds] Add nova neutron support (#311)
Posted by Everett Toews <no...@github.com>.
> + protected String networkUuid;
> + protected String portUuid;
> + protected String fixedIp;
> +
> + /**
> + * @param networkUuid The UUID for the nova network or neutron subnet to be attached.
> + * @return The builder object.
> + * @see Network#getNetworkUuid()
> + */
> + public Builder networkUuid(String networkUuid) {
> + this.networkUuid = networkUuid;
> + return this;
> + }
> +
> + /**
> + * @param portUuid The port UUID for this neutron Network.
Capitalize neutron everywhere.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10478545
Re: [jclouds] Add nova neutron support (#311)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1120](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1120/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37328301
Re: [jclouds] Add nova neutron support (#311)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1115](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1115/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37084674
Re: [jclouds] Add nova neutron support (#311)
Posted by Andrew Phillips <no...@github.com>.
> @@ -110,7 +110,11 @@ public NovaComputeServiceAdapter(NovaApi novaApi, @Zone Supplier<Set<String>> zo
> options.userData(templateOptions.getUserData());
> options.diskConfig(templateOptions.getDiskConfig());
> options.configDrive(templateOptions.getConfigDrive());
> - options.networks(templateOptions.getNetworks());
> + if(templateOptions.getNovaNetworks()!=null) {
[minor] Spacing:
```
if (templateOptions.getNovaNetworks() != null) {
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10410758
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
I think I was able to add enough to explain which network properties are neutron vs nova.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37320628
Re: [jclouds] Add nova neutron support (#311)
Posted by Everett Toews <no...@github.com>.
@zack-shoylev Can you please provide a minimal sample of code for how this would work with nova-network *and* Neutron?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37236443
Re: [jclouds] Add nova neutron support (#311)
Posted by Everett Toews <no...@github.com>.
> @@ -545,6 +566,14 @@ public NovaTemplateOptions networks(Iterable<String> networks) {
> }
>
> /**
> + * {@inheritDoc}
The inherited doc won't be much help here. Comment on what this method does and the difference between novaNetworks().
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10478823
Re: [jclouds] Add nova neutron support (#311)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #652](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/652/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37348841
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
> + * Used to provide support for network, port, and fixed_ip when booting Nova servers.
> + * @author Zack Shoylev
> + */
> +public class Network implements Comparable<Network> {
> + private String networkUuid;
> + private String portUuid;
> + private String fixedIp;
> +
> + @ConstructorProperties({
> + "networkUuid", "portUuid", "fixedIp"
> + })
> + protected Network(String networkUuid, String portUuid, String fixedIp) {
> + checkArgument(networkUuid!=null || portUuid!=null, "At least one of networkUuid or portUuid should be specified");
> + this.networkUuid = networkUuid;
> + this.portUuid = portUuid;
> + this.fixedIp = fixedIp;
I think the only constraint is that either a networkUuid or portUuid have to be specified.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10442176
Re: [jclouds] Add nova neutron support (#311)
Posted by Everett Toews <no...@github.com>.
+1
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37341757
Re: [jclouds] Add nova neutron support (#311)
Posted by Andrew Phillips <no...@github.com>.
> @@ -396,6 +409,14 @@ public static NovaTemplateOptions configDrive(boolean configDrive) {
> NovaTemplateOptions options = new NovaTemplateOptions();
> return NovaTemplateOptions.class.cast(options.configDrive(configDrive));
> }
> +
> + /**
> + * @see org.jclouds.openstack.nova.v2_0.options.CreateServerOptions#getNetworks()
> + */
> + public static NovaTemplateOptions novaNetworks(Set<Network> novaNetworks) {
Any reason not to add an overridden `networks(Iterable\<Network\> networks)` (or `Set\<Network\> networks`), if there are erasure problems) method in alignment with http://javadocs.jclouds.cloudbees.net/org/jclouds/compute/options/TemplateOptions.Builder.html#networks(java.lang.Iterable)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10410817
Re: [jclouds] Add nova neutron support (#311)
Posted by Andrew Phillips <no...@github.com>.
> @@ -218,8 +221,19 @@ private ServerRequest(String name, String imageRef, String flavorRef) {
>
> if (!networks.isEmpty()) {
> server.networks = Sets.newLinkedHashSet(); // ensures ordering is preserved - helps testing and more intuitive for users.
> - for (String network : networks) {
> - server.networks.add(ImmutableMap.of("uuid", network));
> + for (Network network : networks) {
> + // Avoid serializing null values, which are common here.
> + Map<String, String> networkMap = Maps.newHashMap();
Use ImmutableMap.builder()?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10410845
Re: [jclouds] Add nova neutron support (#311)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #900](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/900/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37329032
Re: [jclouds] Add nova neutron support (#311)
Posted by Andrew Phillips <no...@github.com>.
Apart from the question about the property naming, I think we need:
* some Javadoc to describe the "either-or" behaviour of `networks` vs. `novaNetworks`
* a unit test to verify what happens if you set `networks` **and** `novaNetworks` in the template options
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37108038
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
I am thinking of adding live tests for this, but unsure how applicable such would be. Thoughts?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37081592
Re: [jclouds] Add nova neutron support (#311)
Posted by Zack Shoylev <no...@github.com>.
> +
> +/**
> + * Nova (or Neutron) network definition
> + * Used to provide support for network, port, and fixed_ip when booting Nova servers.
> + * @author Zack Shoylev
> + */
> +public class Network implements Comparable<Network> {
> + private String networkUuid;
> + private String portUuid;
> + private String fixedIp;
> +
> + @ConstructorProperties({
> + "networkUuid", "portUuid", "fixedIp"
> + })
> + protected Network(String networkUuid, String portUuid, String fixedIp) {
> + checkArgument(networkUuid!=null || portUuid!=null, "At least one of networkUuid or portUuid should be specified");
I usually see those checks in the constructor for domain classes. Do you have some counter-examples in mind?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10442028
Re: [jclouds] Add nova neutron support (#311)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #645](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/645/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37084620
Re: [jclouds] Add nova neutron support (#311)
Posted by Andrew Phillips <no...@github.com>.
> +
> + /**
> + * @param in The target Network
> + * @return A Builder from the provided Network
> + */
> + public Builder fromNetwork(Network in) {
> + return this
> + .networkUuid(in.getNetworkUuid())
> + .portUuid(in.getPortUuid())
> + .fixedIp(in.getFixedIp());
> + }
> + }
> +
> + @Override
> + public int compareTo(Network that) {
> + return this.toString().compareTo(that.toString());
Compare based on strings? Do we know that `Objects.toStringHelper` is sufficiently deterministic?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311/files#r10410841
Re: [jclouds] Add nova neutron support (#311)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #895](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/895/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/311#issuecomment-37085372