Skip to content

Conversation

@arde0708
Copy link
Contributor

This PR is a hotfix candidate for the recent Ignite Train. It includes :

  • Changing Vnet Name and subnet name to exclude server name.( to allow servers to be provisioned with HA enabled )
  • Change the MySQL default region to West US2 as MySQL Flexservers do not exist in the current default - EASTUS
  • Update the connection string in the postgres create instance response.
  • Skip adding special characters in the password
  • Convert user supplied servername to lower case.

@yungezz
Copy link
Member

yungezz commented Sep 25, 2020

hotfix candidate

@yungezz
Copy link
Member

yungezz commented Sep 25, 2020

hi @rajsell @mjain2 @DaeunYim could you pls review this PR? I cannot add you as reviewers, it need to be in azure-sdk-read team I think

@mjain2
Copy link
Contributor

mjain2 commented Sep 25, 2020

hi @rajsell @mjain2 @DaeunYim could you pls review this PR? I cannot add you as reviewers, it need to be in azure-sdk-read team I think

@yungezz we've signed off + checks looks green, thanks.

# if location is not passed as a parameter or is missing from local context
if location is None:
location = DEFAULT_LOCATION
if db_engine == 'postgres':
Copy link
Member

Choose a reason for hiding this comment

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

db_engine.lower()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something that we are explicitly passing as lower case from the caller. Line 61 in flexible_server_custom_postgres.py.

Copy link
Member

Choose a reason for hiding this comment

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

could you pls add some test to change the overall change?

Copy link
Contributor Author

@arde0708 arde0708 Sep 25, 2020

Choose a reason for hiding this comment

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

For this, we are just changing the default location from eastus to westus2 for MySQL. We already have tests for both MySQL and Postgres SQL creation otherwise.

