improved nat rules to survive existing sets, and also made sure we lock machines before attempting to delete them

sandbox-apis/virtualbox/src/main/java/org/jclouds/virtualbox/functions/AttachNATAdapterToMachineIfNotAlreadyExists.java

@@ -27,18 +27,19 @@ import org.jclouds.virtualbox.domain.NatAdapter;
 import org.jclouds.virtualbox.domain.RedirectRule;
 import org.virtualbox_4_1.IMachine;
 import org.virtualbox_4_1.INetworkAdapter;
+import org.virtualbox_4_1.VBoxException;
 
 import com.google.common.base.Function;
 
 /**
  * @author Mattias Holmqvist
  */
-public class AttachNATAdapterToMachine implements Function<IMachine, Void> {
+public class AttachNATAdapterToMachineIfNotAlreadyExists implements Function<IMachine, Void> {
 
    private long adapterSlot;
    private NatAdapter natAdapter;
 
-   public AttachNATAdapterToMachine(long adapterSlot, NatAdapter natAdapter) {
+   public AttachNATAdapterToMachineIfNotAlreadyExists(long adapterSlot, NatAdapter natAdapter) {
       this.adapterSlot = adapterSlot;
       this.natAdapter = natAdapter;
    }
@@ -48,12 +49,15 @@ public class AttachNATAdapterToMachine implements Function<IMachine, Void> {
       INetworkAdapter networkAdapter = machine.getNetworkAdapter(adapterSlot);
       networkAdapter.setAttachmentType(NAT);
       for (RedirectRule rule : natAdapter.getRedirectRules()) {
-         networkAdapter.getNatDriver().addRedirect("guestssh",
+         try {
-                 rule.getProtocol(),
+            String ruleName = String.format("%s@%s:%s->%s:%s",rule.getProtocol(), rule.getHost(), rule.getHostPort(), 
-                 rule.getHost(),
+                     rule.getGuest(), rule.getGuestPort());
-                 rule.getHostPort(),
+            networkAdapter.getNatDriver().addRedirect(ruleName, rule.getProtocol(), rule.getHost(), rule.getHostPort(),
-                 rule.getGuest(),
+                     rule.getGuest(), rule.getGuestPort());
-                 rule.getGuestPort());
+         } catch (VBoxException e) {
+            if (e.getMessage().indexOf("already exists") == -1)
+               throw e;
+         }
       }
       networkAdapter.setEnabled(true);
       machine.saveSettings();

sandbox-apis/virtualbox/src/main/java/org/jclouds/virtualbox/functions/CreateAndInstallVm.java

@@ -207,7 +207,7 @@ public class CreateAndInstallVm implements Function<VmSpec, IMachine> {
    }
 
    private void ensureNATNetworkingIsAppliedToMachine(String vmName, long slotId, NatAdapter natAdapter) {
-      lockMachineAndApply(manager, Write, vmName, new AttachNATAdapterToMachine(slotId, natAdapter));
+      lockMachineAndApply(manager, Write, vmName, new AttachNATAdapterToMachineIfNotAlreadyExists(slotId, natAdapter));
    }
 
    public void ensureMachineHasIDEControllerNamed(String vmName, StorageController storageController) {

sandbox-apis/virtualbox/src/main/java/org/jclouds/virtualbox/functions/CreateAndRegisterMachineFromIsoIfNotAlreadyExists.java

@@ -161,7 +161,7 @@ public class CreateAndRegisterMachineFromIsoIfNotAlreadyExists implements Functi
    }
 
    private void ensureNATNetworkingIsAppliedToMachine(String vmName, long slotId, NatAdapter natAdapter) {
-      lockMachineAndApply(manager, Write, vmName, new AttachNATAdapterToMachine(slotId, natAdapter));
+      lockMachineAndApply(manager, Write, vmName, new AttachNATAdapterToMachineIfNotAlreadyExists(slotId, natAdapter));
    }
 
    public void ensureMachineHasStorageControllerNamed(String vmName, StorageController storageController) {

sandbox-apis/virtualbox/src/main/java/org/jclouds/virtualbox/functions/admin/UnregisterMachineIfExistsAndDeleteItsMedia.java

@@ -39,7 +39,6 @@ import org.virtualbox_4_1.IMachine;
 import org.virtualbox_4_1.IMedium;
 import org.virtualbox_4_1.IProgress;
 import org.virtualbox_4_1.VBoxException;
-import org.virtualbox_4_1.VirtualBoxManager;
 
 import com.google.common.base.Function;
 import com.google.common.base.Predicate;
@@ -47,24 +46,22 @@ import com.google.common.base.Throwables;
 import com.google.common.collect.Lists;
 
 public class UnregisterMachineIfExistsAndDeleteItsMedia implements
-      Function<VmSpec, Void> {
+      Function<IMachine, Void> {
 
    @Resource
    @Named(ComputeServiceConstants.COMPUTE_LOGGER)
    protected Logger logger = Logger.NULL;
 
-   private VirtualBoxManager manager;
+   private final VmSpec vmSpec;
 
-   public UnregisterMachineIfExistsAndDeleteItsMedia(VirtualBoxManager manager) {
+   public UnregisterMachineIfExistsAndDeleteItsMedia(VmSpec vmSpec) {
-      this.manager = manager;
+      this.vmSpec = vmSpec;
    }
 
    @Override
-   public Void apply(VmSpec vmSpec) {
+   public Void apply(IMachine machine) {
       List<IMedium> mediaToBeDeleted = Collections.emptyList();
-      IMachine machine = null;
       try {
-         machine = manager.getVBox().findMachine(vmSpec.getVmName());
          mediaToBeDeleted = machine.unregister(vmSpec.getCleanupMode());
       } catch (VBoxException e) {
          ErrorCode errorCode = ErrorCode.valueOf(e);

sandbox-apis/virtualbox/src/main/java/org/jclouds/virtualbox/util/MachineUtils.java

@@ -19,8 +19,14 @@
 
 package org.jclouds.virtualbox.util;
 
+import org.jclouds.util.Throwables2;
+import org.virtualbox_4_1.IMachine;
+import org.virtualbox_4_1.ISession;
+import org.virtualbox_4_1.LockType;
+import org.virtualbox_4_1.VBoxException;
+import org.virtualbox_4_1.VirtualBoxManager;
+
 import com.google.common.base.Function;
-import org.virtualbox_4_1.*;
 
 /**
  * Utilities for executing functions on a VirtualBox machine.
@@ -72,7 +78,7 @@ public class MachineUtils {
 
       });
    }
-
+   
    /**
     * Locks the machine and executes the given function using the current session.
     * Since the machine is locked it is possible to perform some modifications to the IMachine.
@@ -101,4 +107,28 @@ public class MachineUtils {
                  type, e.getMessage()), e);
       }
    }
+   
+   /**
+    * Locks the machine and executes the given function using the current session, if the machine is registered.
+    * Since the machine is locked it is possible to perform some modifications to the IMachine.
+    * <p/>
+    * Unlocks the machine before returning.
+    *
+    * @param manager   the VirtualBoxManager
+    * @param type      the kind of lock to use when initially locking the machine.
+    * @param machineId the id of the machine
+    * @param function  the function to execute
+    * @return the result from applying the function to the session.
+    */
+   public static <T> T lockMachineAndApplyOrReturnNullIfNotRegistered(VirtualBoxManager manager, LockType type, String machineId,
+                                                    Function<IMachine, T> function) {
+      try {
+         return lockMachineAndApply(manager, type, machineId, function);
+      } catch (RuntimeException e) {
+         VBoxException vbex = Throwables2.getFirstThrowableOfType(e, VBoxException.class);
+         if (vbex != null && vbex.getMessage().indexOf("not find a registered") == -1)
+            throw e;
+         return null;
+      }
+   }
 }

sandbox-apis/virtualbox/src/test/java/org/jclouds/virtualbox/functions/AttachNATAdapterToMachineIfNotAlreadyExistsTest.java

@@ -19,6 +19,14 @@
 
 package org.jclouds.virtualbox.functions;
 
+import static org.easymock.EasyMock.*;
+import static org.easymock.classextension.EasyMock.createMock;
+import static org.easymock.classextension.EasyMock.createNiceMock;
+import static org.easymock.classextension.EasyMock.replay;
+import static org.easymock.classextension.EasyMock.verify;
+import static org.virtualbox_4_1.NATProtocol.TCP;
+import static org.virtualbox_4_1.NetworkAttachmentType.NAT;
+
 import org.jclouds.virtualbox.domain.NatAdapter;
 import org.testng.annotations.Test;
 import org.virtualbox_4_1.IMachine;
@@ -26,16 +34,11 @@ import org.virtualbox_4_1.INATEngine;
 import org.virtualbox_4_1.INetworkAdapter;
 import org.virtualbox_4_1.VBoxException;
 
-import static org.easymock.EasyMock.expect;
-import static org.easymock.classextension.EasyMock.*;
-import static org.virtualbox_4_1.NATProtocol.TCP;
-import static org.virtualbox_4_1.NetworkAttachmentType.NAT;
-
 /**
  * @author Mattias Holmqvist
  */
-@Test(groups = "unit", testName = "AttachNATAdapterToMachineTest")
+@Test(groups = "unit", testName = "AttachNATAdapterToMachineIfNotAlreadyExistsTest")
-public class AttachNATAdapterToMachineTest {
+public class AttachNATAdapterToMachineIfNotAlreadyExistsTest {
 
    @Test
    public void testApplyNetworkingToNonExistingAdapter() throws Exception {
@@ -47,17 +50,43 @@ public class AttachNATAdapterToMachineTest {
       expect(machine.getNetworkAdapter(slotId)).andReturn(networkAdapter);
       networkAdapter.setAttachmentType(NAT);
       expect(networkAdapter.getNatDriver()).andReturn(natEngine);
-      natEngine.addRedirect("guestssh", TCP, "127.0.0.1", 2222, "", 22);
+      
+      natEngine.addRedirect("TCP@127.0.0.1:2222->guest:22", TCP, "127.0.0.1", 2222, "guest", 22);
       networkAdapter.setEnabled(true);
       machine.saveSettings();
 
       replay(machine, networkAdapter, natEngine);
-      NatAdapter natAdapter = NatAdapter.builder().tcpRedirectRule("127.0.0.1", 2222, "", 22).build();
+      NatAdapter natAdapter = NatAdapter.builder().tcpRedirectRule("127.0.0.1", 2222, "guest", 22).build();
-      new AttachNATAdapterToMachine(slotId, natAdapter).apply(machine);
+      new AttachNATAdapterToMachineIfNotAlreadyExists(slotId, natAdapter).apply(machine);
 
       verify(machine, networkAdapter, natEngine);
    }
 
+   @Test
+   public void testApplySkipsWhenAlreadyExists() throws Exception {
+      Long slotId = 0l;
+      IMachine machine = createMock(IMachine.class);
+      INetworkAdapter networkAdapter = createMock(INetworkAdapter.class);
+      INATEngine natEngine = createMock(INATEngine.class);
+
+      expect(machine.getNetworkAdapter(slotId)).andReturn(networkAdapter);
+      networkAdapter.setAttachmentType(NAT);
+      expect(networkAdapter.getNatDriver()).andReturn(natEngine);
+      
+      natEngine.addRedirect("TCP@127.0.0.1:2222->guest:22", TCP, "127.0.0.1", 2222, "guest", 22);
+      expectLastCall().andThrow(
+               new VBoxException(null, "VirtualBox error: A NAT rule of this name already exists (0x80070057)"));      
+      
+      networkAdapter.setEnabled(true);
+      machine.saveSettings();
+
+      replay(machine, networkAdapter, natEngine);
+      NatAdapter natAdapter = NatAdapter.builder().tcpRedirectRule("127.0.0.1", 2222, "guest", 22).build();
+      new AttachNATAdapterToMachineIfNotAlreadyExists(slotId, natAdapter).apply(machine);
+
+      verify(machine, networkAdapter, natEngine);
+   }
+   
    @Test(expectedExceptions = VBoxException.class)
    public void testRethrowInvalidAdapterSlotException() throws Exception {
       Long slotId = 30l;
@@ -73,8 +102,8 @@ public class AttachNATAdapterToMachineTest {
 
       replay(machine, networkAdapter, natEngine);
 
-      NatAdapter natAdapter = NatAdapter.builder().tcpRedirectRule("127.0.0.1", 2222, "", 22).build();
+      NatAdapter natAdapter = NatAdapter.builder().tcpRedirectRule("127.0.0.1", 2222, "guest", 22).build();
-      new AttachNATAdapterToMachine(slotId, natAdapter).apply(machine);
+      new AttachNATAdapterToMachineIfNotAlreadyExists(slotId, natAdapter).apply(machine);
 
       verify(machine, networkAdapter, natEngine);
    }

sandbox-apis/virtualbox/src/test/java/org/jclouds/virtualbox/functions/CloneAndRegisterMachineFromIsoIfNotAlreadyExistsLiveTest.java

@@ -20,7 +20,9 @@
 package org.jclouds.virtualbox.functions;
 
 import static org.jclouds.virtualbox.experiment.TestUtils.computeServiceForLocalhostAndGuest;
+import static org.jclouds.virtualbox.util.MachineUtils.lockMachineAndApplyOrReturnNullIfNotRegistered;
 import static org.testng.Assert.assertEquals;
+import static org.virtualbox_4_1.LockType.Write;
 
 import org.jclouds.compute.ComputeServiceContext;
 import org.jclouds.domain.Credentials;
@@ -36,6 +38,8 @@ import org.virtualbox_4_1.ISession;
 import org.virtualbox_4_1.StorageBus;
 import org.virtualbox_4_1.VirtualBoxManager;
 
+import com.google.common.collect.ImmutableSet;
+
 /**
  * @author Andrea Turli
  */
@@ -79,8 +83,8 @@ public class CloneAndRegisterMachineFromIsoIfNotAlreadyExistsLiveTest extends
       IMachine clone = new CloneAndRegisterMachineFromIMachineIfNotAlreadyExists(
             manager, workingDir, clonedVmSpec, IS_LINKED_CLONE).apply(master);
       assertEquals(clone.getName(), clonedVmSpec.getVmName());
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(clonedVmSpec);
+      for (VmSpec spec : ImmutableSet.of(clonedVmSpec, new IMachineToVmSpec().apply(master)))
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(new IMachineToVmSpec().apply(master));
+         lockMachineAndApplyOrReturnNullIfNotRegistered(manager, Write, spec.getVmName(), new UnregisterMachineIfExistsAndDeleteItsMedia(spec));
    }
 
    private IMachine getMasterNode(VirtualBoxManager manager,

sandbox-apis/virtualbox/src/test/java/org/jclouds/virtualbox/functions/CreateAndInstallVmLiveTest.java

@@ -24,7 +24,9 @@ import static com.google.common.collect.Iterables.any;
 import static com.google.common.collect.Iterables.transform;
 import static org.jclouds.virtualbox.domain.ExecutionType.HEADLESS;
 import static org.jclouds.virtualbox.experiment.TestUtils.computeServiceForLocalhostAndGuest;
+import static org.jclouds.virtualbox.util.MachineUtils.lockMachineAndApplyOrReturnNullIfNotRegistered;
 import static org.testng.Assert.assertTrue;
+import static org.virtualbox_4_1.LockType.Write;
 
 import java.util.Map;
 import java.util.Set;
@@ -94,8 +96,7 @@ public class CreateAndInstallVmLiveTest extends BaseVirtualBoxClientLiveTest {
               .forceOverwrite(true)
               .cleanUpMode(CleanupMode.Full)
               .natNetworkAdapter(0, NatAdapter.builder().tcpRedirectRule("127.0.0.1", 2222, "", 22).build()).build();
-      
+      lockMachineAndApplyOrReturnNullIfNotRegistered(manager, Write, vmName, new UnregisterMachineIfExistsAndDeleteItsMedia(vmSpecification));
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(vmSpecification);
    }
 
    public void testCreateImageMachineFromIso() throws Exception {

sandbox-apis/virtualbox/src/test/java/org/jclouds/virtualbox/functions/CreateAndRegisterMachineFromIsoIfNotAlreadyExistsLiveTest.java

@@ -19,8 +19,10 @@
 
 package org.jclouds.virtualbox.functions;
 
+import static org.jclouds.virtualbox.util.MachineUtils.lockMachineAndApplyOrReturnNullIfNotRegistered;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.fail;
+import static org.virtualbox_4_1.LockType.Write;
 
 import org.jclouds.virtualbox.BaseVirtualBoxClientLiveTest;
 import org.jclouds.virtualbox.domain.ErrorCode;
@@ -68,14 +70,14 @@ public class CreateAndRegisterMachineFromIsoIfNotAlreadyExistsLiveTest extends
       VmSpec launchSpecification = VmSpec.builder().id(vmName).name(vmName)
             .memoryMB(512).controller(ideController).cleanUpMode(mode)
             .osTypeId("Debian").forceOverwrite(true).build();
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager)
+      lockMachineAndApplyOrReturnNullIfNotRegistered(manager, Write, launchSpecification.getVmName(), new UnregisterMachineIfExistsAndDeleteItsMedia(launchSpecification));
-            .apply(launchSpecification);
+
       IMachine debianNode = new CreateAndRegisterMachineFromIsoIfNotAlreadyExists(
             manager, workingDir).apply(launchSpecification);
       IMachine machine = manager.getVBox().findMachine(vmName);
       assertEquals(debianNode.getName(), machine.getName());
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager)
+      lockMachineAndApplyOrReturnNullIfNotRegistered(manager, Write, launchSpecification.getVmName(), new UnregisterMachineIfExistsAndDeleteItsMedia(launchSpecification));
-            .apply(launchSpecification);
+
    }
 
    @Test
@@ -84,8 +86,7 @@ public class CreateAndRegisterMachineFromIsoIfNotAlreadyExistsLiveTest extends
       VmSpec launchSpecification = VmSpec.builder().id(vmName).name(vmName)
             .memoryMB(512).controller(ideController).cleanUpMode(mode)
             .osTypeId("SomeWeirdUnknownOs").forceOverwrite(true).build();
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager)
+      lockMachineAndApplyOrReturnNullIfNotRegistered(manager, Write, launchSpecification.getVmName(), new UnregisterMachineIfExistsAndDeleteItsMedia(launchSpecification));
-            .apply(launchSpecification);
 
       try {
          new CreateAndRegisterMachineFromIsoIfNotAlreadyExists(manager, workingDir)

sandbox-apis/virtualbox/src/test/java/org/jclouds/virtualbox/functions/admin/UnregisterMachineIfExistsAndDeleteItsMediaTest.java

@@ -81,7 +81,7 @@ public class UnregisterMachineIfExistsAndDeleteItsMediaTest {
 
       replay(manager, vBox, registeredMachine, progress);
 
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(vmSpecification);
+      new UnregisterMachineIfExistsAndDeleteItsMedia(vmSpecification).apply(registeredMachine);
    }
 
 }

sandbox-apis/virtualbox/src/test/java/org/jclouds/virtualbox/predicates/IsLinkedClonesLiveTest.java

@@ -19,8 +19,10 @@
 
 package org.jclouds.virtualbox.predicates;
 
+import static org.jclouds.virtualbox.util.MachineUtils.lockMachineAndApplyOrReturnNullIfNotRegistered;
 import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertTrue;
+import static org.virtualbox_4_1.LockType.Write;
 
 import org.jclouds.virtualbox.BaseVirtualBoxClientLiveTest;
 import org.jclouds.virtualbox.domain.HardDisk;
@@ -37,6 +39,8 @@ import org.virtualbox_4_1.IMachine;
 import org.virtualbox_4_1.StorageBus;
 import org.virtualbox_4_1.VirtualBoxManager;
 
+import com.google.common.collect.ImmutableSet;
+
 /**
  * 
  * @author Andrea Turli
@@ -75,28 +79,28 @@ public class IsLinkedClonesLiveTest extends BaseVirtualBoxClientLiveTest {
    public void testLinkedClone() {
 
       VirtualBoxManager manager = (VirtualBoxManager) context.getProviderSpecificContext().getApi();
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(masterSpec);
+      lockMachineAndApplyOrReturnNullIfNotRegistered(manager, Write, masterSpec.getVmName(), new UnregisterMachineIfExistsAndDeleteItsMedia(masterSpec));
 
       IMachine master = new CreateAndRegisterMachineFromIsoIfNotAlreadyExists(manager, workingDir).apply(masterSpec);
       IMachine clone = new CloneAndRegisterMachineFromIMachineIfNotAlreadyExists(manager, workingDir, cloneSpec,
                IS_LINKED_CLONE).apply(master);
 
       assertTrue(new IsLinkedClone(manager).apply(clone));
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(new IMachineToVmSpec().apply(clone));
+      for (VmSpec spec : ImmutableSet.of(new IMachineToVmSpec().apply(clone), masterSpec))
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(masterSpec);
+         lockMachineAndApplyOrReturnNullIfNotRegistered(manager, Write, spec.getVmName(), new UnregisterMachineIfExistsAndDeleteItsMedia(spec));
    }
 
    public void testFullClone() {
 
       VirtualBoxManager manager = (VirtualBoxManager) context.getProviderSpecificContext().getApi();
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(masterSpec);
+      lockMachineAndApplyOrReturnNullIfNotRegistered(manager, Write, masterSpec.getVmName(), new UnregisterMachineIfExistsAndDeleteItsMedia(masterSpec));
 
       IMachine master = new CreateAndRegisterMachineFromIsoIfNotAlreadyExists(manager, workingDir).apply(masterSpec);
       IMachine clone = new CloneAndRegisterMachineFromIMachineIfNotAlreadyExists(manager, workingDir, cloneSpec,
                !IS_LINKED_CLONE).apply(master);
 
       assertFalse(new IsLinkedClone(manager).apply(clone));
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(new IMachineToVmSpec().apply(clone));
+      for (VmSpec spec : ImmutableSet.of(new IMachineToVmSpec().apply(clone), masterSpec))
-      new UnregisterMachineIfExistsAndDeleteItsMedia(manager).apply(masterSpec);
+         lockMachineAndApplyOrReturnNullIfNotRegistered(manager, Write, spec.getVmName(), new UnregisterMachineIfExistsAndDeleteItsMedia(spec));
    }
 }