Skip to content

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

Merged
zhoxing-ms merged 3 commits intoAzure:devfrom
a-santamaria:fix_21697
May 19, 2022
Merged

[Service Fabric] az sf cluster node-type add: Fix the unexpected error that 'StorageAccountsOperations' object has no attribute 'create'#22283
zhoxing-ms merged 3 commits intoAzure:devfrom
a-santamaria:fix_21697

Conversation

@a-santamaria
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

zhoxing-ms commented May 7, 2022

Could you add some tests for these cases?

@navba-MSFT
Copy link
Copy Markdown
Contributor

navba-MSFT commented May 9, 2022

Could you add some tests for these cases?

@a-santamaria for visibility

@zhoxing-ms
Copy link
Copy Markdown
Contributor

zhoxing-ms commented May 13, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented May 13, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@navba-MSFT
Copy link
Copy Markdown
Contributor

navba-MSFT commented May 16, 2022

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
Copy Markdown
Member Author

a-santamaria commented May 16, 2022

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
Copy Markdown
Member

wangzelin007 commented May 17, 2022

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
Copy Markdown
Member Author

a-santamaria commented May 17, 2022

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
Copy Markdown
Member

wangzelin007 commented May 18, 2022

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
Copy Markdown
Member

wangzelin007 commented May 18, 2022

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
Copy Markdown
Member Author

a-santamaria commented May 18, 2022

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
Copy Markdown
Member Author

a-santamaria commented May 18, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented May 18, 2022

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

@wangzelin007
Copy link
Copy Markdown
Member

wangzelin007 commented May 19, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented May 19, 2022

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
Copy Markdown
Contributor

zhoxing-ms commented May 19, 2022

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
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
Copy Markdown
Contributor

@zhoxing-ms zhoxing-ms May 19, 2022

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
Copy Markdown
Member Author

@a-santamaria a-santamaria May 19, 2022

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

@jsntcy jsntcy Awaiting requested review from jsntcy

@yonzhan yonzhan Awaiting requested review from yonzhan

+1 more reviewer

@LukeSlev LukeSlev LukeSlev approved these changes

Reviewers whose approvals may not affect merge requirements

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