@yonzhan yonzhan added this to the S176 milestone Sep 25, 2020
if command_group == 'mysql':
c.argument('sku_name', options_list=['--sku-name'], validator=mysql_sku_name_validator,
c.argument('tier', options_list=['--tier'],
help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized ')
Copy link
Contributor

Choose a reason for hiding this comment

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

For allowed values. prefer using get_enume_type()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in the SDK comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean all allowed values for tier and sku_name. Could we also get the allowed array from sdk?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we do have validators for tier, sku_name, storage size validated by the array from sdk. We will include that for the next train.

# Populate desired parameters
location, resource_group_name, server_name = generate_missing_parameters(cmd, location, resource_group_name,
server_name)
server_name, 'postgres')
Copy link
Contributor

Choose a reason for hiding this comment

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

please avoid positional arguments here

return _form_response(user, sku, loc, server_id, host, version,
administrator_login_password if administrator_login_password is not None else '*****',
_create_postgresql_connection_string(host, administrator_login_password), firewall_id,
_create_postgresql_connection_string(host, user, administrator_login_password), firewall_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid positional arguments here

help='Compute tier of the server. Accepted values: Burstable, GeneralPurpose, Memory Optimized ')
c.argument('sku_name', default='Standard_B1ms', options_list=['--sku-name'], validator=mysql_sku_name_validator,
c.argument('sku_name', default='Standard_B1ms', options_list=['--sku-name'],
help='The name of the compute SKU. Follows the convention Standard_{VM name}. Examples: Standard_B1ms, Standard_D4s_v3 ')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to get allowed sku values through SDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a permanent fix for this in S176 where we fetch the SKU's from the SDK. We are currently removing the validators in the PR as there is a bug in the validator which prevents provisioning servers with certain SKU's

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

@Juliehzl
Copy link
Contributor

HI @arde0708, as what I see in the e-mail, you need to fix the issue when creating a new flexible-server with HA in a VNET. May I know how you fix it?

@arde0708
Copy link
Contributor Author

HI @arde0708, as what I see in the e-mail, you need to fix the issue when creating a new flexible-server with HA in a VNET. May I know how you fix it?

@Juliehzl All those changes are in src/azure-cli/azure/cli/command_modules/rdbms/flexible_server_virtual_network.py. Basically, we are removing the servername from the VNET name and subnet Name in all places to address this issue.

@Juliehzl
Copy link
Contributor

HI @arde0708

        Aritra De
        FTE, as what I see in the e-mail, you need to fix the issue when creating a new flexible-server with HA in a VNET. May I know how you fix it?

@Juliehzl Zunli Hu FTE All those changes are in src/azure-cli/azure/cli/command_modules/rdbms/flexible_server_virtual_network.py. Basically, we are removing the servername from the VNET name and subnet Name in all places to address this issue.

Do you have test for related scenarios? I will recommend to add test cases to prevent potential issues in the future, including sdk changes.

@Juliehzl
Copy link
Contributor

In addition, your command interface doesn't align with CLI command guideline. From the command, I have no idea which parameters are required.
image

See other command:
image

@arde0708
Copy link
Contributor Author

In addition, your command interface doesn't align with CLI command guideline. From the command, I have no idea which parameters are required.
image

See other command:
image

@Juliehzl Please note that none of the arguments in az postgres flexible-server create are Required. Hence, we dont have any of them marked as required. If the user passes them, we use that. Else, we create a default one for the parameters not passed. as a result, you see the difference.

@yungezz
Copy link
Member

yungezz commented Sep 28, 2020

I would suggest create an another issue to track the interface refinement work. Since this is a hotfix to mitigate immediate customer impact, better be more specific more efficient.

@Juliehzl
Copy link
Contributor

Hi @arde0708, to do a quick hotfix, we will help you merge the PR.
But to avoid issues in the future, please add test for all your new commands and arguments. Especially there will be track2 migration in the future.
For command design, I will have some concern for custom easy usage, and we should also align with CLI command guideline.

@arde0708
Copy link
Contributor Author

HI @arde0708

        Aritra De
        FTE, as what I see in the e-mail, you need to fix the issue when creating a new flexible-server with HA in a VNET. May I know how you fix it?

@Juliehzl Zunli Hu FTE All those changes are in src/azure-cli/azure/cli/command_modules/rdbms/flexible_server_virtual_network.py. Basically, we are removing the servername from the VNET name and subnet Name in all places to address this issue.

Do you have test for related scenarios? I will recommend to add test cases to prevent potential issues in the future, including sdk changes.

@Juliehzl : This is a temporary patch. The Backend Team is actually fixing the root cause of this issue, but it will be deployed later in October. So this is a patch just until then after which we will revert to what it was.

@Juliehzl Juliehzl changed the base branch from release to dev September 28, 2020 02:54
@Juliehzl Juliehzl changed the base branch from dev to release September 28, 2020 02:56
@Juliehzl Juliehzl changed the title [RDBMS]Hotfix: az postgres flexible-server create --high-availability Enabled: Update VnetName to exclude servername and update default region for MySQL [RDBMS] az postgres flexible-server create : Update VnetName to exclude servername and update default region for MySQL Sep 28, 2020
@Juliehzl
Copy link
Contributor

Hi @arde0708, please make sure there is no breaking changes for existing commands and I will merge the PR for hotfix.
Here is to note that All CLI team will be on China National Day holiday from 10.1 ~ 10.8, and we could not support you quickly if there is any furthur issue.

@Juliehzl Juliehzl changed the title [RDBMS] az postgres flexible-server create : Update VnetName to exclude servername and update default region for MySQL [RDBMS] Hotfix: az postgres flexible-server create : Update VnetName to exclude servername and update default region for MySQL Sep 28, 2020
@yonzhan yonzhan merged commit 23801a4 into Azure:release Sep 28, 2020
arde0708 added a commit to arde0708/azure-cli that referenced this pull request Sep 30, 2020
…e to exclude servername and update default region for MySQL (Azure#15295)

* hotfix chnages

* Remove - from password if thats the first character

* Style check fix and Mysql sku

* Remove MySQL SKU validator

* Remove MySQL SKu and tier validators
Juliehzl added a commit that referenced this pull request Sep 30, 2020
* [RDBMS] Hotfix: `az postgres flexible-server create` : Update VnetName to exclude servername and update default region for MySQL (#15295)

* hotfix chnages

* Remove - from password if thats the first character

* Style check fix and Mysql sku

* Remove MySQL SKU validator

* Remove MySQL SKu and tier validators

* {KeyVault} Hotfix: Fix CloudSuffixNotSetException while using `az keyvault` in Azure Stack and Air-Gaped Cloud (#15321)

* {Release} Hotfix: Upgrade version to 2.12.1 (#15324)

* upgrade version

* upgrade version

Co-authored-by: arde0708 <[email protected]>
Co-authored-by: Feng Zhou <[email protected]>
Juliehzl pushed a commit that referenced this pull request Oct 9, 2020
…PI version from network client. (#15392)

* [RDBMS] Hotfix: `az postgres flexible-server create` : Update VnetName to exclude servername and update default region for MySQL (#15295)

* hotfix chnages

* Remove - from password if thats the first character

* Style check fix and Mysql sku

* Remove MySQL SKU validator

* Remove MySQL SKu and tier validators

* Trigger checks

* Trigger checks with change in PR title

* Remove specific API version

* Trigger check for PR Title

* Trigger check for PR Title

* Trigger CI checks

* validate with list-skus info

* style fixed

* move validators to command body to speed up

* test code for validator and restored mgmt testcode

* minor fix

* flexible mgmt mysql test

* new test recording files

* style fixed, location changed, linter fixed

* linter pass

Co-authored-by: Daeun Yim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants