-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Adding CRUD azure cli commands for Managed instance and Managed database resources #6428
Conversation
* Managed instance commands: * az sql managed instance create * az sql managed instance show * az sql managed instance list * az sql managed instance update * az sql managed instance delete * Managed database commands: * az sql managed db create * az sql managed db show * az sql managed db list * az sql managed instance restore * az sql managed instance delete
View a preview at https://prompt.ws/r/Azure/azure-cli/6428 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a quick glance, there are a number of issues that do not conform with our command authoring guidance. Please set up a 30 minute Skype command review.
I am trying to check out your branch on my machine but https://github.com/petrajkogit/azure-cli is empty. Something weird going on? |
short-summary: Create a managed instance. | ||
examples: | ||
- name: Create a managed instance with specified parameters and with identity | ||
text: az sql managed instance create -g mygroup -mi myinstance -l mylocation -i -u myusername -p mypassword --license-type mylicensetype --subnet-id mysubnetid --vcores vcorenum --storage-size-in-gb storagesizenum --sku-name skuname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For managed instance
commands, the "managed instance name parameter" must be -n
|
||
c.argument('vcores', | ||
required=True, | ||
help='Number of vcores to search for. If unspecified, all vcore sizes are shown.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like MI to be consistent with DB and pool. So this parameter should be --capacity
. Also the help text here was copied from capabilities command which is searching - you should use help text similar to db create
.
|
||
c.argument('storage_size_in_gb', | ||
required=True, | ||
help='Number of vcores to search for. If unspecified, all vcore sizes are shown.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use --storage
and use unit converter like db and pool commands
required=True) | ||
|
||
c.argument('subnet_id', | ||
required=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all these parameters really required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, subnet_id, admin-user and admin-password are required. license-type, vcores, storage and sku_name are not required and they get default values if not specified in request. I will talk to the PMs to see if we want to force explicit setting of this parameters, or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameters really should not be required if they are not required. More required parameters = more difficult for new users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, checked, will remove requirement where it is not needed.
help='Number of vcores to search for. If unspecified, all vcore sizes are shown.') | ||
|
||
c.argument('sku_name', | ||
required=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use same design as DB & pool: --edition/--tier and --family parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will do. I have --family, --tier and --service-objective parameters now, same as for SQL db.
But, now service-objective has to be in sync with --family and --tier in order for command to work (or to have some unsupported value). For example, if I have set --family Gen4 and --tier GeneralPurpose then --service-objective has to be GP_Gen4 in order for command to work. So this is redundant parameter in that case. Is this ok behaviour? Do we have something like parameter sets in Azure CLI, similar to what we have in PowerShell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO you don't need --service-objective
. The reason for having --family
and --tier
is because those are just basic strings with a small number of known values, but SKU name is a combination of those which is not obvious to new users. We have service objective for db create
commands for backwards compatibility, I did not add --service-objective to elastic-pool create
even though I could have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer your question, no there are no parameter sets like in PowerShell. If you look in custom.py you will see that there is code to look up the sku name in capabilities based on family and tier. It's a little complicated, but I strongly feel that it substantially improves the new user experience because the values are more discoverable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also having separate params allows syntax like this: az sql elastic-pool update -e BusinessCritical
which will update the edition but keep the family the same - very convenient for users :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I am trying to remove --service-objective parameter which maps to sku.name, but sku.name is required parameter it seems. So I would need to populate sku.name even if not requiring it in command from other two parameters (--family and --tier)? Or I am missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it needs to be populated in custom.py. See _find_elastic_pool_sku_from_capabilities for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your MI code can be mostly the same, except that:
- MI has family capability and perf level capability at different levels so your code to find perf level will be simpler
- All MI's have family (as opposed to DTU dbs/pools which have empty family) so you don't need allow_reset_family param.
This should reduce some code complexity.
Something like this:
def _find_managed_instance_sku_from_capabilities(cli_ctx, location, sku):
'''
Given a requested sku which may have some properties filled in
(e.g. tier and capacity), finds the canonical matching sku
from the given location's capabilities.
'''
logger.debug('_find_managed_instance_sku_from_capabilities input: %s', sku)
if sku.name:
# User specified sku.name, so nothing else needs to be resolved.
logger.debug('_find_managed_instance_sku_from_capabilities return sku as is')
return sku
if not _any_sku_values_specified(sku):
# User did not request any properties of sku, so just wipe it out.
# Server side will pick a default.
logger.debug('_find_managed_instance_sku_from_capabilities return None')
return None
# Some properties of sku are specified, but not name. Use the requested properties
# to find a matching capability and copy the sku from there.
# Get default server version capability
capabilities_client = get_sql_capabilities_operations(cli_ctx, None)
capabilities = capabilities_client.list_by_location(location, CapabilityGroup.supported_managed_instance_versions)
managed_instance_version_capability = _get_default_capability(capabilities.supported_managed_instance_versions)
# Find edition capability, based on requested sku properties
edition_capability = _find_edition_capability(sku, server_version_capability.supported_editions)
# Find family level capability, based on requested sku properties
family_capability = _find_family_capability(sku, edition_capability.supported_families) # need to implement this function
# Copy sku object from capability
result = Sku(...)
logger.debug('_find_managed_instance_sku_from_capabilities return: %s', result)
return result
Note that db & pool capabilities return the "canonical sku" so we can copy the sku object directly from the chosen capability. You don't have this so you have to construct the sku object. This might be something Nemanja could add in managed instance capabilities.
|
||
c.argument('administrator_login_password', | ||
options_list=['--admin-password', '-p'], | ||
required=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems wrong that this is required. There is no way to not update the password?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be required. This was copied from create parameters where password is required. Fixed
@jaredmoo Please use https://github.com/petrajkogit/azure-cli-1 if you want to checkout my branch. |
@@ -18,7 +18,8 @@ | |||
ExportRequest, | |||
Server, | |||
ServerAzureADAdministrator, | |||
Sku | |||
Sku, | |||
ManagedInstance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make alphabetical please
|
||
c.argument('license_type', | ||
required=False, | ||
arg_type=get_enum_type(DatabaseLicenseType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a ManagedInstanceLicenseType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not. Will have to use this one I guess
@@ -42,6 +42,8 @@ | |||
get_sql_server_usages_operations, | |||
get_sql_subscription_usages_operations, | |||
get_sql_virtual_network_rules_operations, | |||
get_sql_managed_instances_operations, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetical please :)
|
||
c.argument('subnet_id', | ||
required=True, | ||
options_list=['--subnet-id', '-subn']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to remove or change -subn
. These abbreviations are supposed to be just 1 character :) Also let's make sure this parameter aligns with az sql server vnet-rule create
command. You can reused the param validator that the vnet team wrote.
--subnet [Required]: Name or ID of the subnet that allows access to an Azure Sql
Server. If subnet name is provided, --vnet-name must be
provided.
--vnet-name : The virtual network name.
Actually I just realized that --vnet-name
should just be --vnet
. I'll make that update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it, left only --subnet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From our command review:
- change "managed db" and "managed instance" to "managed-db" and "managed-instance" respectively so they appear at the same level as the non-managed alternatives.
- Suppress --ids on the
managed-db list
command. - Support name or ID for --subnet (by exposing optional --vnet-name)
- Update examples for changes to parameter aliasing
- Ideally, show the formats of --collation and --time. Using actual values in your examples will help too.
- Add some help extra text for --assign-identity flag in update to describe what happens if an identity is already assigned.
* az sql managed db show | ||
* az sql managed db list | ||
* az sql managed instance restore | ||
* az sql managed instance delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming these should say "db" instead of "instance" right?
Does this aim for sprint 38 (release on 6/5)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, it's getting close to being done :)
kB=1.0 / (1024 * 1024), | ||
MB=1.0 / 1024, | ||
GB=1, | ||
TB=1024)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result_type and unit_map don't need to be explicitly specified here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we have to, otherwise wrong dict will be used. In our case storage should be specified in GB and converted to it before sending the request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't see that the multipliers are different. Cool!
|
||
c.argument('vcores', | ||
options_list=['--capacity', '-c'], | ||
help='Determines how much VCore to associate with Managed instance.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help='The capacity of the managed instance in vcores'
c.argument('storage_size_in_gb', | ||
options_list=['--storage'], | ||
arg_type=storage_param_type, | ||
help='Determines how much storage size to associate with Managed instance. ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help='The storage size of the managed instance'
|
||
db_service_objective_examples = 'Basic, S0, P1, GP_Gen4_1, BC_Gen5_2.' | ||
dw_service_objective_examples = 'DW100, DW1000c' | ||
mi_service_objective_examples = 'GP_Gen4, GP_Gen5' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unused, indeed, removed it
# Create args that will be used to build up the ManagedInstance object | ||
create_args_for_complex_type( | ||
c, 'parameters', ManagedInstance, [ | ||
'administrator_login_password', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not have sku
arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can sku not be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not currently. We cannot update to different edition. We can only update password, storage, capacity and identity info
@@ -426,3 +426,83 @@ | |||
- name: Create a vnet rule by providing the vnet and subnet name. The subnet id is created by taking the resource group name and subscription id of the SQL server. | |||
text: az sql server vnet-rule create --subnet subnetName --vnet-name vnetName | |||
""" | |||
helps['sql managed-instance'] = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider abbreviating to sql mi
? managed-instance
is pretty long to type. Similar for sql mdb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong preferences here. sql mi looks ok. "sql mdb" for me sound a bit strange and confusing. What do you say about "sql midb"?
try: | ||
return next(e for e in supported_families if e.name == sku.family) | ||
except StopIteration: | ||
candiate_families = [e.name for e in supported_families] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: candidate_families
# Find family level capability, based on requested sku properties | ||
family_capability = _find_family_capability(sku, edition_capability.supported_families) | ||
|
||
result = Sku(name=family_capability.sku) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sku name is just "GP_Gen4", so so you also need to assign capacity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, vcores is in the properties, not in the sku.
parameters=kwargs) | ||
|
||
|
||
def managed_db_list( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This custom func doesn't do anything other than list_by_instance
, so you should remove this and just directly use list_by_instance
in commands.py
''' | ||
|
||
if not restore_point_in_time: | ||
raise CLIError('--restore_point_in_time must be specified.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove =None
so that it is required, then it will be validated at higher layer and you don't need this check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think you can remove restore_point_in_time
param completely from this func so that it falls into kwargs
. Then just make sure that it is marked as required in params.py
@troydai Yes, this should aim the first next release, so I guess that is sprint 38 (release on 6/5) |
…ore command -Fixing issue when passing subnet_id parameter -Comments resolved
'restore_point_in_time' | ||
]) | ||
|
||
c.argument('target_managed_database_name', | ||
options_list=['--target-db'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just remembered that sql db restore
uses --dest-name
param names. See https://docs.microsoft.com/en-us/cli/azure/sql/db?view=azure-cli-latest#az-sql-db-restore Can you match that? Also --dest-server
. --target-db
and --target-mi
sound good too, but I'd like to aim for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to --dest-name and --dest-mi
c.argument('target_managed_instance_name', | ||
options_list=['--target-mi'], | ||
help='Name of the managed instance to restore managed database to. ' | ||
'This can be same managed instance, or another managed instance withing same resource group. ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo withing
-> within
Is it not possible to specify another resource group? We have this for sql db copy
command, see https://docs.microsoft.com/en-us/cli/azure/sql/db?view=azure-cli-latest#az-sql-db-copy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! You are right, we can specify different rg. Fixed.
' earliestRestoreDate value. Time should be in following format: "YYYY-MM-DDTHH:MM:SS"') | ||
|
||
with self.argument_context('sql managed-db list') as c: | ||
c.argument('managed_instance_name', id_part=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add comment explaining what this does and why? That will help the next person to read this :)
++++++ | ||
* Added new Managed instance and Managed database CRUD commands. | ||
* Managed instance commands: | ||
* az sql managed instance create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update this too :)
kB=1.0 / (1024 * 1024), | ||
MB=1.0 / 1024, | ||
GB=1, | ||
TB=1024)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't see that the multipliers are different. Cool!
'administrator_login_password', | ||
]) | ||
|
||
c.argument('administrator_login_password', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
Please fix the travis build failures. |
This PR needs to be merged by the end of the day to make in to the sprint 38 (release on 6/5) |
short-summary: Create a managed instance. | ||
examples: | ||
- name: Create a managed instance with specified parameters and with identity | ||
text: az sql mi create -g mygroup -n myinstance -l mylocation -i -u myusername -p mypassword --license-type mylicensetype --subnet mysubnetid --capacity vcorecapacity --storage storagesize --edition editionname --family familyname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have more real-looking values where possible. i.e.:
az sql mi create -g mygroup -n myinstance -l mylocation -i -u myusername -p mypassword --license-type LicenseIncluded --subnet /subscriptions/{SubID}/resourceGroups/{ResourceGroup}/providers/Microsoft.Network/virtualNetworks/{VNETName}/subnets/{SubnetName} --capacity 2 --storage 32GB --edition BusinessCritical --family Gen4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding example btw :) Can you also add example with minimal parameters?
Is that right? GP is the only edition? |
""" | ||
helps['sql midb'] = """ | ||
type: group | ||
short-summary: Manage SQL managed databases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to Manage SQL managed instance databases.
?
My only major comment is on --target-db versus --dest-name param names. And the travis build needs to succeed. Overall looks awesome! |
@jaredmoo . Not sure why I cannot reply directly on your question, but here it is. As for why the build is failing, I believe it is due to python SDK. Do you know if it has been published? |
0.9.1 has been released with MI support: https://pypi.org/project/azure-mgmt-sql/ You can see the build failure info by clicking 'Details' At the moment it is failing in style check (https://travis-ci.org/Azure/azure-cli/jobs/386200482), you can repro this locally by running |
Thanks for addressing comments. Good to merge as soon as build succeeds. |
@tjprescott are your concerns addressed? |
LGTM. @petrajkogit try re-recording your tests to see if it passes CI. If not, we can look deeper. I didn't see any of the common reasons why the tests would fail. |
@tjprescott @jaredmoo Thanks a lot for your help. I am rerecording my tests again since that's why build is failing, and should fix it soon. |
Codecov Report
@@ Coverage Diff @@
## dev #6428 +/- ##
===================================
Coverage 0% 0%
===================================
Files 11 11
Lines 133 133
Branches 9 9
===================================
Misses 133 133 Continue to review full report at Codecov.
|
Managed instance commands:
Managed database commands:
This checklist is used to make sure that common guidelines for a pull request are followed.
[x ] The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
[x ] I adhere to the Command Guidelines.