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.

Sign up for GitHub

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

Jump to bottom

[Service Fabric] az sf cluster node-type add: Fix the unexpected error that 'StorageAccountsOperations' object has no attribute 'create' #22283

Merged
merged 3 commits into from
May 19, 2022
Merged

[Service Fabric] az sf cluster node-type add: Fix the unexpected error that 'StorageAccountsOperations' object has no attribute 'create' #22283

merged 3 commits into from
May 19, 2022

Conversation

a-santamaria
Copy link
Member

@a-santamaria a-santamaria commented May 6, 2022 •
edited by zhoxing-ms
Loading

Related command
az sf cluster node-type add

Description
Fix adding a new node type. It was failing because due to a storage account method name breaking change. Reported here #21697

also found on #22344 and #22315

Testing Guide

History Notes

[Service Fabric] az sf cluster node-type add: Fix the unexpected error that 'StorageAccountsOperations' object has no attribute 'create'

This checklist is used to make sure that common guidelines for a pull request are followed.

@ghost ghost added Service Fabric az sf Auto-Assign Auto assign by bot labels May 6, 2022
@ghost ghost requested a review from yonzhan May 6, 2022 01:08
@ghost ghost assigned zhoxing-ms May 6, 2022
@ghost ghost added this to the May 2022 (2022-05-24) - For Build milestone May 6, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented May 6, 2022

Service Fabric

@yonzhan yonzhan requested a review from wangzelin007 May 6, 2022 01:19
@a-santamaria a-santamaria requested a review from LukeSlev May 6, 2022 19:41
@zhoxing-ms
Copy link
Contributor

Could you add some tests for these cases?

@navba-MSFT
Copy link
Contributor

Could you add some tests for these cases?

@a-santamaria for visibility

@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@navba-MSFT
Copy link
Contributor

Could you add some tests for these cases?

Thanks @zhoxing-ms .
@a-santamaria As requested in the above comment, Could you please add test cases for this PR so that it could be approved and then merged ?

@a-santamaria
Copy link
Member Author

Could you add some tests for these cases?

I will. I am currently having issues recording the tests, getting
azure.core.exceptions.ServiceResponseError: ('Connection aborted.', ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None))
Only happening when I run the tests but not if I call the commands directly

@wangzelin007
Copy link
Member

Could you add some tests for these cases?

I will. I am currently having issues recording the tests, getting azure.core.exceptions.ServiceResponseError: ('Connection aborted.', ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None)) Only happening when I run the tests but not if I call the commands directly

Hi Alfredo,
Can you provide the CLI command az xxx and the test command azdev test xxx and the test python file which not include in the pr ？

@a-santamaria
Copy link
Member Author

an you provide the CLI command az xxx and the test command azdev test xxx and the test python file which not include in the pr ？

yeah I just pushed the tests changes: fb8ab50#diff-bcbe83fb48caf55eb8ad02c2f23325d7b4c840f38414f68dbacf8cebb76990d6

the tests that fail with this are:
azdev test test_primary_nt_add_remove_node --live
azdev test test_add_secondary_node_type_add_remove_node --live

The command that works if I run outside of test is:
az sf cluster node add -g -c --node-type --number-of-nodes-to-add 2

it seems to me that the test connection is closing

@wangzelin007
Copy link
Member

an you provide the CLI command az xxx and the test command azdev test xxx and the test python file which not include in the pr ？

yeah I just pushed the tests changes: fb8ab50#diff-bcbe83fb48caf55eb8ad02c2f23325d7b4c840f38414f68dbacf8cebb76990d6

the tests that fail with this are: azdev test test_primary_nt_add_remove_node --live azdev test test_add_secondary_node_type_add_remove_node --live

The command that works if I run outside of test is: az sf cluster node add -g -c --node-type --number-of-nodes-to-add 2

it seems to me that the test connection is closing

Hi Alfredo,
This problem is very strange, I suggest that you can use @unittest.skip to skip tests first.
I have tried the following methods, but none of them work:

  1. add time.sleep().
  2. Increase connection_timeout and read_timeout.
  3. add --no-wait to az sf cluster node add/remove command to support no wait.
  4. add retry for self.cmd('az sf cluster node add')

It only fails when its run as a test...

@wangzelin007
Copy link
Member

Hi a-santamaria，
Please make sure to get this pr ready to merge before 2022/05/20.
Otherwise, this PR cannot catch up with the release of this sprint.
And the release time of the next sprint is 2022/07/05.

add tests
ed830be 
skip add/remove node on tests. failing with ConnectionResetError

fix tests and remove secrets

fix api on test record

fix api on test record 2

fix api on test record 3

fix api on test record 4

fix style

tests remove secrets
@a-santamaria
Copy link
Member Author

Thanks @wangzelin007 I fixed the tests and skipped the ones that have the issue. @zhoxing-ms please take a look to merge

@a-santamaria
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 22283 in repo Azure/azure-cli

@wangzelin007
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@zhoxing-ms zhoxing-ms changed the title {Service Fabric} Fix add node type and update methods to format begin_* #21697 [Service Fabric] az sf cluster node-type add: Fix the unexpected error that 'StorageAccountsOperations' object has no attribute 'create' May 19, 2022
@zhoxing-ms
Copy link
Contributor

Because this is a bugfix, it needs to be written into the release notes, so I modified the title and description of PR. @a-santamaria please help to see if it meets your expectation?

zhoxing-ms
zhoxing-ms reviewed May 19, 2022
View reviewed changes
src/azure-cli/azure/cli/command_modules/servicefabric/custom.py
@@ -376,16 +379,19 @@ def _add_common_name(cluster, is_admin, certificate_common_name, certificate_iss

patch_request = ClusterUpdateParameters(client_certificate_thumbprints=cluster.client_certificate_thumbprints,
client_certificate_common_names=cluster.client_certificate_common_names)
return client.update(resource_group_name, cluster_name, patch_request)
update_cluster_poll = client.begin_update(resource_group_name, cluster_name, patch_request)
return LongRunningOperation(cli_ctx)(update_cluster_poll)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is recommended to use sdk_no_wait instead of LongRunningOperation and support the --no-wait parameter.
However, you don't need to complete these in this PR, maybe you can submit a PR separately for this optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok thanks, I will work on this recommendation on a separate pr

@zhoxing-ms zhoxing-ms merged commit f54e4db into Azure:dev May 19, 2022
This was referenced May 19, 2022
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhoxing-ms zhoxing-ms zhoxing-ms approved these changes

@wangzelin007 wangzelin007 wangzelin007 approved these changes

@LukeSlev LukeSlev LukeSlev approved these changes

@jsntcy jsntcy Awaiting requested review from jsntcy jsntcy is a code owner

@yonzhan yonzhan Awaiting requested review from yonzhan

Assignees

@zhoxing-ms zhoxing-ms

Labels
Auto-Assign Auto assign by bot Service Fabric az sf
Projects
None yet
Milestone
May 2022 (2022-05-24) - For Build
Development

Successfully merging this pull request may close these issues.
6 participants
@a-santamaria @yonzhan @zhoxing-ms @navba-MSFT @wangzelin007 @LukeSlev