Merge pull request #90 from opscode/adamed-oc-9783

OC-9783: Knife-azure server create leaks cloud services on failure to create vm

lib/azure/deploy.rb

@@ -89,8 +89,7 @@ class Azure
         ret_val = deploy.create(params, deployXML)
       end
       if ret_val.css('Error Code').length > 0
-          Chef::Log.fatal 'Unable to create role:' + ret_val.at_css('Error Code').content + ' : ' + ret_val.at_css('Error Message').content
-          exit 1
+        raise Chef::Log.fatal 'Unable to create role:' + ret_val.at_css('Error Code').content + ' : ' + ret_val.at_css('Error Message').content
       end
       @connection.roles.find_in_hosted_service(params[:azure_vm_name], params[:azure_dns_name])
     end

lib/azure/storageaccount.rb

@@ -70,6 +70,13 @@ class Azure
         @connection.query_azure('storageaccounts/' + storage.name, 'delete')
       end
     end
+
+    def delete(name)
+      if self.exists?(name)
+          servicecall = "storageservices/" + name
+        @connection.query_azure(servicecall, "delete") 
+      end
+    end
   end
 end
 

lib/chef/knife/azure_server_create.rb

@@ -257,19 +257,31 @@ class Chef
           config[:azure_vm_name] = locate_config_value(:azure_dns_name)
         end
 
+        remove_hosted_service_on_failure = locate_config_value(:azure_dns_name)
+        if connection.hosts.exists?(locate_config_value(:azure_dns_name))
+          remove_hosted_service_on_failure = nil
+        end
+        remove_storage_service_on_failure = locate_config_value(:azure_storage_account)
+
         #If Storage Account is not specified, check if the geographic location has one to re-use
         if not locate_config_value(:azure_storage_account)
           storage_accts = connection.storageaccounts.all
           storage = storage_accts.find { |storage_acct| storage_acct.location.to_s == locate_config_value(:azure_service_location) }
           if not storage
             config[:azure_storage_account] = [strip_non_ascii(locate_config_value(:azure_vm_name)), random_string].join.downcase
+            remove_storage_service_on_failure = config[:azure_storage_account]
           else
+            remove_storage_service_on_failure = nil
             config[:azure_storage_account] = storage.name.to_s
           end
         end
 
-        server = connection.deploys.create(create_server_def)
-        fqdn = server.publicipaddress
+        begin
+          server = connection.deploys.create(create_server_def)
+          fqdn = server.publicipaddress
+        rescue Exception => e
+          cleanup_and_exit(remove_hosted_service_on_failure, remove_storage_service_on_failure)
+        end
 
         puts("\n")
         if is_image_windows?
@@ -484,6 +496,17 @@ class Chef
         end
         server_def
       end
+
+      def cleanup_and_exit(remove_hosted_service_on_failure, remove_storage_service_on_failure)
+        if remove_hosted_service_on_failure
+          connection.hosts.delete(remove_hosted_service_on_failure)
+        end
+        if remove_storage_service_on_failure
+          connection.storageaccounts.delete(remove_storage_service_on_failure)
+        end
+        exit 1
+      end
+      
     end
   end
 end

spec/unit/azure_server_create_spec.rb

@@ -145,6 +145,24 @@ describe "parameter test:" do
 		end
 	end
 
+	context "#cleanup_and_exit" do
+		it "service leak cleanup" do
+			expect {@server_instance.cleanup_and_exit("hosted_srvc", "storage_srvc")}.to raise_error
+		end
+
+		it "service leak cleanup with nil params" do
+			@server_instance.connection.hosts.should_not_receive(:delete)
+			@server_instance.connection.storageaccounts.should_not_receive(:delete)
+			expect {@server_instance.cleanup_and_exit(nil, nil)}.to raise_error
+		end
+
+		it "service leak cleanup with valid params" do
+			@server_instance.connection.hosts.should_receive(:delete).with("hosted_srvc")
+			@server_instance.connection.storageaccounts.should_receive(:delete).with("storage_srvc")
+			expect {@server_instance.cleanup_and_exit("hosted_srvc", "storage_srvc")}.to raise_error
+		end
+	end
+
 	context "connect to existing DNS tests" do
 		before do
 			Chef::Config[:knife][:azure_connect_to_existing_dns] = true