Issue 763: don't retrieve all images when template.imageId supplied

apis/ec2/src/main/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImpl.java

@@ -39,6 +39,7 @@ import org.jclouds.ec2.compute.domain.RegionAndName;
 
 import com.google.common.base.Supplier;
 import com.google.common.cache.Cache;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.UncheckedExecutionException;
 
 /**
@@ -47,7 +48,7 @@ import com.google.common.util.concurrent.UncheckedExecutionException;
  */
 public class EC2TemplateBuilderImpl extends TemplateBuilderImpl {
 
-   private final Supplier<Cache<RegionAndName, ? extends Image>> imageMap;
+   private final Supplier<Cache<RegionAndName, ? extends Image>> lazyImageCache;
 
    @Inject
    protected EC2TemplateBuilderImpl(@Memoized Supplier<Set<? extends Location>> locations,
@@ -55,7 +56,7 @@ public class EC2TemplateBuilderImpl extends TemplateBuilderImpl {
          Supplier<Location> defaultLocation, @Named("DEFAULT") Provider<TemplateOptions> optionsProvider,
          @Named("DEFAULT") Provider<TemplateBuilder> defaultTemplateProvider, Supplier<Cache<RegionAndName, ? extends Image>> imageMap) {
       super(locations, images, sizes, defaultLocation, optionsProvider, defaultTemplateProvider);
-      this.imageMap = imageMap;
+      this.lazyImageCache = imageMap;
    }
 
    final Provider<Image> lazyImageProvider = new Provider<Image>() {
@@ -68,7 +69,7 @@ public class EC2TemplateBuilderImpl extends TemplateBuilderImpl {
                   "amazon image ids must include the region ( ex. us-east-1/ami-7ea24a17 ) you specified: " + imageId);
             RegionAndName key = new RegionAndName(regionName[0], regionName[1]);
             try {
-               return imageMap.get().get(key);
+               return lazyImageCache.get().get(key);
             } catch (ExecutionException e) {
                throw new NoSuchElementException(String.format("could not get imageId(%s/%s)", key.getRegion(), key.getName()));
             } catch (UncheckedExecutionException e) {
@@ -101,14 +102,11 @@ public class EC2TemplateBuilderImpl extends TemplateBuilderImpl {
    @SuppressWarnings("unchecked")
    @Override
    protected Set<? extends Image> getImages() {
-      Set<Image> images = (Set<Image>) this.images.get();
+      if (imageId != null) {
-      if (images.size() == 0) {
+         Image image = lazyImageProvider.get();
-         Image toReturn = lazyImageProvider.get();
+         return ImmutableSet.of(image);
-         if (toReturn != null) {
+      } else {
-            images.add(toReturn);
+         return (Set<Image>) this.images.get();
-         }
       }
-      return images;
    }
-
 }

apis/ec2/src/main/java/org/jclouds/ec2/compute/suppliers/EC2ImageSupplier.java

@@ -18,16 +18,38 @@
  */
 package org.jclouds.ec2.compute.suppliers;
 
+import static com.google.common.collect.Iterables.filter;
+import static com.google.common.collect.Iterables.transform;
+import static com.google.common.collect.Maps.uniqueIndex;
+import static org.jclouds.ec2.options.DescribeImagesOptions.Builder.ownedBy;
+import static org.jclouds.ec2.reference.EC2Constants.PROPERTY_EC2_AMI_OWNERS;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 
+import javax.annotation.Resource;
 import javax.inject.Inject;
+import javax.inject.Named;
 import javax.inject.Singleton;
 
 import org.jclouds.compute.domain.Image;
+import org.jclouds.compute.reference.ComputeServiceConstants;
 import org.jclouds.ec2.compute.domain.RegionAndName;
+import org.jclouds.ec2.compute.functions.EC2ImageParser;
+import org.jclouds.ec2.compute.strategy.DescribeImagesParallel;
+import org.jclouds.ec2.options.DescribeImagesOptions;
+import org.jclouds.location.Region;
+import org.jclouds.logging.Logger;
 
+import com.google.common.base.Function;
+import com.google.common.base.Predicates;
 import com.google.common.base.Supplier;
 import com.google.common.cache.Cache;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMap.Builder;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 
 /**
@@ -36,16 +58,75 @@ import com.google.common.collect.Sets;
  */
 @Singleton
 public class EC2ImageSupplier implements Supplier<Set<? extends Image>> {
-   private final Supplier<Cache<RegionAndName, ? extends Image>> map;
+   @Resource
+   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+   protected Logger logger = Logger.NULL;
+
+   private final Set<String> regions;
+   private final DescribeImagesParallel describer;
+   private final String[] amiOwners;
+   private final EC2ImageParser parser;
+   private final Supplier<Cache<RegionAndName, ? extends Image>> cache;
 
    @Inject
-   EC2ImageSupplier(Supplier<Cache<RegionAndName, ? extends Image>> map) {
+   protected EC2ImageSupplier(@Region Set<String> regions, DescribeImagesParallel describer,
-      this.map = map;
+         @Named(PROPERTY_EC2_AMI_OWNERS) String[] amiOwners, Supplier<Cache<RegionAndName, ? extends Image>> cache,
+         EC2ImageParser parser) {
+      this.regions = regions;
+      this.describer = describer;
+      this.amiOwners = amiOwners;
+      this.cache = cache;
+      this.parser = parser;
    }
 
+   @SuppressWarnings({ "unchecked", "rawtypes" })
    @Override
    public Set<? extends Image> get() {
-      return Sets.newLinkedHashSet(map.get().asMap().values());
+      if (amiOwners.length == 0) {
+         logger.debug(">> no owners specified, skipping image parsing");
+         return Collections.emptySet();
+      
+      } else {
+         logger.debug(">> providing images");
+
+         Iterable<Entry<String, DescribeImagesOptions>> queries = getDescribeQueriesForOwnersInRegions(regions,
+                  amiOwners);
+
+         Iterable<? extends Image> parsedImages = ImmutableSet.copyOf(filter(transform(describer.apply(queries), parser), Predicates
+                  .notNull()));
+
+         ImmutableMap<RegionAndName, ? extends Image> imageMap = uniqueIndex(parsedImages, new Function<Image, RegionAndName>() {
+
+            @Override
+            public RegionAndName apply(Image from) {
+               return new RegionAndName(from.getLocation().getId(), from.getProviderId());
+            }
+
+         });
+         
+         cache.get().invalidateAll();
+         cache.get().asMap().putAll((Map)imageMap);
+         logger.debug("<< images(%d)", imageMap.size());
+         
+         return Sets.newLinkedHashSet(imageMap.values());
+      }
    }
 
-}
+   public Iterable<Entry<String, DescribeImagesOptions>> getDescribeQueriesForOwnersInRegions(Set<String> regions,
+         String[] amiOwners) {
+      DescribeImagesOptions options = getOptionsForOwners(amiOwners);
+      Builder<String, DescribeImagesOptions> builder = ImmutableMap.<String, DescribeImagesOptions> builder();
+      for (String region : regions)
+         builder.put(region, options);
+      return builder.build().entrySet();
+   }
+
+   public DescribeImagesOptions getOptionsForOwners(String... amiOwners) {
+      DescribeImagesOptions options;
+      if (amiOwners.length == 1 && amiOwners[0].equals("*"))
+         options = new DescribeImagesOptions();
+      else
+         options = ownedBy(amiOwners);
+      return options;
+   }
+}

apis/ec2/src/main/java/org/jclouds/ec2/compute/suppliers/RegionAndNameToImageSupplier.java

@@ -18,38 +18,16 @@
  */
 package org.jclouds.ec2.compute.suppliers;
 
-import static com.google.common.collect.Iterables.filter;
-import static com.google.common.collect.Iterables.transform;
-import static com.google.common.collect.Maps.uniqueIndex;
-import static org.jclouds.ec2.options.DescribeImagesOptions.Builder.ownedBy;
-import static org.jclouds.ec2.reference.EC2Constants.PROPERTY_EC2_AMI_OWNERS;
-
-import java.util.Set;
-import java.util.Map.Entry;
-
-import javax.annotation.Resource;
 import javax.inject.Inject;
-import javax.inject.Named;
 import javax.inject.Singleton;
 
 import org.jclouds.compute.domain.Image;
-import org.jclouds.compute.reference.ComputeServiceConstants;
 import org.jclouds.ec2.compute.domain.RegionAndName;
-import org.jclouds.ec2.compute.functions.EC2ImageParser;
-import org.jclouds.ec2.compute.strategy.DescribeImagesParallel;
-import org.jclouds.ec2.options.DescribeImagesOptions;
-import org.jclouds.location.Region;
-import org.jclouds.logging.Logger;
 
-import com.google.common.base.Function;
-import com.google.common.base.Predicates;
 import com.google.common.base.Supplier;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableMap.Builder;
 
 /**
  * 
@@ -57,69 +35,15 @@ import com.google.common.collect.ImmutableMap.Builder;
  */
 @Singleton
 public class RegionAndNameToImageSupplier implements Supplier<Cache<RegionAndName, ? extends Image>> {
-   @Resource
+   private final Cache<RegionAndName, Image> cache;
-   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
-   protected Logger logger = Logger.NULL;
-
-   private final Set<String> regions;
-   private final DescribeImagesParallel describer;
-   private final String[] amiOwners;
-   private final EC2ImageParser parser;
-   private final CacheLoader<RegionAndName, Image> regionAndIdToImage;
 
    @Inject
-   protected RegionAndNameToImageSupplier(@Region Set<String> regions, DescribeImagesParallel describer,
+   protected RegionAndNameToImageSupplier(CacheLoader<RegionAndName, Image> regionAndIdToImage) {
-         @Named(PROPERTY_EC2_AMI_OWNERS) String[] amiOwners, EC2ImageParser parser, CacheLoader<RegionAndName, Image> regionAndIdToImage) {
+      cache = CacheBuilder.newBuilder().build(regionAndIdToImage);
-      this.regions = regions;
-      this.describer = describer;
-      this.amiOwners = amiOwners;
-      this.parser = parser;
-      this.regionAndIdToImage = regionAndIdToImage;
    }
 
    @Override
    public Cache<RegionAndName, ? extends Image> get() {
-      Cache<RegionAndName, Image> cache = CacheBuilder.newBuilder().build(regionAndIdToImage);
-
-      if (amiOwners.length == 0) {
-         logger.debug(">> no owners specified, skipping image parsing");
-      } else {
-         logger.debug(">> providing images");
-
-         Iterable<Entry<String, DescribeImagesOptions>> queries = getDescribeQueriesForOwnersInRegions(regions,
-                  amiOwners);
-
-         Iterable<? extends Image> parsedImages = ImmutableSet.copyOf(filter(transform(describer.apply(queries), parser), Predicates
-                  .notNull()));
-
-         cache.asMap().putAll(uniqueIndex(parsedImages, new Function<Image, RegionAndName>() {
-
-            @Override
-            public RegionAndName apply(Image from) {
-               return new RegionAndName(from.getLocation().getId(), from.getProviderId());
-            }
-
-         }));
-         logger.debug("<< images(%d)",  cache.asMap().size());
-      }
       return cache;
    }
-
-   public Iterable<Entry<String, DescribeImagesOptions>> getDescribeQueriesForOwnersInRegions(Set<String> regions,
-         String[] amiOwners) {
-      DescribeImagesOptions options = getOptionsForOwners(amiOwners);
-      Builder<String, DescribeImagesOptions> builder = ImmutableMap.<String, DescribeImagesOptions> builder();
-      for (String region : regions)
-         builder.put(region, options);
-      return builder.build().entrySet();
-   }
-
-   public DescribeImagesOptions getOptionsForOwners(String... amiOwners) {
-      DescribeImagesOptions options;
-      if (amiOwners.length == 1 && amiOwners[0].equals("*"))
-         options = new DescribeImagesOptions();
-      else
-         options = ownedBy(amiOwners);
-      return options;
-   }
 }

apis/ec2/src/test/java/org/jclouds/ec2/compute/EC2ComputeServiceLiveTest.java

@@ -22,6 +22,7 @@ import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertTrue;
 
 import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.Set;
 
 import org.jclouds.compute.BaseComputeServiceLiveTest;
@@ -52,6 +53,7 @@ import org.jclouds.sshj.config.SshjSshClientModule;
 import org.testng.annotations.Test;
 
 import com.google.common.base.Predicate;
+import com.google.common.base.Stopwatch;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
@@ -237,6 +239,45 @@ public class EC2ComputeServiceLiveTest extends BaseComputeServiceLiveTest {
       }
    }
 
+   /**
+    * e.g. on aws-ec2: timeByImageId=534ms; timeByOsFamily=11587ms.
+    * Expecting it to be at least 2 times faster seems reasonable, including on other ec2 flavours.
+    */
+   @Test(enabled = true)
+   public void testTemplateBuildsFasterByImageIdThanBySearchingAllImages() throws Exception {
+      Stopwatch stopwatch = new Stopwatch();
+      
+      // Find any image, and get its id
+      template = buildTemplate(client.templateBuilder());
+      String imageId = template.getImage().getId();
+      
+      // Build a template using that specific image-id
+      context.close();
+      setupClient();
+      stopwatch.start();
+      client.templateBuilder().imageId(imageId).build();
+      stopwatch.stop();
+      long timeByImageId = stopwatch.elapsedMillis();
+
+      // Build a template using that specific image-id
+      context.close();
+      setupClient();
+      stopwatch.reset();
+      stopwatch.start();
+      try {
+         client.templateBuilder().osFamily(OsFamily.UBUNTU).build();
+      } catch (NoSuchElementException e) {
+         // ignore; we are only interested in how long it took to establish this fact!
+      }
+      stopwatch.stop();
+      long timeByOsFamily = stopwatch.elapsedMillis();
+
+      System.out.println("testTemplateBuildsFasterByImageIdThanBySearchingAllImages: " +
+               "timeByImageId="+timeByImageId+"ms; timeByOsFamily="+timeByOsFamily+"ms");
+      
+      assertTrue((timeByImageId*2) < timeByOsFamily, "timeByImageId="+timeByImageId+"; timeByOsFamily="+timeByOsFamily);
+   }
+
    protected RunningInstance getInstance(InstanceClient instanceClient, String id) {
       RunningInstance instance = Iterables.getOnlyElement(Iterables.getOnlyElement(instanceClient
                .describeInstancesInRegion(null, id)));

apis/ec2/src/test/java/org/jclouds/ec2/compute/EC2TemplateBuilderTest.java

@@ -18,6 +18,7 @@
  */
 package org.jclouds.ec2.compute;
 
+import static com.google.common.collect.Maps.uniqueIndex;
 import static java.lang.String.format;
 import static org.easymock.EasyMock.expect;
 import static org.easymock.classextension.EasyMock.createMock;
@@ -34,6 +35,8 @@ import static org.jclouds.ec2.compute.domain.EC2HardwareBuilder.m2_xlarge;
 import static org.jclouds.ec2.compute.domain.EC2HardwareBuilder.t1_micro;
 import static org.testng.Assert.assertEquals;
 
+import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.Set;
 
 import javax.inject.Provider;
@@ -52,11 +55,18 @@ import org.jclouds.domain.Location;
 import org.jclouds.domain.LocationBuilder;
 import org.jclouds.domain.LocationScope;
 import org.jclouds.domain.LoginCredentials;
+import org.jclouds.ec2.compute.domain.RegionAndName;
+import org.jclouds.ec2.compute.internal.EC2TemplateBuilderImpl;
 import org.testng.annotations.Test;
 
 import com.google.common.base.Function;
+import com.google.common.base.Functions;
 import com.google.common.base.Supplier;
 import com.google.common.base.Suppliers;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 
 /**
@@ -137,9 +147,69 @@ public class EC2TemplateBuilderTest {
                         .getHardware().getId());
    }
 
-   @SuppressWarnings("unchecked")
+   @Test
-   private TemplateBuilder newTemplateBuilder() {
+   public void testTemplateChoiceForInstanceByImageId() throws Exception {
+      Template template = newTemplateBuilder().imageId("us-east-1/cc-image").build();
 
+      assert template != null : "The returned template was null, but it should have a value.";
+      assertEquals(template.getImage().getId(), "us-east-1/cc-image");
+   }
+
+   @Test
+   public void testTemplateChoiceForInstanceByImageIdDoesNotGetAllImages() throws Exception {
+      @SuppressWarnings("unchecked")
+      Supplier<Set<? extends Image>> images = createMock(Supplier.class);
+      replay(images);
+      
+      final Image image = new ImageBuilder().providerId("cc-image").name("image").id("us-east-1/cc-image").location(location)
+               .operatingSystem(new OperatingSystem(OsFamily.UBUNTU, null, "1.0", "hvm", "ubuntu", true))
+               .description("description").version("1.0").defaultCredentials(new LoginCredentials("root", null, null, false))
+               .build();
+      Map<RegionAndName, Image> imageMap = ImmutableMap.of(
+               new RegionAndName(image.getLocation().getId(), image.getProviderId()), image);
+      
+      // weird compilation error means have to declare extra generics for call to build() - see https://bugs.eclipse.org/bugs/show_bug.cgi?id=365818
+      Supplier<Cache<RegionAndName, ? extends Image>> imageCache = Suppliers.<Cache<RegionAndName, ? extends Image>> ofInstance(
+               CacheBuilder.newBuilder().<RegionAndName,Image>build(CacheLoader.from(Functions.forMap(imageMap))));
+
+      Template template = newTemplateBuilder(images, imageCache).imageId("us-east-1/cc-image").build();
+
+      assert template != null : "The returned template was null, but it should have a value.";
+      assertEquals(template.getImage().getId(), "us-east-1/cc-image");
+   }
+
+   @Test(expectedExceptions={NoSuchElementException.class})
+   public void testNegativeTemplateChoiceForInstanceByImageId() throws Exception {
+      newTemplateBuilder().imageId("wrongregion/wrongimageid").build();
+   }
+
+   private TemplateBuilder newTemplateBuilder() {
+      final Supplier<Set<? extends Image>> images = Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of(
+               new ImageBuilder().providerId("cc-image").name("image").id("us-east-1/cc-image").location(location)
+                        .operatingSystem(new OperatingSystem(OsFamily.UBUNTU, null, "1.0", "hvm", "ubuntu", true))
+                        .description("description").version("1.0").defaultCredentials(new LoginCredentials("root", null, null, false))
+                        .build(), 
+               new ImageBuilder().providerId("normal-image").name("image").id("us-east-1/normal-image").location(location)
+                        .operatingSystem(new OperatingSystem(OsFamily.UBUNTU, null, "1.0", "paravirtual", "ubuntu", true))
+                        .description("description").version("1.0").defaultCredentials(new LoginCredentials("root", null, null, false))
+                        .build()));
+      
+      // weird compilation error means have to cast this - see https://bugs.eclipse.org/bugs/show_bug.cgi?id=365818
+      @SuppressWarnings("unchecked")
+      ImmutableMap<RegionAndName, Image> imageMap = (ImmutableMap<RegionAndName, Image>) uniqueIndex(images.get(), new Function<Image, RegionAndName>() {
+         @Override
+         public RegionAndName apply(Image from) {
+            return new RegionAndName(from.getLocation().getId(), from.getProviderId());
+         }
+      });
+      Supplier<Cache<RegionAndName, ? extends Image>> imageCache = Suppliers.<Cache<RegionAndName, ? extends Image>> ofInstance(
+               CacheBuilder.newBuilder().<RegionAndName,Image>build(CacheLoader.from(Functions.forMap(imageMap))));
+
+      return newTemplateBuilder(images, imageCache);
+   }
+
+   @SuppressWarnings("unchecked")
+   private TemplateBuilder newTemplateBuilder(Supplier<Set<? extends Image>> images, Supplier<Cache<RegionAndName, ? extends Image>> imageCache) {
       Provider<TemplateOptions> optionsProvider = createMock(Provider.class);
       Provider<TemplateBuilder> templateBuilderProvider = createMock(Provider.class);
       TemplateOptions defaultOptions = createMock(TemplateOptions.class);
@@ -150,26 +220,16 @@ public class EC2TemplateBuilderTest {
       replay(templateBuilderProvider);
       Supplier<Set<? extends Location>> locations = Suppliers.<Set<? extends Location>> ofInstance(ImmutableSet
                .<Location> of(location));
-      Supplier<Set<? extends Image>> images = Suppliers.<Set<? extends Image>> ofInstance(ImmutableSet.<Image> of(
-               new ImageBuilder().providerId("cc-image").name("image").id("us-east-1/cc-image").location(location)
-                        .operatingSystem(new OperatingSystem(OsFamily.UBUNTU, null, "1.0", "hvm", "ubuntu", true))
-                        .description("description").version("1.0").defaultCredentials(new LoginCredentials("root", null, null, false))
-                        .build(), new ImageBuilder().providerId("normal-image").name("image").id("us-east-1/cc-image")
-                        .location(location).operatingSystem(
-                                 new OperatingSystem(OsFamily.UBUNTU, null, "1.0", "paravirtual", "ubuntu", true))
-                        .description("description").version("1.0").defaultCredentials(new LoginCredentials("root", null, null, false))
-                        .build()));
       Supplier<Set<? extends Hardware>> sizes = Suppliers.<Set<? extends Hardware>> ofInstance(ImmutableSet
                .<Hardware> of(t1_micro().build(), c1_medium().build(), c1_xlarge().build(), m1_large().build(),
                         m1_small32().build(), m1_xlarge().build(), m2_xlarge().build(), m2_2xlarge().build(),
                         m2_4xlarge().build(), CC1_4XLARGE));
 
-      return new TemplateBuilderImpl(locations, images, sizes, Suppliers.ofInstance(location), optionsProvider,
+      return new EC2TemplateBuilderImpl(locations, images, sizes, Suppliers.ofInstance(location), optionsProvider,
-               templateBuilderProvider) {
+               templateBuilderProvider, imageCache) {
-
       };
    }
-
+   
    Function<ComputeMetadata, String> indexer() {
       return new Function<ComputeMetadata, String>() {
          @Override
@@ -178,5 +238,4 @@ public class EC2TemplateBuilderTest {
          }
       };
    }
-
 }

apis/ec2/src/test/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImplTest.java

@@ -18,6 +18,7 @@
  */
 package org.jclouds.ec2.compute.internal;
 
+import static com.google.common.collect.Maps.uniqueIndex;
 import static org.easymock.EasyMock.expect;
 import static org.easymock.classextension.EasyMock.createMock;
 import static org.easymock.classextension.EasyMock.replay;
@@ -45,6 +46,7 @@ import org.jclouds.ec2.compute.domain.RegionAndName;
 import org.jclouds.ec2.compute.options.EC2TemplateOptions;
 import org.testng.annotations.Test;
 
+import com.google.common.base.Function;
 import com.google.common.base.Supplier;
 import com.google.common.base.Suppliers;
 import com.google.common.cache.Cache;
@@ -67,18 +69,38 @@ public class EC2TemplateBuilderImplTest extends TemplateBuilderImplTest {
 
    @Override
    protected EC2TemplateBuilderImpl createTemplateBuilder(final Image knownImage,
-         @Memoized Supplier<Set<? extends Location>> locations, @Memoized Supplier<Set<? extends Image>> images,
+         @Memoized Supplier<Set<? extends Location>> locations, final @Memoized Supplier<Set<? extends Image>> images,
          @Memoized Supplier<Set<? extends Hardware>> sizes, Location defaultLocation,
          Provider<TemplateOptions> optionsProvider, Provider<TemplateBuilder> templateBuilderProvider) {
-      final RegionAndName knownRegionAndName = new RegionAndName("region", "ami");
 
-      Cache<RegionAndName, ? extends Image> imageMap = CacheBuilder.newBuilder().build(new CacheLoader<RegionAndName, Image>() {
+      Cache<RegionAndName, ? extends Image> imageMap;
-         @Override
+      if (knownImage != null) {
-         public Image load(RegionAndName from) {
+         final RegionAndName knownRegionAndName = new RegionAndName(knownImage.getLocation().getId(), knownImage.getProviderId());
-            return from.equals(knownRegionAndName) ? knownImage : null;
-         }
 
-      });
+         imageMap = CacheBuilder.newBuilder().build(new CacheLoader<RegionAndName, Image>() {
+            @Override
+            public Image load(RegionAndName from) {
+               return from.equals(knownRegionAndName) ? knownImage : null;
+            }
+
+         });
+         
+      } else {
+         imageMap = CacheBuilder.newBuilder().build(new CacheLoader<RegionAndName, Image>() {
+            @Override
+            public Image load(RegionAndName from) {
+               return uniqueIndex(images.get(), new Function<Image, RegionAndName>() {
+                  
+                  @Override
+                  public RegionAndName apply(Image from) {
+                     return new RegionAndName(from.getLocation().getId(), from.getProviderId());
+                  }
+                  
+               }).get(from);
+            }
+   
+         });
+      }
 
       return new EC2TemplateBuilderImpl(locations, images, sizes, Suppliers.ofInstance(defaultLocation),
             optionsProvider, templateBuilderProvider, Suppliers.<Cache<RegionAndName, ? extends Image>>ofInstance(imageMap));
@@ -109,6 +131,7 @@ public class EC2TemplateBuilderImplTest extends TemplateBuilderImplTest {
       expect(knownImage.getName()).andReturn(null).atLeastOnce();
       expect(knownImage.getDescription()).andReturn(null).atLeastOnce();
       expect(knownImage.getVersion()).andReturn(null).atLeastOnce();
+      expect(knownImage.getProviderId()).andReturn("ami").atLeastOnce();
 
       expect(knownImage.getOperatingSystem()).andReturn(os).atLeastOnce();
 
@@ -152,6 +175,9 @@ public class EC2TemplateBuilderImplTest extends TemplateBuilderImplTest {
       Provider<TemplateBuilder> templateBuilderProvider = createMock(Provider.class);
       TemplateOptions defaultOptions = createMock(TemplateOptions.class);
       Image knownImage = createMock(Image.class);
+      expect(knownImage.getId()).andReturn("region/ami").anyTimes();
+      expect(knownImage.getProviderId()).andReturn("ami").anyTimes();
+      expect(knownImage.getLocation()).andReturn(location).anyTimes();
 
       expect(optionsProvider.get()).andReturn(defaultOptions);
 
@@ -190,6 +216,9 @@ public class EC2TemplateBuilderImplTest extends TemplateBuilderImplTest {
       Provider<TemplateBuilder> templateBuilderProvider = createMock(Provider.class);
       TemplateOptions defaultOptions = createMock(TemplateOptions.class);
       Image knownImage = createMock(Image.class);
+      expect(knownImage.getId()).andReturn("region/ami").anyTimes();
+      expect(knownImage.getProviderId()).andReturn("ami").anyTimes();
+      expect(knownImage.getLocation()).andReturn(location).anyTimes();
 
       expect(defaultLocation.getId()).andReturn("region");
       expect(optionsProvider.get()).andReturn(defaultOptions);

compute/src/test/java/org/jclouds/compute/domain/internal/TemplateBuilderImplTest.java

@@ -139,7 +139,7 @@ public class TemplateBuilderImplTest {
       expect(image2.getLocation()).andReturn(defaultLocation).atLeastOnce();
       expect(image.getOperatingSystem()).andReturn(os).atLeastOnce();
       expect(image2.getOperatingSystem()).andReturn(os2).atLeastOnce();
-      expect(image.getId()).andReturn("1");
+      expect(image.getId()).andReturn("myregion/1");
 
       expect(os.getArch()).andReturn("X86_32").atLeastOnce();
       expect(os2.getArch()).andReturn("X86_64").atLeastOnce();
@@ -175,7 +175,7 @@ public class TemplateBuilderImplTest {
       Image image = createMock(Image.class);
       OperatingSystem os = createMock(OperatingSystem.class);
 
-      Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals("imageId"))
+      Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals("myregion/imageId"))
                .build();
 
       Supplier<Set<? extends Location>> locations = Suppliers.<Set<? extends Location>> ofInstance(ImmutableSet
@@ -189,12 +189,15 @@ public class TemplateBuilderImplTest {
       TemplateBuilder defaultTemplate = createMock(TemplateBuilder.class);
 
       expect(optionsProvider.get()).andReturn(new TemplateOptions());
-      expect(image.getId()).andReturn("imageId").atLeastOnce();
+      expect(image.getId()).andReturn("myregion/imageId").atLeastOnce();
       expect(image.getLocation()).andReturn(defaultLocation).atLeastOnce();
       expect(image.getName()).andReturn(null).atLeastOnce();
       expect(image.getDescription()).andReturn(null).atLeastOnce();
       expect(image.getVersion()).andReturn(null).atLeastOnce();
       expect(image.getOperatingSystem()).andReturn(os).atLeastOnce();
+      expect(image.getProviderId()).andReturn("imageId").anyTimes();
+      
+      expect(defaultLocation.getId()).andReturn("myregion").anyTimes();
 
       expect(os.getName()).andReturn(null).atLeastOnce();
       expect(os.getVersion()).andReturn(null).atLeastOnce();
@@ -215,7 +218,7 @@ public class TemplateBuilderImplTest {
       TemplateBuilderImpl template = createTemplateBuilder(null, locations, images, hardwares, defaultLocation,
                optionsProvider, templateBuilderProvider);
 
-      template.imageId("imageId").build();
+      template.imageId("myregion/imageId").build();
 
       verify(image);
       verify(os);
@@ -232,7 +235,7 @@ public class TemplateBuilderImplTest {
       Image image = createMock(Image.class);
       OperatingSystem os = createMock(OperatingSystem.class);
 
-      Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals("imageId"))
+      Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals("differentImageId"))
                .build();
 
       Supplier<Set<? extends Location>> locations = Suppliers.<Set<? extends Location>> ofInstance(ImmutableSet
@@ -246,12 +249,15 @@ public class TemplateBuilderImplTest {
       TemplateBuilder defaultTemplate = createMock(TemplateBuilder.class);
 
       expect(optionsProvider.get()).andReturn(new TemplateOptions());
-      expect(image.getId()).andReturn("notImageId").atLeastOnce();
+      expect(image.getId()).andReturn("myregion/imageId").atLeastOnce();
       expect(image.getLocation()).andReturn(defaultLocation).atLeastOnce();
       expect(image.getOperatingSystem()).andReturn(os).atLeastOnce();
       expect(image.getName()).andReturn(null).atLeastOnce();
       expect(image.getDescription()).andReturn(null).atLeastOnce();
       expect(image.getVersion()).andReturn(null).atLeastOnce();
+      expect(image.getProviderId()).andReturn("imageId").anyTimes();
+      
+      expect(defaultLocation.getId()).andReturn("myregion").anyTimes();
 
       expect(os.getName()).andReturn(null).atLeastOnce();
       expect(os.getVersion()).andReturn(null).atLeastOnce();
@@ -272,13 +278,13 @@ public class TemplateBuilderImplTest {
       TemplateBuilderImpl template = createTemplateBuilder(image, locations, images, hardwares, defaultLocation,
                optionsProvider, templateBuilderProvider);
       try {
-         template.imageId("notImageId").build();
+         template.imageId("myregion/imageId").build();
          assert false;
       } catch (NoSuchElementException e) {
          // make sure big data is not in the exception message
          assertEquals(
                   e.getMessage(),
-                  "no hardware profiles support images matching params: [biggest=false, fastest=false, imageName=null, imageDescription=null, imageId=notImageId, imagePredicate=null, imageVersion=null, location=EasyMock for interface org.jclouds.domain.Location, minCores=0.0, minRam=0, osFamily=null, osName=null, osDescription=null, osVersion=null, osArch=null, os64Bit=false, hardwareId=null]");
+                  "no hardware profiles support images matching params: [biggest=false, fastest=false, imageName=null, imageDescription=null, imageId=myregion/imageId, imagePredicate=null, imageVersion=null, location=EasyMock for interface org.jclouds.domain.Location, minCores=0.0, minRam=0, osFamily=null, osName=null, osDescription=null, osVersion=null, osArch=null, os64Bit=false, hardwareId=null]");
          verify(image);
          verify(os);
          verify(defaultTemplate);
@@ -376,7 +382,7 @@ public class TemplateBuilderImplTest {
       Location defaultLocation = createMock(Location.class);
       Image image = createMock(Image.class);
 
-      Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals("foo")).build();
+      Hardware hardware = new HardwareBuilder().id("hardwareId").supportsImage(ImagePredicates.idEquals("myregion/foo")).build();
 
       Supplier<Set<? extends Location>> locations = Suppliers.<Set<? extends Location>> ofInstance(ImmutableSet
                .<Location> of(defaultLocation));
@@ -392,15 +398,16 @@ public class TemplateBuilderImplTest {
       TemplateOptions defaultOptions = createMock(TemplateOptions.class);
       expect(optionsProvider.get()).andReturn(from).atLeastOnce();
 
-      expect(defaultLocation.getId()).andReturn("location").atLeastOnce();
+      expect(defaultLocation.getId()).andReturn("myregion").atLeastOnce();
 
-      expect(image.getId()).andReturn("foo").atLeastOnce();
+      expect(image.getId()).andReturn("myregion/foo").atLeastOnce();
       expect(image.getLocation()).andReturn(defaultLocation).atLeastOnce();
       expect(image.getOperatingSystem()).andReturn(os).atLeastOnce();
       expect(image.getName()).andReturn(null).atLeastOnce();
       expect(image.getDescription()).andReturn(null).atLeastOnce();
       expect(image.getVersion()).andReturn(null).atLeastOnce();
-
+      expect(image.getProviderId()).andReturn("foo").anyTimes();
+      
       expect(os.getName()).andReturn(null).atLeastOnce();
       expect(os.getVersion()).andReturn(null).atLeastOnce();
       expect(os.getFamily()).andReturn(null).atLeastOnce();
@@ -421,7 +428,7 @@ public class TemplateBuilderImplTest {
       TemplateBuilderImpl template = createTemplateBuilder(null, locations, images, hardwares, defaultLocation,
                optionsProvider, templateBuilderProvider);
 
-      assertEquals(template.imageId("foo").locationId("location").build().getLocation(), defaultLocation);
+      assertEquals(template.imageId("myregion/foo").locationId("myregion").build().getLocation(), defaultLocation);
 
       verify(defaultOptions);
       verify(imageLocation);
@@ -525,11 +532,11 @@ public class TemplateBuilderImplTest {
                optionsProvider, templateBuilderProvider);
 
       try {
-         template.imageId("region/ami").build();
+         template.imageId("region/imageId").build();
          assert false;
       } catch (NoSuchElementException e) {
          // make sure big data is not in the exception message
-         assertEquals(e.getMessage(), "imageId(region/ami) not found");
+         assertEquals(e.getMessage(), "imageId(region/imageId) not found");
       }
 
       verify(defaultOptions);

providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/config/AWSEC2BindComputeSuppliersByClass.java

@@ -0,0 +1,36 @@
+/**
+ * Licensed to jclouds, Inc. (jclouds) under one or more
+ * contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  jclouds licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.jclouds.aws.ec2.compute.config;
+
+import java.util.Set;
+
+import org.jclouds.aws.ec2.compute.suppliers.AWSEC2ImageSupplier;
+import org.jclouds.compute.domain.Image;
+import org.jclouds.ec2.compute.config.EC2BindComputeSuppliersByClass;
+
+import com.google.common.base.Supplier;
+/**
+ * @author Aled Sage
+ */
+public class AWSEC2BindComputeSuppliersByClass extends EC2BindComputeSuppliersByClass {
+   @Override
+   protected Class<? extends Supplier<Set<? extends Image>>> defineImageSupplier() {
+      return AWSEC2ImageSupplier.class;
+   }
+}

providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/config/AWSEC2ComputeServiceContextModule.java

@@ -24,6 +24,7 @@ import static org.jclouds.compute.domain.OsFamily.AMZN_LINUX;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
+import org.jclouds.aws.ec2.AWSEC2PropertiesBuilder;
 import org.jclouds.aws.ec2.compute.AWSEC2TemplateBuilderImpl;
 import org.jclouds.aws.ec2.compute.functions.AWSRunningInstanceToNodeMetadata;
 import org.jclouds.aws.ec2.compute.predicates.AWSEC2InstancePresent;
@@ -34,13 +35,11 @@ import org.jclouds.aws.ec2.compute.strategy.AWSEC2ListNodesStrategy;
 import org.jclouds.aws.ec2.compute.strategy.AWSEC2ReviseParsedImage;
 import org.jclouds.aws.ec2.compute.strategy.CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptions;
 import org.jclouds.aws.ec2.compute.suppliers.AWSEC2HardwareSupplier;
-import org.jclouds.aws.ec2.compute.suppliers.AWSRegionAndNameToImageSupplier;
 import org.jclouds.compute.config.BaseComputeServiceContextModule;
 import org.jclouds.compute.domain.Image;
 import org.jclouds.compute.domain.TemplateBuilder;
 import org.jclouds.compute.options.TemplateOptions;
 import org.jclouds.ec2.compute.config.EC2BindComputeStrategiesByClass;
-import org.jclouds.ec2.compute.config.EC2BindComputeSuppliersByClass;
 import org.jclouds.ec2.compute.domain.RegionAndName;
 import org.jclouds.ec2.compute.functions.RunningInstanceToNodeMetadata;
 import org.jclouds.ec2.compute.internal.EC2TemplateBuilderImpl;
@@ -53,6 +52,7 @@ import org.jclouds.ec2.compute.strategy.EC2GetNodeMetadataStrategy;
 import org.jclouds.ec2.compute.strategy.EC2ListNodesStrategy;
 import org.jclouds.ec2.compute.strategy.ReviseParsedImage;
 import org.jclouds.ec2.compute.suppliers.EC2HardwareSupplier;
+import org.jclouds.ec2.compute.suppliers.RegionAndNameToImageSupplier;
 import org.jclouds.rest.suppliers.MemoizedRetryOnTimeOutButNotOnAuthorizationExceptionSupplier;
 
 import com.google.common.base.Supplier;
@@ -70,7 +70,7 @@ public class AWSEC2ComputeServiceContextModule extends BaseComputeServiceContext
       super.configure();
       installDependencies();
       install(new EC2BindComputeStrategiesByClass());
-      install(new EC2BindComputeSuppliersByClass());
+      install(new AWSEC2BindComputeSuppliersByClass());
       bind(ReviseParsedImage.class).to(AWSEC2ReviseParsedImage.class);
       bind(CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptions.class).to(
                CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptions.class);
@@ -91,7 +91,7 @@ public class AWSEC2ComputeServiceContextModule extends BaseComputeServiceContext
    @Provides
    @Singleton
    protected Supplier<Cache<RegionAndName, ? extends Image>> provideRegionAndNameToImageSupplierCache(
-            @Named(PROPERTY_SESSION_INTERVAL) long seconds, final AWSRegionAndNameToImageSupplier supplier) {
+            @Named(PROPERTY_SESSION_INTERVAL) long seconds, final RegionAndNameToImageSupplier supplier) {
       return new MemoizedRetryOnTimeOutButNotOnAuthorizationExceptionSupplier<Cache<RegionAndName, ? extends Image>>(
                authException, seconds, new Supplier<Cache<RegionAndName, ? extends Image>>() {
                   @Override

providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/suppliers/AWSEC2ImageSupplier.java

@@ -25,6 +25,7 @@ import static org.jclouds.aws.ec2.reference.AWSEC2Constants.PROPERTY_EC2_AMI_QUE
 import static org.jclouds.aws.ec2.reference.AWSEC2Constants.PROPERTY_EC2_CC_AMI_QUERY;
 import static org.jclouds.aws.ec2.reference.AWSEC2Constants.PROPERTY_EC2_CC_REGIONS;
 
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
@@ -47,8 +48,7 @@ import com.google.common.base.Splitter;
 import com.google.common.base.Supplier;
 import com.google.common.base.Throwables;
 import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
-import com.google.common.cache.CacheLoader;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -60,12 +60,14 @@ import com.google.common.util.concurrent.Futures;
  * @author Adrian Cole
  */
 @Singleton
-public class AWSRegionAndNameToImageSupplier implements Supplier<Cache<RegionAndName, ? extends Image>> {
+public class AWSEC2ImageSupplier implements Supplier<Set<? extends Image>> {
+   
+   // TODO could/should this sub-class EC2ImageSupplier? Or does that confuse guice?
+   
    @Resource
    @Named(ComputeServiceConstants.COMPUTE_LOGGER)
    protected Logger logger = Logger.NULL;
-
+   
-   private final CacheLoader<RegionAndName, Image> regionAndIdToImage;
    private final Set<String> clusterComputeIds;
    private final CallForImages.Factory factory;
    private final ExecutorService executor;
@@ -74,11 +76,13 @@ public class AWSRegionAndNameToImageSupplier implements Supplier<Cache<RegionAnd
    private final String amiQuery;
    private final Iterable<String> clusterRegions;
    private final String ccAmiQuery;
-
+   private final Supplier<Cache<RegionAndName, ? extends Image>> cache;
+   
    @Inject
-   protected AWSRegionAndNameToImageSupplier(@Region Set<String> regions,
+   protected AWSEC2ImageSupplier(@Region Set<String> regions,
             @Named(PROPERTY_EC2_AMI_QUERY) String amiQuery, @Named(PROPERTY_EC2_CC_REGIONS) String clusterRegions,
-            @Named(PROPERTY_EC2_CC_AMI_QUERY) String ccAmiQuery, CacheLoader<RegionAndName, Image> regionAndIdToImage,
+            @Named(PROPERTY_EC2_CC_AMI_QUERY) String ccAmiQuery, 
+            Supplier<Cache<RegionAndName, ? extends Image>> cache,
             CallForImages.Factory factory, @ClusterCompute Set<String> clusterComputeIds,
             @Named(Constants.PROPERTY_USER_THREADS) ExecutorService executor) {
       this.factory = factory;
@@ -86,21 +90,21 @@ public class AWSRegionAndNameToImageSupplier implements Supplier<Cache<RegionAnd
       this.amiQuery = amiQuery;
       this.clusterRegions = Splitter.on(',').split(clusterRegions);
       this.ccAmiQuery = ccAmiQuery;
-      this.regionAndIdToImage = regionAndIdToImage;
+      this.cache = cache;
       this.clusterComputeIds = clusterComputeIds;
       this.executor = executor;
    }
-
+   
+   @SuppressWarnings({ "unchecked", "rawtypes" })
    @Override
-   public Cache<RegionAndName, ? extends Image> get() {
+   public Set<? extends Image> get() {
       Future<Iterable<Image>> normalImages = images(regions, amiQuery, PROPERTY_EC2_AMI_QUERY);
       ImmutableSet<Image> clusterImages;
       try {
          clusterImages = ImmutableSet.copyOf(images(clusterRegions, ccAmiQuery, PROPERTY_EC2_CC_AMI_QUERY).get());
       } catch (Exception e) {
          logger.warn(e, "Error parsing images in query %s", ccAmiQuery);
-         Throwables.propagate(e);
+         throw Throwables.propagate(e);
-         return null;
       }
       Iterables.addAll(clusterComputeIds, transform(clusterImages, new Function<Image, String>() {
 
@@ -108,29 +112,34 @@ public class AWSRegionAndNameToImageSupplier implements Supplier<Cache<RegionAnd
          public String apply(Image arg0) {
             return arg0.getId();
          }
+
       }));
       Iterable<? extends Image> parsedImages;
       try {
          parsedImages = ImmutableSet.copyOf(concat(clusterImages, normalImages.get()));
       } catch (Exception e) {
          logger.warn(e, "Error parsing images in query %s", amiQuery);
-         Throwables.propagate(e);
+         throw Throwables.propagate(e);
-         return null;
       }
-      Cache<RegionAndName, Image> cache = CacheBuilder.newBuilder().build(regionAndIdToImage);
 
-      cache.asMap().putAll(uniqueIndex(parsedImages, new Function<Image, RegionAndName>() {
+      // TODO Need to clear out old entries; previously it was a new cache object every time 
+      // (and enclosed within the cache provider so didn't risk someone getting the cache while it was empty)!
+      ImmutableMap<RegionAndName, ? extends Image> imageMap = uniqueIndex(parsedImages, new Function<Image, RegionAndName>() {
 
          @Override
          public RegionAndName apply(Image from) {
             return new RegionAndName(from.getLocation().getId(), from.getProviderId());
          }
 
-      }));
+      });
-      logger.debug("<< images(%d)", cache.asMap().size());
+      cache.get().invalidateAll();
-      return cache;
+      cache.get().asMap().putAll((Map)imageMap);
+      logger.debug("<< images(%d)", imageMap.size());
+      
+      // TODO Used to be mutable; was this assumed anywhere?
+      return ImmutableSet.copyOf(imageMap.values());
    }
-
+   
    private Future<Iterable<Image>> images(Iterable<String> regions, String query, String tag) {
       if (query.equals("")) {
          logger.debug(">> no %s specified, skipping image parsing", tag);

providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/AWSEC2TemplateBuilderLiveTest.java

@@ -332,6 +332,16 @@ public class AWSEC2TemplateBuilderLiveTest extends BaseTemplateBuilderLiveTest {
       }
    }
 
+   @Test
+   public void testTemplateBuilderCanUseImageIdFromNonDefaultOwner() {
+      // This is the id of a public image, not owned by one of the four default owners
+      String imageId = "us-east-1/ami-44d02f2d";
+      Template defaultTemplate = context.getComputeService().templateBuilder().imageId(imageId)
+               .imageMatches(EC2ImagePredicates.rootDeviceType(RootDeviceType.INSTANCE_STORE)).build();
+      assert (defaultTemplate.getImage().getProviderId().startsWith("ami-")) : defaultTemplate;
+      assertEquals(defaultTemplate.getImage().getId(), imageId);
+   }
+
    @Override
    protected Set<String> getIso3166Codes() {
       return ImmutableSet.<String> of("US-VA", "US-CA", "US-OR", "IE", "SG", "JP-13");