Re: [jclouds] fix support for private images in SoftLayer (#568)
Closed #568. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#event-179079167
Re: [jclouds] fix support for private images in SoftLayer (#568)
@demobox, I've cherry-picked the commit onto master, shall I close the PR manually? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59170282
Re: [jclouds] fix support for private images in SoftLayer (#568)
:+1: LGTM --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59127997
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds » jclouds #1794](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1794/) 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/568#issuecomment-59052622
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1290](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1290/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59047923
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests-java-6 #201](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/201/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59042279
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds » jclouds #1793](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1793/) FAILURE Looks like there's a problem with this pull request [(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/568#issuecomment-59007053
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1289](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1289/) FAILURE 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/568#issuecomment-59004492
Re: [jclouds] fix support for private images in SoftLayer (#568)
Thanks @adriancole @demobox, I've addressed your comments and I've got a LGTM so good to merge --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59002034
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests-java-6 #200](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/200/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59001837
Re: [jclouds] fix support for private images in SoftLayer (#568)
PS @andreaturli since you are a committer, once you have a LGTM, cherry-pick this into master and 1.8.x on your own. You have the power! --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58916605
Re: [jclouds] fix support for private images in SoftLayer (#568)
> @@ -67,15 +67,18 @@ public void testLaunchClusterWithMinDisk() throws > RunNodesException { > >TemplateBuilder templateBuilder = > context.getComputeService().templateBuilder(); >templateBuilder.imageId("CENTOS_6_64"); > + //templateBuilder.imageVersionMatches("6.5"); agreed, we don't check-in comments. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18778933
Re: [jclouds] fix support for private images in SoftLayer (#568)
Made some notes of things I care about, which addressed == LGTM --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58917002
Re: [jclouds] fix support for private images in SoftLayer (#568)
> + private final String osReferenceCode; > + > + public HasSameOsReferenceCode(String osReferenceCode) { > + this.osReferenceCode = osReferenceCode; > + } > + > + @Override > + public boolean apply(SoftwareDescription input) { > + return input.getReferenceCode().equals(osReferenceCode); > + } > + } > + > + private class IsOperatingSystem implements Predicate > { > + @Override > + public boolean apply(SoftwareDescription softwareDescription) { > + return softwareDescription.getOperatingSystem() == 1; agreed --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18778919
Re: [jclouds] fix support for private images in SoftLayer (#568)
> + }); > + if (!optionalBootableVolume.isPresent()) { > + return Type.LOCAL.ordinal(); > + } > + return optionalBootableVolume.get().getType().ordinal(); > + } > + > + private Optional tryExtractOperatingSystemFrom(final > String imageId) { > + Set operatingSystemsAvailable = > createObjectOptionsSupplier.get().getVirtualGuestOperatingSystems(); > + return tryFind(FluentIterable.from(operatingSystemsAvailable) > + .filter(new Predicate() { > + @Override > + public boolean apply(OperatingSystem input) { > +return input.getId().contains(imageId); > + } > + }), Predicates.notNull()); agreed. the not-null filter is confusing, as you would have gotten a NPE here, at `input.getId()` right? ```java +return input.getId().contains(imageId); ``` --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18778892
Re: [jclouds] fix support for private images in SoftLayer (#568)
Thanks @demobox Main reason for this cleanup is that the private image support is currently `broken` so it would be nice to have that merged soon. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58907317
Re: [jclouds] fix support for private images in SoftLayer (#568)
Thanks for this, @andreaturli! Most of my comments we can take care of during merging, I guess the main one is whether we can create singletons for the various new transformation functions this PR adds. For background: could you explain what the motivation for the PR is? Is private image support currently _broken_, or is this mainly a cleanup, or..? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58804644
Re: [jclouds] fix support for private images in SoftLayer (#568)
> @@ -67,15 +67,18 @@ public void testLaunchClusterWithMinDisk() throws > RunNodesException { > >TemplateBuilder templateBuilder = > context.getComputeService().templateBuilder(); >templateBuilder.imageId("CENTOS_6_64"); > + //templateBuilder.imageVersionMatches("6.5"); Here and below: are these commented-out on purpose? If so, remove? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18748161
Re: [jclouds] fix support for private images in SoftLayer (#568)
> + .filter(new Predicate() { > + @Override > + public boolean apply(OperatingSystem input) { > +return input.getId().contains(imageId); > + } > + }), Predicates.notNull()); > + } > + > + private Optional > tryExtractOperatingSystemFrom(VirtualGuestBlockDeviceTemplateGroup image) { > + return FluentIterable.from(image.getChildren()) > + .transformAndConcat(new > BlockDeviceTemplateGroupToBlockDeviceTemplateIterable()) > + .filter(new IsBootableDevice()) > + .transformAndConcat(new > VirtualGuestBlockDeviceTemplateIterableFunction()) > + .transform(new DiskImageSoftwareToSoftwareDescription()) > + .filter(new IsOperatingSystem()) > + .transform(new > SoftwareDescriptionToOperatingSystem(image.getGlobalIdentifier())) Do all the functions being passed here need to be instantiated each time? Or could we use a singleton for them? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18748155
Re: [jclouds] fix support for private images in SoftLayer (#568)
> + private final String osReferenceCode; > + > + public HasSameOsReferenceCode(String osReferenceCode) { > + this.osReferenceCode = osReferenceCode; > + } > + > + @Override > + public boolean apply(SoftwareDescription input) { > + return input.getReferenceCode().equals(osReferenceCode); > + } > + } > + > + private class IsOperatingSystem implements Predicate > { > + @Override > + public boolean apply(SoftwareDescription softwareDescription) { > + return softwareDescription.getOperatingSystem() == 1; Could you add a comment to describe what the value of `1` means here? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18748156
Re: [jclouds] fix support for private images in SoftLayer (#568)
> + private static class IsBootableDevice implements > Predicate { > + @Override > + public boolean apply(VirtualGuestBlockDeviceTemplate > blockDeviceTemplate) { > + return blockDeviceTemplate.getDevice().equals(BOOTABLE_DEVICE); > + } > + } > + > + private static class > BlockDeviceTemplateGroupToBlockDeviceTemplateIterable implements > Function + Iterable> { > + @Override > + public Iterable > apply(VirtualGuestBlockDeviceTemplateGroup input) { > + return input.getBlockDevices(); > + } > + } > + > + private static class VirtualGuestBlockDeviceTemplateIterableFunction > implements Function Iterable> { [minor] As with the other functions, name this so it is called `XToY`, e.g. `BlockDeviceTemplateToDiskImageSoftware` or so? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18748146
Re: [jclouds] fix support for private images in SoftLayer (#568)
> + }); > + if (!optionalBootableVolume.isPresent()) { > + return Type.LOCAL.ordinal(); > + } > + return optionalBootableVolume.get().getType().ordinal(); > + } > + > + private Optional tryExtractOperatingSystemFrom(final > String imageId) { > + Set operatingSystemsAvailable = > createObjectOptionsSupplier.get().getVirtualGuestOperatingSystems(); > + return tryFind(FluentIterable.from(operatingSystemsAvailable) > + .filter(new Predicate() { > + @Override > + public boolean apply(OperatingSystem input) { > +return input.getId().contains(imageId); > + } > + }), Predicates.notNull()); [minor] What is the `notNull()` for? Does `operatingSystemsAvailable` possible contain `null` elements? And just to check I'm understanding the logic correctly here: the image ID can be a substring of the the ID of an OperatingSystem, and that is what we are looking? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18748139
Re: [jclouds] fix support for private images in SoftLayer (#568)
> @@ -445,8 +355,66 @@ public void suspendNode(String id) { >api.getVirtualGuestApi().pauseVirtualGuest(Long.parseLong(id)); > } > > - private boolean isOperatingSystem(SoftwareDescription input) { > - return input.getOperatingSystem() == 1; > + /** > +* This method will deliberately skip device position 1 as it is reserved > to SWAP > +* @param blockDeviceCapacities list of blockDevices to be attached > +* @param diskType disks can be LOCAL or SAN > +* @return > +*/ > + private List getBlockDevices(List > blockDeviceCapacities, String diskType) { > + List blockDevices = Lists.newArrayList(); [minor] Does the result need to me a mutable list? If not, use `ImmutableList.builder()`? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18748117
Re: [jclouds] fix support for private images in SoftLayer (#568)
> @@ -445,8 +355,66 @@ public void suspendNode(String id) { >api.getVirtualGuestApi().pauseVirtualGuest(Long.parseLong(id)); > } > > - private boolean isOperatingSystem(SoftwareDescription input) { > - return input.getOperatingSystem() == 1; > + /** > +* This method will deliberately skip device position 1 as it is reserved > to SWAP > +* @param blockDeviceCapacities list of blockDevices to be attached > +* @param diskType disks can be LOCAL or SAN > +* @return > +*/ > + private List getBlockDevices(List > blockDeviceCapacities, String diskType) { [minor] Can be `static`? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18748113
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds » jclouds #1775](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1775/) 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/568#issuecomment-58687619
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1279](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1279/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58683136
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests-java-6 #190](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/190/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58680089
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds » jclouds #1774](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1774/) FAILURE Looks like there's a problem with this pull request [(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/568#issuecomment-58673274
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1278](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1278/) FAILURE 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/568#issuecomment-58670156
Re: [jclouds] fix support for private images in SoftLayer (#568)
thank @nacx I took the occasion to refactor listImages a bit more, now looks to me simpler to read and maintain. Thoughts? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58666540
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests-java-6 #189](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/189/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58666264
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1277](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1277/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58631814
Re: [jclouds] fix support for private images in SoftLayer (#568)
> private Map > extractSoftwareDescriptions(Set images) > { >Map softwareDescriptions = > Maps.newHashMap(); >for (VirtualGuestBlockDeviceTemplateGroup image : images) { > final String globalIdentifier = image.getGlobalIdentifier(); > for (VirtualGuestBlockDeviceTemplateGroup child : > image.getChildren()) { > for (VirtualGuestBlockDeviceTemplate blockDeviceTemplate : > child.getBlockDevices()) { > - for (VirtualDiskImageSoftware softwareReference : > blockDeviceTemplate.getDiskImage().getSoftwareReferences()) { > - softwareDescriptions.put(globalIdentifier, > softwareReference.getSoftwareDescription()); > + // filter for the device 0 (bootable) > + if (blockDeviceTemplate.getDevice().equals("0")) { Wirth refactoring to a reusable `isBootableDevice` predicate, and use it to filter the `child.getBlockDevices()` collection in the loop, instead of having this `if` statement? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18695190
Re: [jclouds] fix support for private images in SoftLayer (#568)
> @@ -339,7 +339,9 @@ public boolean apply(SoftwareDescription input) { > > private OperatingSystem getOperatingSystem(Map.Entry SoftwareDescription> entry) { >SoftwareDescription softwareDescription = entry.getValue(); > - if (isOperatingSystem(softwareDescription)) { > + if (!isOperatingSystem(softwareDescription)) { > + return null; I think this method should be changed: * Why does it receive a `Map.Entry` parameter? Change it to receive a String and a SoftwareDescription object, to make its signature more clean and meaningful. * Also, returning `null` is evil. Avoid that. As this is a private method only used to populate values in a Set, just make it return void and refactor to something like `addOperatingSystemToSet`, and pass the Set as a parameter too. This way the method logic is self-contained and you're not propagating details such as the null value (which can introduce NPE issues) to the outside. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568/files#r18695140
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests-java-6 #188](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/188/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58629131