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.

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

expose the resource group for load balancer, and wrap config for availability set #541

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
26 changes: 26 additions & 0 deletions src/bosh_azure_cpi/lib/cloud/azure/models/config.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,32 @@
# frozen_string_literal: true

module Bosh::AzureCloud
class LoadBalancerConfig
attr_reader :name, :resource_group_name
def initialize(resource_group_name, name)
@resource_group_name = resource_group_name
@name = name
end

def to_s
"name: #{@name}, resource_group_name: #{@resource_group_name}"
end
end

class AvailabilitySetConfig
attr_reader :name
attr_reader :platform_update_domain_count, :platform_fault_domain_count
def initialize(name, platform_update_domain_count, platform_fault_domain_count)
@name = name
@platform_update_domain_count = platform_update_domain_count
@platform_fault_domain_count = platform_fault_domain_count
end

def to_s
"name: #{@name}, platform_update_domain_count: #{@platform_update_domain_count} platform_fault_domain_count: #{@platform_fault_domain_count}"
end
end

class AzureStackConfig
attr_reader :domain, :authentication, :resource, :endpoint_prefix
attr_writer :authentication
Expand Down
96 changes: 72 additions & 24 deletions src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,23 @@ class VMCloudProps
include Helpers
attr_reader :resource_group_name, :instance_type
attr_reader :storage_account_type, :storage_account_kind, :storage_account_max_disk_number, :storage_account_name
attr_reader :availability_zone, :availability_set
attr_reader :availability_zone
attr_reader :availability_set
attr_reader :tags
attr_reader :caching
attr_reader :root_disk, :ephemeral_disk
attr_reader :ip_forwarding, :accelerated_networking, :assign_dynamic_public_ip, :load_balancer, :application_gateway
attr_reader :ip_forwarding, :accelerated_networking, :assign_dynamic_public_ip, :application_gateway
attr_reader :load_balancer
attr_reader :application_security_groups, :security_group
attr_reader :platform_update_domain_count, :platform_fault_domain_count

attr_writer :availability_zone, :availability_set
attr_writer :availability_zone
attr_writer :availability_set
attr_writer :assign_dynamic_public_ip
# In AzureStack, availability sets can only be configured with 1 update domain.
# In Azure, the max update domain count of a managed/unmanaged availability set is 5.
def default_update_domain_count(global_azure_config)
global_azure_config.environment == ENVIRONMENT_AZURESTACK ? 1 : 5
end

# In AzureStack, availability sets can only be configured with 1 fault domain and 1 update domain.
# In Azure, the max fault domain count of an unmanaged availability set is 3;
# the max fault domain count of a managed availability set is 2 in some regions.
# When all regions support 3 fault domains, the default value should be changed to 3.
def default_fault_domain_count(global_azure_config)
if global_azure_config.environment == ENVIRONMENT_AZURESTACK
1
else
global_azure_config.use_managed_disks ? 2 : 3
end
end
AVAILABILITY_SET_KEY = 'availability_set'
LOAD_BALANCER_KEY = 'load_balancer'
RESOURCE_GROUP_NAME_KEY = 'resource_group_name'
NAME_KEY = 'name'

def initialize(vm_properties, global_azure_config)
@vm_properties = vm_properties.dup
Expand All @@ -40,7 +30,7 @@ def initialize(vm_properties, global_azure_config)
@resource_group_name = vm_properties.fetch('resource_group_name', global_azure_config.resource_group_name)
@storage_account_type = vm_properties['storage_account_type']
@availability_zone = vm_properties['availability_zone']
@availability_set = vm_properties['availability_set']
@availability_set = _parse_availability_set_config(vm_properties, global_azure_config)
@storage_account_name = vm_properties['storage_account_name']
@storage_account_max_disk_number = vm_properties.fetch('storage_account_max_disk_number', 30)
@storage_account_kind = vm_properties.fetch('storage_account_kind', STORAGE_ACCOUNT_KIND_GENERAL_PURPOSE_V1)
Expand All @@ -50,7 +40,9 @@ def initialize(vm_properties, global_azure_config)
@ip_forwarding = vm_properties['ip_forwarding']
@accelerated_networking = vm_properties['accelerated_networking']
@assign_dynamic_public_ip = vm_properties['assign_dynamic_public_ip']
@load_balancer = vm_properties['load_balancer']

