Added leadership to implicit selectors. #11

Merged
merged 8 commits into from Nov 22, 2016

Conversation

Projects
None yet
3 participants
Collaborator

petevg commented Nov 3, 2016

To make this work, I had to refactor the way that we do the fetching and
composing of units for the implicit selectors. It's not as elegant or
flexible any more, but it does support asynchrounous operations, which
was critical.

I also had to make selectors asynchronous, and get rid of type checking
for their returns -- verifying that they all return a coroutine object
isn't as interesting as being able to verify what that coroutine
actually returns ...

@johnsca @bcsaller

Collaborator

petevg commented Nov 4, 2016

Per discussion w/ bcsaller, we don't actually want asynchronous selectors -- I'm going to work on a commit that will track leadership in python-libjuju's model instead.

Collaborator

petevg commented Nov 7, 2016

@bcsaller I ran into an interesting issue w/ integrating leadership into python-libjuju.

Specifically, we were forgetting that leadership is not actually a juju feature! It's a layer feature, and all the info lives in the relation data. That means that we don't get any deltas related to leadership over the juju websocket api.

I see two possibilities:

  1. I am missing a clever way of getting at the relation data over the websocket api.
  2. The implementation in the PR is actually the correct implementation. (My first instinct is to try to add caching ... but glitch does stuff that specifically would invalidate cached leader data by triggering the election of a new leader ... it's probably not a great idea to cache it.)
Contributor

bcsaller commented Nov 7, 2016

This isn't true. https://github.com/juju/juju/blob/staging/cmd/juju/status/output_tabular.go#L180 shows leader status being added to the output of units. I haven't verified this lives in the AllWatcher delta stream or not but leadership is first class and if for some reason it isn't there we can argue that it should be. Ask someone in core for a clear answer here. I can't dig into this from the sprint right now.

Collaborator

petevg commented Nov 8, 2016

bcsaller: You're correct -- there are facades related to leadership in the api. I'm a bit sidetracked debugging deploy stuff in python-libjuju (the hadoop-processing bundle exposes some bugs), but I think the code is something like this:

        u = client.UniterFacade()
        u.connect(self.model.connection)
        await u.WatchLeadershipSettings(entity)

Where entity is either an application or a unit (need to test both, and see which works -- it's a little bit unclear from just reading the code.)

Collaborator

petevg commented Nov 21, 2016

@bcsaller @johnsca Per my convo w/ Ben and then on the #juju-dev channel last Friday, I refactored to using fullstatus, as leadership is not exposed directly over the API.

At some point, we might want to cache the return, so that we're not calling it so much. It isn't causing a noticeable delay in the runtime of matrix, however, so I'd like to leave caching as a separate exercise (we're messing with things, so we have to be careful about the cache not falling out of date).

matrix/plugins/glitch/utils.py
+import subprocess
+import yaml
+
+async def is_leader(model, rule, unit_name):
@petevg

petevg Nov 21, 2016

Collaborator

Passing in rule because I was doing debug logging. We might want to think of a better way to pass the log around.

@johnsca

johnsca Nov 21, 2016

Owner

Why is this in matrix instead of libjuju?

@petevg

petevg Nov 21, 2016

Collaborator

johnsca: it's a kluge, so it felt like it belonged here rather than in the library that we're offering as the "correct" way of doing things.

I can move it into libjuju if you'd rather, though.

@petevg

petevg Nov 21, 2016

Collaborator

@johnsca PR for moving this into python-libjuju here: juju/python-libjuju#13

(I dealt with my concerns about hackiness and conflicting with a non asynchronous call in the future by just giving it a slightly scary name that won't conflict with the name of the "right" method in the future ...)

petevg added some commits Oct 28, 2016

Added leadership to implicit selectors.
To make this work, I had to refactor the way that we do the fetching and
composing of units for the implicit selectors. It's not as elegant or
flexible any more, but it does support asynchrounous operations, which
was critical.

I also had to make selectors asynchronous, and get rid of type checking
for their returns -- verifying that they all return a coroutine object
isn't as interesting as being able to verify what that coroutine
actually returns ...
Fixed errors in tests/glitch/test_glitch
Fixed incorrect indentation in code involving waiters. Got rid of some
extraneous logging messages. Incorporated a fix from python-libjuju.
Better is_leader check.
No longer relies on running an action on a machine -- just checks output
of 'juu status' (fullstatus in the model) instead.
Moved is_leader into python-libjuju.
Now we just call unit.is_leader_from_status, instead of messing about
with FullStatus and ClientFacade in our matrix codebase.
Added catch for exception when running pkill jujud.
Putting the catch here rather than in libjuju because we are doing
something exceptional (killing the juju daemon), and it's probably
appropriate for python-libjuju to complain.

Thanks

matrix/plugins/glitch/utils.py
+ """
+ app = unit_name.split("/")[0]
+
+ client = ClientFacade()
@bcsaller

bcsaller Nov 22, 2016

Contributor

Is this needed? Doesn't the connected model we pass in have access to the FullStatus method? If it doesn't I would suggest adding to the model in libjuju directly.

@petevg petevg merged commit c3a4f23 into master Nov 22, 2016

@petevg petevg deleted the feature/todo-cleanup branch Nov 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment