You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by jasdeep-hundal <no...@github.com> on 2014/03/25 22:35:45 UTC
[jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Fixes: https://issues.apache.org/jira/browse/JCLOUDS-514
Looking for comments to get going in the right direction with this, still needs:
- Tests
- Possibly a way to access this through the ComputeService API? Not necessary for my use case, but if it's easy I'm willing to help out here.
You can merge this Pull Request by running:
git pull https://github.com/jasdeep-hundal/jclouds nova_attach_volume_at_boot
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/326
-- Commit Summary --
* Add ability to attach block volumes at boot through the Nova ServerApi
-- File Changes --
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/options/CreateServerOptions.java (54)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/326.patch
https://github.com/jclouds/jclouds/pull/326.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -155,6 +178,8 @@ protected ToStringHelper string() {
> toString.add("networks", networks);
> toString.add("availability_zone", availabilityZone == null ? null : availabilityZone);
> toString.add("configDrive", configDrive);
> + if (!blockDeviceMapping.isEmpty())
> I think he should stick with mine and your preference and go with isEmpty
Yay ;-) Thanks, @everett-toews!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16819205
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-6 #37](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/37/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53895461
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
Reopened #326.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#event-166730008
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> + * A representation of a block device that should be attached to the Nova instance to be launched
> + *
> + */
> +public class BlockDeviceMapping {
> +
> + @Named("delete_on_termination")
> + String deleteOnTermination = "0";
> + @Named("device_name")
> + String deviceName = null;
> + @Named("volume_id")
> + String volumeId = null;
> + @Named("volume_size")
> + String volumeSize = "";
> +
> + @ConstructorProperties({"volume_id", "volume_size", "device_name", "delete_on_termination"})
> + private BlockDeviceMapping(String volumeId, String volumeSize, String deviceName, String deleteOnTermination) {
Domain object constructors should be `protected`. This applies to the other constructors as well.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r17753990
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
> @@ -102,6 +103,27 @@ public String toString() {
>
> }
>
> + public static class BlockDevice{
Going to work through getting this running through the ComputeServiceApi this week, should have a good answer for you then. Right now, I'm not sure.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999079
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
@demobox @everett-toews : It's been a while, but finally got the tests up for this PR, would much appreciate another review.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-42706969
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
Sounds good to me, thanks @jdaggett !
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-56116944
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Everett Toews <no...@github.com>.
> @@ -102,6 +103,27 @@ public String toString() {
>
> }
>
> + public static class BlockDevice{
> + @Named("volume_size")
> + String volumeSize = "";
> + @Named("volume_id")
> + String volumeId;
> + @Named("delete_on_termination")
> + int deleteOnTermination = 0;
> + @Named("device_name")
> + String deviceName;
> +
> + public BlockDevice(String volumeId, String deviceName){
> + this(volumeId, deviceName, true);
> + }
> +
> + public BlockDevice(String volumeId, String deviceName, boolean deleteWhenInstanceTerminated){
We don't typically expose ctors like here and above directly. These should be factory methods.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999095
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -150,7 +154,53 @@ public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> if (server_id != null)
> api.getServerApiForZone(zone).delete(server_id);
> }
> + }
> + }
> +
> + @Test(dependsOnMethods = "testCreateVolume")
> + public void testAttachmentAtBoot() {
> + if (volumeApi.isPresent()) {
> + String server_id = null;
> + try {
> + CreateServerOptions createServerOptions =
> + CreateServerOptions.Builder.blockDeviceMapping(
> + Collections.singletonList(new BlockDeviceMapping().deviceName("/dev/vdf").volumeId(testVolume.getId())));
`ImmutableList.of(...)` rather than `singletonList`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12544857
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
Hrmph, locally only got 21 "Redundant 'public' modifier" warnings in a file I didn't touch, I'll just reverify the license and send a new commit up.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53935134
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -113,6 +135,7 @@ public String toString() {
> private Set<Network> novaNetworks = ImmutableSet.of();
> private String availabilityZone;
> private boolean configDrive;
> + private List<BlockDevice> blockDeviceMapping = Lists.newArrayList();
`ImmutableList.of()`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999274
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -113,6 +161,7 @@ public String toString() {
> private Set<Network> novaNetworks = ImmutableSet.of();
> private String availabilityZone;
> private boolean configDrive;
> + private List<BlockDeviceMapping> blockDeviceMapping = Lists.newArrayList();
Is there a reason not to use `Set<BlockDeviceMapping>` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16251823
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
@jasdeep-hundal Ping! Is there anything else that needs to be done with this PR?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-54337653
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
>
> + assertTrue(foundIt, "Failed to find the attachment we set in listAttachments() response");
Include the attachment we were looking for, and the response, in the failure message?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12545045
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
>
> + assertTrue(foundIt, "Failed to find the attachment we set in listAttachments() response");
> +
> + volumeAttachmentApi.get().detachVolumeFromServer(testVolume.getId(), serverId);
> + assertTrue(retry(new Predicate<VolumeAttachmentApi>() {
> + public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> + return volumeAttachmentApi.listAttachmentsOnServer(serverId).size() == before - 1;
> + }
> + }, 60 * 1000L).apply(volumeAttachmentApi.get()));
This is not so easy to understand. Could we not simply wait here until the operation has completed and _then_ check that the size is "before - 1"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12545121
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-6 #36](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/36/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53895373
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
>
> + assertTrue(foundIt, "Failed to find the attachment we set in listAttachments() response");
> +
> + volumeAttachmentApi.get().detachVolumeFromServer(testVolume.getId(), serverId);
Would it make sense to move this into a separate `testDetachVolume` test that depends on this one?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12545073
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
@jasdeep-hundal Weird... Try copying the ASF header from another file and run checkstyle locally to see if the issue persists.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53920481
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
> + String deviceName = null;
> +
> + /**
> + * @param volumeId ID of the volume within the block store that you wish to attach
> + */
> + public BlockDeviceMapping volumeId(String volumeId){
> + this.volumeId = volumeId;
> + return this;
> + }
> +
> + /**
> + * @param deleteOnTermination True if this volume should be deleted when the instance is terminated
> + */
> + public BlockDeviceMapping deleteOnTermination(boolean deleteOnTermination){
> + // Nova expects this to be a string
> + this.deleteOnTermination = deleteOnTermination ? "1" : "0";
@demobox @jdaggett Since I'm moving BlockDeviceMapping to its on class, is it now worth it to create a custom mapping for the strings here and instead use true/false internally within JClouds?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16900706
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -85,9 +86,9 @@ protected Properties setupProperties() {
> return props;
> }
>
> - protected Server createServerInZone(String zoneId) {
> + protected Server createServerInZone(String zoneId, CreateServerOptions... options) {
Please remove the varargs `CreateServerOptions...` as we have decided to move away from them with Option classes, thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16251461
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for taking care of this @jasdeep-hundal!
To add it to the compute service api, what I'd do is to extend the [TemplateOptions](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/options/TemplateOptions.java) class by adding a method that accepts a list of [Volumes](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/domain/Volume.java). Then you can modify the [NovaComputeServiceAdapter](https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/NovaComputeServiceAdapter.java) to read the volumes from the template options, transform them to `BlockDevices` and build the `CreateServerOptions` accordingly.
This approach will open the door to do the same for other providers such as AWS or GCE.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-38629376
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> +import com.google.common.base.Objects;
> +
> +/**
> + * A representation of a block device that should be attached to the Nova instance to be launched
> + *
> + */
> +public class BlockDeviceMapping {
> +
> + @Named("delete_on_termination")
> + String deleteOnTermination = "0";
> + @Named("device_name")
> + String deviceName = null;
> + @Named("volume_id")
> + String volumeId = null;
> + @Named("volume_size")
> + String volumeSize = "";
3 space indent for lines 35-42
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r17753942
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-6 #28](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/28/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53511101
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -102,6 +103,27 @@ public String toString() {
>
> }
>
> + public static class BlockDevice{
> + @Named("volume_size")
> + String volumeSize = "";
> + @Named("volume_id")
> + String volumeId;
> + @Named("delete_on_termination")
> + int deleteOnTermination = 0;
> + @Named("device_name")
> + String deviceName;
> +
> + public BlockDevice(String volumeId, String deviceName){
[minor] Space before `{`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999123
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> + String deviceName = null;
> +
> + /**
> + * @param volumeId ID of the volume within the block store that you wish to attach
> + */
> + public BlockDeviceMapping volumeId(String volumeId){
> + this.volumeId = volumeId;
> + return this;
> + }
> +
> + /**
> + * @param deleteOnTermination True if this volume should be deleted when the instance is terminated
> + */
> + public BlockDeviceMapping deleteOnTermination(boolean deleteOnTermination){
> + // Nova expects this to be a string
> + this.deleteOnTermination = deleteOnTermination ? "1" : "0";
[minor] Worth extracting `"0"` and `"1"` into constants here? What do you think, @jdaggett?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16819190
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> + String volumeSize = "";
> + @Named("volume_id")
> + String volumeId;
> + @Named("delete_on_termination")
> + int deleteOnTermination = 0;
> + @Named("device_name")
> + String deviceName;
> +
> + public BlockDevice(String volumeId, String deviceName){
> + this(volumeId, deviceName, true);
> + }
> +
> + public BlockDevice(String volumeId, String deviceName, boolean deleteWhenInstanceTerminated){
> + this.volumeId = volumeId;
> + this.deviceName = deviceName;
> + this.deleteOnTermination = deleteWhenInstanceTerminated ? 1 : 0;
No need to use a different parameter name here. @everett-toews: would you normally do something like creating constants for 0 and 1 here? Or is there some kind of "boolean to string JSON mapper" we could use?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999246
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
:+1: Thanks @jasdeep-hundal Closing with replacement PR #527
I really appreciate the work that you did with the new builders! Great job! Unfortunately, we will need to revert to them to the old style `Option` classes. The problem is that if we introduce a single domain object that uses the pattern in Nova, it will really confuse our users. :-\ I will make those changes in the [boot from volume](https://issues.apache.org/jira/browse/JCLOUDS-281) PR.
That said, any *new* APIs we develop should definitely use the new pattern! In moving to this new style, it really needs to be impl'd for the entire API across the board. We will definitely consider it when we look at the Nova v2.1 API.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-56117922
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
@jdaggett : What's the right way to do that? Not too familiar w/ maven...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53927624
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1179](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1179/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-55925551
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1568](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1568/) 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/326#issuecomment-53900657
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1116](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1116/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53510826
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1563](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1563/) ABORTED
[(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/326#issuecomment-53668729
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Everett Toews <no...@github.com>.
> @@ -155,6 +178,8 @@ protected ToStringHelper string() {
> toString.add("networks", networks);
> toString.add("availability_zone", availabilityZone == null ? null : availabilityZone);
> toString.add("configDrive", configDrive);
> + if (!blockDeviceMapping.isEmpty())
I think he should stick with mine and your preference and go with isEmpty
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999478
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1263](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1263/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-42714816
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #949](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/949/) 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/326#issuecomment-38632347
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -150,7 +154,53 @@ public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> if (server_id != null)
> api.getServerApiForZone(zone).delete(server_id);
> }
> + }
> + }
> +
> + @Test(dependsOnMethods = "testCreateVolume")
> + public void testAttachmentAtBoot() {
> + if (volumeApi.isPresent()) {
> + String server_id = null;
> + try {
> + CreateServerOptions createServerOptions =
> + CreateServerOptions.Builder.blockDeviceMapping(
> + Collections.singletonList(new BlockDeviceMapping().deviceName("/dev/vdf").volumeId(testVolume.getId())));
@jasdeep-hundal Please address this comment, thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16251599
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1163](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1163/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-38629844
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> + public BlockDeviceMapping volumeId(String volumeId){
> + this.volumeId = volumeId;
> + return this;
> + }
> +
> + /**
> + * @param deleteOnTermination True if this volume should be deleted when the instance is terminated
> + */
> + public BlockDeviceMapping deleteOnTermination(boolean deleteOnTermination){
> + // Nova expects this to be a string
> + this.deleteOnTermination = deleteOnTermination ? "1" : "0";
> + return this;
> + }
> +
> + /**
> + * Set the name of the device on which to mount this block volume.
[minor] "Sets..."
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16819195
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-6 #90](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/90/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-55921542
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
Scratch that, swapping in the license from several different files generated no diff :(
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53935311
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> + String volumeSize = "";
> + @Named("volume_id")
> + String volumeId;
> + @Named("delete_on_termination")
> + int deleteOnTermination = 0;
> + @Named("device_name")
> + String deviceName;
> +
> + public BlockDevice(String volumeId, String deviceName){
> + this(volumeId, deviceName, true);
> + }
> +
> + public BlockDevice(String volumeId, String deviceName, boolean deleteWhenInstanceTerminated){
> + this.volumeId = volumeId;
> + this.deviceName = deviceName;
> + this.deleteOnTermination = deleteWhenInstanceTerminated ? 1 : 0;
> You could do a mapper or binder but I honestly don't think all that code is worth it in this case.
If we don't have one off the shelf that we could simply plug in - fully agree. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999762
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> + final String serverId = server_id = createServerInZone(zone, createServerOptions).getId();
> +
> + Set<? extends VolumeAttachment> attachments = volumeAttachmentApi.get().listAttachmentsOnServer(serverId).toSet();
> + assertNotNull(attachments);
> +
> + assertEquals(volumeApi.get().get(testVolume.getId()).getStatus(), Volume.Status.IN_USE);
> +
> + final int before = attachments.size();
> + boolean foundIt = false;
> + for (VolumeAttachment att : attachments) {
> + VolumeAttachment details = volumeAttachmentApi.get()
> + .getAttachmentForVolumeOnServer(att.getVolumeId(), serverId);
> + assertNotNull(details);
> + assertNotNull(details.getId());
> + assertNotNull(details.getServerId());
> + assertNotNull(details.getVolumeId());
@jasdeep-hundal Take a look at the checks [here](https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/extensions/VolumeAttachmentApiExpectTest.java#L61).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16252374
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
@jdaggett : Still have to resolve the checkstyle issue, but kind of stuck because there was no diff when I copied the license header from another file. Otherwise I think it's ready for another review.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-54343262
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Everett Toews <no...@github.com>.
> @@ -102,6 +103,27 @@ public String toString() {
>
> }
>
> + public static class BlockDevice{
Are you confident that BlockDevice will only ever be used within the context of CreateServerOptions?
Also there should be a space before the {
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10998875
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1081](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1081/) 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/326#issuecomment-42711901
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
@jdaggett : Comments should be addressed. Thanks for the review!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53521162
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
> @@ -150,7 +154,53 @@ public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> if (server_id != null)
> api.getServerApiForZone(zone).delete(server_id);
> }
> + }
> + }
> +
> + @Test(dependsOnMethods = "testCreateVolume")
> + public void testAttachmentAtBoot() {
> + if (volumeApi.isPresent()) {
> + String server_id = null;
> + try {
> + CreateServerOptions createServerOptions =
> + CreateServerOptions.Builder.blockDeviceMapping(
> + Collections.singletonList(new BlockDeviceMapping().deviceName("/dev/vdf").volumeId(testVolume.getId())));
> + final String serverId = server_id = createServerInZone(zone, createServerOptions).getId();
@demobox : For this and most of your other comments: mostly tried to stick close to the original volume attachment test (for postboot attachment) because I figured I wouldn't know all the decisions behind the writing of the test. Perfectly happy to make the corresponding changes over the entire class if you think that's a good idea. (Though I do like this particular bit of Java cuteness; it's a pretty clean way of ensuring that we have some value for the server id in our "finally" clause even though we want to treat it as a final variable in the "try").
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12566254
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
> @@ -102,6 +103,27 @@ public String toString() {
>
> }
>
> + public static class BlockDevice{
Will go that route, and I'm fine with that name change (I actually started out with that and then noticed that Nova called the entire list of block device mappings a block device mapping itself.)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r11002003
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #793](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/793/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-42713978
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -150,7 +154,53 @@ public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> if (server_id != null)
> api.getServerApiForZone(zone).delete(server_id);
> }
> + }
> + }
> +
> + @Test(dependsOnMethods = "testCreateVolume")
> + public void testAttachmentAtBoot() {
> + if (volumeApi.isPresent()) {
> + String server_id = null;
> + try {
> + CreateServerOptions createServerOptions =
> + CreateServerOptions.Builder.blockDeviceMapping(
> + Collections.singletonList(new BlockDeviceMapping().deviceName("/dev/vdf").volumeId(testVolume.getId())));
> + final String serverId = server_id = createServerInZone(zone, createServerOptions).getId();
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12544861
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Everett Toews <no...@github.com>.
> + String volumeSize = "";
> + @Named("volume_id")
> + String volumeId;
> + @Named("delete_on_termination")
> + int deleteOnTermination = 0;
> + @Named("device_name")
> + String deviceName;
> +
> + public BlockDevice(String volumeId, String deviceName){
> + this(volumeId, deviceName, true);
> + }
> +
> + public BlockDevice(String volumeId, String deviceName, boolean deleteWhenInstanceTerminated){
> + this.volumeId = volumeId;
> + this.deviceName = deviceName;
> + this.deleteOnTermination = deleteWhenInstanceTerminated ? 1 : 0;
I'm okay with the way it's implemented here. You could do a mapper or binder but I honestly don't think all that code is worth it in this case.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999427
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
> @@ -150,7 +154,53 @@ public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> if (server_id != null)
> api.getServerApiForZone(zone).delete(server_id);
> }
> + }
> + }
> +
> + @Test(dependsOnMethods = "testCreateVolume")
> + public void testAttachmentAtBoot() {
> + if (volumeApi.isPresent()) {
I notice this pattern in a lot of tests, and am in fact in favor of a general review of where it's used and make sure to skip the tests instead of passing them. Is it the right idea to do that here, or should I maintain consistency (mostly to make sure that the general solution does not get missed in this class as well)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12566182
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
@jasdeep-hundal Oh, no magic there! Just copy the license header from another Java file :+1:
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53930325
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -98,6 +98,51 @@ public String toString() {
>
> }
>
> + /**
> + * A representation of block device that should be attached to the instance to be launched
> + *
> + */
> + public static class BlockDeviceMapping {
I would extract this class into its own domain class `org.jclouds.openstack.nova.v2_0.domain.BlockDeviceMapping` similar to other domain classes. Refer to the Neutron [Network](https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2/domain/Network.java) as an example of how domain classes are being modeled going forward.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16849785
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
Oh, was wondering about the running checkstyle.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53931297
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -155,6 +178,8 @@ protected ToStringHelper string() {
> toString.add("networks", networks);
> toString.add("availability_zone", availabilityZone == null ? null : availabilityZone);
> toString.add("configDrive", configDrive);
> + if (!blockDeviceMapping.isEmpty())
Much as I prefer `!isEmpty` over `size() > 0`, the latter is used everywhere else in this method, so switch to that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999425
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
@jasdeep-hundal Please disregard the last two comments... I have a dependency on this PR for implementing Nova boot from volume and really need these changes in asap.
What I am going to do is fetch your branch, squash the commits you have provided and submit a new PR. Once that is done, I will reference this work in the new PR and close it. You will still be the author, but it will show me as the committer.
Thanks again for the contribution, we really appreciate it, and please let me know if you have any questions! :+1:
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-56115950
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -150,7 +154,53 @@ public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> if (server_id != null)
> api.getServerApiForZone(zone).delete(server_id);
> }
> + }
> + }
> +
> + @Test(dependsOnMethods = "testCreateVolume")
> + public void testAttachmentAtBoot() {
> + if (volumeApi.isPresent()) {
> most of the Nova Live Extension API tests follow this pattern
Lazy me...I was only looking at the diff ;-) Fine to keep as-is, and thanks for explaining, @jasdeep-hundal and @jdaggett!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16816980
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
Can I also get some help w/ this checkstyle violation? https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1125/org.apache.jclouds.api$openstack-nova/violations/file/src/main/java/org/jclouds/openstack/nova/v2_0/domain/BlockDeviceMapping.java/
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53917655
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Everett Toews <no...@github.com>.
I won't be able to review this as I'll be heading out the OpenStack Summit this weekend. Are you going to be there too @jasdeep-hundal?
It'd be great to meet you!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-42717032
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-6 #34](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/34/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53661934
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
>
> + assertTrue(foundIt, "Failed to find the attachment we set in listAttachments() response");
> +
> + volumeAttachmentApi.get().detachVolumeFromServer(testVolume.getId(), serverId);
> + assertTrue(retry(new Predicate<VolumeAttachmentApi>() {
> + public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> + return volumeAttachmentApi.listAttachmentsOnServer(serverId).size() == before - 1;
> + }
> + }, 60 * 1000L).apply(volumeAttachmentApi.get()));
> +
> + } finally {
> + if (server_id != null)
[minor] Add `{...}` after the if?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12545128
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -463,6 +494,21 @@ public CreateServerOptions networks(String... networks) {
> return networks(ImmutableSet.copyOf(networks));
> }
>
> + /**
> + * Block volumes that should be attached to the instance at boot time
> + */
> + public List<BlockDevice> getBlockDeviceMapping() {
> + return blockDeviceMapping;
> + }
> +
> + /**
> + * @see #getBlockDeviceMapping
> + */
> + public CreateServerOptions blockDeviceMapping(List<BlockDevice> blockDeviceMapping) {
> + this.blockDeviceMapping = blockDeviceMapping;
`ImmutableList.copyOf(blockDeviceMapping)`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999736
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -113,6 +161,7 @@ public String toString() {
> private Set<Network> novaNetworks = ImmutableSet.of();
> private String availabilityZone;
> private boolean configDrive;
> + private List<BlockDeviceMapping> blockDeviceMapping = Lists.newArrayList();
`ImmutableList.of()`? I don't see any way to add anything here, except through `ImmutableList.copyOf(...)` later?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12544762
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -150,7 +154,53 @@ public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> if (server_id != null)
> api.getServerApiForZone(zone).delete(server_id);
> }
> + }
> + }
> +
> + @Test(dependsOnMethods = "testCreateVolume")
> + public void testAttachmentAtBoot() {
> + if (volumeApi.isPresent()) {
@demobox As @jasdeep-hundal pointed out, most of the Nova Live Extension API tests follow this pattern, so this should be fine for the time being. In the work I am doing with the `BaseOpenStackLiveApiTest` many of the live tests will be updated.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16251620
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1569](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1569/) 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/326#issuecomment-53907704
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
Closed #326.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#event-167476755
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> import org.jclouds.openstack.nova.v2_0.parse.ParseServerDiagnostics;
>
> /**
> * Tests annotation parsing of {@code ServerAsyncApi}
> *
> * @author Adrian Cole
> + * @author Jasdeep Hundal
Please remove `@author` tags
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16251515
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
@demobox : It's been a while and I dropped the tracking on this, but in the interest of clearing this out, could you clarify a couple things for me here?: https://github.com/jclouds/jclouds/pull/326#discussion_r12566182
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-49822823
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1642](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1642/) 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/326#issuecomment-55934555
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1123](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1123/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53665626
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -40,6 +43,7 @@
> * Tests behavior of Volume Attachment API
> *
> * @author Everett Toews
> + * @author Jasdeep Hundal
Please remove `@author`, thx
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16251663
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -49,6 +50,7 @@
> * @author Adrian Cole
> * @author Inbar Stolberg
> * @author Zack Shoylev
> + * @author Jasdeep Hundal
Please remove `@author`, thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16251878
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
@demobox @jdaggett : Should be relatively ready for another review, besides the question I added in the comments. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53895323
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-6 #27](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/27/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53505930
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
Ha! All you need to do is run `mvn checkstyle:check`, or `mvn checkstyle:checkstyle` if you want a report. HTH
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53933620
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -463,6 +520,23 @@ public CreateServerOptions networks(String... networks) {
> return networks(ImmutableSet.copyOf(networks));
> }
>
> + /**
> + * @see #getBlockDeviceMapping
> + */
> + public CreateServerOptions blockDeviceMapping(List<BlockDeviceMapping> blockDeviceMapping) {
> + this.blockDeviceMapping = ImmutableList.copyOf(blockDeviceMapping);
> + return this;
> + }
> +
> + /**
> + * Block volumes that should be attached to the instance at boot time.
> + *
> + * @see <a href="http://docs.openstack.org/trunk/openstack-ops/content/attach_block_storage.html">Attach Block Storage<a/>
Sources should not have external links. Please remove!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16251709
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1125](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1125/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53902833
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Everett Toews <no...@github.com>.
> @@ -102,6 +103,27 @@ public String toString() {
>
> }
>
> + public static class BlockDevice{
My suggestion would be to get it running properly in the NovaApi with expect tests and live tests first. That stuff really needs to be done before you can add proper support in the ComputeService anyway. We'll wind up with a better feature this way.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r11000148
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1555](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1555/) ABORTED
[(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/326#issuecomment-53520895
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1117](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1117/) ABORTED
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53658433
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
>
> + assertTrue(foundIt, "Failed to find the attachment we set in listAttachments() response");
> +
> + volumeAttachmentApi.get().detachVolumeFromServer(testVolume.getId(), serverId);
> + assertTrue(retry(new Predicate<VolumeAttachmentApi>() {
> + public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> + return volumeAttachmentApi.listAttachmentsOnServer(serverId).size() == before - 1;
> + }
> + }, 60 * 1000L).apply(volumeAttachmentApi.get()));
No idea what I was thinking here, looking over the API docs, the detach volume from server function should only return when the volume has successfully detached. Fixed in latest commit.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16751282
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
>
> + assertTrue(foundIt, "Failed to find the attachment we set in listAttachments() response");
> +
> + volumeAttachmentApi.get().detachVolumeFromServer(testVolume.getId(), serverId);
Not sure I agree with that. For me this is clearer as it tests the whole fea/prevents us from having to preserve info about the server between the tests and I think that the stack trace should be enough to indicate where the problem is. The other tests here require volume detachment as well.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r16750604
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Everett Toews <no...@github.com>.
We'll also need more Javadoc for all of the parameters that can be used to create a BlockDevice.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-38742092
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> + final String serverId = server_id = createServerInZone(zone, createServerOptions).getId();
> +
> + Set<? extends VolumeAttachment> attachments = volumeAttachmentApi.get().listAttachmentsOnServer(serverId).toSet();
> + assertNotNull(attachments);
> +
> + assertEquals(volumeApi.get().get(testVolume.getId()).getStatus(), Volume.Status.IN_USE);
> +
> + final int before = attachments.size();
> + boolean foundIt = false;
> + for (VolumeAttachment att : attachments) {
> + VolumeAttachment details = volumeAttachmentApi.get()
> + .getAttachmentForVolumeOnServer(att.getVolumeId(), serverId);
> + assertNotNull(details);
> + assertNotNull(details.getId());
> + assertNotNull(details.getServerId());
> + assertNotNull(details.getVolumeId());
Can we make these checks more specific? E.g. do we not know what the server or volume IDs should be?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12544993
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
@everett-toews : No problem at all, enjoy your time at the summit! I won't be there, but definitely drop me a line whenever you're in SF.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-42720201
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by jasdeep-hundal <no...@github.com>.
@jdaggett : Thanks for the review. Will make those changes when I get a bit of time, feel free to ping me if that doesn't happen before then end of August.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-52276538
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -463,6 +494,21 @@ public CreateServerOptions networks(String... networks) {
> return networks(ImmutableSet.copyOf(networks));
> }
>
> + /**
> + * Block volumes that should be attached to the instance at boot time
> + */
> + public List<BlockDevice> getBlockDeviceMapping() {
> + return blockDeviceMapping;
> + }
Can you move this method to after `blockDeviceMapping`? A cleanup of this class that untangled the current mix of getters and "builder option" methods would be nice, but that's not really part of this PR, I guess.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r10999601
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #693](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/693/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-38629321
Re: [jclouds] WIP JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Everett Toews <no...@github.com>.
> @@ -102,6 +103,27 @@ public String toString() {
>
> }
>
> + public static class BlockDevice{
Considering there's already a feature in Nova that boots from block devices using the term block-device, I think it's more appropriate to call this object BlockDeviceMapping.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r11001032
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by Andrew Phillips <no...@github.com>.
> @@ -150,7 +154,53 @@ public boolean apply(VolumeAttachmentApi volumeAttachmentApi) {
> if (server_id != null)
> api.getServerApiForZone(zone).delete(server_id);
> }
> + }
> + }
> +
> + @Test(dependsOnMethods = "testCreateVolume")
> + public void testAttachmentAtBoot() {
> + if (volumeApi.isPresent()) {
We shouldn't need this `if`? If volumeApi is _not_ present, `testCreateVolume` should hopefully fail or be skipped?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326/files#r12544835
Re: [jclouds] JCLOUDS-514: Add ability to attach block volumes at
boot through the Nova ServerApi (#326)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1126](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1126/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/326#issuecomment-53902911