incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Fojtik <mfoj...@redhat.com>
Subject Re: [PATCH core] Initial run-on-instance support for EC2 and GoGrid
Date Mon, 17 Jan 2011 09:56:16 GMT
On 14/01/11 17:04 -0800, David Lutterkort wrote:
>Some comments:
>
>On Thu, 2011-01-13 at 14:05 +0100, mfojtik@redhat.com 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, mfojtik@redhat.com
Deltacloud API: http://deltacloud.org
--------------------------------------------------------

Mime
View raw message