grim/convey

Merged in efritz/convey (pull request #25)

2017-10-04, Gary Kramlich
cf13ffd0f7c7
Parents 407dca2329ab21a0c5f85e7d
Children 674d936a6785
Merged in efritz/convey (pull request #25)

Fix concurrent state modification

Approved-by: Gary Kramlich
6 files changed, 150 insertions(+), 95 deletions(-)
  • +1 -1
    docker/run.go
  • +5 -3
    intrinsic/extend.go
  • +9 -10
    main.go
  • +1 -1
    plans/plans.go
  • +86 -37
    state/state.go
  • +48 -43
    state/state_test.go
    • --- a/docker/run.go Tue Oct 03 20:49:16 2017 -0500
    +++ b/docker/run.go Wed Oct 04 01:59:14 2017 +0000
    @@ -245,7 +245,7 @@
    }
    cid := strings.TrimSpace(stdout)
    - st.DetachedContainers = append(st.DetachedContainers, cid)
    + st.MarkDetached(cid)
    logger.Infof("started detached container %s", cid)
    --- a/intrinsic/extend.go Tue Oct 03 20:49:16 2017 -0500
    +++ b/intrinsic/extend.go Wed Oct 04 01:59:14 2017 +0000
    @@ -45,10 +45,12 @@
    // While extending, certain environment variables can be expanded into
    // lists. We store this meta information as a stack on the state, which
    // is passed into the inner task execute method (and so on).
    - st.PushExtendFrame(e.Environment, e.Expand, e.ExpandDelimiter)
    - defer st.PopExtendFrame()
    - return e.InnerTask.Execute(name, logger, env, st)
    + return e.InnerTask.Execute(name, logger, env, st.WrapWithExpandableEnv(
    + e.Environment,
    + e.Expand,
    + e.ExpandDelimiter,
    + ))
    }
    func (e *Extend) New() tasks.Task {
    --- a/main.go Tue Oct 03 20:49:16 2017 -0500
    +++ b/main.go Wed Oct 04 01:59:14 2017 +0000
    @@ -131,17 +131,16 @@
    os.Exit(1)
    }
    - st := &state.State{
    - KeepWorkspace: *keep,
    - ForceSequential: *forceSequential,
    - EnableSSHAgent: enableSSHAgent,
    - TaskTimeout: *taskTimeout,
    - Environment: environment.Merge(defEnv, *env),
    + st := state.New()
    + st.KeepWorkspace = *keep
    + st.ForceSequential = *forceSequential
    + st.EnableSSHAgent = enableSSHAgent
    + st.TaskTimeout = *taskTimeout
    + st.Environment = environment.Merge(defEnv, *env)
    + st.DockerConfig = *dockerConfig
    + st.CPUShares = *cpuShares
    + st.Memory = *memory
    - DockerConfig: *dockerConfig,
    - CPUShares: *cpuShares,
    - Memory: *memory,
    - }
    if err := st.Valid(); err != nil {
    fmt.Printf("%s\n", err)
    --- a/plans/plans.go Tue Oct 03 20:49:16 2017 -0500
    +++ b/plans/plans.go Wed Oct 04 01:59:14 2017 +0000
    @@ -64,7 +64,7 @@
    func (p *Plan) teardown(logger *gomol.LogAdapter, st *state.State) {
    // run through the DetachedContainers and stop them
    - for _, cid := range st.DetachedContainers {
    + for _, cid := range st.GetDetached() {
    logger.Infof("removing detached container %s", cid)
    docker.StopContainer(cid, logger, st)
    --- a/state/state.go Tue Oct 03 20:49:16 2017 -0500
    +++ b/state/state.go Wed Oct 04 01:59:14 2017 +0000
    @@ -18,10 +18,10 @@
    package state
    import (
    - "errors"
    "fmt"
    "os"
    "strings"
    + "sync"
    "time"
    "bitbucket.org/rw_grim/convey/environment"
    @@ -38,24 +38,42 @@
    EnableSSHAgent bool
    TaskTimeout time.Duration
    Environment []string
    - ExtendFrames []extendFrame
    DockerConfig string
    CPUShares string
    Memory string
    - DetachedContainers []string
    + // States have the ability to "wrap" another one without
    + // changing the underlying state. This is used by the
    + // extends intrinsic in order to modify the stat without
    + // requiring unique access to the state object during the
    + // execution of the extended task. A past implementation
    + // had modified the stack directly, but that causes an
    + // extended task to clobber other tasks in concurrent mode.
    + parent *State
    + expandables []string
    + expandableDelimiter string
    +
    + // This list is a stash of container names which are run
    + // in detached mode. Appending to this may happen from
    + // multiple goroutines, so this needs to be guarded via
    + // mutex.
    + detachedContainers map[string]struct{}
    + mutex *sync.RWMutex
    }
    -type extendFrame struct {
    - expandable []string
    - delimiter string
    - stashedEnv []string
    +func New() *State {
    + return &State{
    + detachedContainers: map[string]struct{}{},
    + mutex: &sync.RWMutex{},
    + }
    }
    -var errEmptyExtendStack = errors.New("empty extend stack")
    +func (st *State) Valid() error {
    + if st.parent == nil && (st.detachedContainers == nil || st.mutex == nil) {
    + return fmt.Errorf("state must be constructed via New")
    + }
    -func (st *State) Valid() error {
    if st.EnableSSHAgent {
    if val := os.Getenv("SSH_AUTH_SOCK"); val == "" {
    return fmt.Errorf("ssh-agent forwarding requested, but agent not running")
    @@ -88,46 +106,77 @@
    // GetDelimiter returns the highest (outermost extend task) delimiter registered
    // with a given expandable environment variable. Returns true if found by name.
    func (st *State) getDelimiter(name string) (string, bool) {
    - for i := len(st.ExtendFrames) - 1; i >= 0; i-- {
    - frame := st.ExtendFrames[i]
    + if st.parent == nil {
    + return "", false
    + }
    - for _, expandable := range frame.expandable {
    - if expandable == name {
    - return frame.delimiter, true
    - }
    + for _, expandable := range st.expandables {
    + if expandable == name {
    + return st.expandableDelimiter, true
    }
    }
    - return "", false
    + return st.parent.getDelimiter(name)
    +}
    +
    +// WrapWithExpandableEnv will create a shallow clone of the state with a reference
    +// to the current state as "parent" with a modified environment. This creates a local
    +// stack of states which do not interfere with other goroutines. A pop operation is
    +// the same as ignoring the wrapped values and using the underlying state. This stack
    +// is used to map a slice within an extended task.
    +func (st *State) WrapWithExpandableEnv(env, expandable []string, delimiter string) *State {
    + // Merge the environment into this map, but do NOT override anything that
    + // is currently in the state's environment - this has higher precedence.
    + env = environment.Merge(env, st.Environment)
    +
    + return &State{
    + Network: st.Network,
    + Workspace: st.Workspace,
    + KeepWorkspace: st.KeepWorkspace,
    + ForceSequential: st.ForceSequential,
    + EnableSSHAgent: st.EnableSSHAgent,
    + TaskTimeout: st.TaskTimeout,
    + Environment: env,
    + DockerConfig: st.DockerConfig,
    + CPUShares: st.CPUShares,
    + Memory: st.Memory,
    + parent: st,
    + expandables: expandable,
    + expandableDelimiter: delimiter,
    + }
    }
    -// PushExtendFrame will store the set of expandable environment variables
    -// and the current set of environments which are restored on pop. These frames
    -// are used to map a slice within an extended task.
    -func (st *State) PushExtendFrame(env, expandable []string, delimiter string) {
    - st.ExtendFrames = append(st.ExtendFrames, extendFrame{
    - expandable: expandable,
    - delimiter: delimiter,
    - stashedEnv: st.Environment,
    - })
    +// MarkDetached will add the given container name into the list
    +// of containers running in detached mode which must be shut down
    +// at the end of the plan. This falls through directly to the root
    +// state so that states wrapping the global one do not have to sync
    +// additional detached container names.
    +func (st *State) MarkDetached(name string) {
    + if st.parent != nil {
    + st.parent.MarkDetached(name)
    + return
    + }
    - // Merge the environment into this map, but do NOT override anything that
    - // is currently in the state's enviornment - this has higher precedence.
    - st.Environment = environment.Merge(env, st.Environment)
    + st.mutex.Lock()
    + defer st.mutex.Unlock()
    + st.detachedContainers[name] = struct{}{}
    }
    -// PopExtendFrame will remove one frame from the top of the stack.
    -func (st *State) PopExtendFrame() error {
    - idx := len(st.ExtendFrames) - 1
    - if idx < 0 {
    - return errEmptyExtendStack
    +// GetDetached returns a list of all detached containers.
    +func (st *State) GetDetached() []string {
    + if st.parent != nil {
    + return st.parent.GetDetached()
    }
    - last := st.ExtendFrames[idx]
    - st.ExtendFrames = st.ExtendFrames[:idx]
    - st.Environment = last.stashedEnv
    + st.mutex.RLock()
    + defer st.mutex.RUnlock()
    - return nil
    + names := []string{}
    + for key := range st.detachedContainers {
    + names = append(names, key)
    + }
    +
    + return names
    }
    // getName returns the name of the variable `var` if the string is of the
    --- a/state/state_test.go Tue Oct 03 20:49:16 2017 -0500
    +++ b/state/state_test.go Wed Oct 04 01:59:14 2017 +0000
    @@ -37,64 +37,69 @@
    })
    }
    -func (e *stateSuite) TestMap(t sweet.T) {
    - st := &State{}
    - st.Environment = []string{"FOO=BAR"}
    -
    - mapEnv := func(val string) []string {
    +func (s *stateSuite) TestMap(t sweet.T) {
    + mapEnv := func(st *State, val string) []string {
    mapped, err := st.MapSlice([]string{val}, st.Environment)
    Expect(err).To(BeNil())
    return mapped
    }
    - Expect(mapEnv("$X")).To(Equal([]string{"$X"}))
    - Expect(mapEnv("$FOO")).To(Equal([]string{"BAR"}))
    + st1 := &State{}
    + st1.Environment = []string{"FOO=BAR"}
    - st.PushExtendFrame([]string{"X=A;B;C"}, []string{"X"}, ";")
    - Expect(mapEnv("$X")).To(Equal([]string{"A", "B", "C"}))
    - Expect(mapEnv("$FOO")).To(Equal([]string{"BAR"}))
    + Expect(mapEnv(st1, "$X")).To(Equal([]string{"$X"}))
    + Expect(mapEnv(st1, "$FOO")).To(Equal([]string{"BAR"}))
    - st.PushExtendFrame([]string{"BAR=B;A;R::B;A;Z", "FOO=SAME"}, []string{"BAR"}, "::")
    - Expect(mapEnv("$X")).To(Equal([]string{"A", "B", "C"}))
    - Expect(mapEnv("$FOO")).To(Equal([]string{"BAR"}))
    - Expect(mapEnv("$BAR")).To(Equal([]string{"B;A;R", "B;A;Z"}))
    + st2 := st1.WrapWithExpandableEnv([]string{"X=A;B;C"}, []string{"X"}, ";")
    + Expect(mapEnv(st2, "$X")).To(Equal([]string{"A", "B", "C"}))
    + Expect(mapEnv(st2, "$FOO")).To(Equal([]string{"BAR"}))
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(mapEnv("$X")).To(Equal([]string{"$X"}))
    - Expect(mapEnv("$FOO")).To(Equal([]string{"BAR"}))
    + st3 := st2.WrapWithExpandableEnv([]string{"BAR=B;A;R::B;A;Z", "FOO=SAME"}, []string{"BAR"}, "::")
    + Expect(mapEnv(st3, "$X")).To(Equal([]string{"A", "B", "C"}))
    + Expect(mapEnv(st3, "$FOO")).To(Equal([]string{"BAR"}))
    + Expect(mapEnv(st3, "$BAR")).To(Equal([]string{"B;A;R", "B;A;Z"}))
    }
    -func (e *stateSuite) TestPopExtendFrameSize(t sweet.T) {
    - st := &State{}
    - st.PushExtendFrame(nil, nil, "")
    - Expect(st.ExtendFrames).To(HaveLen(1))
    - st.PushExtendFrame(nil, nil, "")
    - Expect(st.ExtendFrames).To(HaveLen(2))
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(st.ExtendFrames).To(HaveLen(1))
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(st.ExtendFrames).To(HaveLen(0))
    +func (s *stateSuite) TestWrapParent(t sweet.T) {
    + st1 := &State{}
    + st1.WrapWithExpandableEnv(nil, nil, "")
    + Expect(st1.parent).To(BeNil())
    +
    + st2 := st1.WrapWithExpandableEnv(nil, nil, "")
    + Expect(st2.parent).To(BeIdenticalTo(st1))
    }
    -func (e *stateSuite) TestPopExtendFrameRestoresEnv(t sweet.T) {
    - st := &State{}
    - st.Environment = []string{"FOO=BAR", "BAR=BAZ"}
    - st.PushExtendFrame([]string{"FOO=BONK", "BAZ=BONK"}, nil, "")
    - Expect(st.Environment).To(HaveLen(3))
    - Expect(st.Environment).To(ConsistOf([]string{"FOO=BAR", "BAR=BAZ", "BAZ=BONK"}))
    - st.PopExtendFrame()
    - Expect(st.Environment).To(HaveLen(2))
    - Expect(st.Environment).To(ConsistOf([]string{"FOO=BAR", "BAR=BAZ"}))
    +func (s *stateSuite) TestWrapWithExpandableEnvMap(t sweet.T) {
    + st1 := &State{}
    + st1.Environment = []string{"FOO=BAR", "BAR=BAZ"}
    + Expect(st1.Environment).To(HaveLen(2))
    + Expect(st1.Environment).To(ConsistOf([]string{"FOO=BAR", "BAR=BAZ"}))
    +
    + st2 := st1.WrapWithExpandableEnv([]string{"FOO=BONK", "BAZ=BONK"}, nil, "")
    + Expect(st2.Environment).To(HaveLen(3))
    + Expect(st2.Environment).To(ConsistOf([]string{"FOO=BAR", "BAR=BAZ", "BAZ=BONK"}))
    }
    -func (e *stateSuite) TestPopExtendFrameEmpty(t sweet.T) {
    - st := &State{}
    - Expect(st.PopExtendFrame()).To(Equal(errEmptyExtendStack))
    -}
    -
    -func (e *stateSuite) TestGetName(t sweet.T) {
    +func (s *stateSuite) TestGetName(t sweet.T) {
    Expect(getName("foo")).To(Equal(""))
    Expect(getName("$FOO")).To(Equal("FOO"))
    Expect(getName("${FOO}")).To(Equal("FOO"))
    }
    +
    +func (s *stateSuite) TestDetached(t sweet.T) {
    + st1 := New()
    + Expect(st1.GetDetached()).To(BeEmpty())
    +
    + st1.MarkDetached("foo")
    + Expect(st1.GetDetached()).To(HaveLen(1))
    + Expect(st1.GetDetached()).To(ConsistOf([]string{"foo"}))
    +
    + st2 := st1.WrapWithExpandableEnv(nil, nil, "")
    + st2.MarkDetached("bar")
    + Expect(st2.GetDetached()).To(HaveLen(2))
    + Expect(st2.GetDetached()).To(ConsistOf([]string{"foo", "bar"}))
    +
    + Expect(st1.GetDetached()).To(HaveLen(2))
    + Expect(st1.GetDetached()).To(ConsistOf([]string{"foo", "bar"}))
    +
    +}