Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow different instance type per role #2672

Merged
merged 17 commits into from
Mar 1, 2018
Merged

Allow different instance type per role #2672

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d3a1303
Use the compute instance type to autoscale instances
tmarballi Feb 7, 2018
8e517bc
Pass in the instance type for autoscaling compute nodes
tmarballi Feb 7, 2018
22bebce
Merge branch 'master' into allow-type-per-role
tmarballi Feb 8, 2018
6cedc8d
Merge branch 'master' into allow-type-per-role
tmarballi Feb 12, 2018
3fc1270
Fix broken unit tests for compute instance type
tmarballi Feb 12, 2018
273a4eb
Merge branch 'master' into allow-type-per-role
tmarballi Feb 15, 2018
0c5738e
Merge branch 'master' into allow-type-per-role
tmarballi Feb 16, 2018
ab443b4
Pass in instance type in the autoscaled node information.
tmarballi Feb 17, 2018
01571ce
Fix some broken unit tests
tmarballi Feb 20, 2018
a217689
Merge branch 'master' into allow-type-per-role
tmarballi Feb 20, 2018
9a3d3e6
Fix broken unit tests expected results
tmarballi Feb 20, 2018
96dba43
Allow compatibility with older AppScale tools which do not pass insta…
tmarballi Feb 21, 2018
d4b1f67
Remove debugging log line.
tmarballi Feb 21, 2018
74f4a75
Address code review comments
tmarballi Feb 22, 2018
686b120
Merge branch 'master' into allow-type-per-role
tmarballi Feb 23, 2018
c87aacf
Merge branch 'master' into allow-type-per-role
tmarballi Feb 26, 2018
9284db5
Merge branch 'master' into allow-type-per-role
tmarballi Feb 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions AppController/djinn.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,12 +754,13 @@ def check_layout(layout, keyname)
raise AppScaleException.new(msg)
end
if !node['public_ip'] || !node['private_ip'] || !node['jobs'] ||
!node['instance_id']
!node['instance_id'] || !node['instance_type']
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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any change in the AppScalefile for the new type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we would now specify the different instance types within the roles section of the ips layout.

msg = "Error: node layout is missing information #{node}."
Djinn.log_error(msg)
raise AppScaleException.new(msg)
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that means that if we have multiple compute node with different instance type, we pick the last one as default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

break
end
}

'OK'
end

Expand Down
6 changes: 4 additions & 2 deletions AppController/lib/djinn_job_data.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# costs, which may charge on an hourly basis).
class DjinnJobData
attr_accessor :public_ip, :private_ip, :jobs, :instance_id, :cloud, :ssh_key
attr_accessor :disk
attr_accessor :disk, :instance_type

def initialize(json_data, keyname)
if json_data.class != Hash
Expand All @@ -39,6 +39,7 @@ def initialize(json_data, keyname)
@instance_id = 'i-APPSCALE'
@instance_id = json_data['instance_id'] if json_data['instance_id']
@disk = json_data['disk']
@instance_type = json_data['instance_type']
@ssh_key = File.expand_path("/etc/appscale/keys/#{@cloud}/#{keyname}.key")
end

Expand Down Expand Up @@ -68,7 +69,8 @@ def to_hash
'instance_id' => @instance_id,
'cloud' => @cloud,
'ssh_key' => @ssh_key,
'disk' => @disk
'disk' => @disk,
'instance_type' => @instance_type
}
end

Expand Down
1 change: 1 addition & 0 deletions AppController/lib/infrastructure_manager_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


run_result = run_instances(parameters)
Djinn.log_debug("[IM] Run instances info says [#{run_result}]")
Expand Down
12 changes: 8 additions & 4 deletions AppController/test/tc_djinn.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ def test_set_params_w_bad_params
'private_ip' => 'private_ip',
'jobs' => ['compute', 'shadow', 'taskqueue_master', 'db_master',
'load_balancer', 'login', 'zookeeper', 'memcache'],
'instance_id' => 'instance_id'
'instance_id' => 'instance_id',
'instance_type' => 'instance_type'
}])

djinn = Djinn.new
Expand Down Expand Up @@ -198,7 +199,8 @@ def test_set_params_w_good_params
'private_ip' => '1.2.3.4',
'jobs' => ['compute', 'shadow', 'taskqueue_master', 'db_master',
'load_balancer', 'login', 'zookeeper', 'memcache'],
'instance_id' => 'instance_id'
'instance_id' => 'instance_id',
'instance_type' => 'instance_type'
}])

