Actions document - suggested changes

[roger peppe]

This looks great, with one or two remarks below.

On 26 June 2014 05:47, Tim Penhey <tim.penhey at 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 think these should be combined, 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.
>
> Consider the following structure:
>
> type actionDoc struct {
>         // InternalId is opaque and you shouldn't care
>         InternalId string `bson:"_id"`
>
>         // UUID is the unique identifier for this action
>         UUID string

Why not just use the UUID as _id ?

>         // EnvUUID is the environment UUID, and is here because
>         // we know we want it when we have multiple environments
>         EnvUUID string

As there are a zillion documents that would already need this in state,
and the exact approach to implementing multi-environment
state servers is not yet decided, I'd suggest leaving this out
for now.

>         // Action is the name of the action, eg 'backup', 'restart'
>         Action string
>
>         // UnitName is the name of the unit that the action will
>         // run on
>         UnitName string
>
>         // Payload holds the action's parameters, if any; it should
>         // validate against the schema defined by the named action
>         // in the unit's charm
>         Payload map[string]interface{}
>
>         // User is the user that requested this action
>         User string
>         // Invoked is when the action was requested
>         Invoked time.Time

How about combining all the below fields into an ActionStatus
struct? Then it's obvious which fields are mutable. The Status
field could be nil until the action has been started,
meaning the Started field wouldn't need to be a pointer.

>         // Started is recorded when the execution of the
>         // action is started.
>         Started *time.Time
>
>         // Finished is recorded when the execution of the
>         // action has completed.
>         Finished *time.Time
>
>         // ReturnCode is the result of the action
>         ReturnCode *int

This isn't quite sufficient, I think - a command doesn't
always exit with a return code (it can be killed, for example).

So perhaps just a string might work (perhaps just the string
form of the error returned from exec.Command.Run):
e.g.
    exit status 1
    signal: terminated
    exec: "someaction": executable file not found in $PATH

What's the plan for the failure status set by the action? I don't
see that here.

>         // 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

When the files are small (e.g. less than 100 bytes), we might consider
just putting them in this document directly. That's going to be
very common for stdout and stderr, and would save a client-server
round trip for little cost. It means that you don't have to worry
about garbage collecting them either, (that's something that I'd
like to see some discussion of actually).

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

I like this.

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

Lists? Do you mean Logs?

  cheers,
    rog.