Fix: Race between layer and Lambda update (#5927) #6259
Conversation
This file contains 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6259 +/- ##
===========================================
- Coverage 85.38% 85.35% -0.03%
===========================================
Files 155 155
Lines 20735 20743 +8
===========================================
+ Hits 17704 17706 +2
- Misses 3031 3037 +6 ☔ View full report in Codecov by Sentry. |
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/5927-layer-and-lambda-race
branch
2 times, most recently
from
1de4271
to
6275fec
Compare
|
|
It seems AWS keeps an internal counter of the published version of each Lambda, so even if the published version(s) are deleted before the deploy, the next published version will be +1 larger than the last. Since this is the case, there seems no benefit in running the delete prior to the deploy as opposed to after, so I opted for the latter. |
nadove-ucsc
requested changes
May 16, 2024
src/azul/terraform.py
Outdated
Comment on lines
712
to
713
| # race conditions when an update to the function's configuration and | ||
| # code rely on the update of each other in order to work correctly. |
There was a problem hiding this comment.
Suggested change
| # race conditions when an update to the function's configuration and | |
| # code rely on the update of each other in order to work correctly. | |
| # race conditions when there's a cyclic dependency between an update | |
| # to the function's configuration and an update to its code |
There was a problem hiding this comment.
I think my way is easier to read, assuming I've correctly parsed the intent here
scripts/delete_lambda_versions.py
Outdated
Comment on lines
28
to
35
| if version['Version'] == '$LATEST': | ||
| pass | ||
| else: | ||
| version_number = version['Version'] | ||
| log.info('Deleting published version %s of %s', version_number, lambda_.name) | ||
| aws.lambda_.delete_function( | ||
| FunctionName=lambda_.name, | ||
| Qualifier=version_number | ||
| ) |
There was a problem hiding this comment.
Suggested change
| if version['Version'] == '$LATEST': | |
| pass | |
| else: | |
| version_number = version['Version'] | |
| log.info('Deleting published version %s of %s', version_number, lambda_.name) | |
| aws.lambda_.delete_function( | |
| FunctionName=lambda_.name, | |
| Qualifier=version_number | |
| ) | |
| version = version['Version'] | |
| if version == '$LATEST': | |
| pass | |
| else: | |
| log.info('Deleting published version %s of %s', version, lambda_.name) | |
| aws.lambda_.delete_function( | |
| FunctionName=lambda_.name, | |
| Qualifier=version | |
| ) |
There was a problem hiding this comment.
I think it's better to reuse the version variable instead of introducing a new one (especially since I'm confused about the difference in their names). Also, we can assign to it earlier to avoid repeating the dictionary lookup.
scripts/delete_lambda_versions.py
Outdated
Comment on lines
24
to
26
| response = aws.lambda_.list_versions_by_function( | ||
| FunctionName=lambda_.name | ||
| ) |
There was a problem hiding this comment.
Fits on one line
Suggested change
| response = aws.lambda_.list_versions_by_function( | |
| FunctionName=lambda_.name | |
| ) | |
| response = aws.lambda_.list_versions_by_function(FunctionName=lambda_.name) |
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/5927-layer-and-lambda-race
branch
from
6275fec
to
e5d5938
Compare
|
nadove-ucsc
requested changes
May 17, 2024
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/5927-layer-and-lambda-race
branch
from
e5d5938
to
d126563
Compare
nadove-ucsc
requested changes
May 17, 2024
src/azul/lambdas.py
Outdated
| return [ | ||
| Lambda.from_response(function) | ||
| for response in self._lambda.get_paginator('list_functions').paginate() | ||
| for function in response['Functions'] | ||
| if deployment_only is False or deployment(function) == config.deployment_stage |
There was a problem hiding this comment.
Suggested change
| if deployment_only is False or deployment(function) == config.deployment_stage | |
| if not deployment_only or deployment(function) == config.deployment_stage |
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/5927-layer-and-lambda-race
branch
from
d126563
to
184a0e0
Compare
nadove-ucsc
previously approved these changes
May 17, 2024
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/5927-layer-and-lambda-race
branch
from
c04e0bc
to
fd98952
Compare
manage_lambdas.pymanage_queues.py purge_all |
hannes-ucsc
reviewed
Aug 22, 2024
There was a problem hiding this comment.
I did not ask for any code changes in my previous review, only for additional evidence, yet the code was changed. Was this intentional, and if so, why?
|
Yes, a fixup commit was added to re-combine the two Background: When I refactored the code out of When testing This prompted me to take another look at the two |
hannes-ucsc
requested changes
Aug 24, 2024
src/azul/lambdas.py
Outdated
| all_versions: bool = False | ||
| ) -> list[Lambda]: | ||
| """ | ||
| Return a list of AWS Lambda functions |
There was a problem hiding this comment.
Suggested change
| Return a list of AWS Lambda functions | |
| Return a list of all AWS Lambda functions (or function versions) in the current account, or the given deployment. |
src/azul/lambdas.py
Outdated
| for lambda_name in [metadata['FunctionName'] for metadata in lambda_page['Functions']]: | ||
| if any(lambda_name.startswith(prefix) for prefix in lambda_prefixes): | ||
| self.manage_lambda(lambda_name, enabled) | ||
| for lambda_ in self.list_lambdas(deployment=config.deployment_stage): |
There was a problem hiding this comment.
Suggested change
| for lambda_ in self.list_lambdas(deployment=config.deployment_stage): | |
| for function in self.list_lambdas(deployment=config.deployment_stage): |
As noted before, we are phasing this term out. You don't have to remove all usages but just don't add new ones.
src/azul/lambdas.py
Outdated
|
|
||
| def delete_stale_function_versions(self): | ||
| """ | ||
| Delete all but the latest published version of every AWS Lambda function |
There was a problem hiding this comment.
Suggested change
| Delete all but the latest published version of every AWS Lambda function | |
| Delete all but the latest published version of every AWS Lambda function |
Can there be more than one published version? If not, "the latest" qualifier seems redundant.
There was a problem hiding this comment.
Can there be more than one published version?
Yes, however I was using the term "published" incorrectly. In AWS terminology a published version is a copy of the unpublished ($LATEST) version. There can be many published versions (each with a unique version number), but only one unpublished version.
https://docs.aws.amazon.com/lambda/latest/dg/configuration-versions.html
In my latest fixup commit I've changed the name and docstring of this method accordingly.
There was a problem hiding this comment.
Then I believe either the deletion logic is wrong or the publish TF functionality doesn't work. One of the published versions must be live so the logic below deletes it, or the latest version is live which raises the question as to why we published an immutable version.
There was a problem hiding this comment.
I might of misunderstood your original question. There can be multiple published versions of a function in the sense that AWS allows it, however with this PR there will only ever be at most one published version, along with the unpublished version.
Both the published and unpublished versions are live, however only the unpublished version is used since our code does not qualify the function ARN with a version. https://docs.aws.amazon.com/lambda/latest/dg/configuration-versions.html#versioning-versions-using
There was a problem hiding this comment.
Still don't understand it. Please raise in PL.
src/azul/lambdas.py
Outdated
| paginator = self._lambda.get_paginator('list_functions') | ||
| lambda_prefixes = [ | ||
| config.qualified_resource_name(lambda_infix, stage=deployment) | ||
| for lambda_infix in config.lambda_names() |
There was a problem hiding this comment.
Suggested change
| for lambda_infix in config.lambda_names() | |
| for lambda_name in config.lambda_names() |
src/azul/lambdas.py
Outdated
| lambda_prefixes = [ | ||
| config.qualified_resource_name(lambda_infix, stage=deployment) | ||
| for lambda_infix in config.lambda_names() | ||
| if deployment is not None |
There was a problem hiding this comment.
In larger constructs I see the point of a constant comprehension guard, but here no.
src/azul/lambdas.py
Outdated
| for function in response['Functions'] | ||
| if deployment is None or any( |
There was a problem hiding this comment.
Suggested change
| if deployment is None or any( | |
| if lambda_prefixes is None or any( |
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/5927-layer-and-lambda-race
branch
from
fd98952
to
d21db43
Compare
hannes-ucsc
requested changes
Aug 28, 2024
src/azul/lambdas.py
Outdated
|
|
||
| def delete_stale_function_versions(self): | ||
| """ | ||
| Delete all but the latest published version of every AWS Lambda function |
There was a problem hiding this comment.
Then I believe either the deletion logic is wrong or the publish TF functionality doesn't work. One of the published versions must be live so the logic below deletes it, or the latest version is live which raises the question as to why we published an immutable version.
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/5927-layer-and-lambda-race
branch
from
d21db43
to
2567ecb
Compare
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/5927-layer-and-lambda-race
branch
from
2567ecb
to
0ba2a94
Compare
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/5927-layer-and-lambda-race
branch
from
0ba2a94
to
f612650
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Connected issues: #5927
Checklist
Author
developissues/<GitHub handle of author>/<issue#>-<slug>1 when the issue title describes a problem, the corresponding PR
title is
Fix:followed by the issue titleAuthor (partiality)
ptag to titles of partial commitspartialor completely resolves all connected issuespartiallabelAuthor (chains)
baseor this PR is not chained to another PRchainedor is not chained to another PRAuthor (reindex, API changes)
rtag to commit title or the changes introduced by this PR will not require reindexing of any deploymentreindex:devor the changes introduced by it will not require reindexing ofdevreindex:anvildevor the changes introduced by it will not require reindexing ofanvildevreindex:anvilprodor the changes introduced by it will not require reindexing ofanvilprodreindex:prodor the changes introduced by it will not require reindexing ofprodreindex:partialand its description documents the specific reindexing procedure fordev,anvildev,anvilprodandprodor requires a full reindex or carries none of the labelsreindex:dev,reindex:anvildev,reindex:anvilprodandreindex:prodAPIor this PR does not modify a REST APIa(A) tag to commit title for backwards (in)compatible changes or this PR does not modify a REST APIapp.pyor this PR does not modify a REST APIAuthor (upgrading deployments)
make image_manifests.jsonand committed the resulting changes or this PR does not modifyazul_docker_images, or any other variables referenced in the definition of that variableutag to commit title or this PR does not require upgrading deploymentsupgradeor does not require upgrading deploymentsdeploy:sharedor does not modifyimage_manifests.json, and does not require deploying thesharedcomponent for any other reasondeploy:gitlabor does not require deploying thegitlabcomponentdeploy:runneror does not require deploying therunnerimageAuthor (hotfixes)
Ftag to main commit title or this PR does not include permanent fix for a temporary hotfixanvilprodandprod) have temporary hotfixes for any of the issues connected to this PRAuthor (before every review)
develop, squashed old fixupsmake requirements_updateor this PR does not modifyrequirements*.txt,common.mk,MakefileandDockerfileRtag to commit title or this PR does not modifyrequirements*.txtreqsor does not modifyrequirements*.txtmake integration_testpasses in personal deployment or this PR does not modify functionality that could affect the IT outcomePeer reviewer (after approval)
System administrator (after approval)
demoorno demono demono sandboxN reviewslabel is accurateOperator (before pushing merge the commit)
reindex:…labels andrcommit title tagno demodevelop_select dev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select dev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlab_select anvildev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select anvildev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlabdeploy:gitlabdeploy:gitlabSystem administrator
dev.gitlabare complete or this PR is not labeleddeploy:gitlabanvildev.gitlabare complete or this PR is not labeleddeploy:gitlabOperator (before pushing merge the commit)
_select dev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runner_select anvildev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runnersandboxlabel or PR is labeledno sandboxdevor PR is labeledno sandboxanvildevor PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxor this PR does not remove catalogs or otherwise causes unreferenced indices indevanvilboxor this PR does not remove catalogs or otherwise causes unreferenced indices inanvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevpif the PR is also labeledpartialOperator (chain shortening)
developor this PR is not labeledbasechainedlabel from the blocked PR or this PR is not labeledbasebasebaselabel from this PR or this PR is not labeledbaseOperator (after pushing the merge commit)
devanvildevdevdevanvildevanvildev_select dev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shared_select anvildev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shareddevanvildevOperator (reindex)
devor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevOperator
deploy:shared,deploy:gitlab,deploy:runner,reindex:partial,reindex:anvilprodandreindex:prodlabels to the next promotion PRs or this PR carries none of these labelsdeploy:shared,deploy:gitlab,deploy:runner,reindex:partial,reindex:anvilprodandreindex:prodlabels, from the description of this PR to that of the next promotion PRs or this PR carries none of these labelsShorthand for review comments
Lline is too longWline wrapping is wrongQbad quotesFother formatting problem