veyron/runtimes/google: allow for command line args to override runtime env vars.
This change allows for command line flags, in particular, those defined
in the flags.Runtime group (formerly called 'Essentials') to override
their corresponding environment variables.
Also, got rid of an unused option for specifying namespace roots and
sundry cleanups+bugfixes.
Change-Id: I32dd2e2849730029e35e9bf3e6a9a849ca2f4c98
diff --git a/lib/exec/parent.go b/lib/exec/parent.go
index a3aebec..b0d995d 100644
--- a/lib/exec/parent.go
+++ b/lib/exec/parent.go
@@ -66,7 +66,6 @@
// NewParentHandle creates a ParentHandle for the child process represented by
// an instance of exec.Cmd.
func NewParentHandle(c *exec.Cmd, opts ...ParentHandleOpt) *ParentHandle {
-
cfg, secret := NewConfig(), ""
tk := timekeeper.RealTime()
for _, opt := range opts {
@@ -92,7 +91,18 @@
// Start starts the child process, sharing a secret with it and
// setting up a communication channel over which to read its status.
func (p *ParentHandle) Start() error {
- p.c.Env = append(p.c.Env, VersionVariable+"="+version1)
+ // Make sure that there are no instances of the VersionVariable
+ // already in the environment (which can happen when a subprocess
+ // creates a subprocess etc)
+ nenv := make([]string, 0, len(p.c.Env)+1)
+ for _, e := range p.c.Env {
+ if strings.HasPrefix(e, VersionVariable+"=") {
+ continue
+ }
+ nenv = append(nenv, e)
+ }
+ p.c.Env = append(nenv, VersionVariable+"="+version1)
+
// Create anonymous pipe for communicating data between the child
// and the parent.
dataRead, dataWrite, err := os.Pipe()
diff --git a/lib/flags/flags.go b/lib/flags/flags.go
index f6c2ef1..102ae14 100644
--- a/lib/flags/flags.go
+++ b/lib/flags/flags.go
@@ -11,11 +11,11 @@
type FlagGroup int
const (
- // Essential identifies the flags and associated environment variables
- // required by all Vanadium processes. Namely:
+ // Runtime identifies the flags and associated environment variables
+ // used by the Vanadium process runtime. Namely:
// --veyron.namespace.root (which may be repeated to supply multiple values)
// --veyron.credentials
- Essential FlagGroup = iota
+ Runtime FlagGroup = iota
// Listen identifies the flags typically required to configure
// ipc.ListenSpec. Namely:
// --veyron.tcp.protocol
@@ -44,8 +44,8 @@
return nil
}
-// EssentialFlags contains the values of the Essential flag group.
-type EssentialFlags struct {
+// RuntimeFlags contains the values of the Runtime flag group.
+type RuntimeFlags struct {
// NamespaceRoots may be initialized by NAMESPACE_ROOT* enivornment
// variables as well as --veyron.namespace.root. The command line
// will override the environment.
@@ -65,10 +65,10 @@
ListenProxy string
}
-// createAndRegisterEssentialFlags creates and registers the EssentialFlags
+// createAndRegisterRuntimeFlags creates and registers the RuntimeFlags
// group with the supplied flag.FlagSet.
-func createAndRegisterEssentialFlags(fs *flag.FlagSet) *EssentialFlags {
- f := &EssentialFlags{}
+func createAndRegisterRuntimeFlags(fs *flag.FlagSet) *RuntimeFlags {
+ f := &RuntimeFlags{}
fs.Var(&f.namespaceRootsFlag, "veyron.namespace.root", "local namespace root; can be repeated to provided multiple roots")
fs.StringVar(&f.Credentials, "veyron.credentials", "", "directory to use for storing security credentials")
return f
@@ -88,14 +88,16 @@
// CreateAndRegister creates a new set of flag groups as specified by the
// supplied flag group parameters and registers them with the supplied
-// flag.Flagset. The Essential flag group is always included.
+// flag.Flagset.
func CreateAndRegister(fs *flag.FlagSet, groups ...FlagGroup) *Flags {
+ if len(groups) == 0 {
+ return nil
+ }
f := &Flags{FlagSet: fs, groups: make(map[FlagGroup]interface{})}
- f.groups[Essential] = createAndRegisterEssentialFlags(fs)
- for _, s := range groups {
- switch s {
- case Essential:
- // do nothing, always included
+ for _, g := range groups {
+ switch g {
+ case Runtime:
+ f.groups[Runtime] = createAndRegisterRuntimeFlags(fs)
case Listen:
f.groups[Listen] = createAndRegisterListenFlags(fs)
}
@@ -103,10 +105,13 @@
return f
}
-// EssentialFlags returns the Essential flag subset stored in its Flags
+// RuntimeFlags returns the Runtime flag subset stored in its Flags
// instance.
-func (f *Flags) EssentialFlags() EssentialFlags {
- from := f.groups[Essential].(*EssentialFlags)
+func (f *Flags) RuntimeFlags() RuntimeFlags {
+ if p := f.groups[Runtime]; p == nil {
+ return RuntimeFlags{}
+ }
+ from := f.groups[Runtime].(*RuntimeFlags)
to := *from
to.NamespaceRoots = make([]string, len(from.NamespaceRoots))
copy(to.NamespaceRoots, from.NamespaceRoots)
@@ -138,7 +143,7 @@
// legacyEnvInit provides support for the legacy NAMESPACE_ROOT? and
// VEYRON_CREDENTIALS env vars.
-func (es *EssentialFlags) legacyEnvInit() {
+func (es *RuntimeFlags) legacyEnvInit() {
for _, ev := range os.Environ() {
p := strings.SplitN(ev, "=", 2)
if len(p) != 2 {
@@ -156,15 +161,22 @@
// Parse parses the supplied args, as per flag.Parse
func (f *Flags) Parse(args []string) error {
- f.groups[Essential].(*EssentialFlags).legacyEnvInit()
+ hasrt := f.groups[Runtime] != nil
+ if hasrt {
+ f.groups[Runtime].(*RuntimeFlags).legacyEnvInit()
+ }
+
// TODO(cnicolaou): implement a single env var 'VANADIUM_OPTS'
// that can be used to specify any command line.
if err := f.FlagSet.Parse(args); err != nil {
return err
}
- essential := f.groups[Essential].(*EssentialFlags)
- if len(essential.namespaceRootsFlag.roots) > 0 {
- essential.NamespaceRoots = essential.namespaceRootsFlag.roots
+
+ if hasrt {
+ runtime := f.groups[Runtime].(*RuntimeFlags)
+ if len(runtime.namespaceRootsFlag.roots) > 0 {
+ runtime.NamespaceRoots = runtime.namespaceRootsFlag.roots
+ }
}
return nil
}
diff --git a/lib/flags/flags_test.go b/lib/flags/flags_test.go
index e8ca273..8d10c8a 100644
--- a/lib/flags/flags_test.go
+++ b/lib/flags/flags_test.go
@@ -11,25 +11,32 @@
)
func TestFlags(t *testing.T) {
- fl := flags.CreateAndRegister(flag.NewFlagSet("test", flag.ContinueOnError))
+ fs := flag.NewFlagSet("test", flag.ContinueOnError)
+ if flags.CreateAndRegister(fs) != nil {
+ t.Errorf("should have failed")
+ }
+ fl := flags.CreateAndRegister(fs, flags.Runtime)
+ if fl == nil {
+ t.Fatalf("should have returned a non-nil value")
+ }
creds := "creddir"
roots := []string{"ab:cd:ef"}
args := []string{"--veyron.credentials=" + creds, "--veyron.namespace.root=" + roots[0]}
fl.Parse(args)
- es := fl.EssentialFlags()
- if got, want := es.NamespaceRoots, roots; !reflect.DeepEqual(got, want) {
+ rtf := fl.RuntimeFlags()
+ if got, want := rtf.NamespaceRoots, roots; !reflect.DeepEqual(got, want) {
t.Errorf("got %v, want %v", got, want)
}
- if got, want := es.Credentials, creds; !reflect.DeepEqual(got, want) {
+ if got, want := rtf.Credentials, creds; !reflect.DeepEqual(got, want) {
t.Errorf("got %v, want %v", got, want)
}
if got, want := fl.HasGroup(flags.Listen), false; got != want {
t.Errorf("got %t, want %t", got, want)
}
// Make sure we have a deep copy.
- es.NamespaceRoots[0] = "oooh"
- es = fl.EssentialFlags()
- if got, want := es.NamespaceRoots, roots; !reflect.DeepEqual(got, want) {
+ rtf.NamespaceRoots[0] = "oooh"
+ rtf = fl.RuntimeFlags()
+ if got, want := rtf.NamespaceRoots, roots; !reflect.DeepEqual(got, want) {
t.Errorf("got %v, want %v", got, want)
}
}
@@ -37,7 +44,7 @@
func TestFlagError(t *testing.T) {
fs := flag.NewFlagSet("test", flag.ContinueOnError)
fs.SetOutput(ioutil.Discard)
- fl := flags.CreateAndRegister(fs)
+ fl := flags.CreateAndRegister(fs, flags.Runtime)
addr := "192.168.10.1:0"
args := []string{"--xxxveyron.tcp.address=" + addr, "not an arg"}
err := fl.Parse(args)
@@ -50,7 +57,7 @@
}
func TestFlagsGroups(t *testing.T) {
- fl := flags.CreateAndRegister(flag.NewFlagSet("test", flag.ContinueOnError), flags.Listen)
+ fl := flags.CreateAndRegister(flag.NewFlagSet("test", flag.ContinueOnError), flags.Runtime, flags.Listen)
if got, want := fl.HasGroup(flags.Listen), true; got != want {
t.Errorf("got %t, want %t", got, want)
}
@@ -59,7 +66,7 @@
args := []string{"--veyron.tcp.address=" + addr, "--veyron.namespace.root=" + roots[0]}
fl.Parse(args)
lf := fl.ListenFlags()
- if got, want := fl.EssentialFlags().NamespaceRoots, roots; !reflect.DeepEqual(got, want) {
+ if got, want := fl.RuntimeFlags().NamespaceRoots, roots; !reflect.DeepEqual(got, want) {
t.Errorf("got %v, want %v", got, want)
}
if got, want := lf.ListenAddress.String(), addr; got != want {
@@ -81,39 +88,41 @@
defer os.Setenv(rootEnvVar0, oldroot0)
os.Setenv(credEnvVar, "bar")
- fl := flags.CreateAndRegister(flag.NewFlagSet("test", flag.ContinueOnError))
+ fl := flags.CreateAndRegister(flag.NewFlagSet("test", flag.ContinueOnError), flags.Runtime)
if err := fl.Parse([]string{}); err != nil {
t.Fatalf("unexpected error: %s", err)
}
- es := fl.EssentialFlags()
- if got, want := es.Credentials, "bar"; got != want {
+ rtf := fl.RuntimeFlags()
+ if got, want := rtf.Credentials, "bar"; got != want {
t.Errorf("got %q, want %q", got, want)
}
if err := fl.Parse([]string{"--veyron.credentials=baz"}); err != nil {
t.Fatalf("unexpected error: %s", err)
}
- es = fl.EssentialFlags()
- if got, want := es.Credentials, "baz"; got != want {
+ rtf = fl.RuntimeFlags()
+ if got, want := rtf.Credentials, "baz"; got != want {
t.Errorf("got %q, want %q", got, want)
}
os.Setenv(rootEnvVar, "a:1")
os.Setenv(rootEnvVar0, "a:2")
- fl = flags.CreateAndRegister(flag.NewFlagSet("test", flag.ContinueOnError))
+ fl = flags.CreateAndRegister(flag.NewFlagSet("test", flag.ContinueOnError), flags.Runtime)
if err := fl.Parse([]string{}); err != nil {
t.Fatalf("unexpected error: %s", err)
}
- es = fl.EssentialFlags()
- if got, want := es.NamespaceRoots, []string{"a:1", "a:2"}; !reflect.DeepEqual(got, want) {
+ rtf = fl.RuntimeFlags()
+ if got, want := rtf.NamespaceRoots, []string{"a:1", "a:2"}; !reflect.DeepEqual(got, want) {
t.Errorf("got %q, want %q", got, want)
}
- if err := fl.Parse([]string{"--veyron.namespace.root=b:1", "--veyron.namespace.root=b:2", "--veyron.namespace.root=b:3"}); err != nil {
+ if err := fl.Parse([]string{"--veyron.namespace.root=b:1", "--veyron.namespace.root=b:2", "--veyron.namespace.root=b:3", "--veyron.credentials=b:4"}); err != nil {
t.Fatalf("unexpected error: %s", err)
}
- es = fl.EssentialFlags()
- if got, want := es.NamespaceRoots, []string{"b:1", "b:2", "b:3"}; !reflect.DeepEqual(got, want) {
+ rtf = fl.RuntimeFlags()
+ if got, want := rtf.NamespaceRoots, []string{"b:1", "b:2", "b:3"}; !reflect.DeepEqual(got, want) {
t.Errorf("got %q, want %q", got, want)
}
-
+ if got, want := rtf.Credentials, "b:4"; got != want {
+ t.Errorf("got %q, want %q", got, want)
+ }
}
diff --git a/lib/modules/exec.go b/lib/modules/exec.go
index e81d38b..26c8b61 100644
--- a/lib/modules/exec.go
+++ b/lib/modules/exec.go
@@ -101,7 +101,16 @@
newargs := []string{os.Args[0]}
newargs = append(newargs, testFlags()...)
newargs = append(newargs, args...)
- return newargs, append(env, eh.entryPoint)
+ // Be careful to remove any existing ShellEntryPoint env vars. This
+ // can happen when subprocesses run other subprocesses etc.
+ cleaned := make([]string, 0, len(env)+1)
+ for _, e := range env {
+ if strings.HasPrefix(e, ShellEntryPoint+"=") {
+ continue
+ }
+ cleaned = append(cleaned, e)
+ }
+ return newargs, append(cleaned, eh.entryPoint)
}
func (eh *execHandle) start(sh *Shell, env []string, args ...string) (Handle, error) {
@@ -136,8 +145,8 @@
if err := handle.Start(); err != nil {
return nil, err
}
- // TODO(cnicolaou): make this timeout configurable
- err = handle.WaitForReady(time.Minute)
+ vlog.VI(1).Infof("Started: %q, pid %d", eh.name, cmd.Process.Pid)
+ err = handle.WaitForReady(sh.startTimeout)
return eh, err
}
@@ -223,11 +232,17 @@
}
func (child *childRegistrar) dispatch() error {
- ch, _ := vexec.GetChildHandle()
+ ch, err := vexec.GetChildHandle()
+ if err != nil {
+ // This is for debugging only. It's perfectly reasonable for this
+ // error to occur if the process is started by a means other
+ // than the exec library.
+ vlog.VI(1).Infof("failed to get child handle: %s", err)
+ }
+
// Only signal that the child is ready or failed if we successfully get
// a child handle. We most likely failed to get a child handle
// because the subprocess was run directly from the command line.
-
command := os.Getenv(ShellEntryPoint)
if len(command) == 0 {
err := fmt.Errorf("Failed to find entrypoint %q", ShellEntryPoint)
diff --git a/lib/modules/shell.go b/lib/modules/shell.go
index 6919bd3..1bf3e61 100644
--- a/lib/modules/shell.go
+++ b/lib/modules/shell.go
@@ -47,17 +47,19 @@
"os"
"strings"
"sync"
+ "time"
"veyron.io/veyron/veyron2/vlog"
)
// Shell represents the context within which commands are run.
type Shell struct {
- mu sync.Mutex
- env map[string]string
- cmds map[string]*commandDesc
- handles map[Handle]struct{}
- credDir string
+ mu sync.Mutex
+ env map[string]string
+ cmds map[string]*commandDesc
+ handles map[Handle]struct{}
+ credDir string
+ startTimeout time.Duration
}
type commandDesc struct {
@@ -90,9 +92,10 @@
// TODO(cnicolaou): should create a new identity if one doesn't
// already exist
sh := &Shell{
- env: make(map[string]string),
- cmds: make(map[string]*commandDesc),
- handles: make(map[Handle]struct{}),
+ env: make(map[string]string),
+ cmds: make(map[string]*commandDesc),
+ handles: make(map[Handle]struct{}),
+ startTimeout: time.Minute,
}
if flag.Lookup("test.run") != nil && os.Getenv("VEYRON_CREDENTIALS") == "" {
if err := sh.CreateAndUseNewCredentials(); err != nil {
@@ -181,7 +184,9 @@
// ones override the Shell and the Shell ones override the OS ones.
//
// The Shell tracks all of the Handles that it creates so that it can shut
-// them down when asked to.
+// them down when asked to. The returned Handle may be non-nil even when an
+// error is returned, in which case it may be used to retrieve any output
+// from the failed command.
//
// Commands may have already been registered with the Shell using AddFunction
// or AddSubprocess, but if not, they will treated as subprocess commands
@@ -196,7 +201,9 @@
expanded := append([]string{name}, sh.expand(args...)...)
h, err := cmd.factory().start(sh, cenv, expanded...)
if err != nil {
- return nil, err
+ // If the error is a timeout, then h can be used to recover
+ // any output from the process.
+ return h, err
}
sh.mu.Lock()
sh.handles[h] = struct{}{}
@@ -214,6 +221,11 @@
return cmd
}
+// SetStartTimeout sets the timeout for starting subcommands.
+func (sh *Shell) SetStartTimeout(d time.Duration) {
+ sh.startTimeout = d
+}
+
// CommandEnvelope returns the command line and environment that would be
// used for running the subprocess or function if it were started with the
// specifed arguments.