@load_balancer = _parse_load_balancer_config(vm_properties, global_azure_config)

@application_gateway = vm_properties['application_gateway']

@application_security_groups = vm_properties['application_security_groups']
Expand All @@ -64,9 +56,65 @@ def initialize(vm_properties, global_azure_config)
ephemeral_disk_hash['size'],
ephemeral_disk_hash['type']
)
end

private

# In AzureStack, availability sets can only be configured with 1 update domain.
# In Azure, the max update domain count of a managed/unmanaged availability set is 5.
def _default_update_domain_count(global_azure_config)
global_azure_config.environment == ENVIRONMENT_AZURESTACK ? 1 : 5
end

# In AzureStack, availability sets can only be configured with 1 fault domain and 1 update domain.
# In Azure, the max fault domain count of an unmanaged availability set is 3;
# the max fault domain count of a managed availability set is 2 in some regions.
# When all regions support 3 fault domains, the default value should be changed to 3.
def _default_fault_domain_count(global_azure_config)
if global_azure_config.environment == ENVIRONMENT_AZURESTACK
1
else
global_azure_config.use_managed_disks ? 2 : 3
end
end

def _parse_load_balancer_config(vm_properties, global_azure_config)
if vm_properties[LOAD_BALANCER_KEY].is_a?(Hash)
resource_group_name = if vm_properties[LOAD_BALANCER_KEY][RESOURCE_GROUP_NAME_KEY].nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

make it simple?
resource_group_name = vm_properties[LOAD_BALANCER_KEY][RESOURCE_GROUP_NAME_KEY] || global_azure_config.resource_group_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion.

global_azure_config.resource_group_name
else
vm_properties[LOAD_BALANCER_KEY][RESOURCE_GROUP_NAME_KEY]
end
Bosh::AzureCloud::LoadBalancerConfig.new(
resource_group_name,
vm_properties[LOAD_BALANCER_KEY][NAME_KEY]
)
else
Bosh::AzureCloud::LoadBalancerConfig.new(
global_azure_config.resource_group_name,
vm_properties[LOAD_BALANCER_KEY]
)
end
end

@platform_update_domain_count = vm_properties['platform_update_domain_count'] || default_update_domain_count(global_azure_config)
@platform_fault_domain_count = vm_properties['platform_fault_domain_count'] || default_fault_domain_count(global_azure_config)
def _parse_availability_set_config(vm_properties, global_azure_config)
if vm_properties[AVAILABILITY_SET_KEY].is_a?(Hash)
platform_update_domain_count = vm_properties[AVAILABILITY_SET_KEY]['platform_update_domain_count'] || _default_update_domain_count(global_azure_config)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about defining two varialbes?

default_update_domain_count = _default_update_domain_count(global_azure_config)
default_fault_domain_count = _default_fault_domain_count(global_azure_config)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are the same. and if define two variable will increate two line of code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to avoid call them twice. Anyway, both ways are fine for me.

