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

andyliuliming · 2018-09-26T12:24:05Z

No description provided.

…lability set.

cfdreddbot · 2018-09-26T12:24:06Z

Hey andyliuliming!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

bingosummer · 2018-09-26T15:43:29Z

src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb

-      @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)


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)

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

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

bingosummer · 2018-09-26T15:59:35Z

src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb

+          expect(vm_cloud_props.load_balancer.resource_group_name).to eq(resource_group_name)
+        end
+      end
+      context 'when resource group empty' do


Add one blank line

bingosummer · 2018-09-26T16:01:10Z

src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb

+describe Bosh::AzureCloud::VMCloudProps do
+  include_context 'shared stuff'
+  describe '#initialize' do
+    context 'when availability_set is a hash' do


Add one case for "when availability_set is a string".
Same for load_balancer

it's covered in other case.
because we track the 100% coverage.
the

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

must be already go through now.

bingosummer · 2018-09-26T16:05:19Z

src/bosh_azure_cpi/spec/unit/vm_manager/create/vm_is_not_created_spec.rb

@@ -45,7 +45,11 @@
          end

          before do
-            vm_props.availability_set = availability_set_name
+            vm_props.availability_set = Bosh::AzureCloud::AvailabilitySetConfig.new(


It seems that attr_writer for availability_set is only for tests. Can we use other ways here so that we can remove attr_writer?

no very very good way to do it. any suggestions?

Okay. Mark it pending.

gossion · 2018-09-27T06:13:26Z

src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb

@@ -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)


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

make it private now.

gossion · 2018-09-27T06:28:34Z

src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb

+
+    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?


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

good suggestion.

bingosummer · 2018-09-27T06:57:29Z

src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb

      # network.resource_group_name may return the default resource group name in global configurations. See network.rb.
      default_resource_group_name = @azure_config.resource_group_name
      if network_security_group.nil? && resource_group_name != default_resource_group_name
-        @logger.info("Cannot find the network security group '#{network_security_group_name}' in the resource group '#{resource_group_name}', trying to search it in the default resource group '#{default_resource_group_name}'")
-        network_security_group = @azure_client.get_network_security_group_by_name(default_resource_group_name, network_security_group_name)
+        @logger.info("Cannot find the network security group '#{network_security_group_cfg.name}' in the resource group '#{network_security_group_cfg.resource_group_name}', trying to search it in the default resource group '#{default_resource_group_name}'")


network_security_group_cfg.resource_group_name should be changed to resource_group_name

good catch.

…s related to the following Azure CPI PRs/issues: - cloudfoundry/bosh-azure-cpi-release#651 (Fixes ) - cloudfoundry/bosh-azure-cpi-release#541 (Fixed cloudfoundry/bosh-azure-cpi-release#328) - cloudfoundry/bosh-azure-cpi-release#638 (Fixed cloudfoundry/bosh-azure-cpi-release#111) Note: This commit assumes that PR cloudfoundry#651 will be included in a new `v37.7.0` release of the Azure CPI (in order to properly document the CPI version requirements of new features). If PR cloudfoundry#651 is included in a different CPI version, then this PR will need to be amended appropriately.

…s related to the following Azure CPI PRs/issues: - cloudfoundry/bosh-azure-cpi-release#651 (Fixes cloudfoundry/bosh-azure-cpi-release#644) - cloudfoundry/bosh-azure-cpi-release#541 (Fixed cloudfoundry/bosh-azure-cpi-release#328) - cloudfoundry/bosh-azure-cpi-release#638 (Fixed cloudfoundry/bosh-azure-cpi-release#111) Note: This commit assumes that PR cloudfoundry#651 will be included in a new `v37.7.0` release of the Azure CPI (in order to properly document the CPI version requirements of new features). If PR cloudfoundry#651 is included in a different CPI version, then this PR will need to be amended appropriately.

…ample of" sections for alternate Load Balancer configurations, for new and existing features related to the following Azure CPI PRs/issues: - cloudfoundry/bosh-azure-cpi-release#651 (Fixes cloudfoundry/bosh-azure-cpi-release#644) - cloudfoundry/bosh-azure-cpi-release#541 (Fixed cloudfoundry/bosh-azure-cpi-release#328) - cloudfoundry/bosh-azure-cpi-release#638 (Fixed cloudfoundry/bosh-azure-cpi-release#111) Note: This commit assumes that PR cloudfoundry#651 will be included in a new `v37.7.0` release of the Azure CPI (in order to properly document the CPI version requirements of new features). If PR cloudfoundry#651 is included in a different CPI version, then this PR will need to be amended appropriately.

