RPC server for remote command invocation

RPC server for remote command invocation



https://code.launchpad.net/~fwereade/juju/go-jujuc-server/+merge/103678

Requires: https://code.launchpad.net/~fwereade/juju/go-extract-cmd-log/+merge/103479

(do not edit description out of merge proposal)

[fwereade, 2012-04-26 12:11:41]

Please take a look.

[rog, 2012-04-27 10:29:46]

looks good, modulo comments below.
will look at tests when comments addressed.

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go
File cmd/jujuc/server/server.go (right):

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:35: Stdout string
if you made Stdout and Stderr []byte, you could save
the conversion later.

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:40: // context identified by contextId.
// CmdsGetter returns the commands available to the
// context identified by contextId.
?

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:43: // Jujuc wraps a set of Commands for RPC.
// Jujuc implements the jujuc command methods in the form
// required by net/rpc.

?

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:56: Name: "(-> jujuc)", Purpose: jujucPurpose, Doc:
jujucDoc,
separate lines would read better, i think.

also, no Log? if we aren't allowing any global arguments to the command, then
don't really need SuperCommand at all, do we?

in fact, i don't believe we do need SuperCommand. the invocation of jujuc knows
the subcommand name (from argv[0]), so it could pass that as a field in Request.
then we could just call cmd.Main directly on the command as returned from
getCmds.

if we do that, then perhaps we can simplify getCmds too, because this code
wouldn't need to know *all* the possible commands for a given context.

type CmdGetter func(contextId string, name string) (cmd.Command, error)

if the command returned from CmdGetter wants logging flags, it can use the Log
type as desired.

the usage becomes more obvious too. (no need for the "(-> jujuc)"
and the jujuc doc seems wrong, given that we know by this stage that the client
*has* correctly set the environment variables and invoked jujuc through a
symlink)

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:82: stderr := &bytes.Buffer{}
var stdout, stderr bytes.Buffer

is marginally nicer to my eyes. (use &stdout and &stderr below)

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:85: resp.Stdout = stdout.String()
stdout.Bytes if you go with the Stdout change suggested above.

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:91: // process on behalf of another.
// Server implements a server that serves command
// invocations via a unix-domain socket.
?

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:136: }(conn)
nice.

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:138: select {
this really wants a comment.

// We don't know if we got an real error from Accept
// or if it only returned an error because the listener
// was closed, but if Close has been called,
// the likelyhood is strong that it's the latter,
// so we'll ignore the error in that case.

?

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:153: os.Remove(s.socketPath)
i wonder if we should have some package that allows us to clean up garbage files
when the program is interrupted. food for thought.

[fwereade, 2012-04-27 12:45:59]

Please take a look.

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go
File cmd/jujuc/server/server.go (right):

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:35: Stdout string
On 2012/04/27 10:29:46, rog wrote:
> if you made Stdout and Stderr []byte, you could save
> the conversion later.

Ha, much nicer. Done.

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:40: // context identified by contextId.
On 2012/04/27 10:29:46, rog wrote:
> // CmdsGetter returns the commands available to the
> // context identified by contextId.
> ?
Better?

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:43: // Jujuc wraps a set of Commands for RPC.
On 2012/04/27 10:29:46, rog wrote:
> // Jujuc implements the jujuc command methods in the form
> // required by net/rpc.
> 
> ?
Better?

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:56: Name: "(-> jujuc)", Purpose: jujucPurpose, Doc:
jujucDoc,
On 2012/04/27 10:29:46, rog wrote:
> separate lines would read better, i think.
> 
> also, no Log? if we aren't allowing any global arguments to the command, then
> don't really need SuperCommand at all, do we?
> 
> in fact, i don't believe we do need SuperCommand. the invocation of jujuc
knows
> the subcommand name (from argv[0]), so it could pass that as a field in
Request.
> then we could just call cmd.Main directly on the command as returned from
> getCmds.
> 
> if we do that, then perhaps we can simplify getCmds too, because this code
> wouldn't need to know *all* the possible commands for a given context.
> 
> type CmdGetter func(contextId string, name string) (cmd.Command, error)
> 
> if the command returned from CmdGetter wants logging flags, it can use the Log
> type as desired.
> 
> the usage becomes more obvious too. (no need for the "(-> jujuc)"
> and the jujuc doc seems wrong, given that we know by this stage that the
client
> *has* correctly set the environment variables and invoked jujuc through a
> symlink)

I don't entirely agree, but I'm cutting out SC for now on the basis that it
allows me to punt on selection mechanism for now; we can revisit the decision
when we have a live source of Commands.

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:82: stderr := &bytes.Buffer{}
On 2012/04/27 10:29:46, rog wrote:
> var stdout, stderr bytes.Buffer
> 
> is marginally nicer to my eyes. (use &stdout and &stderr below)

Done.

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:85: resp.Stdout = stdout.String()
On 2012/04/27 10:29:46, rog wrote:
> stdout.Bytes if you go with the Stdout change suggested above.

Done.

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:91: // process on behalf of another.
On 2012/04/27 10:29:46, rog wrote:
> // Server implements a server that serves command
> // invocations via a unix-domain socket.
> ?
Better?

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:136: }(conn)
On 2012/04/27 10:29:46, rog wrote:
> nice.

