Merge lp://staging/~allenap/maas/rpc-get-secrets into lp://staging/~maas-committers/maas/trunk

I'm not going to block it, but I'm not going to approve it either. Out of
interest, what constitutes too much work here?

The tags stuff is also blocked by the problem we talked about yesterday,
namely the GIL block. As I said in the other review I don't think the process
pipe will help as the reader blocks until it closes, so it needs twisted IO
instead (which is why ampoule was created I suspect).

I think that we need to think harder about the architecture of these jobs
rather than throwing RPC at everything. We might want to include API
parameters as part of the RPC call-out to do the job, for example, especially
in the case of the long running jobs where we don't want to wait around. We
can then reactor.spawnProcess() and leave it running and get notified when it
finishes.

(Not sure if Jool's comments still stand in the light of the pipe fork stuff not running in the reactor thread… I suspect they do to a point, though).

If the pipe stuff prevents the IO block, then we're golden. I still think it might be easier to send API creds to each job that needs them though.

> I'm not going to block it, but I'm not going to approve it either. Out
> of interest, what constitutes too much work here?

There are three functions in p.tags that correspond to API calls:

    def get_nodes_for_node_group(client, nodegroup_uuid):
        """Retrieve the UUIDs of nodes in a particular group.

    def get_details_for_nodes(client, nodegroup_uuid, system_ids):
        """Retrieve details for a set of nodes.

    def post_updated_nodes(client, tag_name, tag_definition, ...):
        """Update the nodes relevant for a particular tag.

The first and last of those could be rewritten as RPC calls as a
straight port. The middle one is the problem: it downloads many
megabytes (of XML) in a single call. Rewriting that to work over RPC
would mean inventing a mechanism for packing and unpacking the data over
the wire. That's square-nail-round-hammer territory, at least for now
when we'd have to rush a design and implementation.

This code works as it is right now. I can port the Celery tasks that
trigger it to RPC calls, but I think we should leave this code well
alone for now. It really isn't doing us any harm.

>
> The tags stuff is also blocked by the problem we talked about
> yesterday, namely the GIL block. As I said in the other review I don't
> think the process pipe will help as the reader blocks until it closes,
> so it needs twisted IO instead (which is why ampoule was created I
> suspect).

As discussed elsewhere, the process pipe (aka `pipefork` and
`objectfork`) needs to be run from a non-reactor thread. I'll add a
guard to prevent it from being used in the reactor thread.

>
> I think that we need to think harder about the architecture of these
> jobs rather than throwing RPC at everything. We might want to include
> API parameters as part of the RPC call-out to do the job, for example,
> especially in the case of the long running jobs where we don't want to
> wait around. We can then reactor.spawnProcess() and leave it running
> and get notified when it finishes.

I can live with passing the API parameters per-task rather than having a
GetSecrets call.

On Wednesday 10 Sep 2014 23:20:44 you wrote:
> The first and last of those could be rewritten as RPC calls as a
> straight port. The middle one is the problem: it downloads many
> megabytes (of XML) in a single call. Rewriting that to work over RPC
> would mean inventing a mechanism for packing and unpacking the data over
> the wire. That's square-nail-round-hammer territory, at least for now
> when we'd have to rush a design and implementation.
>
> This code works as it is right now. I can port the Celery tasks that
> trigger it to RPC calls, but I think we should leave this code well
> alone for now. It really isn't doing us any harm.

Fair enough, it sounds like we're better off with the API then.

> As discussed elsewhere, the process pipe (aka `pipefork` and
> `objectfork`) needs to be run from a non-reactor thread. I'll add a
> guard to prevent it from being used in the reactor thread.

Tiptop! Thanks.

> I can live with passing the API parameters per-task rather than having a
> GetSecrets call.

The more I think about it the more I like it. I indicates to the casual
viewer that this RPC will be using the API to return results, and that is a
special situation.

Cheers!