Skip to content

openstack CreateServerOptions added availability zone option #180

Closed
wants to merge 1 commit into from

5 participants

@inbarsto

https://issues.apache.org/jira/browse/JCLOUDS-349

create server options for Openstack, added the option to select availability zone

@cloudbees-pull-request-builder

jclouds-pull-requests #302 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #764 SUCCESS
This pull request looks good

@buildhive

jclouds » jclouds #510 SUCCESS
This pull request looks good
(what's this?)

@cloudbees-pull-request-builder

jclouds-pull-requests #310 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #772 SUCCESS
This pull request looks good

@buildhive

jclouds » jclouds #519 SUCCESS
This pull request looks good
(what's this?)

@demobox demobox commented on the diff Oct 15, 2013
.../openstack/nova/v2_0/options/CreateServerOptions.java
+import org.jclouds.rest.binders.BindToJsonPayload;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+
+import static com.google.common.base.Objects.equal;
+import static com.google.common.base.Objects.toStringHelper;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Strings.emptyToNull;
+import static com.google.common.io.BaseEncoding.base64;
@demobox
jclouds member
demobox added a note Oct 15, 2013

Please do not re-order imports - keep the original order so that it is easier to track the actual changes.

@inbarsto
inbarsto added a note Oct 16, 2013

sorry

@demobox
jclouds member
demobox added a note Oct 16, 2013

sorry

No problem! Just a comment for future reviews ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@demobox demobox and 1 other commented on an outdated diff Oct 15, 2013
.../openstack/nova/v2_0/options/CreateServerOptions.java
@@ -121,9 +119,9 @@ public boolean equals(Object object) {
if (object instanceof CreateServerOptions) {
final CreateServerOptions other = CreateServerOptions.class.cast(object);
return equal(keyName, other.keyName) && equal(securityGroupNames, other.securityGroupNames)
- && equal(metadata, other.metadata) && equal(personality, other.personality)
- && equal(adminPass, other.adminPass) && equal(diskConfig, other.diskConfig)
- && equal(adminPass, other.adminPass) && equal(networks, other.networks);
+ && equal(metadata, other.metadata) && equal(personality, other.personality)
+ && equal(adminPass, other.adminPass) && equal(diskConfig, other.diskConfig)
+ && equal(adminPass, other.adminPass) && equal(networks, other.networks) && equal(availabilityZone, other.availabilityZone);
@demobox
jclouds member
demobox added a note Oct 15, 2013

[minor] Move the last equal to a new line?

@inbarsto
inbarsto added a note Oct 16, 2013

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@demobox demobox and 1 other commented on an outdated diff Oct 15, 2013
.../openstack/nova/v2_0/options/CreateServerOptions.java
@@ -150,6 +148,8 @@ protected ToStringHelper string() {
toString.add("userData", userData == null ? null : new String(userData));
if (!networks.isEmpty())
toString.add("networks", networks);
+ if (availabilityZone != null)
+ toString.add("availability_zone", availabilityZone);
@demobox
jclouds member
demobox added a note Oct 15, 2013

Any reason why the logic here is different than for userData just above?

@inbarsto
inbarsto added a note Oct 16, 2013

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@demobox demobox commented on the diff Oct 15, 2013
.../openstack/nova/v2_0/options/CreateServerOptions.java
@@ -354,12 +367,12 @@ public CreateServerOptions securityGroupNames(Iterable<String> securityGroupName
}
/**
- * When you create a server from an image with the diskConfig value set to
- * {@link Server#DISK_CONFIG_AUTO}, the server is built with a single partition that is expanded to
- * the disk size of the flavor selected. When you set the diskConfig attribute to
- * {@link Server#DISK_CONFIG_MANUAL}, the server is built by using the partition scheme and file
+ * When you create a server from an image with the diskConfig value set to
+ * {@link Server#DISK_CONFIG_AUTO}, the server is built with a single partition that is expanded to
+ * the disk size of the flavor selected. When you set the diskConfig attribute to
+ * {@link Server#DISK_CONFIG_MANUAL}, the server is built by using the partition scheme and file
@demobox
jclouds member
demobox added a note Oct 15, 2013

Spurious change?

@inbarsto
inbarsto added a note Oct 16, 2013

yes

@demobox
jclouds member
demobox added a note Oct 16, 2013

In that case, can we remove it from the PR? Not a big deal, so no need to worry about this if it's difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@demobox demobox and 1 other commented on an outdated diff Oct 15, 2013
...ack/nova/v2_0/extensions/CreateServerApiLiveTest.java
+
+import static org.testng.Assert.assertEquals;
+
+@Test(groups = "live", testName = "CreateServerApiLiveTest", singleThreaded = true)
+public class CreateServerApiLiveTest extends BaseNovaApiLiveTest {
+ private ServerApi serverApi;
+ private String zone;
+
+ @BeforeGroups(groups = {"integration", "live"})
+ public void setupContext() {
+ super.setup();
+ zone = Iterables.getLast(api.getConfiguredZones(), "nova");
+ serverApi = api.getServerApiForZone(zone);
+ }
+
+ private Server createServer(String regionId, String availabilityZoneId, boolean shouldWork) {
@demobox
jclouds member
demobox added a note Oct 15, 2013

Replace the "magic boolean" with an expectedState parameter? I think that would make the test easier to read...

@inbarsto
inbarsto added a note Oct 16, 2013

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@demobox demobox and 1 other commented on an outdated diff Oct 15, 2013
...ack/nova/v2_0/extensions/CreateServerApiLiveTest.java
+ options = options.availabilityZone(availabilityZoneId);
+ ServerCreated server = serverApi.create(hostName, imageIdForZone(regionId), flavorRefForZone(regionId), options);
+ if (shouldWork) {
+ blockUntilServerInState(server.getId(), serverApi, Server.Status.ACTIVE);
+ } else {
+ blockUntilServerInState(server.getId(), serverApi, Server.Status.ERROR);
+ }
+
+ return serverApi.get(server.getId());
+ }
+
+ @Test
+ public void testCreateInAvailabilityZone() {
+
+ String server_id1 = null;
+ String server_id2 = null;
@demobox
jclouds member
demobox added a note Oct 15, 2013

Use Java variable naming, e.g. "serverId1"?

@inbarsto
inbarsto added a note Oct 16, 2013

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@demobox demobox and 1 other commented on an outdated diff Oct 15, 2013
...ack/nova/v2_0/extensions/CreateServerApiLiveTest.java
+ private Server createServer(String regionId, String availabilityZoneId, boolean shouldWork) {
+ ServerApi serverApi = api.getServerApiForZone(regionId);
+ CreateServerOptions options = new CreateServerOptions();
+ options = options.availabilityZone(availabilityZoneId);
+ ServerCreated server = serverApi.create(hostName, imageIdForZone(regionId), flavorRefForZone(regionId), options);
+ if (shouldWork) {
+ blockUntilServerInState(server.getId(), serverApi, Server.Status.ACTIVE);
+ } else {
+ blockUntilServerInState(server.getId(), serverApi, Server.Status.ERROR);
+ }
+
+ return serverApi.get(server.getId());
+ }
+
+ @Test
+ public void testCreateInAvailabilityZone() {
@demobox
jclouds member
demobox added a note Oct 15, 2013

Can we split this into two tests - one that should succeed and one that should fail? If this one fails, we now don't know what the problem is.

@inbarsto
inbarsto added a note Oct 16, 2013

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@demobox
jclouds member
demobox commented Oct 15, 2013

Thanks for the PR, @inbarsto. In general: could you keep the formatting and other "misc" changes that are not related to this functionality to a separate PR? That makes it easy to concentrate on new stuff during the review, and we are also not mixing cleanup with new functionality in one PR.

Also: could you add a JIRA issue for the new functionality, and rename the commit to reference it? Thanks!

@inbarsto

added a jira ticket please let me know if i need to change add anything since this is my first jira ticket.
also i noticed i cant assign my self to the task, any idea why?

https://issues.apache.org/jira/browse/JCLOUDS-349

@inbarsto inbarsto added a commit that referenced this pull request Oct 16, 2013
@inbarsto inbarsto fixes after review for PR #180 3459b24
@cloudbees-pull-request-builder

jclouds-pull-requests #316 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #778 SUCCESS
This pull request looks good

@buildhive

jclouds » jclouds #525 SUCCESS
This pull request looks good
(what's this?)

@demobox demobox commented on an outdated diff Oct 16, 2013
.../openstack/nova/v2_0/options/CreateServerOptions.java
@@ -150,6 +149,7 @@ protected ToStringHelper string() {
toString.add("userData", userData == null ? null : new String(userData));
if (!networks.isEmpty())
toString.add("networks", networks);
+ toString.add("availavility_zone", availabilityZone == null ? null : availabilityZone);
@demobox
jclouds member
demobox added a note Oct 16, 2013

Typo? "availavility_zone"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@demobox
jclouds member
demobox commented Oct 16, 2013

Thanks for the fixes, @inbarsto! Almost there, I think ;-)

@cloudbees-pull-request-builder

jclouds-pull-requests #318 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #780 SUCCESS
This pull request looks good

@buildhive

jclouds » jclouds #527 SUCCESS
This pull request looks good
(what's this?)

@demobox
jclouds member
demobox commented Oct 16, 2013

Some minor spurious changes still here but that shouldn't stop this PR, in my opinion. +1 - looks good to me.

@inbarsto: could you create a JIRA issue and squash-n-rebase your commits, mentioning the issue number in the new commit message? Thanks!

@cloudbees-pull-request-builder

jclouds-pull-requests #322 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #784 SUCCESS
This pull request looks good

@demobox
jclouds member
demobox commented Oct 16, 2013

Thanks! But we now have 6 commits where we really want only 1 :-( Could you have a look at squashing them?

@cloudbees-pull-request-builder

jclouds-pull-requests #325 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #787 SUCCESS
This pull request looks good

@inbarsto

i am having problem with the squash basically all the changes for master are in the commit JCLOUDS-349 … 64e8308

can you take them from there, i am having problems reverting the remote branch to its original state...

@demobox
jclouds member
demobox commented Oct 16, 2013

can you take them from there, i am having problems reverting the remote branch to its original state...

OK, I'll try and take the diff from there later today (but of a rush here right now). Thanks!

@inbarsto

you now have only one squashed commit on the remote branch...

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #788 FAILURE
Looks like there's a problem with this pull request

@buildhive

jclouds » jclouds #531 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@cloudbees-pull-request-builder

jclouds-pull-requests #326 SUCCESS
This pull request looks good

@demobox
jclouds member
demobox commented Oct 16, 2013

Let me close-n-reopen to trigger the CI builds again...thanks, @inbarsto!

@demobox demobox closed this Oct 16, 2013
@demobox demobox reopened this Oct 16, 2013
@cloudbees-pull-request-builder

jclouds-pull-requests #329 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #792 SUCCESS
This pull request looks good

@everett-toews everett-toews commented on an outdated diff Oct 16, 2013
...ack/nova/v2_0/extensions/CreateServerApiLiveTest.java
+ * @author Inbar Stolberg
+ */
+
+import com.google.common.base.Optional;
+import com.google.common.collect.Iterables;
+import org.jclouds.openstack.nova.v2_0.domain.Server;
+import org.jclouds.openstack.nova.v2_0.domain.ServerCreated;
+import org.jclouds.openstack.nova.v2_0.features.ServerApi;
+import org.jclouds.openstack.nova.v2_0.internal.BaseNovaApiLiveTest;
+import org.jclouds.openstack.nova.v2_0.options.CreateServerOptions;
+import org.testng.annotations.BeforeGroups;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+@Test(groups = "live", testName = "CreateServerApiLiveTest", singleThreaded = true)
@everett-toews
jclouds member

These tests should be in ServerApiLiveTest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@everett-toews
jclouds member

I ran the live test successfully. Just a few things left to do.

  • Move live tests to ServerApiLiveTest
  • Add an expect test to org.jclouds.openstack.nova.v2_0.features.ServerApiExpectTest
  • Squash down the new commits
@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #794 SUCCESS
This pull request looks good

@inbarsto inbarsto JCLOUDS-349
createServerByZone squashed commit
create server in a selected availability zone + live and expect tests
9f6e8d9
@buildhive

jclouds » jclouds #534 SUCCESS
This pull request looks good
(what's this?)

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #795 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

jclouds-pull-requests #332 SUCCESS
This pull request looks good

@buildhive

jclouds » jclouds #535 SUCCESS
This pull request looks good
(what's this?)

@inbarsto

done :)

@demobox
jclouds member
demobox commented Oct 17, 2013

@everett-toews This good to go for you?

@inbarsto
@demobox
jclouds member
demobox commented Oct 18, 2013

good to go :)

Thanks for confirming, @inbarsto. I was actually checking with @everett-toews, since he was the one that added the last set of "things left to do" ;-)

@inbarsto
@everett-toews
jclouds member

I love the enthusiasm! Nice work @inbarsto

+1

Merged!

@everett-toews
jclouds member

@inbarsto Go ahead and close the JIRA issue too.

@inbarsto
@Bhathiya90 Bhathiya90 pushed a commit to Bhathiya90/jclouds that referenced this pull request Mar 20, 2014
@hsbhathiya hsbhathiya MD5 Audit #180 -First Commit 0dd4e57
@Bhathiya90 Bhathiya90 pushed a commit to Bhathiya90/jclouds that referenced this pull request Mar 20, 2014
@hsbhathiya hsbhathiya #180 MD5 Audit 5211ea7
@Bhathiya90 Bhathiya90 pushed a commit to Bhathiya90/jclouds that referenced this pull request Mar 28, 2014
@hsbhathiya hsbhathiya Audit MD5 - #180 5920fc8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.