-
Notifications
You must be signed in to change notification settings - Fork 166
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
Multi region support #551
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
c8a48ae
to
dab7779
Compare
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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this change.
dd01f19
to
c1ee087
Compare
There was a problem hiding this 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**: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
stacker/actions/build.py
Outdated
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
stacker/commands/stacker/__init__.py
Outdated
@@ -30,7 +30,7 @@ def configure(self, options, **kwargs): | |||
environment=options.environment, | |||
validate=True) | |||
|
|||
options.provider = default.Provider( | |||
options.provider = default.ProviderBuilder( |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
stacker/providers/aws/default.py
Outdated
session = get_session(region=region) | ||
return Provider(session, **self.kwargs) | ||
|
||
def tail_stack(self, stack, cancel, retries=0, **kwargs): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Also - should log messages be updated? Particularly debug logs, to include the region? Maybe not super necessary if we print the stack names themselves. |
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.
This is definitely something I wanted to talk about. At the moment, when we log a step, we log the stack's FQN: 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. |
d785bce
to
42cba20
Compare
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.
Multi region support
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.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
--tail
(breaks because it doesn't happen within_launch_stack
, so it doesn't get the stack specific region applied).FunctionalTests
blueprint to give stacker user access to all regions.