Rails controller to manage EC2 instances

I did a small sample application for an Interview. Since I didn't got accepted I would like your opinion on how to improve it.

The aux methods are private. I have a usage_common to remove duplication in the usage and usage_instance methods add_item_hash is used because the format written in the database is different from the used in the API response

The start stop and status actions are used to simple operations in EC2 instances

Maybe I should've broken it in services? one for usage, one for processes and one for EC2.

Should send the params already in the format for database and thus eliminate the methods build_usage_hash and build_process_hash

https://github.com/mabranches/monitor_api/blob/master/app/controllers/instances_controller.rb

class InstancesController < ApplicationController
before_action :set_instance, only: [:show, :update, :destroy]

def usage
usage_common(:get, (Time.now - 24.hours).utc.iso8601, Time.now.utc.iso8601)
end

def usage_instance
usage_common(:get_instance, params[:id], (Time.now - 24.hours).utc.iso8601, Time.now.utc.iso8601)
end

def processes_instance
items = ProcessList.get_instance(params[:id])
process_items = {}
items.each do |item|
process_items[item['instance_id']] = item
end
render json: process_items
end

def create
usage = build_usage_hash params
process = build_process_hash params

Usage.create(usage)
ProcessList.create(process)

render json: [usage, process], status: :created
rescue => e
Rails.logger.error("Was not possible to update instance data: #{e.message}")
render json: { error: e.message }, status: :unprocessable_entity
end

def processes
items = ProcessList.get
process_items = {}
items.each do |item|
process_items[item['instance_id']] = item
end
render json: process_items
end

def stop
@params = params
ec2_action(:stop, 'stopped')
end

def start
@params = params
ec2_action(:start, 'started')
end

def status
@params = params
ec2_instance do |ec2|
@status = ec2.state.name
end
end

private

def usage_common(method, *args)
items = Usage.send(method, *args)

result = {}
items.each do |item|
end
render json: result
end

def ec2_instance
instance_id = @params[:id]

return render json: {}, status: :unprocessable_entity unless instance_id
ec2 = Aws::EC2::Resource.new(region: 'us-west-2').instance(instance_id)
@status = 'unknown'
begin
yield ec2
rescue => e
Rails.logger.error("Was not possible to execute action #{@params[:action]}: #{e.message}")
end
render json: { instance: instance_id, status: @status }
end

def ec2_action(method, status)
ec2_instance do |ec2|
ec2.send(method)
@status = status
end
end

result[item['instance_id']] ||= Hash.new { |h, k| h[k] = [] }
result[item['instance_id']]['mem'] << item['mem']
result[item['instance_id']]['disk'] << item['disk']
result[item['instance_id']]['cpu'] << item['cpu']
result[item['instance_id']]['usage_time'] << item['usage_time']
end

def build_usage_hash(params)
instance = params[:instance]
{
instance_id: params[:id],
cpu: instance[:cpu],
mem: instance[:mem],
disk: instance[:disk],
usage_time: Time.now.utc.iso8601
}
end

def build_process_hash(params)
instance = params[:instance]
{
instance_id: params[:id],
process: instance[:process].try(:values) || []
}
end
end

• Welcome to CR! You should have edited your previous post to improve your existing question instead of making a new one, the system doesn't like negative user records. Here, have an upvote, to steer yours into positiveness. – Mathieu Guindon Jun 13 '16 at 17:00

My gut reaction is that there's too much going on in this controller.

By just squinting at the code, I'd say that A) more code should be moved to the model(s), and B) you should be consistent in how your actions work.

For the latter point, some of your actions explicitly render a response, while others are more like aliases for other methods. The latter makes the controller confusing. At first glance I wasn't sure if, say, #usage is an action or a helper method, since I didn't see it explicitly respond to anything. More on the strange mix of methods later.

Also structurally, I don't understand your domain modelling. Namely, why don't you have an Instance model anywhere? That appears to be the primary resource your API handles - your controller is even called InstancesController - but an "instance" is not a first-class model in your system.

Instead you have indirections like Usage and ProcessList, which seems backwards; why not ask an Instance object for its processes and usage? And ask it to start and stop etc.? Behind the scenes it could use the same mix of database lookup and Aws methods, but it'd be bundled up in a simpler interface - again, skinny controller, fat model.

Looking at the code more closely, there's more worrying stuff:

• You have this:

def status
@params = params
ec2_instance do |ec2|
@status = ec2.state.name
end
end

# ...

def ec2_instance
instance_id = @params[:id]

return render json: {}, status: :unprocessable_entity unless instance_id
ec2 = Aws::EC2::Resource.new(region: 'us-west-2').instance(instance_id)
@status = 'unknown'
begin
yield ec2
rescue => e
Rails.logger.error("Was not possible to execute action #{@params[:action]}: #{e.message}")
end
render json: { instance: instance_id, status: @status }
end


This is extremely confusing: You have to set the @params instance variable (and hope it contains an id key/value) before you call #ec2_instance. Then you have to pass a block that must set the @status instance variable. And then #ec2_instance renders that status... what?

You've basically split one task into two tightly coupled, brittle methods. Why? This alone would probably get you rejected!

Also, from an API standpoint, if status is returned as unknown, I'd like to know why. That information just gets logged on the server, which doesn't help me, the client.

Again, it'd be much cleaner to have an Instance model that you just call #status on, and have it return the status or raise an exception, which you can then handle appropriately.

• This sort of thing

process_items = {}
items.each do |item|
process_items[item['instance_id']] = item
end


could more elegantly be written as:

process_items = items.each_with_object({}) do |item, hash|
hash[item['instance_id']] = item
end


or you could use inject/reduce, or map + to_h. Lots of ways to make this more functional instead of modifying a closed-over variable in a block. Of course, none of this should be done in the controller to being with: it'd make more sense the have ProcessList return the appropriate data structure, especially since you have to do this wrangling in two different actions. And for that you seem to have copy/pasted the code: Bad.

In fact, I'm not even sure it's doing the right thing in #processes_instance: It creates a hash with the instance_ids as keys, but if you're requesting a specific instance, wouldn't there only be one instance_id meaning the same key gets overridden again and again?

You also have #add_item_hash which is even worse. An each block that modifies a closed-over hash by forwarding it plus the iterated item to a completely different method? Ew. Why doesn't Usage just produce the proper data structure?

• You have this before_action, but don't actually have those actions in your controller - and the filter method doesn't exist either!

before_action :set_instance, only: [:show, :update, :destroy]


This is just sloppy.

Looking in your routes.rb, you have a bug too: Two routes matching get 'instances/usage' but pointing to two different actions. So your API seems to be broken. Haven't looked at your test suite, but it should have revealed this.

• Configuration? What if I'm not looking for something in the us-west-2 region? What if I want more than 24 hours of usage data? Both those things are just magic values right now.

All in all, the code has problems on both the macro and micro level. I'm afraid there are good reasons you didn't get accepted.

I see two things wrong with this controller.

1. It's too 'fat'. You're doing way too much in one controller.
2. It's not RESTful.

In this case, those two problems are much the same. Without reading too much into the code, it looks like you could break that controller up into 3 controllers, UsageController, InstanceController, Instance::PowerController. I'm not certain about the last one, you may find the Instance's power state not to be a RESTful resource.

As well though, I'd say the building of the hashes should be encapsulated by a model.