On 14/01/11 17:04 -0800, David Lutterkort wrote:
Some comments:

On Thu, 2011-01-13 at 14:05 +0100, [email protected] wrote:
diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb 
b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
index 7a4b394..5533c73 100644
--- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
+++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
@@ -200,6 +200,20 @@ module Deltacloud
             new_instance
           end
         end
+
+        def run_on_instance(credentials, opts={})
+          target = instance(credentials, :id => opts[:id])

How about allowing passing the IP to connect to in explicitly ? That
way, users can avoid this lookup, in case they know the IP already.

Yes, that's right but users will execute commands on specific instance, the
URI for executing commands is: /api/instances/1234/run

Another thing is that the user must know key name in order to execute something
(like in Amazon) (except case that user have just one key for all instances).

I though that we want to fully automate this process (getting an IP from
instance) and ask user just for additional credentials (like private key).

An option could be to add a param for an IP, so user can change it in
request.

diff --git a/server/lib/deltacloud/runner.rb b/server/lib/deltacloud/runner.rb
new file mode 100644
index 0000000..52eed40
--- /dev/null
+++ b/server/lib/deltacloud/runner.rb
@@ -0,0 +1,155 @@
+# Copyright (C) 2009, 2010  Red Hat, Inc.
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF 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.
+
+require 'net/ssh'
+require 'socket'
+require 'tempfile'
+
+module Deltacloud
+
+  module Runner
+
+    class RunnerError < StandardError
+      attr_reader :message
+      def initialize(message)
+        @message = message
+        super
+      end
+    end
+
+    class InstancePortError < RunnerError; end
+    class InstanceNetworkError < RunnerError; end
+    class InstanceSSHError < RunnerError; end
+
+    def self.execute(command, opts={})

Shouldn't there be some sanity checking of opts, e.g. that either a
password or private key is present ?

Yes, I can add a check if key or password is present in opts.
+      # First check networking and firewalling
+      network = Network::new(opts[:ip], opts[:port])
+      raise InstanceNetworkError.new unless network.ready?
+      raise InstancePortError.new unless network.port_open?

I don't like this whole network checking business - you are opening two
TCP connections, just to check if establishing the SSH connection
afterwards might work. For one, it's very wasteful timewise; in
addition, there's no guarantee that the success of the two operations
above has any connection (pun intended) to whether the SSH conn will
succeed.

You really need to handle timeouts, conn refused etc. when you establish
the ssh connection.

You're right, I checked Net:SSH documentation and there are some Exceptions
which we can use for error reporting.

Also I can wrap Net:SSH with timeout block and then catch exception here.

diff --git a/server/server.rb b/server/server.rb
index 8c3b72c..c26db48 100644
--- a/server/server.rb
+++ b/server/server.rb
@@ -356,6 +363,22 @@ END
     param :id,           :string, :required
     control { instance_action(:destroy) }
   end
+
+  operation :run, :method => :post, :member => true do
+    description "Run command on instance"
+    with_capability :run_on_instance
+    param :id,          :string,  :required
+    param :cmd,         :string,  :required
+    param :private_key, :string,  :optional
+    param :password,    :string,  :optional

The params need documentation, especially the fact that either
private_key or password need to be given. Also, how will clients know
which one to pass ?

Yes, I'll add a description param here.

Client can query instance authentication method to know what sort of
credentials are needed for successful authentication.
I'll mention that in description of this operation.

-- Michal

--
--------------------------------------------------------
Michal Fojtik, [email protected]
Deltacloud API: http://deltacloud.org
--------------------------------------------------------

Reply via email to