platform_fault_domain_count = vm_properties[AVAILABILITY_SET_KEY]['platform_fault_domain_count'] || _default_fault_domain_count(global_azure_config)
Bosh::AzureCloud::AvailabilitySetConfig.new(
vm_properties[AVAILABILITY_SET_KEY][NAME_KEY],
platform_update_domain_count,
platform_fault_domain_count
)
else
platform_update_domain_count = vm_properties['platform_update_domain_count'] || _default_update_domain_count(global_azure_config)
platform_fault_domain_count = vm_properties['platform_fault_domain_count'] || _default_fault_domain_count(global_azure_config)
Bosh::AzureCloud::AvailabilitySetConfig.new(
vm_properties[AVAILABILITY_SET_KEY],
platform_update_domain_count,
platform_fault_domain_count
)
end
end
end
end
17 changes: 9 additions & 8 deletions src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def rest_api_url(resource_provider, resource_type, resource_group_name: nil, nam
url
end

def parse_name_from_id(id)
def _parse_name_from_id(id)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of renaming it but not making it private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make it private now.

ret = id.match('/subscriptions/([^/]*)/resourceGroups/([^/]*)/providers/([^/]*)/([^/]*)/([^/]*)(.*)')
raise AzureError, "\"#{id}\" is not a valid URL." if ret.nil?

Expand Down Expand Up @@ -1263,9 +1263,10 @@ def delete_public_ip(resource_group_name, name)
#
# @See https://docs.microsoft.com/en-us/rest/api/loadbalancer/get-information-about-a-load-balancer
#
def get_load_balancer_by_name(name)
url = rest_api_url(REST_API_PROVIDER_NETWORK, REST_API_LOAD_BALANCERS, name: name)
get_load_balancer(url)

def get_load_balancer_by_name(resource_group_name, name)
url = rest_api_url(REST_API_PROVIDER_NETWORK, REST_API_LOAD_BALANCERS, resource_group_name: resource_group_name, name: name)
_get_load_balancer(url)
end

# Get a load balancer's information
Expand All @@ -1275,7 +1276,7 @@ def get_load_balancer_by_name(name)
#
# @See https://docs.microsoft.com/en-us/rest/api/loadbalancer/get-information-about-a-load-balancer
#
def get_load_balancer(url)
def _get_load_balancer(url)
load_balancer = nil
result = get_resource_by_id(url)
unless result.nil?
Expand Down Expand Up @@ -1959,15 +1960,15 @@ def parse_network_interface(result, recursive: true)
end
unless ip_configuration_properties['loadBalancerBackendAddressPools'].nil?
if recursive
names = parse_name_from_id(ip_configuration_properties['loadBalancerBackendAddressPools'][0]['id'])
interface[:load_balancer] = get_load_balancer_by_name(names[:resource_name])
names = _parse_name_from_id(ip_configuration_properties['loadBalancerBackendAddressPools'][0]['id'])
interface[:load_balancer] = get_load_balancer_by_name(names[:resource_group_name], names[:resource_name])
else
interface[:load_balancer] = { id: ip_configuration_properties['loadBalancerBackendAddressPools'][0]['id'] }
end
end
unless ip_configuration_properties['applicationGatewayBackendAddressPools'].nil?
if recursive
names = parse_name_from_id(ip_configuration_properties['applicationGatewayBackendAddressPools'][0]['id'])
names = _parse_name_from_id(ip_configuration_properties['applicationGatewayBackendAddressPools'][0]['id'])
interface[:application_gateway] = get_application_gateway_by_name(names[:resource_name])
else
interface[:application_gateway] = { id: ip_configuration_properties['applicationGatewayBackendAddressPools'][0]['id'] }
Expand Down
2 changes: 1 addition & 1 deletion src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def create(bosh_vm_meta, location, vm_props, network_configurator, env)
vm_name = instance_id.vm_name

# When both availability_zone and availability_set are specified, raise an error
cloud_error("Only one of 'availability_zone' and 'availability_set' is allowed to be configured for the VM but you have configured both.") if !vm_props.availability_zone.nil? && !vm_props.availability_set.nil?
cloud_error("Only one of 'availability_zone' and 'availability_set' is allowed to be configured for the VM but you have configured both.") if !vm_props.availability_zone.nil? && !vm_props.availability_set.name.nil?
zone = vm_props.availability_zone
unless zone.nil?
cloud_error("'#{zone}' is not a valid zone. Available zones are: #{AVAILABILITY_ZONES}") unless AVAILABILITY_ZONES.include?(zone.to_s)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class VMManager
private

def _get_availability_set_name(vm_props, env)
availability_set_name = vm_props.availability_set
availability_set_name = vm_props.availability_set.name
if availability_set_name.nil?
unless env.nil? || env['bosh'].nil? || env['bosh']['group'].nil?
availability_set_name = env['bosh']['group']
Expand All @@ -32,8 +32,8 @@ def _get_or_create_availability_set(resource_group_name, availability_set_name,
name: availability_set_name,
location: location,
tags: AZURE_TAGS,
platform_update_domain_count: vm_props.platform_update_domain_count || default_update_domain_count,
platform_fault_domain_count: vm_props.platform_fault_domain_count || default_fault_domain_count,
platform_update_domain_count: vm_props.availability_set.platform_update_domain_count,
platform_fault_domain_count: vm_props.availability_set.platform_fault_domain_count,
managed: @use_managed_disks
}
availability_set = nil
Expand Down
6 changes: 3 additions & 3 deletions src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ def _get_public_ip(vip_network)

def _get_load_balancer(vm_props)
load_balancer = nil
unless vm_props.load_balancer.nil?
load_balancer_name = vm_props.load_balancer
load_balancer = @azure_client.get_load_balancer_by_name(load_balancer_name)
unless vm_props.load_balancer.name.nil?
load_balancer_name = vm_props.load_balancer.name
load_balancer = @azure_client.get_load_balancer_by_name(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name)
cloud_error("Cannot find the load balancer '#{load_balancer_name}'") if load_balancer.nil?
end
load_balancer
Expand Down
10 changes: 5 additions & 5 deletions src/bosh_azure_cpi/spec/unit/azure_client/azure_client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,12 @@
end
end

describe '#parse_name_from_id' do
describe '#_parse_name_from_id' do
context 'when id is empty' do
it 'should raise an error' do
id = ''
expect do
azure_client.parse_name_from_id(id)
azure_client._parse_name_from_id(id)
end.to raise_error /\"#{id}\" is not a valid URL./
end
end
Expand All @@ -107,7 +107,7 @@
id = '/subscriptions/a/resourceGroups/b/providers/c/d'
it 'should raise an error' do
expect do
azure_client.parse_name_from_id(id)
azure_client._parse_name_from_id(id)
end.to raise_error /\"#{id}\" is not a valid URL./
end
end
Expand All @@ -121,7 +121,7 @@
result[:provider_name] = 'c'
result[:resource_type] = 'd'
result[:resource_name] = 'e'
expect(azure_client.parse_name_from_id(id)).to eq(result)
expect(azure_client._parse_name_from_id(id)).to eq(result)
end
end

Expand All @@ -134,7 +134,7 @@
result[:provider_name] = 'c'
result[:resource_type] = 'd'
result[:resource_name] = 'e'
expect(azure_client.parse_name_from_id(id)).to eq(result)
expect(azure_client._parse_name_from_id(id)).to eq(result)
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@
headers: {}
)
expect(
azure_client.get_load_balancer_by_name(load_balancer_name)
azure_client.get_load_balancer_by_name(default_resource_group_name, load_balancer_name)
).to be_nil
end