flexmock(HelperFunctions).should_receive(:shell).with("ifconfig").
Expand Down Expand Up @@ -621,7 +623,8 @@ def test_get_property
'private_ip' => '1.2.3.4',
'jobs' => ['compute', 'shadow', 'taskqueue_master', 'db_master',
'load_balancer', 'login', 'zookeeper', 'memcache'],
'instance_id' => 'instance_id'
'instance_id' => 'instance_id',
'instance_type' => 'instance_type'
}])
flexmock(Djinn).should_receive(:log_run).with(
"mkdir -p /opt/appscale/apps")
Expand Down Expand Up @@ -659,7 +662,8 @@ def test_set_property
'private_ip' => '1.2.3.4',
'jobs' => ['compute', 'shadow', 'taskqueue_master', 'db_master',
'load_balancer', 'login', 'zookeeper', 'memcache'],
'instance_id' => 'instance_id'
'instance_id' => 'instance_id',
'instance_type' => 'instance_type'
}])
flexmock(Djinn).should_receive(:log_run).with(
"mkdir -p /opt/appscale/apps")
Expand Down
12 changes: 8 additions & 4 deletions AppController/test/tc_infrastructure_manager_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def test_spawn_one_vm_in_ec2
'group' => 'boogroup',
'image_id' => 'booid',
'infrastructure' => 'booinfrastructure',
'instance_type' => 'booinstancetype',
'instance_type' => 'boocomputeinstancetype',
'keyname' => 'bookeyname',
'num_vms' => '1',
'cloud' => 'cloud1',
Expand Down Expand Up @@ -90,6 +90,7 @@ def test_spawn_one_vm_in_ec2
'machine' => 'booid',
'infrastructure' => 'booinfrastructure',
'instance_type' => 'booinstancetype',
'compute_instance_type' => 'boocomputeinstancetype',
'keyname' => 'bookeyname',
'ec2_access_key' => 'booaccess',
'ec2_secret_key' => 'boosecret',
Expand Down Expand Up @@ -126,7 +127,7 @@ def test_spawn_three_vms_in_ec2
'group' => 'boogroup',
'image_id' => 'booid',
'infrastructure' => 'booinfrastructure',
'instance_type' => 'booinstancetype',
'instance_type' => 'boocomputeinstancetype',
'keyname' => 'bookeyname',
'num_vms' => '3',
'cloud' => 'cloud1',
Expand Down Expand Up @@ -183,6 +184,7 @@ def test_spawn_three_vms_in_ec2
'machine' => 'booid',
'infrastructure' => 'booinfrastructure',
'instance_type' => 'booinstancetype',
'compute_instance_type' => 'boocomputeinstancetype',
'keyname' => 'bookeyname',
'ec2_access_key' => 'booaccess',
'ec2_secret_key' => 'boosecret',
Expand Down Expand Up @@ -233,7 +235,7 @@ def test_spawn_three_vms_in_gce
'group' => 'boogroup',
'image_id' => 'booid',
'infrastructure' => 'booinfrastructure',
'instance_type' => 'booinstancetype',
'instance_type' => 'boocomputeinstancetype',
'keyname' => 'bookeyname',
'num_vms' => '3',
'cloud' => 'cloud1',
Expand Down Expand Up @@ -290,6 +292,7 @@ def test_spawn_three_vms_in_gce
'machine' => 'booid',
'infrastructure' => 'booinfrastructure',
'instance_type' => 'booinstancetype',
'compute_instance_type' => 'boocomputeinstancetype',
'keyname' => 'bookeyname',
'ec2_access_key' => nil,
'ec2_secret_key' => nil,
Expand Down Expand Up @@ -337,7 +340,7 @@ def test_spawn_one_vm_in_azure
'group' => nil,
'image_id' => 'booid',
'infrastructure' => 'booinfrastructure',
'instance_type' => 'booinstancetype',
'instance_type' => 'boocomputeinstancetype',
'keyname' => 'bookeyname',
'num_vms' => '1',
'cloud' => 'cloud1',
Expand Down Expand Up @@ -393,6 +396,7 @@ def test_spawn_one_vm_in_azure
'infrastructure' => 'booinfrastructure',
'machine' => 'booid',
'instance_type' => 'booinstancetype',
'compute_instance_type' => 'boocomputeinstancetype',
'keyname' => 'bookeyname',
'zone' => 'my-zone-1b',
'azure_subscription_id' => 'boosubscriptionid',
Expand Down