10000 Multi region support by ejholmes · Pull Request #551 · cloudtools/stacker · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Multi region support #551

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

Merged
merged 2 commits into from
Mar 14, 2018
Merged

Multi region support #551

merged 2 commits into from
Mar 14, 2018

Conversation

ejholmes
Copy link
Contributor
@ejholmes ejholmes commented Mar 9, 2018

This implements support for provisioning stacks in multiple regions. You can target a stack to a specific region using the new per stack region option.

Example

Here's a simple pseudo stack config that shows how one might use this to create some resources in us-west-1, and reference them in us-east-1 with the output lookup.

stacks:
  - name: west/encryption-key
    stack_name: encryption-key
    region: us-west-1
    class_path: stacker_blueprints.kms.Key
    variables:
      KeyAlias: alias/backups

  # You can use the `output` lookup to use an output from a stack in one
  # region, as the input to a stack in another region:
  - name: east/db-backups
    stack_name: db-backups
    class_path: stacker.tests.fixtures.mock_blueprints.Dummy
    variables:
      # Reference the KMS key created in us-west-1!
      EncryptionKeyARN: ${output west/encryption-key}

In the long term, I'm also planning on adding multi account support, aws profiles, but I still think having a region flag on a stack is still valuable for simplicity, and everything in this PR is a pre-req to supporting aws profiles.

Depends on #550

TODO

  • Fix --tail (breaks because it doesn't happen within _launch_stack, so it doesn't get the stack specific region applied).
  • Fix info command
  • Fix FunctionalTests blueprint to give stacker user access to all regions.

@ejholmes ejholmes requested a review from a team March 9, 2018 07:58
@codecov-io
Copy link
codecov-io commented Mar 10, 2018

Codecov Report

Merging #551 into master will decrease coverage by 0.08%.
The diff coverage is 83.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
- Coverage   87.44%   87.36%   -0.09%     
==========================================
  Files          95       95              
  Lines        6127     6167      +40     
==========================================
+ Hits         5358     5388      +30     
- Misses        769      779      +10
Impacted Files Coverage Δ
stacker/commands/stacker/destroy.py 76.47% <ø> (ø) ⬆️
stacker/tests/fixtures/mock_blueprints.py 62.06% <ø> (ø) ⬆️
stacker/commands/stacker/build.py 84.21% <ø> (ø) ⬆️
stacker/actions/info.py 33.33% <0%> (-1.97%) ⬇️
stacker/actions/diff.py 59.28% <0%> (-0.43%) ⬇️
stacker/commands/stacker/info.py 71.42% <0%> (ø) ⬆️
stacker/commands/stacker/diff.py 73.33% <0%> (ø) ⬆️
stacker/actions/build.py 91.33% <100%> (+0.05%) ⬆️
stacker/actions/destroy.py 85.71% <100%> (+0.29%) ⬆️
stacker/tests/actions/test_build.py 96.85% <100%> (+0.08%) ⬆️
... and 10 more

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 55aa8a8...42cba20. Read the comment docs.

