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

{Redis} Add support for 2022-06-01 API version #24979

Merged
merged 13 commits into from
Jan 3, 2023
Merged

{Redis} Add support for 2022-06-01 API version #24979

merged 13 commits into from
Jan 3, 2023

Conversation

austintolani
Copy link
Contributor

@austintolani austintolani commented Dec 24, 2022

Related command

az redis

Description

This PR adds supports for a new API version for Azure Cache for Redis (2022-06-01). Specifically, these changes include:

  • Upgrading azure-mgmt-redis package to new version (14.1.0), which updates all requests to use the new 2022-06-01 API version
  • Add a new optional parameter, --preferred-data-archive-method to az redis import and az redis export operation.
  • Linked server deletion (az redis server-link delete) is now a long running operation so it now calls begin_delete instead of delete. Linked server deletion behavior is unchanged for the user.
  • Updating a cache (az redis update) is now a long running operation so it now calls begin_update instead of update. Implemented a customer setter function so as not to break current behavior.
  • A linked server has two new properties: geoReplicatedPrimaryHostName and primaryHostName.
  • Relevant updates to help text
  • Relevant updates to tests

Testing Guide

Updating a cache (example below shows updating the SKU):

az redis update --name MyRedisCache --resource-group MyResourceGroup --set "sku.capacity"="2"

Linked server operations:

az redis create -n {primaryName} -g {rg} -l {location} --sku {sku} --vm-size {size} # create primary
az redis create -n {secondaryName} -g {rg} -l {location} --sku {sku} --vm-size {size} # create secondary

az redis server-link create -n {primaryName} -g {rg} --replication-role Secondary --server-to-link {secondaryName} # link servers

az redis server-link show -n {primaryName} -g {rg} --linked-server-name {secondaryName} # see link details

az redis server-link delete -n {primaryName} -g {rg} --linked-server-name {secondaryName} # remove link

History Notes

[Redis] az redis import/export: Add new optional parameter --preferred-data-archive-method
[Redis] az redis server-link: Linked server has two new properties: geoReplicatedPrimaryHostName and primaryHostName


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

@ghost ghost added Redis Cache Auto-Assign Auto assign by bot labels Dec 24, 2022
@ghost ghost requested review from evelyn-ys, jsntcy and yonzhan December 24, 2022 16:17
@ghost ghost assigned evelyn-ys Dec 24, 2022
@ghost ghost requested review from zhoxing-ms and kairu-ms December 24, 2022 16:17
@austintolani austintolani marked this pull request as ready for review December 24, 2022 16:31
@austintolani austintolani requested a review from jiasli as a code owner December 24, 2022 16:31
@yonzhan yonzhan added this to the Dec 2022 (2023-01-10) milestone Dec 24, 2022
@yonzhan yonzhan requested a review from calvinhzy December 24, 2022 22:06
@yonzhan
Copy link
Collaborator

yonzhan commented Dec 24, 2022

Redis

@evelyn-ys
Copy link
Member

There're still two tests failing:

FAILED src/azure-cli/azure/cli/command_modules/network/tests/latest/test_private_endpoint_commands.py::NetworkPrivateLinkAzureCacheforRedisScenarioTest::test_private_endpoint_connection_acfr
FAILED src/azure-cli/azure/cli/command_modules/network/tests/latest/test_private_endpoint_commands.py::NetworkPrivateLinkAzureCacheforRedisScenarioTest::test_private_link_resource_acfr

Also pls fix the credential scanner failure

@austintolani
Copy link
Contributor Author

@evelyn-ys I have fixed the failing tests and credential scanning issue.

@evelyn-ys evelyn-ys merged commit 30724ca into Azure:dev Jan 3, 2023
@evelyn-ys evelyn-ys changed the title [Redis] Add support for 2022-06-01 API version {Redis} Add support for 2022-06-01 API version Jan 3, 2023
avgale pushed a commit to avgale/azure-cli that referenced this pull request Aug 24, 2023
* upgrade azure-mgmt-redis to 14.1.0

* add preferred_data_archive_auth_method param

* linked server updates

* update redisVersion description

* add sub for MI testing

* update MI test to use begin_update

* Changed update command to use begin_update as setter

* use custom update setter

* tests

* formatting and comments

* remove print statements

* remove credentials

* network test recordings
@@ -72,6 +74,11 @@ def cli_redis_update(cmd, instance, sku=None, vm_size=None):
return update_params


def custom_update_setter(client, resource_group_name, name, parameters):
# Custom update setter is used to match behavior from when update was not a LRO
return client.begin_update(resource_group_name, name, parameters).result(0)
Copy link
Member

@jiasli jiasli Jan 12, 2024

Choose a reason for hiding this comment

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

According to the definition of azure.core.polling._poller.LROPoller.result:

https://github.com/Azure/azure-sdk-for-python/blob/d678ef6e5ebe71bcc7bb7f3665d76576437f0121/sdk/core/azure-core/azure/core/polling/_poller.py#L242-L252

    def result(self, timeout: Optional[float] = None) -> PollingReturnType_co:
        """Return the result of the long running operation, or
        the result available after the specified timeout.


        :param float timeout: Period of time to wait before getting back control.
        :returns: The deserialized resource of the long running operation, if one is available.
        :rtype: any or None
        :raises ~azure.core.exceptions.HttpResponseError: Server problem with the query.
        """
        self.wait(timeout)
        return self._polling_method.resource()

The 0 in result(0) makes the LROPoller return immediately, causing intermittent CI failure:

https://dev.azure.com/azclitools/public/_build/results?buildId=121462&view=logs&j=038813f6-b85b-5e37-acf4-30b28352865f&t=48a22e66-ceab-5c7f-d00b-cb2046709dfd

2024-01-12T08:59:22.0016933Z =================================== FAILURES ===================================
2024-01-12T08:59:22.0017289Z ___________________ RedisCacheTests.test_redis_cache_update ____________________
2024-01-12T08:59:22.0019920Z [gw2] linux -- Python 3.11.5 /opt/az/bin/python3
2024-01-12T08:59:22.0020425Z /opt/az/lib/python3.11/site-packages/azure/cli/testsdk/base.py:145: in tearDown
2024-01-12T08:59:22.0020723Z     super(ScenarioTest, self).tearDown()
2024-01-12T08:59:22.0020984Z _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
2024-01-12T08:59:22.0021110Z 
2024-01-12T08:59:22.0021390Z self = <command_modules.redis.tests.latest.test_redis_scenario.RedisCacheTests testMethod=test_redis_cache_update>
2024-01-12T08:59:22.0021580Z 
2024-01-12T08:59:22.0021788Z     def tearDown(self):
2024-01-12T08:59:22.0022010Z         os.environ = self.original_env
2024-01-12T08:59:22.0022239Z         # Autorest.Python 2.x
2024-01-12T08:59:22.0022527Z         assert not [t for t in threading.enumerate() if t.name.startswith("AzureOperationPoller")], \
2024-01-12T08:59:22.0022994Z             "You need to call 'result' or 'wait' on all AzureOperationPoller you have created"
2024-01-12T08:59:22.0023274Z         # Autorest.Python 3.x
2024-01-12T08:59:22.0027370Z >       assert not [t for t in threading.enumerate() if t.name.startswith("LROPoller")], \
2024-01-12T08:59:22.0028218Z             "You need to call 'result' or 'wait' on all LROPoller you have created"
2024-01-12T08:59:22.0028728Z E       AssertionError: You need to call 'result' or 'wait' on all LROPoller you have created
2024-01-12T08:59:22.0028890Z 
2024-01-12T08:59:22.0029292Z /opt/az/lib/python3.11/site-packages/azure/cli/testsdk/scenario_tests/base.py:160: AssertionError
2024-01-12T08:59:22.0029744Z ------------- generated xml file: /azure_cli_test_result/redis.xml -------------
2024-01-12T08:59:22.0030074Z =========================== short test summary info ============================
2024-01-12T08:59:22.0030422Z FAILED tests/latest/test_redis_scenario.py::RedisCacheTests::test_redis_cache_update

No timeout should be given. result() should be used instead, such as:

redis_resourse = client.begin_update(resource_group_name, cache_name, update_params).result()

Copy link
Member

@evelyn-ys evelyn-ys Jan 15, 2024

Choose a reason for hiding this comment

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

Per the PR description:

Updating a cache (az redis update) is now a long running operation so it now calls begin_update instead of update. Implemented a customer setter function so as not to break current behavior.

I think it's be design to make the operation returning immediately. But they should use sdk_no_wait instead of result(0)

Copy link
Member

Choose a reason for hiding this comment

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

sdk_no_wait turns off polling:

def sdk_no_wait(no_wait, func, *args, **kwargs):
if no_wait:
kwargs.update({'polling': False})
return func(*args, **kwargs)

but result(0) doesn't. result(0) will make the result returned immediately, but SDK still keeps polling in the background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jiasli and @evelyn-ys, thanks for your work identifying this issue. Is the expectation that the service team make this change or is this going to be handled by the CLI team?

Copy link
Member

Choose a reason for hiding this comment

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

appreciated that @austintolani you can create another PR to fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay thanks @evelyn-ys. I am no longer on the Redis team so I don't think it's appropriate for me to make this change but I have forwarded this information to the Redis sub-team that owns the CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot Redis Cache
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants