preparer resource caching framework#10126
Merged
KieranBrantnerMagee merged 15 commits intoAzure:masterfrom Mar 19, 2020
Merged
Conversation
…es use on the servicebus queue tests.
…ent resource is cached if a child is cached, otherwise an exception will be thrown before test run) - Aggregate cache key with parents, such that, for instance, a service bus underneath a EU RG won't be swapped for a service bus underneath an NA RG. (Previously only took into account child cache settings to look up the child in the cache) - Switch cache usage in test classes to use decorators for ease, add helper to resource group preparer. - Clean up logging and documentation, tweaks to ensure pylint is happy. - Add azure-devtools to dev-requirements - Remove spurious delete cleanup code.
remove the need for a distinct cache_pending_deletes structure update servicebus dev requirements add random_name_enabled to resource_testcase cached helper wrapper. fix bug in how trimmed_kwargs are used such that we do not trim the kwargs that we pass to deletion, since we may remove elements needed for removal.
KieranBrantnerMagee
added
the
Client
KieranBrantnerMagee
requested review from
YijunXieMS,
annatisch,
bryevdv,
chlowell,
iscai-msft,
kristapratico,
lmazuel,
rakshith91,
xiangyan99 and
yunhaoling
|
Can one of the admins verify this patch? |
lmazuel
reviewed
Mar 10, 2020
tools/azure-devtools/src/azure_devtools/scenario_tests/preparers.py
Outdated
Show resolved
Hide resolved
lmazuel
previously approved these changes
Mar 10, 2020
… into creation failure exceptions to compensate, as the primary goal of the additional logging was to ensure users saw all possible details of what is typically a Critical Failure.
annatisch
previously approved these changes
Mar 12, 2020
lmazuel
previously approved these changes
Mar 13, 2020
…ke preparer exception catching more comprehensive
KieranBrantnerMagee
dismissed stale reviews from lmazuel and annatisch
via
7ce38a9
annatisch
previously approved these changes
Mar 17, 2020
Member
|
I've read this over and believe it works as advertised. I think test coverage would be helpful to future maintainers who never knew, or have forgotten, how this is supposed to work. |
Member
Author
Agreed. Made an issue against myself to add this #10351 Don't hesitate to shout if you would rather pend this PR on that, otherwise I'm inclined to do this incrementally since this unblocks other work. Edit: Per our OOB discussion, there's enough concern around the complexity of this codebase that we're going to add tests. Sorry for incurring another round and more mailspam reviewers; thanks in advance! >< |
…order to try and emulate as closely as possible the first-in-last-out deletion behavior from non-cached resources.
chlowell
previously approved these changes
Mar 18, 2020
tools/azure-devtools/src/azure_devtools/scenario_tests/tests/test_preparer_order.py
Outdated
Show resolved
Hide resolved
tools/azure-devtools/src/azure_devtools/scenario_tests/tests/test_preparer_order.py
Outdated
Show resolved
Hide resolved
lmazuel
approved these changes
Mar 18, 2020
KieranBrantnerMagee
added a commit
to KieranBrantnerMagee/azure-sdk-for-python
that referenced
this pull request
Mar 26, 2020
* Initial commit of preparer resource caching functionality. Demonstrates use on the servicebus queue tests. This will allow tests that opt-in to utilize resources cached from previous equivalent resource creation steps, to improve test runtime. Deletion is delayed until the end of all pending tests, via a session fixture. * Inferred caching is currently disabled. (You have to explicitly declare a parent resource is cached if a child is cached, otherwise an exception will be thrown before test run) * Aggregates cache key with parents, such that, for instance, a service bus underneath a EU RG won't be swapped for a service bus underneath an NA RG. * Switch cache usage in test classes to use decorators for ease, add helper to resource group preparer. * Clean up preparer logging and documentation, tweaks to ensure pylint is happy. * Add azure-devtools to servicebus dev-requirements (to allow local test runs to properly consume preparer logic) * add random_name_enabled to resource_testcase cached helper wrapper so that this functionality is leveraged by default (solves a lot of engsys problems with resource collision) * fix bug in how trimmed_kwargs are used such that we do not trim the kwargs that we pass to deletion, since we may remove elements needed for removal. * Move resource cache cleanup into base conftest * Default preparer logging to warn and above to ensure visibility of potential failures. * Preparer logging is not on by default, but pushed more detail into creation failure exceptions to compensate, as the primary goal of the additional logging was to ensure users saw all possible details of what is typically a Critical Failure. * Add longer timeout to tests.yml to support still-lengthy sb tests, make preparer exception catching more comprehensive * Add unit tests for the caching preparer. Reverse the deferred delete order to try and emulate as closely as possible the first-in-last-out deletion behavior from non-cached resources.
yunhaoling
added a commit
that referenced
this pull request
Apr 3, 2020
* draft track2 sync client implementation * add retry and backoff for send and receive * improve retry and implement iterator for receiver * improve sync implementation * draft ClientBaseAsync and SenderClientAsync implementation * improve structure and implement async receiver client * add receiver client file * sync version constructor improvement * add sync samples * async implementation * add async sample * refactor mixin and implement receiving deferred message * add receiving defer message sample * adjust method position * optimize async and implement receive deferred message * adjust backoff method name * Update readme for track 2 preview 1 release * add sync client docstring * Convert to the modernized README structure. * async client docstring * Remove en-us localized links take inspiration from EventHubs readme for primary links, and logging and feedback subsections. Adjust version ID Add changelog stanza stub Adjust samples readme Remove top level create_queue and troubleshooting-for-that-endpoint mentions for now until that's sorted. * Remove 3.4 from acceptable python version list Improve readme env var naming clarity, as well as cloud shell CLI verbiage. * introduce new top level servicebus client, rename old client design, update samples * split message types * Add python 3.8 to acceptable versions * Fix broken changelog URL markdown was brackets, needed to be parens. * update retry part * add mgmt retry and update get_xxx factory methods * add renew_lock back to deferred message * remove track1 client/sender/receiver * fix bug in sample code and logging * remove t1 sample code * fix pylint * update __init__.py to relative reference * add new docstring sample * update docstring for the code * move session id to message * bring back the hotfix for ReceiveAndDetele * send batch feature * add peek back and fix pylint * connection sharing in top level client * open handler in the mgmt request * merge deferred message into received message * draft schedule and cancel implementation * Initial test conversion work, port sync tests, note failures with TODOS, fix bugs elsewhere. (settlement mode plumbing, idle_timeout plumbing, session_id plumbing, add reconnect to handler, expose AutoReconnect) * update common to _common and update module import accordingly * update according to reviews * update code according to comments * session receive support * Remove spurious comments into a formal doc, clean up tests and imports, preliminary session test porting. * add ServiceBusSession support * Adjust with exception statements to the new and improved throw-at-client-init. 108 Remove spurious TODOS from sb_client into a proper doc. * reenable batching * Make doc review comments to readme; adding links for key concepts, readding troubleshooting and putting logging under it, fixing URIs, adjusting header levels, and making samples fully copy-pastable * improve connection sharing for entity-specific conn str, improve session related function * add retry for iterator * Make PR comments. Change deadletter sample to defer remove some EH bits left over. Have migration guide include batch logic. Add documentation to point to portal namespace creation. * specify error type when entity mismatch and specify auth error in create_exception * Adjust code post-merge to accomodate new path names and package structure * Fix variable naming bugs, put missing properties on servicebussession and access them properly, adjust session tests accordingly left notes of failures (these will have to be removed for final PR) * fix bug when resetting the error * idle timeout fix * fix message settled not being set properly * fix bug in session and improve error handling * add session_id into message constructor * revert to separate connection * fix several bugs in session implementation and error handling * add session expiration check * Adjust tests after latest T2 changes, including making a public accessor for locked_until, catching some exceptions if a session isn't present on a receiver, and tweaking exception types and property paths. * add comment for session locked until property * preparer resource caching framework (#10126) * Initial commit of preparer resource caching functionality. Demonstrates use on the servicebus queue tests. This will allow tests that opt-in to utilize resources cached from previous equivalent resource creation steps, to improve test runtime. Deletion is delayed until the end of all pending tests, via a session fixture. * Inferred caching is currently disabled. (You have to explicitly declare a parent resource is cached if a child is cached, otherwise an exception will be thrown before test run) * Aggregates cache key with parents, such that, for instance, a service bus underneath a EU RG won't be swapped for a service bus underneath an NA RG. * Switch cache usage in test classes to use decorators for ease, add helper to resource group preparer. * Clean up preparer logging and documentation, tweaks to ensure pylint is happy. * Add azure-devtools to servicebus dev-requirements (to allow local test runs to properly consume preparer logic) * add random_name_enabled to resource_testcase cached helper wrapper so that this functionality is leveraged by default (solves a lot of engsys problems with resource collision) * fix bug in how trimmed_kwargs are used such that we do not trim the kwargs that we pass to deletion, since we may remove elements needed for removal. * Move resource cache cleanup into base conftest * Default preparer logging to warn and above to ensure visibility of potential failures. * Preparer logging is not on by default, but pushed more detail into creation failure exceptions to compensate, as the primary goal of the additional logging was to ensure users saw all possible details of what is typically a Critical Failure. * Add longer timeout to tests.yml to support still-lengthy sb tests, make preparer exception catching more comprehensive * Add unit tests for the caching preparer. Reverse the deferred delete order to try and emulate as closely as possible the first-in-last-out deletion behavior from non-cached resources. * Enable preparers to implicitly handle async tests (#10394) * Add a clause in the preparer to attempt to consume fn as an async coroutine, falling back to synchronous if not. This allows use of preparers without an additional decorator to wrap the function. * Add unit tests for the preparer async handling capabilities * Add an additional exception check to the async fallback, for certain deployments of 2.7 where this is the response when trying to import asyncio that has been pip installed. * Port storage async tests to give an example of the new async preparer working on another test suite. * add prefech back * fix check session expiry error and align the behavior of prefetch with T1 * fix bug in async receiver close * begin converting async queue tests to T2 make small fixes to repair flaky test_queue tests * Add reconnect to async base handler Begin porting async tests small tweaks to sync tests to adjust according to the base sdk changes * remove test skip on now-passing test. * move _open from get_receiver/sender into contextmgr * Tweak test notes * enable test to be cached in async queue test * normalize caching preparers, and change session->session_id * update samples and code snippets * Fix async tests (batch addition, running/open naming) and enable logging to try and catch memory access failure. * fix bug when a handler dont get closed if it failed to open * continue removing todos as more tests become functional. * orphan message error type changed from ValueError to MessageSettleFailed * Update exception catches with latest changes. Use open_with_retry rather than just open to get proper "close-on-failure" behavior * Move utility methods into separate file. Write new stress testing infra that ties into pytest/preparers. convert existing stress tests (reconnecttest still pending) * PR comments change debug -> logging_enable Remove reconnect, is now implicit. * Fix small lingering bugs in stress_test, make duration overrideable from command line, remove historical stress tests now that they're ported. * improve stress test result printout * fix ci bug * fix syntax error * bump version to v7 and fix syntax error in py3.5 * Adjust version to 7 Bolster migration guide Flesh out changelog with features vs breaking changes, long-form. * Add additional detail to pending-features list in changelog * Adjust sample readme product name * add utc suffix to time relevant variables and parameters, fix pylint and mypy, move unused code * change MessageSettleFailed to MessageLockExpired where appropriate * add reson for dead_letter according to service spec * constify strings * more string constifying * add ivar docstring and remove async message lock token * fix bug in kwargs and improve from_connection_string implementation * add docstring, update async_message to be private module * convert tests to timezone aware datetimes ensure Azure capitalization in docs Correct changelog verbiagei * make control_client module private * remove aio module duplicates * rename timeout to max_wait_time in receive method * Make test timezone usage be py27 compatible via consuming the util utc_now helper. Fix constant MGMT_RESPONSE_EXPIRATION expiration->expirations * update control_client to _contral_client * rename message_timeout to timeout in send to align with EH * make queue_name and topic_name docstrings more explicit about collision case. * Message expiration and Receiver expiration are subtlely different constants, the former being pluralized. * Readd buildtargetingstring that was removed in a merge Remove duplicate setcache call in sb preparer * set logging_enable=False by default for tests to not mess up OSX * update migration_guide * remove additional dev_requirement from azure-mgmt-servicebus Co-authored-by: Kieran Brantner-Magee <kibrantn@microsoft.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(Pre-Note: Excuse the exceedingly wide net I threw for reviewers, feel free to ignore or add others as relevant, consider this both a "for your attention and consumption" to try and get coverage among all of the current SDKs and out of paranoia for what could be a widely impactful change if there are sharp edges or special cases I'm not aware of.)
This PR adds automatic resource caching capabilities within the preparer framework.
Individual preparers must utilize the set_cache function to define their cache key, the set of parameters that makes a resource unique. (This might be SKU, or region, some flags or combinations thereof)
If a preparer is cached, all parent preparers must declare themselves cached as well, or an error will be thrown. This is to enforce rigorous opt-in and prevent misunderstanding of the caching functionality, wherein a parent resource must necessarily be cached for situations where it contains the child, e.g. resource groups.
There is the added constraint that a child's cache lookup is based not only on its own parameters, but on those of its parents, such that for instance a queue within a premium SKU servicebus won't substitute for a queue within a normal SKU servicebus, even if the queue parameters are the same. This has the consequence that if your preparer stack has indirection (e.g. RG->Keyvault->Servicebus vs RG->Servicebus->Keyvault ) it will not cache even if it were theoretically possible since the cache has no knowledge of Azure resource relationships beyond what can be assumed from preparer ordering.
Resources that are cached are torn down at the end of the test run via a session fixture.
Additionally fixes a small bug in how KWargs are trimmed, ensuring un-trimmed args are passed to removal so that one doesn't run the risk of removing args required for removal if the test function doesn't require them.