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. > 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 ? > + # 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. > 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 ? David
