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

Remove DescribeStacks call from prepare_stack_for_update #529

Merged
merged 2 commits into from
Feb 9, 2018

Conversation

ejholmes
Copy link
Contributor

@ejholmes ejholmes commented Feb 7, 2018

This is a small performance optimization to remove an unnecessary DescribeStacks call from the prepare_stack_for_update method.

Before this change, the outbound network calls for a single noop stack update would look like this:

  1. DescribeStacks: Check if stack is created or not, and to build a list of parameters
  2. DescribeStacks * N: Resolve any output lookups from other stacks
  3. HeadObject: Check if the template has already been uploaded.
  4. DescribeStacks: Check if the stack is in a rollback_complete state, and needs to be deleted)
  5. UpdateStack: Perform the update.

The 3rd DescribeStacks (step 4) is unnecessary, and can be easily removed, since we already have a DescribeStacks response from the first call.

@ejholmes
Copy link
Contributor Author

ejholmes commented Feb 7, 2018

@danielkza since you originally added prepare_stack_for_update in #473, do you mind sanity checking this as well?

@ejholmes ejholmes mentioned this pull request Feb 7, 2018
4 tasks
Copy link
Member

@phobologic phobologic left a comment

Choose a reason for hiding this comment

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

This LGTM - have you ran the integration tests? Probably worthwhile just in case.

@@ -694,7 +694,7 @@ def select_update_method(self, force_interactive, force_change_set):
else:
return self.default_update_stack

def prepare_stack_for_update(self, fqn, tags):
Copy link
Member

Choose a reason for hiding this comment

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

Need to update the docstring.

@ejholmes
Copy link
Contributor Author

ejholmes commented Feb 7, 2018

have you ran the integration tests?

Yep, integration tests pass.

@codecov-io
Copy link

Codecov Report

Merging #529 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #529      +/-   ##
=========================================
- Coverage   88.63%   88.6%   -0.03%     
=========================================
  Files          94      94              
  Lines        6000    5987      -13     
=========================================
- Hits         5318    5305      -13     
  Misses        682     682
Impacted Files Coverage Δ
stacker/actions/build.py 92.19% <100%> (ø) ⬆️
stacker/providers/aws/default.py 64.87% <100%> (-0.1%) ⬇️
stacker/tests/providers/aws/test_default.py 99.54% <100%> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 044ca52...7657f7a. Read the comment docs.

@ejholmes ejholmes merged commit 075b083 into master Feb 9, 2018
@ejholmes ejholmes deleted the perf-optimizations branch February 9, 2018 02:30
phrohdoh pushed a commit to phrohdoh/stacker that referenced this pull request Dec 18, 2018
Remove DescribeStacks call from prepare_stack_for_update
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.

3 participants