On Thu, Jun 26, 2014 at 6:47 AM, Tim Penhey <tim.pen...@canonical.com>
wrote:
>
> On reading the spec[1] and looking at the state documents as they are in
> trunk now, I was quickly coming to the conclusion that the documents
> need to change.
>

I agree with that, but...


>
> Right now we have two action related documents:
> ...
> I think these should be combined,


Strong -1. We've made this mistake too many times in the past; we need to
group the fields in separate docs according to who needs to read/write
them, so we don't end up with embarrassments like the N^2 add-unit problem.


> and there are quite a few missing
> fields that are clearly expected from the spec:
>
>         invoked:  TIME
>         started:  TIME
>         finished: TIME
>         files:
>             - this is a tricky one
>
> Obviously we are also missing the user that asked for the action.
>

Agreed. These fields will come when we're ready to use them.

Consider the following structure:
>
> type actionDoc struct {
>          ...
>         // UnitName is the name of the unit that the action will
>         // run on
>         UnitName string
>

Not necessarily a unit. We need service actions, and probably one day stack
actions (even if they're likely to be represented as service actions).

         ...
>         // User is the user that requested this action
>         User string
>

Not necessarily a user. I think there's a class of service-level action
that will only be possible if the leader has a way of invoking actions on
its followers.

         ...
>         // ReturnCode is the result of the action
>         ReturnCode *int
>

Not so convinced that this is really what we want, more on this later.

        // Results are defined and set by the action themselves.
>         Results map[string]interface{}
>
>         // Files is interesting, in that they should probably live
>         // in the environment file storage, what was cloud storage
>         // and is now GridFS, under some particular directory:
>         // perhaps  /actions/<uuid>/<filename>
>         // Both stdout and stderr are recorded as files.  They are
>         // only added if they are not empty.
>         // Other files can be added by the action.
>         Files []string
>

I'm concerned that this overlaps unhelpfully with other features. Hook
output is already logged elsewhere, and I think I'd prefer to just make
sure they're badged properly so that they're easily accessible via
debug-log.

Other files, and/or the possibility of multiple output documents, feel like
they're an independent feature. Sorry I didn't flag this before, but that
spec has had some interesting changes since I +1ed it -- in short, I'm
concerned that we're sacrificing orthogonality for the sake of gilding the
actions feature. I think a single output map for the action results will
give us a lot of value, and I anticipate that a separate output feature --
in which we can store structured maps, or binary blobs -- is valuable in
many contexts, not just in actions.


> status becomes an emergent property:
>   if Started is nil
>     "pending"
>   if Finished is nil
>     "started"
>   if ReturnCode is 0 (not nil)
>     "success"
>   else
>     "failure"
>

Up to a point, yes, but I think there's a bit more granularity to status.
There's also the possibility that the unit agent went down mid-action; and
I wouldn't be surprised if we came up with other edge cases. Basically I
think we really do want Status instead of ReturnCode; while we could
plausibly infer pending/started statuses from other fields, but I'm not
sure it's *that* much of a win -- we may as well set "started" when we set
the started time, and then we can just get the status by looking without
having to infer anything. (Although we would have to infer pending from the
lack of an ActionResult anyway.)

Queues, Lists, and Results are just queries across this document set for
> the environment, optionally scoped to unit names and users.
>

Not sure how to watch them. Remember that we can't really do db queries
inside watches, because any blocked watcher blocks the whole watching
infrastructure. This is not to say that we *can't* get around it, but it's
a bunch of complexity for relatively little benefit IMO; and people
generally find the watchers hard enough to deal with without adding
additional layers.

Does that seem reasonable to other people?
>

Some of it does, yeah. My overriding concern is about just sticking
everything in the same doc just because they're conceptually related: I
really don't think it's a good idea.

Cheers
William
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Reply via email to