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

[Compute] Add AppGateway support to VMSS create #2570

Merged
merged 5 commits into from Mar 24, 2017
Merged

[Compute] Add AppGateway support to VMSS create #2570

merged 5 commits into from Mar 24, 2017

Conversation

tjprescott
Copy link
Member

@tjprescott tjprescott commented Mar 21, 2017

Closes #2335. Permits creation of an app gateway as the frontend for VMSS.

Known issues:

  • Scenario tests needed (may be a separate PR due to the amount of time needed to provision an app gateway)
  • Ideally the app gateway subnet can be defaulted when creating a new app-gateway, but this will result in much more complex logic. For now, it is required.
  • When using an existing app-gateway, backend-pool-name could be looked up. Currently it is just required.

Supported Scenarios

  • Create VMSS with new app-gateway, new VNet and new/existing public IP
  • Create VMSS with existing app-gateway, existing VNet and existing public IP (which is already associated with the VNet)

Unsupported Scenarios

  • Create VMSS with new app-gateway but existing VNet.
  • Create VMSS with existing app-gateway but new VNet and/or new public IP.

@tjprescott tjprescott added Compute az vm/vmss/image/disk/snapshot Network az network vnet/lb/nic/dns/etc... labels Mar 21, 2017
@tjprescott tjprescott added this to the Sprint 14 milestone Mar 21, 2017
@tjprescott
Copy link
Member Author

Updated help text:

Command
    az vmss create: Create an Azure Virtual Machine Scale Set.
        For an end-to-end tutorial, see https://docs.microsoft.com/azure/virtual-machine-scale-
        sets/virtual-machine-scale-sets-linux-create-cli.

