grim/convey

Fix merge conflict.
expand-list
2017-10-03, Eric Fritz
a5aa9f92ae63
Parents cad327117fc4
Children 9fffd37ec00d
Fix merge conflict.
8 files changed, 184 insertions(+), 146 deletions(-)
  • +2 -1
    ChangeLog
  • +2 -2
    command/command.go
  • +1 -1
    docker/run.go
  • +5 -3
    intrinsic/extend.go
  • +9 -10
    main.go
  • +1 -1
    plans/plans.go
  • +86 -37
    state/state.go
  • +78 -91
    state/state_test.go
    • --- a/ChangeLog Mon Oct 02 12:06:42 2017 -0500
    +++ b/ChangeLog Tue Oct 03 21:16:34 2017 -0500
    @@ -1,5 +1,6 @@
    0.10.2:
    - * Nothing yet! Be the first!!
    + * Fix Push/Pop Extend Frame breakage under concurrent execution. Fixed #126 (Eric Fritz)
    + * Fritz fixed a goof. Fixed #125. (Eric Fritz)
    0.10.1: 20170926
    * Add support for multiple tags to the build task. (Eric Fritz)
    --- a/command/command.go Mon Oct 02 12:06:42 2017 -0500
    +++ b/command/command.go Tue Oct 03 21:16:34 2017 -0500
    @@ -26,7 +26,7 @@
    "text/template"
    "time"
    - "github.com/mattn/go-shellwords"
    + "github.com/kballard/go-shellquote"
    "bitbucket.org/rw_grim/convey/logging"
    "github.com/aphistic/gomol"
    @@ -80,7 +80,7 @@
    return nil, err
    }
    - return shellwords.Parse(cmd.String())
    + return shellquote.Split(cmd.String())
    }
    func run(name, cmdTemplate string, params map[string]interface{}, timeout time.Duration, outHandler, errHandler collector) error {
    --- a/docker/run.go Mon Oct 02 12:06:42 2017 -0500
    +++ b/docker/run.go Tue Oct 03 21:16:34 2017 -0500
    @@ -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 Mon Oct 02 12:06:42 2017 -0500
    +++ b/intrinsic/extend.go Tue Oct 03 21:16:34 2017 -0500
    @@ -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 Mon Oct 02 12:06:42 2017 -0500
    +++ b/main.go Tue Oct 03 21:16:34 2017 -0500
    @@ -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 Mon Oct 02 12:06:42 2017 -0500
    +++ b/plans/plans.go Tue Oct 03 21:16:34 2017 -0500
    @@ -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 Mon Oct 02 12:06:42 2017 -0500
    +++ b/state/state.go Tue Oct 03 21:16:34 2017 -0500
    @@ -18,10 +18,10 @@
    package state
    import (
    - "errors"
    "fmt"
    "os"
    "strings"
    + "sync"
    "time"
    "bitbucket.org/rw_grim/convey/environment"
    @@ -40,24 +40,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")
    @@ -162,44 +180,75 @@
    // 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
    }
    --- a/state/state_test.go Mon Oct 02 12:06:42 2017 -0500
    +++ b/state/state_test.go Tue Oct 03 21:16:34 2017 -0500
    @@ -37,131 +37,74 @@
    })
    }
    -func (e *stateSuite) TestMapSlice(t sweet.T) {
    +func (s *stateSuite) TestMapSlice(t sweet.T) {
    + st1 := &State{}
    + st1.Environment = []string{"FOO=BAR"}
    +
    + Expect(mapEnv(st1, "$X")).To(ConsistOf([]string{"$X"}))
    + Expect(mapEnv(st1, "$FOO")).To(ConsistOf([]string{"BAR"}))
    +
    + st2 := st1.WrapWithExpandableEnv([]string{"X=A;B;C"}, []string{"X"}, ";")
    + Expect(mapEnv(st2, "$X")).To(ConsistOf([]string{"A", "B", "C"}))
    + Expect(mapEnv(st2, "$FOO")).To(ConsistOf([]string{"BAR"}))
    +
    + st3 := st2.WrapWithExpandableEnv([]string{"BAR=B;A;R::B;A;Z", "FOO=SAME"}, []string{"BAR"}, "::")
    + Expect(mapEnv(st3, "$X")).To(ConsistOf([]string{"A", "B", "C"}))
    + Expect(mapEnv(st3, "$FOO")).To(ConsistOf([]string{"BAR"}))
    + Expect(mapEnv(st3, "$BAR")).To(ConsistOf([]string{"B;A;R", "B;A;Z"}))
    +}
    +
    +func (s *stateSuite) TestMapSliceComplex(t sweet.T) {
    st := &State{}
    st.Environment = []string{"FOO=BAR"}
    -
    - mapEnv := func(val string) []string {
    - mapped, err := st.MapSlice([]string{val}, st.Environment)
    - Expect(err).To(BeNil())
    - return mapped
    - }
    -
    - Expect(mapEnv("$X")).To(ConsistOf([]string{"$X"}))
    - Expect(mapEnv("$FOO")).To(ConsistOf([]string{"BAR"}))
    -
    - st.PushExtendFrame([]string{"X=A;B;C"}, []string{"X"}, ";")
    - Expect(mapEnv("$X")).To(ConsistOf([]string{"A", "B", "C"}))
    - Expect(mapEnv("$FOO")).To(ConsistOf([]string{"BAR"}))
    -
    - st.PushExtendFrame([]string{"BAR=B;A;R::B;A;Z", "FOO=SAME"}, []string{"BAR"}, "::")
    - Expect(mapEnv("$X")).To(ConsistOf([]string{"A", "B", "C"}))
    - Expect(mapEnv("$FOO")).To(ConsistOf([]string{"BAR"}))
    - Expect(mapEnv("$BAR")).To(ConsistOf([]string{"B;A;R", "B;A;Z"}))
    -
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(mapEnv("$X")).To(ConsistOf([]string{"$X"}))
    - Expect(mapEnv("$FOO")).To(ConsistOf([]string{"BAR"}))
    -}
    -
    -func (e *stateSuite) TestMapSliceComplex(t sweet.T) {
    - st := &State{}
    - st.Environment = []string{"FOO=BAR"}
    - st.PushExtendFrame([]string{"X=A;B;C", "Y=D;E;F"}, []string{"X", "Y"}, ";")
    -
    - mapEnv := func(val string) []string {
    - mapped, err := st.MapSlice([]string{val}, st.Environment)
    - Expect(err).To(BeNil())
    - return mapped
    - }
    + st = st.WrapWithExpandableEnv([]string{"X=A;B;C", "Y=D;E;F"}, []string{"X", "Y"}, ";")
    // No expansion
    - Expect(mapEnv("BONK")).To(ConsistOf([]string{"BONK"}))
    - Expect(mapEnv("$FOO")).To(ConsistOf([]string{"BAR"}))
    + Expect(mapEnv(st, "BONK")).To(ConsistOf([]string{"BONK"}))
    + Expect(mapEnv(st, "$FOO")).To(ConsistOf([]string{"BAR"}))
    // Simple
    - Expect(mapEnv("$X")).To(ConsistOf([]string{"A", "B", "C"}))
    - Expect(mapEnv("$Y")).To(ConsistOf([]string{"D", "E", "F"}))
    + Expect(mapEnv(st, "$X")).To(ConsistOf([]string{"A", "B", "C"}))
    + Expect(mapEnv(st, "$Y")).To(ConsistOf([]string{"D", "E", "F"}))
    // Templated
    - Expect(mapEnv("x${X}x")).To(ConsistOf([]string{"xAx", "xBx", "xCx"}))
    - Expect(mapEnv("x${Y}x")).To(ConsistOf([]string{"xDx", "xEx", "xFx"}))
    + Expect(mapEnv(st, "x${X}x")).To(ConsistOf([]string{"xAx", "xBx", "xCx"}))
    + Expect(mapEnv(st, "x${Y}x")).To(ConsistOf([]string{"xDx", "xEx", "xFx"}))
    // Cartesian product
    - Expect(mapEnv("$X/$Y")).To(ConsistOf([]string{
    + Expect(mapEnv(st, "$X/$Y")).To(ConsistOf([]string{
    "A/D", "A/E", "A/F",
    "B/D", "B/E", "B/F",
    "C/D", "C/E", "C/F",
    }))
    }
    -func (e *stateSuite) TestMapSliceIterative(t sweet.T) {
    +func (s *stateSuite) TestMapSliceIterative(t sweet.T) {
    st := &State{}
    st.Environment = []string{"FOO=BAR"}
    - st.PushExtendFrame([]string{"X=A;x$Y", "Y=B;y$Z", "Z=C;D;E"}, []string{"X", "Y", "Z"}, ";")
    + st = st.WrapWithExpandableEnv([]string{"X=A;x$Y", "Y=B;y$Z", "Z=C;D;E"}, []string{"X", "Y", "Z"}, ";")
    - mapEnv := func(val string) []string {
    - mapped, err := st.MapSlice([]string{val}, st.Environment)
    - Expect(err).To(BeNil())
    - return mapped
    - }
    -
    - Expect(mapEnv("$X")).To(ConsistOf([]string{"A", "xB", "xyC", "xyD", "xyE"}))
    + Expect(mapEnv(st, "$X")).To(ConsistOf([]string{"A", "xB", "xyC", "xyD", "xyE"}))
    }
    -func (e *stateSuite) TestMapSliceIndirect(t sweet.T) {
    +func (s *stateSuite) TestMapSliceIndirect(t sweet.T) {
    st := &State{}
    st.Environment = []string{"FOO=BAR"}
    - st.PushExtendFrame([]string{"X=$Y", "Y=A;$Z;C", "Z=B;$W", "W=D"}, []string{"X", "Y", "Z"}, ";")
    + st = st.WrapWithExpandableEnv([]string{"X=$Y", "Y=A;$Z;C", "Z=B;$W", "W=D"}, []string{"X", "Y", "Z"}, ";")
    - mapEnv := func(val string) []string {
    - mapped, err := st.MapSlice([]string{val}, st.Environment)
    - Expect(err).To(BeNil())
    - return mapped
    - }
    -
    - Expect(mapEnv("$X")).To(ConsistOf([]string{"A", "B", "C", "D"}))
    + Expect(mapEnv(st, "$X")).To(ConsistOf([]string{"A", "B", "C", "D"}))
    }
    -func (e *stateSuite) TestMapSliceRecursive(t sweet.T) {
    +func (s *stateSuite) TestMapSliceRecursive(t sweet.T) {
    st := &State{}
    st.Environment = []string{"FOO=BAR"}
    - st.PushExtendFrame([]string{"X=A;x$Y", "Y=B;y$Z", "Z=C;D;$X"}, []string{"X", "Y", "Z"}, ";")
    + st = st.WrapWithExpandableEnv([]string{"X=A;x$Y", "Y=B;y$Z", "Z=C;D;$X"}, []string{"X", "Y", "Z"}, ";")
    _, err := st.MapSlice([]string{"$X"}, st.Environment)
    Expect(err).NotTo(BeNil())
    Expect(err.Error()).To(ContainSubstring("hit limit"))
    }
    -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 (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 (e *stateSuite) TestPopExtendFrameEmpty(t sweet.T) {
    - st := &State{}
    - Expect(st.PopExtendFrame()).To(Equal(errEmptyExtendStack))
    -}
    -
    func (e *stateSuite) TestGetNames(t sweet.T) {
    names := getNames("foo $bar ${baz} ${bar} $bonk_quux honk")
    Expect(names).To(HaveLen(4))
    @@ -190,3 +133,47 @@
    map[string]string{"a": "3", "b": "5", "c": "7"},
    }))
    }
    +
    +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 (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 (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"}))
    +
    +}
    +
    +func mapEnv(st *State, val string) []string {
    + mapped, err := st.MapSlice([]string{val}, st.Environment)
    + Expect(err).To(BeNil())
    + return mapped
    +}