Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
[WIP] Renamed chaos -> glitch and fleshed it out. #1
Conversation
petevg
added some commits
Oct 11, 2016
johnsca
requested changes
Oct 12, 2016
I still think plugins/ should live under matrix/plugins/ and we might also want to think about using entry-points to make it extensible.
| + units = None | ||
| + for selector in selectors: | ||
| + selectf = selector_map[selector.pop['selector']] | ||
| + units = selectf(context.model, **selector) |
johnsca
Oct 12, 2016
Owner
This isn't passing units through. Also, it should be entities or perhaps even working_set, since not all selectors will work on units (and some may even change the entity type in the working set).
petevg
Oct 13, 2016
Collaborator
Fixed. Changed "units" to "objects". "entities", "things", or "working_set" might be better, though, now that I reread this comment.
bcsaller
Oct 13, 2016
Contributor
I suspect that we can adapt the decorators to deal with things a bit differently here. Though I am open to if/how we do this.
What if for example actions (in particular) were written to operate on a single instance but the decorator included the state machine for things like inflight, interval, max_retries and completor.
@action(inflight=1, interval=5*60)
async def reboot(context, obj):
await obj.reboot()
which returned the task doing all the interval, retry things.
Likewise,
@action(interval=60, max_retries=3)
async def health(context, obj):
return all(await obj.healthcheck(),
await obj.agent_state == "idle",
await obj.workload_state == "active")
Might get called on many units (the whole set at once), each one waiting for a success return, if there are failures it can be retried each interval up to max retries.
The intention here is avoid each action having to include all of this logic (as we already expect them all to operate on sets)
Another way this could work is that @action creates and returns a callable Action instance which manages this more directly. This way we can treat this as a plug point by having @action resolve a factory class (to make it plugable) and by detecting if the action in the plan already points to an Action class.
What do you think?
petevg
Oct 14, 2016
Collaborator
bcsaller: this makes a lot of sense. In the latest iteration of the code, I just added hooks for looping over a set, but everything is nicely encapsulated so that we can add the other hooks that you suggested around it, as we need them.
| + selectors = [ | ||
| + {'selector': 'units', 'application': unit.application}, | ||
| + {'selector': 'leader', 'application': unit.is_leader()}, | ||
| + {'selector': 'count', 'value': 1}, |
petevg
Oct 13, 2016
Collaborator
I think that "one", with no args, is probably the right name for it. Changed.
| + on whether 'value' is truthy or falsy. | ||
| + | ||
| + ''' | ||
| + units = units or model.units |
johnsca
Oct 12, 2016
Owner
This will do unexpected things if the selectors pare down to an empty list. I think that it would be better to require that the units selector always explicitly precede the selectors that require units is better than allowing a default to all units.
We might also want to type-check that we're actually given units and not some other ModelEntity like Relation, Machine, etc. Ideally, we'd be able to do that in the selector loop above, but I'm not certain we could surface that in a generic way, so possibly just some validation helpers would be sufficient.
petevg
Oct 13, 2016
Collaborator
Did a bit refactor of selectors to address this.
Each selector now specifies what type of object it accepts and what type of object it returns. The objects get passed properly everywhere. And I expanded the selector map into a class that can do validation that chains of selectors actually work together.
petevg
added some commits
Oct 12, 2016
| + rule.log.info("Starting glitch") | ||
| + | ||
| + glitch_output = [] | ||
| + output_filename = DEFAULT_OUTPUT # TODO: make configurable. |
bcsaller
Oct 13, 2016
Contributor
Yeah, context can define paths and filenames for things like this.
| + # Execute glitch plan. We perform destructive operations here! | ||
| + for action in glitch_plan: | ||
| + actionf = action_map[action.pop('action')] | ||
| + selectors = action.pop('selectors') |
bcsaller
Oct 13, 2016
Contributor
This style of destroying the plan as we execute it might make some class of error reporting harder to understand down the line, but nothing we need to change today. Still, based around my other comments this might be resolved to an instance of an Action class and managed a little more formally.
petevg
and others
added some commits
Oct 13, 2016
|
and I pushed to to this branch rather than the other :-/ apologies |
|
This code was subsumed and merged by glitch_typing. Thanks |
petevg commentedOct 11, 2016
None of this code will execute, because we don't have it working against a real and finished version of libjuju. I wanted to get everything down on paper, though, and get feedback.
@bcsaller @johnsca