Thanks :).

https://codereview.appspot.com/6120054/diff/1/cmd/jujuc/server/server.go#newc...
cmd/jujuc/server/server.go:138: select {
On 2012/04/27 10:29:46, rog wrote:
> this really wants a comment.
> 
> // We don't know if we got an real error from Accept
> // or if it only returned an error because the listener
> // was closed, but if Close has been called,
> // the likelyhood is strong that it's the latter,
> // so we'll ignore the error in that case.
> 
> ?
Better?

[fwereade, 2012-04-27 12:49:25]

Please take a look.

[rog, 2012-04-27 12:52:42]

LGTM assuming jujucDoc and Purpose go as discussed on IRC.

https://codereview.appspot.com/6120054/diff/5001/cmd/jujuc/server/server_test.go
File cmd/jujuc/server/server_test.go (right):

https://codereview.appspot.com/6120054/diff/5001/cmd/jujuc/server/server_test...
cmd/jujuc/server/server_test.go:127: func lines(b []byte) []string {
this can go, i think

[fwereade, 2012-04-27 13:41:20]

Please take a look.

[fwereade, 2012-04-27 13:41:39]

https://codereview.appspot.com/6120054/diff/5001/cmd/jujuc/server/server_test.go
File cmd/jujuc/server/server_test.go (right):

https://codereview.appspot.com/6120054/diff/5001/cmd/jujuc/server/server_test...
cmd/jujuc/server/server_test.go:127: func lines(b []byte) []string {
On 2012/04/27 12:52:42, rog wrote:
> this can go, i think

Done.

[fwereade, 2012-04-27 15:47:36]

Please take a look.

[niemeyer, 2012-05-02 22:36:04]

That's very nice. Just a note about the test values, and LGTM.

https://codereview.appspot.com/6120054/diff/13002/cmd/jujuc/server/server_tes...
File cmd/jujuc/server/server_test.go (right):

https://codereview.appspot.com/6120054/diff/13002/cmd/jujuc/server/server_tes...
cmd/jujuc/server/server_test.go:33: if c.Value != "zyxxy" {
I feel bad for removing some of the fun here, but it'd be useful to have values
that give a hint about their meaning for the test. I doubt we'll know how
"zyxxy", "merlin", and "magic" affect these tests in a week. :-)

[fwereade, 2012-05-03 08:24:36]

*** Submitted:

RPC server for remote command invocation

R=
CC=
https://codereview.appspot.com/6120054

https://codereview.appspot.com/6120054/diff/13002/cmd/jujuc/server/server_tes...
File cmd/jujuc/server/server_test.go (right):

https://codereview.appspot.com/6120054/diff/13002/cmd/jujuc/server/server_tes...
cmd/jujuc/server/server_test.go:33: if c.Value != "zyxxy" {
On 2012/05/02 22:36:04, niemeyer wrote:
> I feel bad for removing some of the fun here, but it'd be useful to have
values
> that give a hint about their meaning for the test. I doubt we'll know how
> "zyxxy", "merlin", and "magic" affect these tests in a week. :-)

Fair enough :). (Don't feel bad, they're not close to my heart of anything)