Updated Branches: refs/heads/master aa8fab16f -> 8207c53cf
JCLOUDS-331 - support specifying an imageChooser function in TemplateBuilder Project: http://git-wip-us.apache.org/repos/asf/incubator-jclouds/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-jclouds/commit/8207c53c Tree: http://git-wip-us.apache.org/repos/asf/incubator-jclouds/tree/8207c53c Diff: http://git-wip-us.apache.org/repos/asf/incubator-jclouds/diff/8207c53c Branch: refs/heads/master Commit: 8207c53cf23d04ace95d84bd50eb875dc0ab7438 Parents: aa8fab1 Author: Alex Heneveld <[email protected]> Authored: Wed Oct 2 05:48:32 2013 -0700 Committer: Andrew Phillips <[email protected]> Committed: Fri Oct 4 18:38:53 2013 +0200 ---------------------------------------------------------------------- .../internal/EC2TemplateBuilderImplTest.java | 7 +- .../jclouds/compute/domain/TemplateBuilder.java | 14 +- .../domain/internal/TemplateBuilderImpl.java | 54 ++++++- .../internal/TemplateBuilderImplTest.java | 155 +++++++++++++++++-- 4 files changed, 204 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-jclouds/blob/8207c53c/apis/ec2/src/test/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImplTest.java ---------------------------------------------------------------------- diff --git a/apis/ec2/src/test/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImplTest.java b/apis/ec2/src/test/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImplTest.java index 46bc0f4..7e9cb8f 100644 --- a/apis/ec2/src/test/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImplTest.java +++ b/apis/ec2/src/test/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImplTest.java @@ -90,9 +90,14 @@ public class EC2TemplateBuilderImplTest extends TemplateBuilderImplTest { return new EC2TemplateBuilderImpl(locations, images, sizes, Suppliers.ofInstance(defaultLocation), optionsProvider, templateBuilderProvider, Suppliers.<LoadingCache<RegionAndName, ? extends Image>>ofInstance(imageMap)); } - @Override + protected String getProviderFormatId(String uniqueLabel) { + return "us-east-1/"+uniqueLabel; + } + + + @Override @Test public void testHardwareWithImageIdPredicateOnlyAcceptsImageWhenLocationNull() { // not possible to have null region in ec2 http://git-wip-us.apache.org/repos/asf/incubator-jclouds/blob/8207c53c/compute/src/main/java/org/jclouds/compute/domain/TemplateBuilder.java ---------------------------------------------------------------------- diff --git a/compute/src/main/java/org/jclouds/compute/domain/TemplateBuilder.java b/compute/src/main/java/org/jclouds/compute/domain/TemplateBuilder.java index 858452a..b60c136 100644 --- a/compute/src/main/java/org/jclouds/compute/domain/TemplateBuilder.java +++ b/compute/src/main/java/org/jclouds/compute/domain/TemplateBuilder.java @@ -16,10 +16,13 @@ */ package org.jclouds.compute.domain; +import java.util.NoSuchElementException; + import org.jclouds.compute.domain.internal.TemplateBuilderImpl; import org.jclouds.compute.options.TemplateOptions; import com.google.common.annotations.Beta; +import com.google.common.base.Function; import com.google.common.base.Predicate; import com.google.inject.ImplementedBy; @@ -157,7 +160,7 @@ public interface TemplateBuilder { TemplateBuilder imageDescriptionMatches(String imageDescriptionRegex); /** - * Configure this template to have an image description that matches the supplied condition + * Configure this template to have an image that matches the supplied condition * * ex. * @@ -168,6 +171,15 @@ public interface TemplateBuilder { TemplateBuilder imageMatches(Predicate<Image> condition); /** + * Configure this template with a specific preference function which operates on + * images which match the other criteria. + * <p> + * If no function is supplied, jclouds will select one according to an internal strategy. + * This strategy may change from version to version. + */ + public TemplateBuilderImpl imageChooser(Function<Iterable<? extends Image>,Image> imageChooser); + + /** * Configure this template to require the minimum cores below */ TemplateBuilder minCores(double minCores); http://git-wip-us.apache.org/repos/asf/incubator-jclouds/blob/8207c53c/compute/src/main/java/org/jclouds/compute/domain/internal/TemplateBuilderImpl.java ---------------------------------------------------------------------- diff --git a/compute/src/main/java/org/jclouds/compute/domain/internal/TemplateBuilderImpl.java b/compute/src/main/java/org/jclouds/compute/domain/internal/TemplateBuilderImpl.java index 5822090..8b60aec 100644 --- a/compute/src/main/java/org/jclouds/compute/domain/internal/TemplateBuilderImpl.java +++ b/compute/src/main/java/org/jclouds/compute/domain/internal/TemplateBuilderImpl.java @@ -115,6 +115,8 @@ public class TemplateBuilderImpl implements TemplateBuilder { @VisibleForTesting protected Predicate<Image> imagePredicate; @VisibleForTesting + protected Function<Iterable<? extends Image>, Image> imageChooser; + @VisibleForTesting protected double minCores; @VisibleForTesting protected int minRam; @@ -482,6 +484,13 @@ public class TemplateBuilderImpl implements TemplateBuilder { }; static final Ordering<Image> DEFAULT_IMAGE_ORDERING = new Ordering<Image>() { public int compare(Image left, Image right) { + /* This currently, and for some time, has *preferred* images whose fields are null, + * and prefers those which come last alphabetically. + * It seems preferable to take images whose fields are *not* null, ie nullsFirst; + * and to use something like the AlphaNum Algorithm then take the last + * (so "Ubuntu 13.04" would be preferred over "Ubuntu 9.10"). + * However not changing it now as people may be surprised if the images they get back start changing. + */ return ComparisonChain.start() .compare(left.getName(), right.getName(), Ordering.<String> natural().nullsLast()) .compare(left.getVersion(), right.getVersion(), Ordering.<String> natural().nullsLast()) @@ -496,6 +505,25 @@ public class TemplateBuilderImpl implements TemplateBuilder { Ordering.<String> natural().nullsLast()).result(); } }; + + @VisibleForTesting + // non-static for logging + final Function<Iterable<? extends Image>, Image> imageChooserFromOrdering(final Ordering<Image> ordering) { + return new Function<Iterable<? extends Image>, Image>() { + @Override + public Image apply(Iterable<? extends Image> input) { + List<? extends Image> maxImages = multiMax(ordering, input); + if (logger.isTraceEnabled()) + logger.trace("<< best images(%s)", transform(maxImages, imageToId)); + return maxImages.get(maxImages.size() - 1); + } + }; + } + + @VisibleForTesting + Function<Iterable<? extends Image>, Image> defaultImageChooser() { + return imageChooserFromOrdering(DEFAULT_IMAGE_ORDERING); + } /** * {@inheritDoc} @@ -765,6 +793,11 @@ public class TemplateBuilderImpl implements TemplateBuilder { return hardware; } + protected Function<Iterable<? extends Image>, Image> imageChooser() { + if (imageChooser != null) return imageChooser; + return defaultImageChooser(); + } + protected Ordering<Hardware> hardwareSorter() { Ordering<Hardware> hardwareOrdering = DEFAULT_SIZE_ORDERING; if (!biggest) @@ -799,10 +832,7 @@ public class TemplateBuilderImpl implements TemplateBuilder { Iterable<? extends Image> matchingImages = filter(supportedImages, imagePredicate); if (logger.isTraceEnabled()) logger.trace("<< matched images(%s)", transform(matchingImages, imageToId)); - List<? extends Image> maxImages = multiMax(DEFAULT_IMAGE_ORDERING, matchingImages); - if (logger.isTraceEnabled()) - logger.trace("<< best images(%s)", transform(maxImages, imageToId)); - return maxImages.get(maxImages.size() - 1); + return imageChooser().apply(matchingImages); } catch (NoSuchElementException exception) { throwNoSuchElementExceptionAfterLoggingImageIds(format("no image matched params: %s", toString()), supportedImages); @@ -945,6 +975,15 @@ public class TemplateBuilderImpl implements TemplateBuilder { * {@inheritDoc} */ @Override + public TemplateBuilderImpl imageChooser(Function<Iterable<? extends Image>,Image> imageChooser) { + this.imageChooser = imageChooser; + return this; + } + + /** + * {@inheritDoc} + */ + @Override public TemplateBuilder imageVersionMatches(String imageVersionRegex) { this.imageVersion = imageVersionRegex; return this; @@ -1045,9 +1084,9 @@ public class TemplateBuilderImpl implements TemplateBuilder { @VisibleForTesting boolean nothingChangedExceptOptions() { return osFamily == null && location == null && imageId == null && hardwareId == null && hypervisor == null - && osName == null && imagePredicate == null && osDescription == null && imageVersion == null - && osVersion == null && osArch == null && os64Bit == null && imageName == null && imageDescription == null - && minCores == 0 && minRam == 0 && minDisk == 0 && !biggest && !fastest; + && osName == null && imagePredicate == null && imageChooser == null && osDescription == null + && imageVersion == null && osVersion == null && osArch == null && os64Bit == null && imageName == null + && imageDescription == null && minCores == 0 && minRam == 0 && minDisk == 0 && !biggest && !fastest; } /** @@ -1076,6 +1115,7 @@ public class TemplateBuilderImpl implements TemplateBuilder { toString.add("imageDescription", imageDescription); toString.add("imageId", imageId); toString.add("imagePredicate", imagePredicate); + toString.add("imageChooser", imageChooser); toString.add("imageVersion", imageVersion); if (location != null) toString.add("locationId", location.getId()); http://git-wip-us.apache.org/repos/asf/incubator-jclouds/blob/8207c53c/compute/src/test/java/org/jclouds/compute/domain/internal/TemplateBuilderImplTest.java ---------------------------------------------------------------------- diff --git a/compute/src/test/java/org/jclouds/compute/domain/internal/TemplateBuilderImplTest.java b/compute/src/test/java/org/jclouds/compute/domain/internal/TemplateBuilderImplTest.java index e8fd9c9..4661c4b 100644 --- a/compute/src/test/java/org/jclouds/compute/domain/internal/TemplateBuilderImplTest.java +++ b/compute/src/test/java/org/jclouds/compute/domain/internal/TemplateBuilderImplTest.java @@ -21,8 +21,10 @@ import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.replay; import static org.easymock.EasyMock.verify; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; import static org.testng.Assert.fail; +import java.util.Arrays; import java.util.NoSuchElementException; import java.util.Set; @@ -46,10 +48,15 @@ import org.jclouds.domain.LocationBuilder; import org.jclouds.domain.LocationScope; import org.testng.annotations.Test; +import com.google.common.base.Function; +import com.google.common.base.Functions; +import com.google.common.base.Predicate; import com.google.common.base.Supplier; import com.google.common.base.Suppliers; +import com.google.common.collect.ComparisonChain; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.collect.Ordering; /** @@ -69,6 +76,24 @@ public class TemplateBuilderImplTest { assertEquals(TemplateBuilderImpl.multiMax(Ordering.natural(), values), ImmutableList.of("3")); } + public void testMultiMax2() { + // check with max buried in the middle + Iterable<String> values = ImmutableList.of("1", "3", "2", "2"); + assertEquals(TemplateBuilderImpl.multiMax(Ordering.natural(), values), ImmutableList.of("3")); + } + + public void testMultiMaxNull() { + // we rely on checking nulls in some Orderings, so assert it also does what we expect + // (unfortunately can't use ImmutableList here as that doesn't allow nulls) + Iterable<String> values = Arrays.asList("1", "3", null, "2", "2"); + assertEquals(TemplateBuilderImpl.multiMax(Ordering.natural().nullsLast(), values), Arrays.asList((Object)null)); + } + + public void testMultiMaxNulls() { + Iterable<String> values = Arrays.asList("1", "3", null, "2", "2", null); + assertEquals(TemplateBuilderImpl.multiMax(Ordering.natural().nullsLast(), values), Arrays.asList((Object)null, null)); + } + protected Location provider = new LocationBuilder().scope(LocationScope.PROVIDER).id("aws-ec2").description("aws-ec2").build(); protected Location region = new LocationBuilder().scope(LocationScope.REGION).id("us-east-1") @@ -80,11 +105,22 @@ public class TemplateBuilderImplTest { protected OperatingSystem os = OperatingSystem.builder().name("osName").version("osVersion") .description("osDescription").arch("X86_32").build(); - protected Image image = new ImageBuilder().id("us-east-1/imageId").providerId("imageId").name("imageName") + protected String getProviderFormatId(String uniqueLabel) { + return uniqueLabel; + } + + protected Image image = new ImageBuilder().id(getProviderFormatId("imageId")).providerId("imageId").name("imageName") .description("imageDescription").version("imageVersion").operatingSystem(os).status(Image.Status.AVAILABLE) .location(region).build(); - protected Image image2 = ImageBuilder.fromImage(image).operatingSystem(os.toBuilder().arch("X86_64").build()).build(); + protected Image image64bit = ImageBuilder.fromImage(image).id(getProviderFormatId("image64bId")).providerId("image64bId") + .operatingSystem(os.toBuilder().arch("X86_64").build()).build(); + + protected Image imageArchNull = ImageBuilder.fromImage(image).id(getProviderFormatId("imageArchNullId")).providerId("imageArchNullId") + .operatingSystem(os.toBuilder().arch(null).build()).build(); + + protected Image imageNameAlt = ImageBuilder.fromImage(image).id(getProviderFormatId("imageNameAltId")).providerId("imageNameAltId") + .name("alternateImageName").build(); @SuppressWarnings("unchecked") public void testLocationPredicateWhenComputeMetadataIsNotLocationBound() { @@ -111,16 +147,12 @@ public class TemplateBuilderImplTest { } @SuppressWarnings("unchecked") - @Test - public void testResolveImages() { - - + protected void doTestResolveImages(Supplier<Set<? extends Image>> images, Image expectedBest, + Function<TemplateBuilderImpl, TemplateBuilderImpl> builderCustomisation) { Hardware hardware = new HardwareBuilder().id("hardwareId").build(); Supplier<Set<? extends Location>> locations = Suppliers.<Set<? extends Location>> ofInstance(ImmutableSet .<Location> of(region)); - Supplier<Set<? extends Image>> images = Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of( - image, image2)); Supplier<Set<? extends Hardware>> hardwares = Suppliers.<Set<? extends Hardware>> ofInstance(ImmutableSet .<Hardware> of(hardware)); Provider<TemplateOptions> optionsProvider = createMock(Provider.class); @@ -131,12 +163,103 @@ public class TemplateBuilderImplTest { TemplateBuilderImpl template = createTemplateBuilder(null, locations, images, hardwares, region, optionsProvider, templateBuilderProvider); + template = builderCustomisation.apply(template); - assertEquals(template.resolveImage(hardware, images.get()), image2); + assertEquals(template.resolveImage(hardware, images.get()), expectedBest); verify(defaultTemplate, optionsProvider, templateBuilderProvider); } + public void testResolveImagesSimple() { + doTestResolveImages(Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of( + image, image64bit)), + image64bit, Functions.<TemplateBuilderImpl>identity()); + } + + public void testResolveImagesPrefersNull() { + // preferring null has been the default behaviour; not sure if this is ideal + // (would make more sense to prefer nonNull) but don't change behaviour by default + doTestResolveImages(Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of( + image, imageArchNull, image64bit)), + imageArchNull, Functions.<TemplateBuilderImpl>identity()); + } + + public void testResolveImagesCustomSorterPreferringNonNull() { + final Ordering<Image> sorterPreferringNonNullArch = new Ordering<Image>() { + @Override + public int compare(Image left, Image right) { + return ComparisonChain.start() + .compare(left.getOperatingSystem().getArch(), right.getOperatingSystem().getArch(), + Ordering.<String> natural().nullsFirst()) + .compare(left, right, TemplateBuilderImpl.DEFAULT_IMAGE_ORDERING) + .result(); + } + }; + assertTrue(TemplateBuilderImpl.DEFAULT_IMAGE_ORDERING.compare(image64bit, imageArchNull) < 0, "wrong default image ordering"); + assertTrue(sorterPreferringNonNullArch.compare(image64bit, imageArchNull) > 0, "wrong custom image ordering"); + + // preferring null has been the default behaviour; + // see comments in TemplateBuilderImpl.DEFAULT_IMAGE_ORDERING + doTestResolveImages(Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of( + image, imageArchNull, image64bit)), + image64bit, new Function<TemplateBuilderImpl,TemplateBuilderImpl>() { + @Override + public TemplateBuilderImpl apply(TemplateBuilderImpl input) { + return input.imageChooser(input.imageChooserFromOrdering(sorterPreferringNonNullArch)); + } + }); + } + + public void testResolveImagesPrefersImageBecauseNameIsLastAlphabetically() { + // preferring that which comes later alphabetically is the default behaviour; + // see comments in TemplateBuilderImpl.DEFAULT_IMAGE_ORDERING + doTestResolveImages(Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of( + imageNameAlt, image)), + image, Functions.<TemplateBuilderImpl>identity()); + } + + public void testResolveImagesCustomSorterPreferringAltImage() { + doTestResolveImages(Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of( + imageNameAlt, image, imageArchNull, image64bit)), + imageNameAlt, new Function<TemplateBuilderImpl,TemplateBuilderImpl>() { + @Override + public TemplateBuilderImpl apply(TemplateBuilderImpl input) { + return input.imageChooser(input.imageChooserFromOrdering(new Ordering<Image>() { + private int score(Image img) { + if (img.getName().contains("alternate")) return 10; + return 0; + } + @Override + public int compare(Image left, Image right) { + return score(left) - score(right); + } + })); + } + }); + } + + public void testResolveImagesCustomChooserPreferringAltImage() { + doTestResolveImages(Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of( + imageNameAlt, image, imageArchNull, image64bit)), + imageNameAlt, new Function<TemplateBuilderImpl,TemplateBuilderImpl>() { + @Override + public TemplateBuilderImpl apply(TemplateBuilderImpl input) { + return input.imageChooser(new Function<Iterable<? extends Image>, Image>() { + @Override + public Image apply(Iterable<? extends Image> input) { + return Iterables.find(input, new Predicate<Image>() { + @Override + public boolean apply(Image input) { + return input.getName()!=null && input.getName().contains("alternate"); + } + }); + } + }); + } + }); + } + + @SuppressWarnings("unchecked") @Test public void testArchWins() { @@ -146,7 +269,7 @@ public class TemplateBuilderImplTest { Supplier<Set<? extends Location>> locations = Suppliers.<Set<? extends Location>> ofInstance(ImmutableSet .<Location> of(region)); Supplier<Set<? extends Image>> images = Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of( - image, image2)); + image, image64bit)); Supplier<Set<? extends Hardware>> hardwares = Suppliers.<Set<? extends Hardware>> ofInstance(ImmutableSet .<Hardware> of(hardware)); Provider<TemplateOptions> optionsProvider = createMock(Provider.class); @@ -169,7 +292,7 @@ public class TemplateBuilderImplTest { @Test public void testHardwareWithImageIdPredicateOnlyAcceptsImage() { - Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals("us-east-1/imageId")) + Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals(getProviderFormatId("imageId"))) .build(); Supplier<Set<? extends Location>> locations = Suppliers.<Set<? extends Location>> ofInstance(ImmutableSet @@ -191,7 +314,7 @@ public class TemplateBuilderImplTest { TemplateBuilderImpl template = createTemplateBuilder(null, locations, images, hardwares, region, optionsProvider, templateBuilderProvider); - template.imageId("us-east-1/imageId").build(); + template.imageId(getProviderFormatId("imageId")).build(); verify(defaultTemplate); verify(optionsProvider); @@ -202,7 +325,7 @@ public class TemplateBuilderImplTest { @Test public void testHardwareWithImageIdPredicateOnlyAcceptsImageWhenLocationNull() { - Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals("us-east-1/imageId")) + Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals(getProviderFormatId("imageId"))) .build(); Supplier<Set<? extends Location>> locations = Suppliers.<Set<? extends Location>> ofInstance(ImmutableSet @@ -222,7 +345,7 @@ public class TemplateBuilderImplTest { TemplateBuilderImpl template = createTemplateBuilder(null, locations, images, hardwares, region, optionsProvider, templateBuilderProvider); - template.imageId("us-east-1/imageId").build(); + template.imageId(getProviderFormatId("imageId")).build(); verify(defaultTemplate, optionsProvider, templateBuilderProvider); @@ -252,7 +375,7 @@ public class TemplateBuilderImplTest { TemplateBuilderImpl template = createTemplateBuilder(image, locations, images, hardwares, region, optionsProvider, templateBuilderProvider); try { - template.imageId("us-east-1/imageId").build(); + template.imageId(getProviderFormatId("imageId")).build(); fail("Expected NoSuchElementException"); } catch (NoSuchElementException e) { // make sure message is succinct @@ -788,8 +911,6 @@ public class TemplateBuilderImplTest { assertEquals(template.getLocation().getId(), "us-east-2"); } - - @Test public void testFromSpecWithLoginUser() {