Expand All @@ -529,7 +529,7 @@
headers: {}
)
expect(
azure_client.get_load_balancer_by_name(load_balancer_name)
azure_client.get_load_balancer_by_name(default_resource_group_name, load_balancer_name)
).to eq(fake_load_balancer)
end
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

require 'spec_helper'

describe Bosh::AzureCloud::AvailabilitySetConfig do
describe '#to_s' do
context 'when called' do
let(:name) { 'fake_av_set' }
let(:fault_domain) { 5 }
let(:update_domain) { 1 }
let(:availability_set_config) { Bosh::AzureCloud::AvailabilitySetConfig.new(name, update_domain, fault_domain) }
let(:av_set_string) { "name: #{name}, platform_update_domain_count: #{update_domain} platform_fault_domain_count: #{fault_domain}" }

it 'should return the correct string' do
expect(availability_set_config.to_s).to eq(av_set_string)
end
end
end
end
18 changes: 18 additions & 0 deletions src/bosh_azure_cpi/spec/unit/models/load_balancer_config_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# frozen_string_literal: true

require 'spec_helper'

describe Bosh::AzureCloud::LoadBalancerConfig do
describe '#to_s' do
context 'when called' do
let(:name) { 'fake_name' }
let(:resource_group_name) { 'fake_rg' }
let(:lb_string) { "name: #{name}, resource_group_name: #{resource_group_name}" }
let(:load_balancer_config) { Bosh::AzureCloud::LoadBalancerConfig.new(resource_group_name, name) }

it 'should return the correct string' do
expect(load_balancer_config.to_s).to eq(lb_string)
end
end
end
end
Loading