@ejholmes ejholmes mentioned this pull request Mar 10, 2018
2 tasks
@ejholmes ejholmes changed the base branch from node-names to master March 10, 2018 03:00
@ejholmes ejholmes force-pushed the multi-region branch 2 times, most recently from c8a48ae to dab7779 Compare March 10, 2018 06:57
stacker/stack.py Outdated
@@ -62,7 +62,8 @@ class Stack(object):
def __init__(self, definition, context, variables=None, mappings=None,
locked=False, force=False, enabled=True, protected=False):
self.name = definition.name
self.fqn = context.get_fqn(self.name)
self.fqn = definition.stack_name or context.get_fqn(self.name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious if anyone has thoughts on this. I intentionally made stack_name not include the include the namespace, but now I think that might be confusing. Would this be better?

self.fqn = context.get_fqn(definition.stack_name or definition.name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change.

@ejholmes ejholmes force-pushed the multi-region branch 4 times, most recently from dd01f19 to c1ee087 Compare March 12, 2018 08:03
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.

Surprised at how little code needed to change for this. How do we deal with various s3 connections/buckets? Anything we need to worry about with hooks/lookups?

since you could have multiple stacks with the same name, but in different
regions or accounts. (note: the namespace from the environment will be
prepended to this)
**region**:
Copy link
Member

Choose a reason for hiding this comment

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

Could you detail here what happens if this is and isn't set? I'm assuming it modifies quite a bit - also, curious what the default is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the docs. The default would be whatever the global default is (e.g. AWS_DEFAULT_REGION, ~/.aws/config). If not provided, it works exactly as before.

Copy link
Member

Choose a reason for hiding this comment

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

That's what I assumed, just wanted to make sure it's immediately apparent for folks that are new to stacker in the future. Thanks.

@@ -219,8 +219,11 @@ def _launch_stack(self, stack, **kwargs):
if not should_submit(stack):
return NotSubmittedStatus()

region = stack.region
provider = self.provider.build(region=region)
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename self.provider to self.provider_builder/provider_factory or something similar? I think you avoided this to keep this PR small, but honestly this will probably lead to confusion in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I only did this to keep the diff small. I'll go ahead and make this change.

@@ -30,7 +30,7 @@ def configure(self, options, **kwargs):
environment=options.environment,
validate=True)

options.provider = default.Provider(
options.provider = default.ProviderBuilder(
Copy link
Member

Choose a reason for hiding this comment

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

options.provider -> options.provider_builder, etc, based on my comment above

self.kwargs = kwargs

def build(self, region=None):
# TODO(ejholmes): This class _could_ cache built providers, however,
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I don't think we need to cache Providers. My guess is they're pretty cheap to build/throw away at the scale we usually operate.

session = get_session(region=region)
return Provider(session, **self.kwargs)

def tail_stack(self, stack, cancel, retries=0, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Curious why you brought this in here, rather than modifying the tail code elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I'm definitely not happy with it, but it's a little tricky because of where we pass the tail_stack function.

I could change that line to something like:

tail=self._tail_stack

And then:

def _tail_stack(...)
  provider = self.build_provider(stack)
  return provider.tail_stack(...)

Which would definitely be better, so we can share and re-use a build_provider method.

@phobologic
Copy link
Member

Also - should log messages be updated? Particularly debug logs, to include the region? Maybe not super necessary if we print the stack names themselves.

@ejholmes
Copy link
Contributor Author

How do we deal with various s3 connections/buckets

We already talked about this offline, but just to mention it here: since this PR only deals with multi-region, there's no special considerations that need to be taken into account for the stacker bucket; stacker will continue to provision the bucket in the default region.

Anything we need to worry about with hooks/lookups?

  • Hooks: We could eventually add region support to this in the future, but since we're talking about re-working hooks to be a part of the graph, probably best to hold off on that for now.
  • Lookups: output just works between multi-region stacks, but there are some special considerations to take into account when using something like xref; if a stack is provisioned in us-west-1, then the xref lookup will also look for the stack in us-west-1.

Also - should log messages be updated? Particularly debug logs, to include the region? Maybe not super necessary if we print the stack names themselves.

This is definitely something I wanted to talk about. At the moment, when we log a step, we log the stack's FQN:

https://github.com/remind101/stacker/blob/b58e01c7281b925311bfad95a20fc68432b57635/stacker/plan.py#L63-L64

This has worked ok for now, since stack FQN's have been unique within a config, but in multi-region that's no longer true. I think we should change it so that the "node name" in the graph is logged:

    def __str__(self):
-        return self.stack.fqn
+        return self.stack.name

That's somewhat of a breaking change (it's just logging to stderr, so not too breaking), but probably necessary.

@ejholmes ejholmes force-pushed the multi-region branch 3 times, most recently from d785bce to 42cba20 Compare March 13, 2018 02:21
@phobologic
Copy link
Member

That's somewhat of a breaking change (it's just logging to stderr, so not too breaking), but probably necessary.

I'm fine with this changing. We can share on the channel/make sure it's outlined in the CHANGELOG (maybe update that now so we don't lose it).

This allows the stack name to be overriden. The main thing that this
allows, is for multiple "stacks" using the same stack name, which will
be important in multi account/region.
@ejholmes ejholmes merged commit f8f83c3 into master Mar 14, 2018
phrohdoh pushed a commit to phrohdoh/stacker that referenced this pull request Dec 18, 2018
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
0