…y configurations, for new and existing features related to the following Azure CPI PRs/issues: - cloudfoundry/bosh-azure-cpi-release#651 (Fixes cloudfoundry/bosh-azure-cpi-release#644) - cloudfoundry/bosh-azure-cpi-release#541 (Fixed cloudfoundry/bosh-azure-cpi-release#328) - cloudfoundry/bosh-azure-cpi-release#638 (Fixed cloudfoundry/bosh-azure-cpi-release#111) Note: This commit assumes that PR cloudfoundry#651 will be included in a new `v37.7.0` release of the Azure CPI (in order to properly document the CPI version requirements of new features). If PR cloudfoundry#651 is included in a different CPI version, then this PR will need to be amended appropriately.

…foundry#111 and cloudfoundry#328, in preparation for cloudfoundry#644) Note: There was already a high degree of symmetry (of both functionality and implementation) between the CPI's support for LBs and AGWs. However, previous CPI enhancements (for example: cloudfoundry#541 for cloudfoundry#111 and cloudfoundry#638 for cloudfoundry#328) had reduced this symmetry to some degree, and also caused some divergence between the names of some methods and variables (vs their implementations/values), which would make it harder to both implement and review the new functionality for cloudfoundry#644, and also harder to maintain the symmetry between the LB and AGW functionality. These changes include: - refactoring of existing code/implementation. E.g. Renamed some vars and methods to more accurately reflect their current values/behavior. - minor configuration API enhancements. E.g. Allow the `load_balancer` config to be configured as an Array, instead of the (previously-undocumented, but already-supported) comma-delimited string of LB names, to support multiple LB configs which differ by more than just the LB name (which now enables the functionality of cloudfoundry#111 and cloudfoundry#328 to be used more independently within a single config). - adding multiple new/missing unit tests, which cover existing functionality (previously added by cloudfoundry#541 for cloudfoundry#111 and cloudfoundry#638 for cloudfoundry#328) which was previously not covered/tested by the unit test suite. --- COMMIT HISTORY: Modified the `VMCloudProps._parse_load_balancer_config` method to allow the `load_balancer` config to be configured as an Array. Note: When specifying an Array of LBs, each element of the Array should be a String and/or Hash of the same format which was previously (and still is) valid when configuring a single LB as a non-Array. Refactoring: Minor refactoring in the `VMManager._get_load_balancers` method. Added additional data type validation to the `VMCloudProps._parse_load_balancer_config` method. Modified the `VMCloudProps._parse_load_balancer_config` method to return `nil` when the `load_balancer` config is omitted/missing. Added additional specs for the `VMCloudProps._parse_load_balancer_config` method. Refactoring: Renamed the `VMCloudProps.load_balancer` attribute. Refactoring of the `VMCloudProps._parse_load_balancer_config` and `VMManager._get_load_balancers` methods' implementations. Refactoring: Cleanup of the `VMCloudProps._parse_load_balancer_config` method's implementation. Refactoring: Minor cleanup of the `VMManager._get_load_balancers` method's implementation. Renamed a key within the Hash returned by the `AzureClient.parse_network_interface` method. Fixed the `AzureClient.parse_network_interface` method to correctly return all of the NIC's load_balancers' info (instead of only the first LB's info). Refactoring: Minor refactoring in the `AzureClient.create_network_interface` method. Refactoring: Renamed some vars in the `AzureClient.create_network_interface` method. Refactoring: Renamed a key within the `nic_params` Hash (from "load_balancer" to "load_balancers"), and fixed inaccurate doc comments for that key. Fixed a comment typo. Refactoring: Renamed a method and some vars: vm_manager_network.rb.

Note: This increases/restores the symmetry between the LB and AGW functionality (and configuration), by enhancing AGW support with new (for AGWs) functionality which had already been previously supported for LBs (by cloudfoundry#541 for cloudfoundry#111 and cloudfoundry#638 for cloudfoundry#328). Restoring this level of symmetry partially resolves cloudfoundry#644. These changes include: - refactoring of existing code/implementation (and tests). E.g. Renamed some vars and methods to more accurately reflect their current values/behavior. - significant configuration API enhancements, to enable the (previously LB-only) functionality of cloudfoundry#111 and cloudfoundry#328 to be used with both LB and/or AGW configs. E.g. Allow the `application_gateway` config to be configured as a Hash; allow the `application_gateway` config to be configured as an Array (new in this PR for both LBs and AGWs); added support for `application_gateway/resource_roup_name` config. --- SQUASHED COMMIT HISTORY: issue-644: multi-AGW: Refactored the `AzureClient.get_application_gateway_by_name` method to make the `resource_group_name` param a positional param (instead of an optional keyword param). Note: This change was made so that the `AzureClient.get_application_gateway_by_name` method's signature is consistent with the signatures of the other `AzureClient.get_*` methods in requiring the `resource_group_name` param to be passed as the first positional param. issue-644: multi-AGW: Modified the `VMCloudProps._parse_application_gateway_config` method to allow the `application_gateway` config to be configured as an Array. Note: When specifying an Array of AGWs, each element of the Array should be a String and/or Hash of the same format which was previously (and still is) valid when configuring a single AGW (as a non-Array). issue-644: multi-AGW: Modified the `VMCloudProps._parse_application_gateway_config` method to accept either String or Hash config (instead of String only). issue-644: multi-AGW: Fixed the `AzureClient.parse_network_interface` method to correctly return all of the NIC's application_gateways' info (instead of only the first LB's info). NOTE: All unit test specs are now passing again. issue-644: multi-AGW: Update `VMManager._get_application_gateways` method to support multi-AGW (and also to use the new `ApplicationGatewayConfig.resource_group_name` property/data). issue-644: multi-AGW: Modified the `VMCloudProps._parse_application_gateway_config` method to init `ApplicationGatewayConfig.resource_group_name` as `nil`, unless a value is explicitly configured. (See code comment for details.) issue-644: multi-AGW: Update `AzureClient.parse_network_interface` method to use the new `resource_group_name` keyword param of the `AzureClient.get_application_gateway_by_name` method. issue-644: multi-AGW: Added a new, optional `resource_group_name` keyword param to the `AzureClient.get_application_gateway_by_name` method. issue-644: multi-AGW: Updated `AzureClient.create_network_interface` method to support multi-AGW. Renamed a spec example in the Integration test suite, to make the meaning clearer: application_gateway_spec.rb. COMMENTS: Added/updated comments and doc comments: AzureClient, VMManager. issue-644: multi-AGW: Renamed a key within the `nic_params` Hash (from "application_gateway" to "application_gateways"). issue-644: multi-AGW: Rename and update `VMManager._get_application_gateways` method issue-644: multi-AGW: Renamed various vars/keys (from `application_gateway` to `application_gateways`) issue-644: multi-AGW: Added additional specs for the `VMCloudProps._parse_application_gateway_config` method. issue-644: multi-AGW: Add new `VMCloudProps._parse_application_gateway_config` method. NOTE: This change is part of a multi-commit set of incremental changes. Some of the existing unit test specs are now failing. They will be fixed/updated by the time the incremental changes are completed.

expose the resource group for load balancer, and wrap config for avai…

01c1657

…lability set.

andyliuliming requested review from gossion and bingosummer and removed request for gossion September 26, 2018 12:24

bingosummer changed the title ~~expose the resource group for load balancer, and wrap config for avai…~~ expose the resource group for load balancer, and wrap config for availability set Sep 26, 2018

bingosummer suggested changes Sep 26, 2018

View reviewed changes

bingosummer approved these changes Sep 27, 2018

View reviewed changes

bingosummer mentioned this pull request Sep 27, 2018

we should find the security group from more places, say, the resource group which the vm are in. #525

Closed

9 tasks

gossion reviewed Sep 27, 2018

View reviewed changes

also for the security group.

30af791

gossion reviewed Sep 27, 2018

View reviewed changes

andyliuliming added 2 commits September 27, 2018 14:34

resolve comments.

808addd

minor kubo fix.

ad5a4f9

bingosummer suggested changes Sep 27, 2018

View reviewed changes

fix.

115e49f

bingosummer approved these changes Sep 27, 2018

View reviewed changes

gossion approved these changes Sep 27, 2018

View reviewed changes

andyliuliming merged commit cdc2cc9 into cloudfoundry:master Sep 27, 2018

andyliuliming deleted the master_find_loadbalancer_in_other_rg branch September 28, 2018 12:35

andyliuliming mentioned this pull request Oct 19, 2018

Specify Load Balancer in Resource Groups other than default #328

Closed

Justin-W mentioned this pull request Dec 10, 2021

Updated Azure CPI config docs to include new and existing features… cloudfoundry/docs-bosh#761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andyliuliming commented Sep 26, 2018

cfdreddbot commented Sep 26, 2018

bingosummer Sep 26, 2018

andyliuliming Sep 27, 2018

bingosummer Sep 27, 2018

bingosummer Sep 26, 2018

andyliuliming Sep 27, 2018

bingosummer Sep 26, 2018

andyliuliming Sep 27, 2018

bingosummer Sep 26, 2018

andyliuliming Sep 27, 2018 •

edited

Loading

bingosummer Sep 27, 2018

gossion Sep 27, 2018

andyliuliming Sep 27, 2018

gossion Sep 27, 2018

andyliuliming Sep 27, 2018

bingosummer Sep 27, 2018

andyliuliming Sep 27, 2018

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

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

Conversation

andyliuliming commented Sep 26, 2018

cfdreddbot commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyliuliming Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyliuliming Sep 27, 2018 •

edited

Loading