DAG #280
DAG #280
Conversation
|
Ran this against our staging environment without issue. Gonna take a look at adding a |
| """ Add an edge (dependency) between the specified nodes. """ | ||
| if not graph: | ||
| graph = self.graph | ||
| if ind_node not in graph or dep_node not in graph: |
There was a problem hiding this comment.
Might be worth it to indicate which node wasn't in the graph.
|
|
||
| def __init__(self, description, sleep_time=5, wait_func=None, | ||
| watch_func=None, *args, **kwargs): | ||
| class Plan(): |
There was a problem hiding this comment.
this should be class Plan(object):
| def __init__(self, description, sleep_time=5, wait_func=None, | ||
| watch_func=None, *args, **kwargs): | ||
| class Plan(): | ||
| def __init__(self, description, reverse=False, sleep_func=sleep): |
There was a problem hiding this comment.
Probably worth adding back the doc string here.
| Returns: | ||
| list: A list of :class:`Step` objects that are in the given status. | ||
| def build(self, stacks): | ||
| """ Builds an internal dag from the stacks and their dependencies """ |
There was a problem hiding this comment.
Add the full docstring here for the API docs.
| try: | ||
| dag.add_edge(stack.fqn, dep) | ||
| except DAGValidationError: | ||
| raise CyclicDependencyError(stack.fqn) |
There was a problem hiding this comment.
You should add the dependency to this exception as well, just so it's easier to see what dependency caused the error.
| raise CyclicDependencyError(stack.fqn) | ||
|
|
||
| self._dag = dag | ||
| return None |
There was a problem hiding this comment.
You don't have to do this -
>>> def blah():
... print "WOO"
...
>>>
>>> x = blah()
WOO
>>> x
>>> type(x)
<type 'NoneType'>
| if not graph: | ||
| graph = self.graph | ||
| for node, edges in graph.items(): | ||
|
|
|
|
||
| def validate(self, graph=None): | ||
| """ Returns (Boolean, message) of whether DAG is valid. """ | ||
| graph = graph if graph is not None else self.graph |
There was a problem hiding this comment.
This is different than the way it's done throughout this class - might be worth changing it just for consistency.
|
Ok. Think I got all the changes in based on feedback. We should probably hold off on merging until #281 is ready, since this will be slower to start without a parallel walk. |
| if not graph: | ||
| graph = self.graph | ||
| nodes = self.topological_sort(graph=graph) | ||
| if reverse: |
There was a problem hiding this comment.
I think this is kinda a naive implementation for reversing a dag. Doing this, we can't walk the graph concurrently in reverse. Instead, we should do a depth first walk, and build a new graph with the edges reversed.
There was a problem hiding this comment.
Added a commit that adds a tranpose method on DAG to build a new DAG with all the edges transposed. Plan uses this now for destroy actions, so it can be parallelized.
| @@ -271,26 +271,10 @@ def _handle_missing_parameters(self, params, required_params, | |||
| return params.items() | |||
|
|
|||
| def _generate_plan(self, tail=False): | |||
| plan_kwargs = {} | |||
| if tail: | |||
| plan_kwargs["watch_func"] = self.provider.tail_stack | |||
There was a problem hiding this comment.
This gets removed in this PR, which may not be correct. This seems to tail the events, but I'm not sure I've ever actually seen stacker do this.
There was a problem hiding this comment.
Yeah - this is used by some people with --tail. Does it cause issues if you leave it in?
| context=self.context) | ||
|
|
||
| plan = Plan(description="Test", sleep_func=None) | ||
|
|
||
| with self.assertRaises(CyclicDependencyError): | ||
| plan.build([vpc, bastion]) | ||
| try: |
There was a problem hiding this comment.
I think what you want here is self.assertRaises(). You can see an example of that here:
There was a problem hiding this comment.
Oh sweet. I couldn't find an example online that used assertRaises to check the message. Much nicer.
|
Oh, also, once you submit this, could you open another PR to merge this against the |
Execute plan in parallel
|
Alright, this has gotten behind I'll follow up with porting this forward to 1.0 once we've moved internally to 1.0. |
This is an internal refactor of
stacker.plan.Planto use a DAG to internally represent steps and their dependencies. The net result provides a simpler method to walk the graph, and update stacks in order of their dependencies.Currently, this doesn't walk the graph in parallel, which is implemented in #281.
The DAG implementation is pulled from https://github.com/thieman/py-dag with the addition of a
walkmethod for walking the graph in topological order.TODO