Allow different instance type per role #2672
Conversation
AppController/djinn.rb
Outdated
| @@ -1010,6 +1011,13 @@ def set_parameters(layout, options, secret) | |||
| @options['ec2_url'] = @options['EC2_URL'] | |||
| end | |||
|
|
|||
| @nodes.each { |node| | |||
| if node.jobs.include? 'compute' | |||
| @options['compute_instance_type'] = node.instance_type | |||
There was a problem hiding this comment.
Does that means that if we have multiple compute node with different instance type, we pick the last one as default?
There was a problem hiding this comment.
Yes, so from an azure perspective, it was important to have the right instance type used for compute nodes, so that it could add to the specific scale set which was created for those existing compute nodes. In case of multiple compute nodes with different instance types specified in the ASF, I thought it would be fine to pick up any one of the compute instance types to be used for autoscaling because there would be no good way of determining which instance type they would actually want to use for that purpose. Unless we think we need an autoscaling_instance_type kind of parameter in the ASF. What do you think @obino ?
There was a problem hiding this comment.
got it. Interesting the idea of a specific autoscaling instance kind: should we have one per role then (say one per compute, one per datastore etc..)? I like your first cut for now, so it's simple to understand it.
| @@ -184,6 +184,7 @@ def spawn_vms(num_vms, options, jobs, disks) | |||
| parameters['zone'] = options['zone'] | |||
| parameters['region'] = options['region'] | |||
| parameters['IS_VERBOSE'] = options['verbose'] | |||
| parameters['instance_type'] = options ['compute_instance_type'] | |||
There was a problem hiding this comment.
shouldn't this be associated with the jobs type? that is, if job == compute (or job.include?(compute) or whatever trickery ruby uses) then do the assignment of the type?
There was a problem hiding this comment.
So this spawn vms method is used only for autoscaling compute nodes right? So it just needs an instance type to be specified which it can use for autoscaling. At this point, I don't believe it would need another check to determine that.
But I might need a check to see if it already has a default instance type specified outside of the ips layout, in which case it might override it. Will test that and update here.
AppController/djinn.rb
Outdated
| msg = "Error: node layout is missing information #{node}." | ||
| Djinn.log_error(msg) | ||
| raise AppScaleException.new(msg) | ||
| elsif node['public_ip'].empty? || node['private_ip'].empty? || | ||
| node['jobs'].empty? || node['instance_id'].empty? | ||
| node['jobs'].empty? || node['instance_id'].empty? || | ||
| node['instance_type'].empty? |
There was a problem hiding this comment.
Is there any change in the AppScalefile for the new type?
There was a problem hiding this comment.
Yes, we would now specify the different instance types within the roles section of the ips layout.
|
@jeanleonov Yes you brought up a good point. I have made the changes, which should allow for compatibility with old tools and new. It would look for instance type under node layout for the new changes and if not it would default to the instance type passed in the options. I was able to test autoscaling using both current master tools and my tools branch and it seems to work. |
AppController/djinn.rb
Outdated
| @@ -1014,7 +1013,11 @@ def set_parameters(layout, options, secret) | |||
|
|
|||
| @nodes.each { |node| | |||
| if node.jobs.include? 'compute' | |||
| @options['compute_instance_type'] = node.instance_type | |||
| if not node.instance_type.nil? | |||
There was a problem hiding this comment.
maybe simplify
if node.instance_type.nil?
@options['compute_instance_type'] = @options['instance_type']
else
@options['compute_instance_type'] = node.instance_type
end
break
also what happens if someone's ASF is:
ips_layout:
-
roles: [master, database, compute]
nodes: 1
instance_type: m3.xlarge
-
roles: [compute]
nodes: 3
instance_type: m1.xlarge
There was a problem hiding this comment.
Sure I can simplify that piece of code. And for multiple compute nodes specified with different instance types, this PR now will pick up randomly the instance type for a compute node it encounters. It will be difficult to maintain a list of compute nodes and their instance types and make the decision of which instance type to use for autoscaling. I would vote for allowing an option for the user to specify a particular instance type to autoscale with if needed so that the decision is not random. But currently, I did not see any harm to assume the instance type to autoscale with as long as it is specified for any one of the compute nodes.
|
Latest build: https://ocd.appscale.com:8080/job/Daily%20Build/4745/ |
|
Latest build: https://ocd.appscale.com:8080/job/Daily%20Build/4763/ and Tools test rerun: https://ocd.appscale.com:8080/job/Tools-test/3115/ |
|
Latest all blue build: https://ocd.appscale.com:8080/job/Daily%20Build/4767/ |
Needs AppScale/appscale-tools#675