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 46bc0f4216..7e9cb8f065 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,8 +90,13 @@ public class EC2TemplateBuilderImplTest extends TemplateBuilderImplTest { return new EC2TemplateBuilderImpl(locations, images, sizes, Suppliers.ofInstance(defaultLocation), optionsProvider, templateBuilderProvider, Suppliers.>ofInstance(imageMap)); } - + @Override + protected String getProviderFormatId(String uniqueLabel) { + return "us-east-1/"+uniqueLabel; + } + + @Override @Test public void testHardwareWithImageIdPredicateOnlyAcceptsImageWhenLocationNull() { 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 858452adcf..b60c136b3a 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. * @@ -167,6 +170,15 @@ public interface TemplateBuilder { */ TemplateBuilder imageMatches(Predicate condition); + /** + * Configure this template with a specific preference function which operates on + * images which match the other criteria. + *

+ * 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,Image> imageChooser); + /** * Configure this template to require the minimum cores below */ 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 58220902db..8b60aec2f0 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 imagePredicate; @VisibleForTesting + protected Function, Image> imageChooser; + @VisibleForTesting protected double minCores; @VisibleForTesting protected int minRam; @@ -482,6 +484,13 @@ public class TemplateBuilderImpl implements TemplateBuilder { }; static final Ordering DEFAULT_IMAGE_ORDERING = new Ordering() { 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. natural().nullsLast()) .compare(left.getVersion(), right.getVersion(), Ordering. natural().nullsLast()) @@ -496,6 +505,25 @@ public class TemplateBuilderImpl implements TemplateBuilder { Ordering. natural().nullsLast()).result(); } }; + + @VisibleForTesting + // non-static for logging + final Function, Image> imageChooserFromOrdering(final Ordering ordering) { + return new Function, Image>() { + @Override + public Image apply(Iterable input) { + List maxImages = multiMax(ordering, input); + if (logger.isTraceEnabled()) + logger.trace("<< best images(%s)", transform(maxImages, imageToId)); + return maxImages.get(maxImages.size() - 1); + } + }; + } + + @VisibleForTesting + Function, Image> defaultImageChooser() { + return imageChooserFromOrdering(DEFAULT_IMAGE_ORDERING); + } /** * {@inheritDoc} @@ -765,6 +793,11 @@ public class TemplateBuilderImpl implements TemplateBuilder { return hardware; } + protected Function, Image> imageChooser() { + if (imageChooser != null) return imageChooser; + return defaultImageChooser(); + } + protected Ordering hardwareSorter() { Ordering hardwareOrdering = DEFAULT_SIZE_ORDERING; if (!biggest) @@ -799,10 +832,7 @@ public class TemplateBuilderImpl implements TemplateBuilder { Iterable matchingImages = filter(supportedImages, imagePredicate); if (logger.isTraceEnabled()) logger.trace("<< matched images(%s)", transform(matchingImages, imageToId)); - List 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); @@ -941,6 +971,15 @@ public class TemplateBuilderImpl implements TemplateBuilder { return this; } + /** + * {@inheritDoc} + */ + @Override + public TemplateBuilderImpl imageChooser(Function,Image> imageChooser) { + this.imageChooser = imageChooser; + return this; + } + /** * {@inheritDoc} */ @@ -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()); 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 e8fd9c9c55..4661c4bde7 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 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 values = Arrays.asList("1", "3", null, "2", "2"); + assertEquals(TemplateBuilderImpl.multiMax(Ordering.natural().nullsLast(), values), Arrays.asList((Object)null)); + } + + public void testMultiMaxNulls() { + Iterable 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> images, Image expectedBest, + Function builderCustomisation) { Hardware hardware = new HardwareBuilder().id("hardwareId").build(); Supplier> locations = Suppliers.> ofInstance(ImmutableSet . of(region)); - Supplier> images = Suppliers.> ofInstance(ImmutableSet. of( - image, image2)); Supplier> hardwares = Suppliers.> ofInstance(ImmutableSet . of(hardware)); Provider 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.> ofInstance(ImmutableSet. of( + image, image64bit)), + image64bit, Functions.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.> ofInstance(ImmutableSet. of( + image, imageArchNull, image64bit)), + imageArchNull, Functions.identity()); + } + + public void testResolveImagesCustomSorterPreferringNonNull() { + final Ordering sorterPreferringNonNullArch = new Ordering() { + @Override + public int compare(Image left, Image right) { + return ComparisonChain.start() + .compare(left.getOperatingSystem().getArch(), right.getOperatingSystem().getArch(), + Ordering. 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.> ofInstance(ImmutableSet. of( + image, imageArchNull, image64bit)), + image64bit, new Function() { + @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.> ofInstance(ImmutableSet. of( + imageNameAlt, image)), + image, Functions.identity()); + } + + public void testResolveImagesCustomSorterPreferringAltImage() { + doTestResolveImages(Suppliers.> ofInstance(ImmutableSet. of( + imageNameAlt, image, imageArchNull, image64bit)), + imageNameAlt, new Function() { + @Override + public TemplateBuilderImpl apply(TemplateBuilderImpl input) { + return input.imageChooser(input.imageChooserFromOrdering(new Ordering() { + 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.> ofInstance(ImmutableSet. of( + imageNameAlt, image, imageArchNull, image64bit)), + imageNameAlt, new Function() { + @Override + public TemplateBuilderImpl apply(TemplateBuilderImpl input) { + return input.imageChooser(new Function, Image>() { + @Override + public Image apply(Iterable input) { + return Iterables.find(input, new Predicate() { + @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> locations = Suppliers.> ofInstance(ImmutableSet . of(region)); Supplier> images = Suppliers.> ofInstance(ImmutableSet. of( - image, image2)); + image, image64bit)); Supplier> hardwares = Suppliers.> ofInstance(ImmutableSet . of(hardware)); Provider 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> locations = Suppliers.> 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> locations = Suppliers.> 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() {