Arguments
    --image                  [Required]: The name of the operating system image (URN alias, URN, or
                                         URI).
        URN aliases: CentOS, CoreOS, Debian, openSUSE, RHEL, SLES, UbuntuLTS, Win2008R2SP1,
        Win2012Datacenter, Win2012R2Datacenter.
        Example URN: MicrosoftWindowsServer:WindowsServer:2012-R2-Datacenter:latest
        Example Custom Image Resource ID or Name: /subscriptions/subscription-
        id/resourceGroups/MyResourceGroup/providers/Microsoft.Compute/images/MyImage
        Example URI: http://<storageAccount>.blob.core.windows.net/vhds/osdiskimage.vhd.
    --name -n                [Required]: Name of the virtual machine scale set.
    --resource-group -g      [Required]: Name of resource group. You can configure the default group
                                         using 'az configure --defaults group=<name>'.
    --app-gateway-type
    --custom-data                      : Custom init script file or text (cloud-init, cloud-config,
                                         etc..).
    --disable-overprovision            : Overprovision option (see https://azure.microsoft.com/en-
                                         us/documentation/articles/virtual-machine-scale-sets-
                                         overview/ for details).
    --instance-count                   : Number of VMs in the scale set.  Default: 2.
    --location -l                      : Location in which to create VM and related resources.
                                         Defaults to the resource group's location.
    --no-wait                          : Do not wait for the long running operation to finish.
    --secrets                          : One or many Key Vault secrets as JSON strings or files via
                                         '@<file path>' containing '[{ "sourceVault": { "id":
                                         "value" }, "vaultCertificates": [{ "certificateUrl":
                                         "value", "certificateStore": "cert store name (only on
                                         windows)"}] }]'.
    --single-placement-group           : Enable single placement group. This flag will default to
                                         True if instance count <=100, and default to False for
                                         instance count >100.  Allowed values: false, true.
    --tags                             : Space separated tags in 'key[=value]' format. Use "" to
                                         clear existing tags.
    --upgrade-policy-mode              : Allowed values: Automatic, Manual.  Default: manual.
    --validate                         : Generate and validate the ARM template without creating any
                                         resources.
    --vm-sku                           : Size of VMs in the scale set.  See
                                         https://azure.microsoft.com/en-us/pricing/details/virtual-
                                         machines/ for size info.  Default: Standard_D1_v2.

Network Arguments
    --public-ip-address                : Name of the public IP address when creating one (default)
                                         or referencing an existing one. Can also reference an
                                         existing public IP by ID or specify "" for None.
    --public-ip-address-allocation     : Allowed values: dynamic, static.  Default: dynamic.
    --public-ip-address-dns-name       : Globally unique DNS name for a newly created Public IP.
    --subnet                           : The name of the subnet when creating a new VNet or
                                         referencing an existing one. Can also reference an existing
                                         subnet by ID. If omitted, an appropriate VNet and subnet
                                         will be selected automatically, or a new one will be
                                         created.
    --subnet-address-prefix            : The subnet IP address prefix to use when creating a new
                                         VNet in CIDR format.
    --vnet-address-prefix              : The IP address prefix to use when creating a new VNet in
                                         CIDR format.  Default: 10.0.0.0/16.
    --vnet-name                        : Name of the virtual network when creating a new one or
                                         referencing an existing one.

Network Balancer Arguments
    --application-gateway              : Name to use when creating a new application gateway
                                         (default) or referencing an existing one. Can also
                                         reference an existing application gateway by ID or specify
                                         "" for none.
    --backend-pool-name                : Name to use for the backend pool when creating a new load
                                         balancer or application gateway.
    --backend-port                     : Backend port to open with NAT rules when creating a new
                                         load balancer. Defaults to 22 on Linux and 3389 on Windows.
    --gateway-subnet-address-prefix    : The subnet IP address prefix to use when creating a new
                                         application gateway in CIDR format.
    --load-balancer                    : Name to use when creating a new load balancer (default) or
                                         referencing an existing one. Can also reference an existing
                                         load balancer by ID or specify "" for none.
    --nat-pool-name                    : Name to use for the NAT pool when creating a new load
                                         balancer.

@codecov-io
Copy link

codecov-io commented Mar 21, 2017

Codecov Report

Merging #2570 into master will decrease coverage by 0.16%.
The diff coverage is 43.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2570      +/-   ##
==========================================
- Coverage   71.95%   71.79%   -0.17%     
==========================================
  Files         375      375              
  Lines       20602    20689      +87     
  Branches     3012     3035      +23     
==========================================
+ Hits        14825    14854      +29     
- Misses       4846     4901      +55     
- Partials      931      934       +3
Impacted Files Coverage Δ
...ure-cli-vm/azure/cli/command_modules/vm/_params.py 96.09% <100%> (+0.03%) ⬆️
...cli-vm/azure/cli/command_modules/vm/_validators.py 68.5% <35.93%> (-3.67%) ⬇️
.../azure/cli/command_modules/vm/_template_builder.py 77.71% <42.1%> (-3.24%) ⬇️
...zure-cli-vm/azure/cli/command_modules/vm/custom.py 72.72% <48.48%> (-1.1%) ⬇️
...-cli-role/azure/cli/command_modules/role/custom.py 17.89% <0%> (ø) ⬆️
...dback/azure/cli/command_modules/feedback/custom.py 34.69% <0%> (ø) ⬆️
src/azure-cli-core/azure/cli/core/_util.py 66.66% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29d8a30...e12a2de. Read the comment docs.

Copy link
Contributor

@yugangw-msft yugangw-msft left a comment

Choose a reason for hiding this comment

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

I suggest we name every gateway specific argument to start with "--application-gateway", and same on "--load-balancer". This way I can easily ignore not related arguments per balancer type. If we prefer shorter name, we can go with '--app-gateway' and '--lb'?
The rest LGTM.

@@ -0,0 +1,205 @@
{
"$schema": "http://schema.management.azure.com/schemas/2015-01-01-preview/deploymentTemplate.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to check in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I was just using it as reference. Good catch.

@@ -481,7 +508,7 @@ def _validate_vm_create_public_ip(namespace):


def _validate_vmss_create_public_ip(namespace):
if namespace.load_balancer_type is None:
if namespace.load_balancer_type is None and namespace.app_gateway_type is None:
if namespace.public_ip_address:
raise CLIError('--public-ip-address is not applicable when there is no load-balancer '
'attached, or implictly disabled due to 100+ instance count')
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update the error message to incorporate app-gateway

if not namespace.single_placement_group:
if not namespace.single_placement_group and namespace.load_balancer:
raise CLIError(
'--load-balancer is not applicable when --single-placement-group is turned off.')
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we need to double check this error message. My understanding is the scenario is still supported that you can have <=100 machine and --single-placement-group is turned off

Copy link
Member Author

Choose a reason for hiding this comment

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

The original error was:
'--load-balancer is not applicable when --single-placement-group is explictly turned off or implictly turned off for 100+ instance count'

This would be the "explicitly turned off" case.

azure-cli.pyproj Outdated
@@ -15,8 +15,7 @@
<InterpreterId>{54f4b6dc-0859-46dc-99bb-b275c9d0aca3}</InterpreterId>
<InterpreterVersion>3.5</InterpreterVersion>
<EnableNativeCodeDebugging>False</EnableNativeCodeDebugging>
<CommandLineArguments>
</CommandLineArguments>
<CommandLineArguments>vmss create -g tjp-vmss -n vmss1 --image UbuntuLTS --authentication-type password --admin-password TestTest12#$ --validate --verbose</CommandLineArguments>
Copy link
Member

Choose a reason for hiding this comment

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

Remove this?

@tjprescott
Copy link
Member Author

@yugangw-msft regarding the parameter names, if I change the existing ones it would be a breaking change. I could introduce aliases for those. So: --nat-pool-name, --lb-nat-pool-name. Then we would have to deprecate the original later when there's profile change. Thoughts?

@tjprescott
Copy link
Member Author

Here's the updated help text:

Network Balancer Arguments
    --app-gateway                      : Name to use when creating a new application gateway
                                         (default) or referencing an existing one. Can also
                                         reference an existing application gateway by ID or specify
                                         "" for none.
    --app-gateway-subnet-address-prefix: The subnet IP address prefix to use when creating a new
                                         application gateway in CIDR format.
    --backend-pool-name                : Name to use for the backend pool when creating a new load
                                         balancer or application gateway.
    --backend-port                     : When creating a new load balancer, backend port to open
                                         with NAT rules (Defaults to 22 on Linux and 3389 on
                                         Windows). When creating an application gateway, the backend
                                         port to use for the backend HTTP settings.
    --lb --load-balancer               : Name to use when creating a new load balancer (default) or
                                         referencing an existing one. Can also reference an existing
                                         load balancer by ID or specify "" for none.
    --lb-nat-pool-name --nat-pool-name : Name to use for the NAT pool when creating a new load
                                         balancer.

@tjprescott tjprescott merged commit 43d788c into Azure:master Mar 24, 2017
@tjprescott tjprescott deleted the VMSSWithAG branch March 24, 2017 20:45
@haroldrandom haroldrandom added cla-not-required Compute az vm/vmss/image/disk/snapshot Network az network vnet/lb/nic/dns/etc... labels Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required Compute az vm/vmss/image/disk/snapshot Network az network vnet/lb/nic/dns/etc...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants