Skip to content

Conversation

@PoeppingT
Copy link
Contributor

This PR consists migrates the existing Installer update logic to an enhanced version.

The original update implementation would blindly update every component of SaaS Boost: re-uploading all resources and running a complete CloudFormation update. In the case of a complicated SaaS Boost installation this could take dozens of minutes to run an update when only a small update was required (e.g. a small update to the Onboarding service would end up updating every service).

The new enhanced update implementation relies on git and the pre-existing Version parameters to determine exactly what changed, only updating what is necessary. In particular this leverages the pre-existing update scripts for each service, function, layer, and custom resource (now all aligned to be named update.sh). In practice this means the average update should take significantly less time than before.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@PoeppingT PoeppingT requested a review from brtrvn September 12, 2022 19:03
@PoeppingT PoeppingT changed the title Instaler update enhancements Installer update enhancements Sep 12, 2022
Copy link
Contributor

@brtrvn brtrvn left a comment

Choose a reason for hiding this comment

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

For the update.sh scripts you've added to the custom resources folder, I don't know that we need to do a query against Lambda (with list-functions) vs just specifying the function name like we do in the scripts in the functions folder.

Also, in the UpdateWorkflow, for any Lambda that might be completely new, can we trust (have we tested) that the shell scripts will properly no-op a call to update-function-code if it doesn't exist?

In AwsClientBuilderFactory we "cache" the SDK client instances. This implies that we may want to clear the cached instance or start over. There doesn't appear to be a way to do that without building a new Factory. Was this intentional? Are they really "cached" then or just instance variables?

getUpdateActionsFromPaths seems a bit fragile with magic numbers. Maybe just some more comments on exactly what's going on and what the expectation is?

@PoeppingT
Copy link
Contributor Author

For the update.sh scripts you've added to the custom resources folder, I don't know that we need to do a query against Lambda (with list-functions) vs just specifying the function name like we do in the scripts in the functions folder.

Also, in the UpdateWorkflow, for any Lambda that might be completely new, can we trust (have we tested) that the shell scripts will properly no-op a call to update-function-code if it doesn't exist?

In AwsClientBuilderFactory we "cache" the SDK client instances. This implies that we may want to clear the cached instance or start over. There doesn't appear to be a way to do that without building a new Factory. Was this intentional? Are they really "cached" then or just instance variables?

getUpdateActionsFromPaths seems a bit fragile with magic numbers. Maybe just some more comments on exactly what's going on and what the expectation is?

  1. Good point: I was being a little shotgun blasty with copy-paste. I'll do a second pass through before posting the next revision.
  2. I haven't tested this, I'll double check to be sure. In completely new Lambda code however we have to assume that the update script will be properly written. I'll test with a copy of a functioning update script for an existing Lambda.
  3. Yeah, the intention was to save the object creation cost. Happy to rename if you don't like cache
  4. I'll add more detail to the getUpdateActionsFromPaths algorithm in the next revision.

@PoeppingT
Copy link
Contributor Author

Also, in the UpdateWorkflow, for any Lambda that might be completely new, can we trust (have we tested) that the shell scripts will properly no-op a call to update-function-code if it doesn't exist?

  1. I haven't tested this, I'll double check to be sure. In completely new Lambda code however we have to assume that the update script will be properly written. I'll test with a copy of a functioning update script for an existing Lambda.

Just checked and if no functions already exist the list-functions call will return no results (with an exit code of zero)

OSX-poeppt ♍︎ aws lambda list-functions --query 'Functions[?starts_with(FunctionName, `sb-test-newservice-`)] | [].FunctionName' --output text
(22-09-14 14:49:17) <0> [~]
OSX-poeppt ♍︎

Which means that no update-function-code call would be made at all.

This actually provides an argument against direct updating of the function code in custom-resources and functions update scripts, since in a net-new situation those would not exist. But even still, an update-function-code call to a non-existent function just errors out, effectively doing a no-op:

OSX-poeppt ♍︎ aws lambda update-function-code --function-name "non-existent-function" --s3-bucket "poeppt" --s3-key "blahkey"

An error occurred (ResourceNotFoundException) when calling the UpdateFunctionCode operation: Function not found: arn:aws:lambda:us-west-2:786938756705:function:non-existent-function

So in either case it's no problem: when the CloudFormation stack is updated (with the new template, if it's been added) those functions will be created using the code uploaded to the proper S3 location by the update.sh script.

Copy link
Contributor

@brtrvn brtrvn left a comment

Choose a reason for hiding this comment

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

The upside of running the update.sh scripts for Lambda code packages that are brand new before running the CloudFormation stack update is that it will compile and then upload the code package to S3 where CloudFormation expects it to be.

Copy link
Contributor

@brtrvn brtrvn left a comment

Choose a reason for hiding this comment

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

Changes look great

@brtrvn brtrvn merged commit ebe713c into awslabs:main Sep 15, 2022
@brtrvn brtrvn mentioned this pull request Sep 29, 2022
2 tasks
@PoeppingT PoeppingT deleted the instaler-update-enhancements branch October 10, 2022 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants