test: shell: Improve load test

This might have failed once in travis because of a short timeout.
Hopefully if this happens again, we'll now know why.

.travis.yml

@@ -1,10 +1,6 @@
 language: go
 os:
   - linux
-go:
-  - 1.10.x
-  - 1.11.x
-  - tip
 go_import_path: github.com/purpleidea/mgmt
 sudo: true
 dist: xenial
@@ -25,17 +21,27 @@ before_install:
   - git config remote.origin.fetch "+refs/heads/*:refs/remotes/origin/*"
   - git fetch --unshallow
 install: 'make deps'
-script: 'make test'
 matrix:
   fast_finish: false
   allow_failures:
-    - go: 1.11.x
-    - go: tip
-    - os: osx
+  - go: 1.11.x
+  - go: tip
+  - os: osx
   # include only one build for osx for a quicker build as the nr. of these runners are sparse
   include:
-    - os: osx
-      go: 1.10.x
+  - name: "basic tests"
+    go: 1.10.x
+    env: TEST_BLOCK=basic
+  - name: "shell tests"
+    go: 1.10.x
+    env: TEST_BLOCK=shell
+  - name: "race tests"
+    go: 1.10.x
+    env: TEST_BLOCK=race
+  - go: 1.11.x
+  - go: tip
+  - os: osx
+script: 'TEST_BLOCK="$TEST_BLOCK" make test'
 
 # the "secure" channel value is the result of running: ./misc/travis-encrypt.sh
 # with a value of: irc.freenode.net#mgmtconfig to eliminate noise from forks...

Makefile

@@ -16,7 +16,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 SHELL = /usr/bin/env bash
-.PHONY: all art cleanart version program lang path deps run race bindata generate build build-debug crossbuild clean test gofmt yamlfmt format docs rpmbuild mkdirs rpm srpm spec tar upload upload-sources upload-srpms upload-rpms copr tag release
+.PHONY: all art cleanart version program lang path deps run race bindata generate build build-debug crossbuild clean test gofmt yamlfmt format docs rpmbuild mkdirs rpm srpm spec tar upload upload-sources upload-srpms upload-rpms copr tag release funcgen
 .SILENT: clean bindata
 
 # a large amount of output from this `find`, can cause `make` to be much slower!
@@ -117,7 +117,6 @@ race:
 
 # generate go files from non-go source
 bindata: ## generate go files from non-go sources
-	@echo "Generating: bindata..."
 	$(MAKE) --quiet -C bindata
 	$(MAKE) --quiet -C lang/funcs
 
@@ -126,8 +125,7 @@ generate:
 
 lang: ## generates the lexer/parser for the language frontend
 	@# recursively run make in child dir named lang
-	@echo "Generating: lang..."
-	$(MAKE) --quiet -C lang
+	@$(MAKE) --quiet -C lang
 
 # build a `mgmt` binary for current host os/arch
 $(PROGRAM): build/mgmt-${GOHOSTOS}-${GOHOSTARCH} ## build an mgmt binary for current host os/arch
@@ -148,7 +146,7 @@ build-debug: $(PROGRAM)
 # extract os and arch from target pattern
 GOOS=$(firstword $(subst -, ,$*))
 GOARCH=$(lastword $(subst -, ,$*))
-build/mgmt-%: $(GO_FILES) | bindata lang
+build/mgmt-%: $(GO_FILES) | bindata lang funcgen
 	@echo "Building: $(PROGRAM), os/arch: $*, version: $(SVERSION)..."
 	@# reassigning GOOS and GOARCH to make build command copy/pastable
 	@# go 1.10 requires specifying the package for ldflags
@@ -166,6 +164,8 @@ clean: ## clean things up
 	$(MAKE) --quiet -C bindata clean
 	$(MAKE) --quiet -C lang/funcs clean
 	$(MAKE) --quiet -C lang clean
+	rm -f lang/funcs/core/generated_funcs.go || true
+	rm -f lang/funcs/core/generated_funcs_test.go || true
 	[ ! -e $(PROGRAM) ] || rm $(PROGRAM)
 	rm -f *_stringer.go	# generated by `go generate`
 	rm -f *_mock.go		# generated by `go generate`
@@ -360,28 +360,28 @@ releases/$(VERSION)/.mkdir:
 	mkdir -p releases/$(VERSION)/{deb,rpm,pacman}/ && touch releases/$(VERSION)/.mkdir
 
 releases/$(VERSION)/rpm/changelog: $(PROGRAM) releases/$(VERSION)/.mkdir
-	@echo "Generating rpm changelog..."
+	@echo "Generating: rpm changelog..."
 	./misc/make-rpm-changelog.sh $(VERSION)
 
 $(RPM_PKG): releases/$(VERSION)/rpm/changelog
-	@echo "Building rpm package..."
+	@echo "Building: rpm package..."
 	./misc/fpm-pack.sh rpm $(VERSION) libvirt-devel augeas-devel
 
 releases/$(VERSION)/deb/changelog: $(PROGRAM) releases/$(VERSION)/.mkdir
-	@echo "Generating deb changelog..."
+	@echo "Generating: deb changelog..."
 	./misc/make-deb-changelog.sh $(VERSION)
 
 $(DEB_PKG): releases/$(VERSION)/deb/changelog
-	@echo "Building deb package..."
+	@echo "Building: deb package..."
 	./misc/fpm-pack.sh deb $(VERSION) libvirt-dev libaugeas-dev
 
 $(PACMAN_PKG): $(PROGRAM) releases/$(VERSION)/.mkdir
-	@echo "Building pacman package..."
+	@echo "Building: pacman package..."
 	./misc/fpm-pack.sh pacman $(VERSION) libvirt augeas
 
 $(SHA256SUMS): $(RPM_PKG) $(DEB_PKG) $(PACMAN_PKG)
 	@# remove the directory separator in the SHA256SUMS file
-	@echo "Generating sha256 sum..."
+	@echo "Generating: sha256 sum..."
 	sha256sum $(RPM_PKG) $(DEB_PKG) $(PACMAN_PKG) | awk -F '/| ' '{print $$1"  "$$6}' > $(SHA256SUMS)
 
 $(SHA256SUMS_ASC): $(SHA256SUMS)
@@ -408,4 +408,14 @@ help: ## show this help screen
 		awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-20s\033[0m %s\n", $$1, $$2}'
 	@echo ''
 
+funcgen: lang/funcs/core/generated_funcs_test.go lang/funcs/core/generated_funcs.go
+
+lang/funcs/core/generated_funcs_test.go: lang/funcs/funcgen/*.go lang/funcs/core/funcgen.yaml lang/funcs/funcgen/templates/generated_funcs_test.go.tpl
+	@echo "Generating: funcs test..."
+	@go run lang/funcs/funcgen/*.go -templates lang/funcs/funcgen/templates/generated_funcs_test.go.tpl 2>/dev/null
+
+lang/funcs/core/generated_funcs.go: lang/funcs/funcgen/*.go lang/funcs/core/funcgen.yaml lang/funcs/funcgen/templates/generated_funcs.go.tpl
+	@echo "Generating: funcs..."
+	@go run lang/funcs/funcgen/*.go -templates lang/funcs/funcgen/templates/generated_funcs.go.tpl 2>/dev/null
+
 # vim: ts=8

bindata/Makefile

@@ -30,6 +30,7 @@ build: bindata.go
 
 # add more input files as dependencies at the end here...
 bindata.go: ../COPYING
+	@echo "Generating: bindata..."
 	# go-bindata --pkg bindata -o <OUTPUT> <INPUT>
 	go-bindata --pkg bindata -o ./$@ $^
 	# gofmt the output file

converger/converger.go

@@ -29,135 +29,248 @@ import (
 	multierr "github.com/hashicorp/go-multierror"
 )
 
-// TODO: we could make a new function that masks out the state of certain
-// UID's, but at the moment the new Timer code has obsoleted the need...
+// New builds a new converger coordinator.
+func New(timeout int64) *Coordinator {
+	return &Coordinator{
+		timeout: timeout,
 
-// Converger is the general interface for implementing a convergence watcher.
-type Converger interface { // TODO: need a better name
-	Register() UID
-	IsConverged(UID) bool         // is the UID converged ?
-	SetConverged(UID, bool) error // set the converged state of the UID
-	Unregister(UID)
-	Start()
-	Pause()
-	Loop(bool)
-	ConvergedTimer(UID) <-chan time.Time
-	Status() map[uint64]bool
-	Timeout() int                              // returns the timeout that this was created with
-	AddStateFn(string, func(bool) error) error // adds a stateFn with a name
-	RemoveStateFn(string) error                // remove a stateFn with a given name
-}
+		mutex: &sync.RWMutex{},
 
-// UID is the interface resources can use to notify with if converged. You'll
-// need to use part of the Converger interface to Register initially too.
-type UID interface {
-	ID() uint64   // get Id
-	Name() string // get a friendly name
-	SetName(string)
-	IsValid() bool // has Id been initialized ?
-	InvalidateID() // set Id to nil
-	IsConverged() bool
-	SetConverged(bool) error
-	Unregister()
-	ConvergedTimer() <-chan time.Time
-	StartTimer() (func() error, error) // cancellable is the same as StopTimer()
-	ResetTimer() error                 // resets counter to zero
-	StopTimer() error
-}
+		//lastid: 0,
+		status: make(map[*UID]struct{}),
 
-// converger is an implementation of the Converger interface.
-type converger struct {
-	timeout   int           // must be zero (instant) or greater seconds to run
-	converged bool          // did we converge (state changes of this run Fn)
-	channel   chan struct{} // signal here to run an isConverged check
-	control   chan bool     // control channel for start/pause
-	mutex     *sync.RWMutex // used for controlling access to status and lastid
-	lastid    uint64
-	status    map[uint64]bool
-	stateFns  map[string]func(bool) error // run on converged state changes with state bool
-	smutex    *sync.RWMutex               // used for controlling access to stateFns
-}
+		//converged: false, // initial state
 
-// cuid is an implementation of the UID interface.
-type cuid struct {
-	converger Converger
-	id        uint64
-	name      string // user defined, friendly name
-	mutex     *sync.Mutex
-	timer     chan struct{}
-	running   bool // is the above timer running?
-	wg        *sync.WaitGroup
-}
+		pokeChan: make(chan struct{}, 1), // must be buffered
+
+		readyChan: make(chan struct{}), // ready signal
+
+		//paused: false, // starts off as started
+		pauseSignal: make(chan struct{}),
+		//resumeSignal: make(chan struct{}), // happens on pause
+		//pausedAck: util.NewEasyAck(), // happens on pause
 
-// NewConverger builds a new converger struct.
-func NewConverger(timeout int) Converger {
-	return &converger{
-		timeout:  timeout,
-		channel:  make(chan struct{}),
-		control:  make(chan bool),
-		mutex:    &sync.RWMutex{},
-		lastid:   0,
-		status:   make(map[uint64]bool),
 		stateFns: make(map[string]func(bool) error),
 		smutex:   &sync.RWMutex{},
-	}
-}
 
-// Register assigns a UID to the caller.
-func (obj *converger) Register() UID {
-	obj.mutex.Lock()
-	defer obj.mutex.Unlock()
-	obj.lastid++
-	obj.status[obj.lastid] = false // initialize as not converged
-	return &cuid{
-		converger: obj,
-		id:        obj.lastid,
-		name:      fmt.Sprintf("%d", obj.lastid), // some default
-		mutex:     &sync.Mutex{},
-		timer:     nil,
-		running:   false,
+		closeChan: make(chan struct{}),
 		wg:        &sync.WaitGroup{},
 	}
 }
 
-// IsConverged gets the converged status of a uid.
-func (obj *converger) IsConverged(uid UID) bool {
-	if !uid.IsValid() {
-		panic(fmt.Sprintf("the ID of UID(%s) is nil", uid.Name()))
-	}
-	obj.mutex.RLock()
-	isConverged, found := obj.status[uid.ID()] // lookup
-	obj.mutex.RUnlock()
-	if !found {
-		panic("the ID of UID is unregistered")
-	}
-	return isConverged
+// Coordinator is the central converger engine.
+type Coordinator struct {
+	// timeout must be zero (instant) or greater seconds to run. If it's -1
+	// then this is disabled, and we never run stateFns.
+	timeout int64
+
+	// mutex is used for controlling access to status and lastid.
+	mutex *sync.RWMutex
+
+	// lastid contains the last uid we used for registration.
+	//lastid    uint64
+	// status contains a reference to each active UID.
+	status map[*UID]struct{}
+
+	// converged stores the last convergence state. When this changes, we
+	// run the stateFns.
+	converged bool
+
+	// pokeChan receives a message every time we might need to re-calculate.
+	pokeChan chan struct{}
+
+	// readyChan closes to notify any interested parties that the main loop
+	// is running.
+	readyChan chan struct{}
+
+	// paused represents if this coordinator is paused or not.
+	paused bool
+	// pauseSignal closes to request a pause of this coordinator.
+	pauseSignal chan struct{}
+	// resumeSignal closes to request a resume of this coordinator.
+	resumeSignal chan struct{}
+	// pausedAck is used to send an ack message saying that we've paused.
+	pausedAck *util.EasyAck
+
+	// stateFns run on converged state changes.
+	stateFns map[string]func(bool) error
+	// smutex is used for controlling access to the stateFns map.
+	smutex *sync.RWMutex
+
+	// closeChan closes when we've been requested to shutdown.
+	closeChan chan struct{}
+	// wg waits for everything to finish.
+	wg *sync.WaitGroup
 }
 
-// SetConverged updates the converger with the converged state of the UID.
-func (obj *converger) SetConverged(uid UID, isConverged bool) error {
-	if !uid.IsValid() {
-		return fmt.Errorf("the ID of UID(%s) is nil", uid.Name())
-	}
+// Register creates a new UID which can be used to report converged state. You
+// must Unregister each UID before Shutdown will be able to finish running.
+func (obj *Coordinator) Register() *UID {
+	obj.wg.Add(1) // additional tracking for each UID
 	obj.mutex.Lock()
-	if _, found := obj.status[uid.ID()]; !found {
-		panic("the ID of UID is unregistered")
+	defer obj.mutex.Unlock()
+	//obj.lastid++
+	uid := &UID{
+		timeout: obj.timeout, // copy the timeout here
+		//id: obj.lastid,
+		//name: fmt.Sprintf("%d", obj.lastid), // some default
+
+		poke: obj.poke,
+
+		// timer
+		mutex:   &sync.Mutex{},
+		timer:   nil,
+		running: false,
+		wg:      &sync.WaitGroup{},
 	}
-	obj.status[uid.ID()] = isConverged // set
-	obj.mutex.Unlock()                 // unlock *before* poke or deadlock!
-	if isConverged != obj.converged {  // only poke if it would be helpful
-		// run in a go routine so that we never block... just queue up!
-		// this allows us to send events, even if we haven't started...
-		go func() { obj.channel <- struct{}{} }()
+	uid.unregister = func() { obj.Unregister(uid) } // add unregister func
+	obj.status[uid] = struct{}{}                    // TODO: add converged state here?
+	return uid
+}
+
+// Unregister removes the UID from the converger coordinator. If you supply an
+// invalid or unregistered uid to this function, it will panic. An unregistered
+// UID is no longer part of the convergence checking.
+func (obj *Coordinator) Unregister(uid *UID) {
+	defer obj.wg.Done() // additional tracking for each UID
+	obj.mutex.Lock()
+	defer obj.mutex.Unlock()
+
+	if _, exists := obj.status[uid]; !exists {
+		panic("uid is not registered")
 	}
+	uid.StopTimer() // ignore any errors
+	delete(obj.status, uid)
+}
+
+// Run starts the main loop for the converger coordinator. It is commonly run
+// from a go routine. It blocks until the Shutdown method is run to close it.
+// NOTE: when we have very short timeouts, if we start before all the resources
+// have joined the map, then it might appear as if we converged before we did!
+func (obj *Coordinator) Run(startPaused bool) {
+	obj.wg.Add(1)
+	wg := &sync.WaitGroup{} // needed for the startPaused
+	defer wg.Wait()         // don't leave any leftover go routines running
+	if startPaused {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			obj.Pause() // ignore any errors
+			close(obj.readyChan)
+		}()
+	} else {
+		close(obj.readyChan) // we must wait till the wg.Add(1) has happened...
+	}
+	defer obj.wg.Done()
+	for {
+		// pause if one was requested...
+		select {
+		case <-obj.pauseSignal: // channel closes
+			obj.pausedAck.Ack() // send ack
+			// we are paused now, and waiting for resume or exit...
+			select {
+			case <-obj.resumeSignal: // channel closes
+				// resumed!
+
+			case <-obj.closeChan: // we can always escape
+				return
+			}
+
+		case _, ok := <-obj.pokeChan: // we got an event (re-calculate)
+			if !ok {
+				return
+			}
+
+			if err := obj.test(); err != nil {
+				// FIXME: what to do on error ?
+			}
+
+		case <-obj.closeChan: // we can always escape
+			return
+		}
+	}
+}
+
+// Ready blocks until the Run loop has started up. This is useful so that we
+// don't run Shutdown before we've even started up properly.
+func (obj *Coordinator) Ready() {
+	select {
+	case <-obj.readyChan:
+	}
+}
+
+// Shutdown sends a signal to the Run loop that it should exit. This blocks
+// until it does.
+func (obj *Coordinator) Shutdown() {
+	close(obj.closeChan)
+	obj.wg.Wait()
+	close(obj.pokeChan) // free memory?
+}
+
+// Pause pauses the coordinator. It should not be called on an already paused
+// coordinator. It will block until the coordinator pauses with an
+// acknowledgment, or until an exit is requested. If the latter happens it will
+// error. It is NOT thread-safe with the Resume() method so only call either one
+// at a time.
+func (obj *Coordinator) Pause() error {
+	if obj.paused {
+		return fmt.Errorf("already paused")
+	}
+
+	obj.pausedAck = util.NewEasyAck()
+	obj.resumeSignal = make(chan struct{}) // build the resume signal
+	close(obj.pauseSignal)
+
+	// wait for ack (or exit signal)
+	select {
+	case <-obj.pausedAck.Wait(): // we got it!
+		// we're paused
+	case <-obj.closeChan:
+		return fmt.Errorf("closing")
+	}
+	obj.paused = true
+
 	return nil
 }
 
-// isConverged returns true if *every* registered uid has converged.
-func (obj *converger) isConverged() bool {
-	obj.mutex.RLock() // take a read lock
-	defer obj.mutex.RUnlock()
-	for _, v := range obj.status {
+// Resume unpauses the coordinator. It can be safely called on a brand-new
+// coordinator that has just started running without incident. It is NOT
+// thread-safe with the Pause() method, so only call either one at a time.
+func (obj *Coordinator) Resume() {
+	// TODO: do we need a mutex around Resume?
+	if !obj.paused { // no need to unpause brand-new resources
+		return
+	}
+
+	obj.pauseSignal = make(chan struct{}) // rebuild for next pause
+	close(obj.resumeSignal)
+	obj.poke() // unblock and notice the resume if necessary
+
+	obj.paused = false
+
+	// no need to wait for it to resume
+	//return // implied
+}
+
+// poke sends a message to the coordinator telling it that it should re-evaluate
+// whether we're converged or not. This does not block. Do not run this in a
+// goroutine. It must not be called after Shutdown has been called.
+func (obj *Coordinator) poke() {
+	// redundant
+	//if len(obj.pokeChan) > 0 {
+	//	return
+	//}
+
+	select {
+	case obj.pokeChan <- struct{}{}:
+	default: // if chan is now full because more than one poke happened...
+	}
+}
+
+// IsConverged returns true if *every* registered uid has converged. If there
+// are no registered UID's, then this will return true.
+func (obj *Coordinator) IsConverged() bool {
+	for _, v := range obj.Status() {
 		if !v { // everyone must be converged for this to be true
 			return false
 		}
@@ -165,145 +278,40 @@ func (obj *converger) isConverged() bool {
 	return true
 }
 
-// Unregister dissociates the ConvergedUID from the converged checking.
-func (obj *converger) Unregister(uid UID) {
-	if !uid.IsValid() {
-		panic(fmt.Sprintf("the ID of UID(%s) is nil", uid.Name()))
+// test evaluates whether we're converged or not and runs the state change. It
+// is NOT thread-safe.
+func (obj *Coordinator) test() error {
+	// TODO: add these checks elsewhere to prevent anything from running?
+	if obj.timeout < 0 {
+		return nil // nothing to do (only run if timeout is valid)
 	}
-	obj.mutex.Lock()
-	uid.StopTimer() // ignore any errors
-	delete(obj.status, uid.ID())
-	obj.mutex.Unlock()
-	uid.InvalidateID()
-}
 
-// Start causes a Converger object to start or resume running.
-func (obj *converger) Start() {
-	obj.control <- true
-}
+	converged := obj.IsConverged()
+	defer func() {
+		obj.converged = converged // set this only at the end...
+	}()
 
-// Pause causes a Converger object to stop running temporarily.
-func (obj *converger) Pause() { // FIXME: add a sync ACK on pause before return
-	obj.control <- false
-}
-
-// Loop is the main loop for a Converger object. It usually runs in a goroutine.
-// TODO: we could eventually have each resource tell us as soon as it converges,
-// and then keep track of the time delays here, to avoid callers needing select.
-// NOTE: when we have very short timeouts, if we start before all the resources
-// have joined the map, then it might appear as if we converged before we did!
-func (obj *converger) Loop(startPaused bool) {
-	if obj.control == nil {
-		panic("converger not initialized correctly")
-	}
-	if startPaused { // start paused without racing
-		select {
-		case e := <-obj.control:
-			if !e {
-				panic("converger expected true")
-			}
+	if !converged {
+		if !obj.converged { // were we previously also not converged?
+			return nil // nothing to do
 		}
-	}
-	for {
-		select {
-		case e := <-obj.control: // expecting "false" which means pause!
-			if e {
-				panic("converger expected false")
-			}
-			// now i'm paused...
-			select {
-			case e := <-obj.control:
-				if !e {
-					panic("converger expected true")
-				}
-				// restart
-				// kick once to refresh the check...
-				go func() { obj.channel <- struct{}{} }()
-				continue
-			}
 
-		case <-obj.channel:
-			if !obj.isConverged() {
-				if obj.converged { // we're doing a state change
-					// call the arbitrary functions (takes a read lock!)
-					if err := obj.runStateFns(false); err != nil {
-						// FIXME: what to do on error ?
-					}
-				}
-				obj.converged = false
-				continue
-			}
-
-			// we have converged!
-			if obj.timeout >= 0 { // only run if timeout is valid
-				if !obj.converged { // we're doing a state change
-					// call the arbitrary functions (takes a read lock!)
-					if err := obj.runStateFns(true); err != nil {
-						// FIXME: what to do on error ?
-					}
-				}
-			}
-			obj.converged = true
-			// loop and wait again...
-		}
+		// we're doing a state change
+		// call the arbitrary functions (takes a read lock!)
+		return obj.runStateFns(false)
 	}
+
+	// we have converged!
+	if obj.converged { // were we previously also converged?
+		return nil // nothing to do
+	}
+
+	// call the arbitrary functions (takes a read lock!)
+	return obj.runStateFns(true)
 }
 
-// ConvergedTimer adds a timeout to a select call and blocks until then.
-// TODO: this means we could eventually have per resource converged timeouts
-func (obj *converger) ConvergedTimer(uid UID) <-chan time.Time {
-	// be clever: if i'm already converged, this timeout should block which
-	// avoids unnecessary new signals being sent! this avoids fast loops if
-	// we have a low timeout, or in particular a timeout == 0
-	if uid.IsConverged() {
-		// blocks the case statement in select forever!
-		return util.TimeAfterOrBlock(-1)
-	}
-	return util.TimeAfterOrBlock(obj.timeout)
-}
-
-// Status returns a map of the converged status of each UID.
-func (obj *converger) Status() map[uint64]bool {
-	status := make(map[uint64]bool)
-	obj.mutex.RLock() // take a read lock
-	defer obj.mutex.RUnlock()
-	for k, v := range obj.status { // make a copy to avoid the mutex
-		status[k] = v
-	}
-	return status
-}
-
-// Timeout returns the timeout in seconds that converger was created with. This
-// is useful to avoid passing in the timeout value separately when you're
-// already passing in the Converger struct.
-func (obj *converger) Timeout() int {
-	return obj.timeout
-}
-
-// AddStateFn adds a state function to be run on change of converged state.
-func (obj *converger) AddStateFn(name string, stateFn func(bool) error) error {
-	obj.smutex.Lock()
-	defer obj.smutex.Unlock()
-	if _, exists := obj.stateFns[name]; exists {
-		return fmt.Errorf("a stateFn with that name already exists")
-	}
-	obj.stateFns[name] = stateFn
-	return nil
-}
-
-// RemoveStateFn adds a state function to be run on change of converged state.
-func (obj *converger) RemoveStateFn(name string) error {
-	obj.smutex.Lock()
-	defer obj.smutex.Unlock()
-	if _, exists := obj.stateFns[name]; !exists {
-		return fmt.Errorf("a stateFn with that name doesn't exist")
-	}
-	delete(obj.stateFns, name)
-	return nil
-}
-
-// runStateFns runs the listed of stored state functions.
-func (obj *converger) runStateFns(converged bool) error {
+// runStateFns runs the list of stored state functions.
+func (obj *Coordinator) runStateFns(converged bool) error {
 	obj.smutex.RLock()
 	defer obj.smutex.RUnlock()
 	var keys []string
@@ -322,70 +330,119 @@ func (obj *converger) runStateFns(converged bool) error {
 	return err
 }
 
-// ID returns the unique id of this UID object.
-func (obj *cuid) ID() uint64 {
-	return obj.id
+// AddStateFn adds a state function to be run on change of converged state.
+func (obj *Coordinator) AddStateFn(name string, stateFn func(bool) error) error {
+	obj.smutex.Lock()
+	defer obj.smutex.Unlock()
+	if _, exists := obj.stateFns[name]; exists {
+		return fmt.Errorf("a stateFn with that name already exists")
+	}
+	obj.stateFns[name] = stateFn
+	return nil
 }
 
-// Name returns a user defined name for the specific cuid.
-func (obj *cuid) Name() string {
-	return obj.name
+// RemoveStateFn removes a state function from running on change of converged
+// state.
+func (obj *Coordinator) RemoveStateFn(name string) error {
+	obj.smutex.Lock()
+	defer obj.smutex.Unlock()
+	if _, exists := obj.stateFns[name]; !exists {
+		return fmt.Errorf("a stateFn with that name doesn't exist")
+	}
+	delete(obj.stateFns, name)
+	return nil
 }
 
-// SetName sets a user defined name for the specific cuid.
-func (obj *cuid) SetName(name string) {
-	obj.name = name
+// Status returns a map of the converged status of each UID.
+func (obj *Coordinator) Status() map[*UID]bool {
+	status := make(map[*UID]bool)
+	obj.mutex.RLock() // take a read lock
+	defer obj.mutex.RUnlock()
+	for k := range obj.status {
+		status[k] = k.IsConverged()
+	}
+	return status
 }
 
-// IsValid tells us if the id is valid or has already been destroyed.
-func (obj *cuid) IsValid() bool {
-	return obj.id != 0 // an id of 0 is invalid
+// Timeout returns the timeout in seconds that converger was created with. This
+// is useful to avoid passing in the timeout value separately when you're
+// already passing in the Coordinator struct.
+func (obj *Coordinator) Timeout() int64 {
+	return obj.timeout
 }
 
-// InvalidateID marks the id as no longer valid.
-func (obj *cuid) InvalidateID() {
-	obj.id = 0 // an id of 0 is invalid
+// UID represents one of the probes for the converger coordinator. It is created
+// by calling the Register method of the Coordinator struct. It should be freed
+// after use with Unregister.
+type UID struct {
+	// timeout is a copy of the main timeout. It could eventually be used
+	// for per-UID timeouts too.
+	timeout int64
+	// isConverged stores the convergence state of this particular UID.
+	isConverged bool
+
+	// poke stores a reference to the main poke function.
+	poke func()
+	// unregister stores a reference to the unregister function.
+	unregister func()
+
+	// timer
+	mutex   *sync.Mutex
+	timer   chan struct{}
+	running bool // is the timer running?
+	wg      *sync.WaitGroup
 }
 
-// IsConverged is a helper function to the regular IsConverged method.
-func (obj *cuid) IsConverged() bool {
-	return obj.converger.IsConverged(obj)
+// Unregister removes this UID from the converger coordinator. An unregistered
+// UID is no longer part of the convergence checking.
+func (obj *UID) Unregister() {
+	obj.unregister()
 }
 
-// SetConverged is a helper function to the regular SetConverged notification.
-func (obj *cuid) SetConverged(isConverged bool) error {
-	return obj.converger.SetConverged(obj, isConverged)
+// IsConverged reports whether this UID is converged or not.
+func (obj *UID) IsConverged() bool {
+	return obj.isConverged
 }
 
-// Unregister is a helper function to unregister myself.
-func (obj *cuid) Unregister() {
-	obj.converger.Unregister(obj)
+// SetConverged sets the convergence state of this UID. This is used by the
+// running timer if one is started. The timer will overwrite any value set by
+// this method.
+func (obj *UID) SetConverged(isConverged bool) {
+	obj.isConverged = isConverged
+	obj.poke() // notify of change
 }
 
-// ConvergedTimer is a helper around the regular ConvergedTimer method.
-func (obj *cuid) ConvergedTimer() <-chan time.Time {
-	return obj.converger.ConvergedTimer(obj)
+// ConvergedTimer adds a timeout to a select call and blocks until then.
+// TODO: this means we could eventually have per resource converged timeouts
+func (obj *UID) ConvergedTimer() <-chan time.Time {
+	// be clever: if i'm already converged, this timeout should block which
+	// avoids unnecessary new signals being sent! this avoids fast loops if
+	// we have a low timeout, or in particular a timeout == 0
+	if obj.IsConverged() {
+		// blocks the case statement in select forever!
+		return util.TimeAfterOrBlock(-1)
+	}
+	return util.TimeAfterOrBlock(int(obj.timeout))
 }
 
-// StartTimer runs an invisible timer that automatically converges on timeout.
-func (obj *cuid) StartTimer() (func() error, error) {
+// StartTimer runs a timer that sets us as converged on timeout. It also returns
+// a handle to the StopTimer function which should be run before exit.
+func (obj *UID) StartTimer() (func() error, error) {
 	obj.mutex.Lock()
-	if !obj.running {
-		obj.timer = make(chan struct{})
-		obj.running = true
-	} else {
-		obj.mutex.Unlock()
+	defer obj.mutex.Unlock()
+	if obj.running {
 		return obj.StopTimer, fmt.Errorf("timer already started")
 	}
-	obj.mutex.Unlock()
+	obj.timer = make(chan struct{})
+	obj.running = true
 	obj.wg.Add(1)
 	go func() {
 		defer obj.wg.Done()
 		for {
 			select {
 			case _, ok := <-obj.timer: // reset signal channel
-				if !ok { // channel is closed
-					return // false to exit
+				if !ok {
+					return
 				}
 				obj.SetConverged(false)
 
@@ -393,8 +450,8 @@ func (obj *cuid) StartTimer() (func() error, error) {
 				obj.SetConverged(true) // converged!
 				select {
 				case _, ok := <-obj.timer: // reset signal channel
-					if !ok { // channel is closed
-						return // false to exit
+					if !ok {
+						return
 					}
 				}
 			}
@@ -403,8 +460,8 @@ func (obj *cuid) StartTimer() (func() error, error) {
 	return obj.StopTimer, nil
 }
 
-// ResetTimer resets the counter to zero if using a StartTimer internally.
-func (obj *cuid) ResetTimer() error {
+// ResetTimer resets the timer to zero.
+func (obj *UID) ResetTimer() error {
 	obj.mutex.Lock()
 	defer obj.mutex.Unlock()
 	if obj.running {
@@ -414,8 +471,8 @@ func (obj *cuid) ResetTimer() error {
 	return fmt.Errorf("timer hasn't been started")
 }
 
-// StopTimer stops the running timer permanently until a StartTimer is run.
-func (obj *cuid) StopTimer() error {
+// StopTimer stops the running timer.
+func (obj *UID) StopTimer() error {
 	obj.mutex.Lock()
 	defer obj.mutex.Unlock()
 	if !obj.running {

converger/converger_test.go

@@ -0,0 +1,31 @@
+// Mgmt
+// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+// +build !root
+
+package converger
+
+import (
+	"testing"
+)
+
+func TestBufferedChan1(t *testing.T) {
+	ch := make(chan bool, 1)
+	ch <- true
+	close(ch) // closing a channel that's not empty should not block
+	// must be able to exit without blocking anywhere
+}

docker/Dockerfile.development

@@ -1,10 +1,10 @@
-FROM golang:1.9
+FROM golang:1.11
 
 MAINTAINER Michał Czeraszkiewicz <contact@czerasz.com>
 
 # Set the reset cache variable
 # Read more here: http://czerasz.com/2014/11/13/docker-tip-and-tricks/#use-refreshedat-variable-for-better-cache-control
-ENV REFRESHED_AT 2017-11-16
+ENV REFRESHED_AT 2019-02-06
 
 RUN apt-get update
 

docs/resource-guide.md

@@ -307,21 +307,18 @@ running.
 The lifetime of most resources `Watch` method should be spent in an infinite
 loop that is bounded by a `select` call. The `select` call is the point where
 our method hands back control to the engine (and the kernel) so that we can
-sleep until something of interest wakes us up. In this loop we must process
-events from the engine via the `<-obj.init.Events` channel, and receive events
-for our resource itself!
+sleep until something of interest wakes us up. In this loop we must wait until
+we get a shutdown event from the engine via the `<-obj.init.Done` channel, which
+closes when we'd like to shut everything down. At this point you should cleanup,
+and let `Watch` close.
 
 #### Events
 
-If we receive an internal event from the `<-obj.init.Events` channel, we should
-read it with the `obj.init.Read` helper function. This function tells us if we
-should shutdown our resource. It also handles pause functionality which blocks
-our resource temporarily in this method. If this channel shuts down, then we
-should treat that as an exit signal.
-
-When we want to send an event, we use the `Event` helper function. It is also
-important to mark the resource state as `dirty` if we believe it might have
-changed. We do this by calling the `obj.init.Dirty` function.
+If the  `<-obj.init.Done` channel closes, we should shutdown our resource. When
+When we want to send an event, we use the `Event` helper function. This
+automatically marks the resource state as `dirty`. If you're unsure, it's not
+harmful to send the event. This will ultimately cause `CheckApply` to run. This
+method can block if the resource is being paused.
 
 #### Startup
 
@@ -330,8 +327,7 @@ to generate one event to notify the `mgmt` engine that we're now listening
 successfully, so that it can run an initial `CheckApply` to ensure we're safely
 tracking a healthy state and that we didn't miss anything when `Watch` was down
 or from before `mgmt` was running. You must do this by calling the
-`obj.init.Running` method. If it returns an error, you must exit and return that
-error.
+`obj.init.Running` method.
 
 #### Converged
 
@@ -358,41 +354,29 @@ func (obj *FooRes) Watch() error {
 	defer obj.whatever.CloseFoo() // shutdown our Foo
 
 	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
 		select {
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				// shutdown engine
-				// (it is okay if some `defer` code runs first)
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
-
 		// the actual events!
 		case event := <-obj.foo.Events:
 			if is_an_event {
 				send = true
-				obj.init.Dirty() // dirty
 			}
 
 		// event errors
 		case err := <-obj.foo.Errors:
 			return err // will cause a retry or permanent failure
+
+		case <-obj.init.Done: // signal for shutdown request
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event()
 		}
 	}
 }
@@ -567,23 +551,10 @@ ready to detect changes.
 Event sends an event notifying the engine of a possible state change. It is
 only called from within `Watch`.
 
-### Events
+### Done
 
-Events is a channel that we must watch for messages from the engine. When it
-closes, this is a signal to shutdown. It is
-only called from within `Watch`.
-
-### Read
-
-Read processes messages that come in from the `Events` channel. It is a helper
-method that knows how to handle the pause mechanism correctly. It is
-only called from within `Watch`.
-
-### Dirty
-
-Dirty marks the resource state as dirty. This signals to the engine that
-CheckApply will have some work to do in order to converge it. It is
-only called from within `Watch`.
+Done is a channel that closes when the engine wants us to shutdown. It is only
+called from within `Watch`.
 
 ### Refresh
 

engine/error.go

@@ -24,9 +24,6 @@ type Error string
 func (e Error) Error() string { return string(e) }
 
 const (
-	// ErrWatchExit represents an exit from the Watch loop via chan closure.
-	ErrWatchExit = Error("watch exit")
-
-	// ErrSignalExit represents an exit from the Watch loop via exit signal.
-	ErrSignalExit = Error("signal exit")
+	// ErrClosed means we couldn't complete a task because we had closed.
+	ErrClosed = Error("closed")
 )

engine/event/event.go

@@ -1,83 +0,0 @@
-// Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
-// Written by James Shubin <james@shubin.ca> and the project contributors
-//
-// This program is free software: you can redistribute it and/or modify
-// it under the terms of the GNU General Public License as published by
-// the Free Software Foundation, either version 3 of the License, or
-// (at your option) any later version.
-//
-// This program is distributed in the hope that it will be useful,
-// but WITHOUT ANY WARRANTY; without even the implied warranty of
-// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-// GNU General Public License for more details.
-//
-// You should have received a copy of the GNU General Public License
-// along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-// Package event provides some primitives that are used for message passing.
-package event
-
-//go:generate stringer -type=Kind -output=kind_stringer.go
-
-// Kind represents the type of event being passed.
-type Kind int
-
-// The different event kinds are used in different contexts.
-const (
-	KindNil Kind = iota
-	KindStart
-	KindPause
-	KindPoke
-	KindExit
-)
-
-// Pre-built messages so they can be used directly without having to use NewMsg.
-// These are useful when we don't want a response via ACK().
-var (
-	Start = &Msg{Kind: KindStart}
-	Pause = &Msg{Kind: KindPause} // probably unused b/c we want a resp
-	Poke  = &Msg{Kind: KindPoke}
-	Exit  = &Msg{Kind: KindExit}
-)
-
-// Msg is an event primitive that represents a kind of event, and optionally a
-// request for an ACK.
-type Msg struct {
-	Kind Kind
-
-	resp chan struct{}
-}
-
-// NewMsg builds a new message struct. It will want an ACK. If you don't want an
-// ACK then use the pre-built messages in the package variable globals.
-func NewMsg(kind Kind) *Msg {
-	return &Msg{
-		Kind: kind,
-		resp: make(chan struct{}),
-	}
-}
-
-// CanACK determines if an ACK is possible for this message. It does not say
-// whether one has already been sent or not.
-func (obj *Msg) CanACK() bool {
-	return obj.resp != nil
-}
-
-// ACK acknowledges the event. It must not be called more than once for the same
-// event. It unblocks the past and future calls of Wait for this event.
-func (obj *Msg) ACK() {
-	close(obj.resp)
-}
-
-// Wait on ACK for this event. It doesn't matter if this runs before or after
-// the ACK. It will unblock either way.
-// TODO: consider adding a context if it's ever useful.
-func (obj *Msg) Wait() error {
-	select {
-	//case <-ctx.Done():
-	//	return ctx.Err()
-	case <-obj.resp:
-		return nil
-	}
-}

engine/graph/actions.go

@@ -24,10 +24,9 @@ import (
 	"time"
 
 	"github.com/purpleidea/mgmt/engine"
-	"github.com/purpleidea/mgmt/engine/event"
 	"github.com/purpleidea/mgmt/pgraph"
 
-	//multierr "github.com/hashicorp/go-multierror"
+	multierr "github.com/hashicorp/go-multierror"
 	errwrap "github.com/pkg/errors"
 	"golang.org/x/time/rate"
 )
@@ -67,26 +66,24 @@ func (obj *Engine) Process(vertex pgraph.Vertex) error {
 		return fmt.Errorf("vertex is not a Res")
 	}
 
-	// Engine Guarantee: Do not allow CheckApply to run while we are paused.
-	// This makes the resource able to know that synchronous channel sending
-	// to the main loop select in Watch from within CheckApply, will succeed
-	// without blocking because the resource went into a paused state. If we
-	// are using the Poll metaparam, then Watch will (of course) not be run.
-	// FIXME: should this lock be here, or wrapped right around CheckApply ?
-	obj.state[vertex].eventsLock.Lock() // this lock is taken within Event()
-	defer obj.state[vertex].eventsLock.Unlock()
-
 	// backpoke! (can be async)
 	if vs := obj.BadTimestamps(vertex); len(vs) > 0 {
 		// back poke in parallel (sync b/c of waitgroup)
+		wg := &sync.WaitGroup{}
 		for _, v := range obj.graph.IncomingGraphVertices(vertex) {
 			if !pgraph.VertexContains(v, vs) { // only poke what's needed
 				continue
 			}
 
-			go obj.state[v].Poke() // async
+			// doesn't really need to be in parallel, but we can...
+			wg.Add(1)
+			go func(vv pgraph.Vertex) {
+				defer wg.Done()
+				obj.state[vv].Poke() // async
+			}(v)
 
 		}
+		wg.Wait()
 		return nil // can't continue until timestamp is in sequence
 	}
 
@@ -244,14 +241,22 @@ func (obj *Engine) Process(vertex pgraph.Vertex) error {
 
 // Worker is the common run frontend of the vertex. It handles all of the retry
 // and retry delay common code, and ultimately returns the final status of this
-// vertex execution.
+// vertex execution. This function cannot be "re-run" for the same vertex. The
+// retry mechanism stuff happens inside of this. To actually "re-run" you need
+// to remove the vertex and build a new one. The engine guarantees that we do
+// not allow CheckApply to run while we are paused. That is enforced here.
 func (obj *Engine) Worker(vertex pgraph.Vertex) error {
 	res, isRes := vertex.(engine.Res)
 	if !isRes {
 		return fmt.Errorf("vertex is not a resource")
 	}
 
-	defer close(obj.state[vertex].stopped) // done signal
+	// bonus safety check
+	if res.MetaParams().Burst == 0 && !(res.MetaParams().Limit == rate.Inf) { // blocked
+		return fmt.Errorf("permanently limited (rate != Inf, burst = 0)")
+	}
+
+	//defer close(obj.state[vertex].stopped) // done signal
 
 	obj.state[vertex].cuid = obj.Converger.Register()
 	obj.state[vertex].tuid = obj.Converger.Register()
@@ -265,7 +270,28 @@ func (obj *Engine) Worker(vertex pgraph.Vertex) error {
 	obj.state[vertex].wg.Add(1)
 	go func() {
 		defer obj.state[vertex].wg.Done()
-		defer close(obj.state[vertex].outputChan) // we close this on behalf of res
+		defer close(obj.state[vertex].eventsChan) // we close this on behalf of res
+
+		// This is a close reverse-multiplexer. If any of the channels
+		// close, then it will cause the doneChan to close. That way,
+		// multiple different folks can send a close signal, without
+		// every worrying about duplicate channel close panics.
+		obj.state[vertex].wg.Add(1)
+		go func() {
+			defer obj.state[vertex].wg.Done()
+
+			// reverse-multiplexer: any close, causes *the* close!
+			select {
+			case <-obj.state[vertex].processDone:
+			case <-obj.state[vertex].watchDone:
+			case <-obj.state[vertex].limitDone:
+			case <-obj.state[vertex].removeDone:
+			case <-obj.state[vertex].eventsDone:
+			}
+
+			// the main "done" signal gets activated here!
+			close(obj.state[vertex].doneChan)
+		}()
 
 		var err error
 		var retry = res.MetaParams().Retry // lookup the retry value
@@ -283,13 +309,8 @@ func (obj *Engine) Worker(vertex pgraph.Vertex) error {
 						case <-timer.C: // the wait is over
 							return errDelayExpired // special
 
-						case event, ok := <-obj.state[vertex].init.Events:
-							if !ok {
-								return nil
-							}
-							if err := obj.state[vertex].init.Read(event); err != nil {
-								return err
-							}
+						case <-obj.state[vertex].init.Done:
+							return nil
 						}
 					}
 				}()
@@ -308,68 +329,121 @@ func (obj *Engine) Worker(vertex pgraph.Vertex) error {
 				obj.Logf("Watch(%s): Exited(%+v)", vertex, err)
 				obj.state[vertex].cuid.StopTimer() // clean up nicely
 			}
-			if err == nil || err == engine.ErrWatchExit || err == engine.ErrSignalExit {
+			if err == nil { // || err == engine.ErrClosed
 				return // exited cleanly, we're done
 			}
 			// we've got an error...
 			delay = res.MetaParams().Delay
 
 			if retry < 0 { // infinite retries
-				obj.state[vertex].reset()
 				continue
 			}
 			if retry > 0 { // don't decrement past 0
 				retry--
 				obj.state[vertex].init.Logf("retrying Watch after %.4f seconds (%d left)", float64(delay)/1000, retry)
-				obj.state[vertex].reset()
 				continue
 			}
 			//if retry == 0 { // optional
 			//	err = errwrap.Wrapf(err, "permanent watch error")
 			//}
 			break // break out of this and send the error
-		}
+		} // for retry loop
+
 		// this section sends an error...
 		// If the CheckApply loop exits and THEN the Watch fails with an
 		// error, then we'd be stuck here if exit signal didn't unblock!
 		select {
-		case obj.state[vertex].outputChan <- errwrap.Wrapf(err, "watch failed"):
+		case obj.state[vertex].eventsChan <- errwrap.Wrapf(err, "watch failed"):
 			// send
-		case <-obj.state[vertex].exit.Signal():
-			// pass
 		}
 	}()
 
-	// bonus safety check
-	if res.MetaParams().Burst == 0 && !(res.MetaParams().Limit == rate.Inf) { // blocked
-		return fmt.Errorf("permanently limited (rate != Inf, burst = 0)")
-	}
-	var limiter = rate.NewLimiter(res.MetaParams().Limit, res.MetaParams().Burst)
-	// It is important that we shutdown the Watch loop if this exits.
-	// Example, if Process errors permanently, we should ask Watch to exit.
-	defer obj.state[vertex].Event(event.Exit) // signal an exit
-	for {
+	// If this exits cleanly, we must unblock the reverse-multiplexer.
+	// I think this additional close is unnecessary, but it's not harmful.
+	defer close(obj.state[vertex].eventsDone) // causes doneChan to close
+	limiter := rate.NewLimiter(res.MetaParams().Limit, res.MetaParams().Burst)
+	var reserv *rate.Reservation
+	var reterr error
+	var failed bool // has Process permanently failed?
+Loop:
+	for { // process loop
 		select {
-		case err, ok := <-obj.state[vertex].outputChan: // read from watch channel
+		case err, ok := <-obj.state[vertex].eventsChan: // read from watch channel
 			if !ok {
-				return nil
+				return reterr // we only return when chan closes
 			}
+			// If the Watch method exits with an error, then this
+			// channel will get that error propagated to it, which
+			// we then save so we can return it to the caller of us.
 			if err != nil {
-				return err // permanent failure
+				failed = true
+				close(obj.state[vertex].watchDone)    // causes doneChan to close
+				reterr = multierr.Append(reterr, err) // permanent failure
+				continue
 			}
+			if obj.Debug {
+				obj.Logf("event received")
+			}
+			reserv = limiter.ReserveN(time.Now(), 1) // one event
+			// reserv.OK() seems to always be true here!
 
-			// safe to go run the process...
-		case <-obj.state[vertex].exit.Signal(): // TODO: is this needed?
-			return nil
+		case _, ok := <-obj.state[vertex].pokeChan: // read from buffered poke channel
+			if !ok { // we never close it
+				panic("unexpected close of poke channel")
+			}
+			if obj.Debug {
+				obj.Logf("poke received")
+			}
+			reserv = nil // we didn't receive a real event here...
+		}
+		if failed { // don't Process anymore if we've already failed...
+			continue Loop
 		}
 
-		now := time.Now()
-		r := limiter.ReserveN(now, 1) // one event
-		// r.OK() seems to always be true here!
-		d := r.DelayFrom(now)
-		if d > 0 { // delay
+		// drop redundant pokes
+		for len(obj.state[vertex].pokeChan) > 0 {
+			select {
+			case <-obj.state[vertex].pokeChan:
+			default:
+				// race, someone else read one!
+			}
+		}
+
+		// pause if one was requested...
+		select {
+		case <-obj.state[vertex].pauseSignal: // channel closes
+			// NOTE: If we allowed a doneChan below to let us out
+			// of the resumeSignal wait, then we could loop around
+			// and run this again, causing a panic. Instead of this
+			// being made safe with a sync.Once, we instead run a
+			// Resume() call inside of the vertexRemoveFn function,
+			// which should unblock it when we're going to need to.
+			obj.state[vertex].pausedAck.Ack() // send ack
+			// we are paused now, and waiting for resume or exit...
+			select {
+			case <-obj.state[vertex].resumeSignal: // channel closes
+				// resumed!
+				// pass through to allow a Process to try to run
+				// TODO: consider adding this fast pause here...
+				//if obj.fastPause {
+				//	obj.Logf("fast pausing on resume")
+				//	continue
+				//}
+			}
+		default:
+			// no pause requested, keep going...
+		}
+		if failed { // don't Process anymore if we've already failed...
+			continue Loop
+		}
+
+		// limit delay
+		d := time.Duration(0)
+		if reserv != nil {
+			d = reserv.DelayFrom(time.Now())
+		}
+		if reserv != nil && d > 0 { // delay
 			obj.state[vertex].init.Logf("limited (rate: %v/sec, burst: %d, next: %v)", res.MetaParams().Limit, res.MetaParams().Burst, d)
-			var count int
 			timer := time.NewTimer(time.Duration(d) * time.Millisecond)
 		LimitWait:
 			for {
@@ -378,35 +452,38 @@ func (obj *Engine) Worker(vertex pgraph.Vertex) error {
 					break LimitWait
 
 				// consume other events while we're waiting...
-				case e, ok := <-obj.state[vertex].outputChan: // read from watch channel
+				case e, ok := <-obj.state[vertex].eventsChan: // read from watch channel
 					if !ok {
-						// FIXME: is this logic correct?
-						if count == 0 {
-							return nil
-						}
-						// loop, because we have
-						// the previous event to
-						// run process on first!
-						continue
+						return reterr // we only return when chan closes
 					}
 					if e != nil {
-						return e // permanent failure
+						failed = true
+						close(obj.state[vertex].limitDone)  // causes doneChan to close
+						reterr = multierr.Append(reterr, e) // permanent failure
+						break LimitWait
 					}
-					count++                         // count the events...
+					if obj.Debug {
+						obj.Logf("event received in limit")
+					}
+					// TODO: does this get added in properly?
 					limiter.ReserveN(time.Now(), 1) // one event
 				}
 			}
 			timer.Stop() // it's nice to cleanup
 			obj.state[vertex].init.Logf("rate limiting expired!")
 		}
+		if failed { // don't Process anymore if we've already failed...
+			continue Loop
+		}
+		// end of limit delay
 
+		// retry...
 		var err error
 		var retry = res.MetaParams().Retry // lookup the retry value
 		var delay uint64
-	Loop:
+	RetryLoop:
 		for { // retry loop
 			if delay > 0 {
-				var count int
 				timer := time.NewTimer(time.Duration(delay) * time.Millisecond)
 			RetryWait:
 				for {
@@ -415,22 +492,20 @@ func (obj *Engine) Worker(vertex pgraph.Vertex) error {
 						break RetryWait
 
 					// consume other events while we're waiting...
-					case e, ok := <-obj.state[vertex].outputChan: // read from watch channel
+					case e, ok := <-obj.state[vertex].eventsChan: // read from watch channel
 						if !ok {
-							// FIXME: is this logic correct?
-							if count == 0 {
-								// last process error
-								return err
-							}
-							// loop, because we have
-							// the previous event to
-							// run process on first!
-							continue
+							return reterr // we only return when chan closes
 						}
 						if e != nil {
-							return e // permanent failure
+							failed = true
+							close(obj.state[vertex].limitDone)  // causes doneChan to close
+							reterr = multierr.Append(reterr, e) // permanent failure
+							break RetryWait
 						}
-						count++                         // count the events...
+						if obj.Debug {
+							obj.Logf("event received in retry")
+						}
+						// TODO: does this get added in properly?
 						limiter.ReserveN(time.Now(), 1) // one event
 					}
 				}
@@ -438,6 +513,9 @@ func (obj *Engine) Worker(vertex pgraph.Vertex) error {
 				delay = 0    // reset
 				obj.state[vertex].init.Logf("the CheckApply delay expired!")
 			}
+			if failed { // don't Process anymore if we've already failed...
+				continue Loop
+			}
 
 			if obj.Debug {
 				obj.Logf("Process(%s)", vertex)
@@ -447,7 +525,7 @@ func (obj *Engine) Worker(vertex pgraph.Vertex) error {
 				obj.Logf("Process(%s): Return(%+v)", vertex, err)
 			}
 			if err == nil {
-				break Loop
+				break RetryLoop
 			}
 			// we've got an error...
 			delay = res.MetaParams().Delay
@@ -464,15 +542,23 @@ func (obj *Engine) Worker(vertex pgraph.Vertex) error {
 			//	err = errwrap.Wrapf(err, "permanent process error")
 			//}
 
-			// If this exits, defer calls: obj.Event(event.Exit),
-			// which will cause the Watch loop to shutdown. Also,
-			// if the Watch loop shuts down, that will cause this
-			// Process loop to shut down. Also the graph sync can
-			// run an: obj.Event(event.Exit) which causes this to
-			// shutdown as well. Lastly, it is possible that more
-			// that one of these scenarios happens simultaneously.
-			return err
-		}
-	}
+			// It is important that we shutdown the Watch loop if
+			// this dies. If Process fails permanently, we ask it
+			// to exit right here... (It happens when we loop...)
+			failed = true
+			close(obj.state[vertex].processDone)  // causes doneChan to close
+			reterr = multierr.Append(reterr, err) // permanent failure
+			continue
+
+		} // retry loop
+
+		// When this Process loop exits, it's because something has
+		// caused Watch() to shutdown (even if it's our permanent
+		// failure from Process), which caused this channel to close.
+		// On or more exit signals are possible, and more than one can
+		// happen simultaneously.
+
+	} // process loop
+
 	//return nil // unreachable
 }

engine/graph/engine.go

@@ -25,7 +25,6 @@ import (
 
 	"github.com/purpleidea/mgmt/converger"
 	"github.com/purpleidea/mgmt/engine"
-	"github.com/purpleidea/mgmt/engine/event"
 	"github.com/purpleidea/mgmt/pgraph"
 	"github.com/purpleidea/mgmt/util/semaphore"
 
@@ -42,7 +41,7 @@ type Engine struct {
 	// Prefix is a unique directory prefix which can be used. It should be
 	// created if needed.
 	Prefix    string
-	Converger converger.Converger
+	Converger *converger.Coordinator
 
 	Debug bool
 	Logf  func(format string, v ...interface{})
@@ -50,13 +49,14 @@ type Engine struct {
 	graph     *pgraph.Graph
 	nextGraph *pgraph.Graph
 	state     map[pgraph.Vertex]*State
-	waits     map[pgraph.Vertex]*sync.WaitGroup
+	waits     map[pgraph.Vertex]*sync.WaitGroup // wg for the Worker func
 
 	slock *sync.Mutex // semaphore lock
 	semas map[string]*semaphore.Semaphore
 
-	wg *sync.WaitGroup
+	wg *sync.WaitGroup // wg for the whole engine (only used for close)
 
+	paused    bool // are we paused?
 	fastPause bool
 }
 
@@ -84,6 +84,8 @@ func (obj *Engine) Init() error {
 
 	obj.wg = &sync.WaitGroup{}
 
+	obj.paused = true // start off true, so we can Resume after first Commit
+
 	return nil
 }
 
@@ -137,6 +139,7 @@ func (obj *Engine) Apply(fn func(*pgraph.Graph) error) error {
 func (obj *Engine) Commit() error {
 	// TODO: Does this hurt performance or graph changes ?
 
+	start := []func() error{} // functions to run after graphsync to start...
 	vertexAddFn := func(vertex pgraph.Vertex) error {
 		// some of these validation steps happen before this Commit step
 		// in Validate() to avoid erroring here. These are redundant.
@@ -192,12 +195,36 @@ func (obj *Engine) Commit() error {
 		if err := obj.state[vertex].Init(); err != nil {
 			return errwrap.Wrapf(err, "the Res did not Init")
 		}
+
+		fn := func() error {
+			// start the Worker
+			obj.wg.Add(1)
+			obj.waits[vertex].Add(1)
+			go func(v pgraph.Vertex) {
+				defer obj.wg.Done()
+				defer obj.waits[v].Done()
+
+				obj.Logf("Worker(%s)", v)
+				// contains the Watch and CheckApply loops
+				err := obj.Worker(v)
+				obj.Logf("Worker(%s): Exited(%+v)", v, err)
+				obj.state[v].workerErr = err // store the error
+				// If the Rewatch metaparam is true, then this will get
+				// restarted if we do a graph cmp swap. This is why the
+				// graph cmp function runs the removes before the adds.
+				// XXX: This should feed into an $error var in the lang.
+			}(vertex)
+			return nil
+		}
+		start = append(start, fn) // do this at the end, if it's needed
 		return nil
 	}
+
 	free := []func() error{} // functions to run after graphsync to reset...
 	vertexRemoveFn := func(vertex pgraph.Vertex) error {
 		// wait for exit before starting new graph!
-		obj.state[vertex].Event(event.Exit) // signal an exit
+		close(obj.state[vertex].removeDone) // causes doneChan to close
+		obj.state[vertex].Resume()          // unblock from resume
 		obj.waits[vertex].Wait()            // sync
 
 		// close the state and resource
@@ -216,15 +243,58 @@ func (obj *Engine) Commit() error {
 		return nil
 	}
 
+	// add the Worker swap (reload) on error decision into this vertexCmpFn
+	vertexCmpFn := func(v1, v2 pgraph.Vertex) (bool, error) {
+		r1, ok1 := v1.(engine.Res)
+		r2, ok2 := v2.(engine.Res)
+		if !ok1 || !ok2 { // should not happen, previously validated
+			return false, fmt.Errorf("not a Res")
+		}
+		m1 := r1.MetaParams()
+		m2 := r2.MetaParams()
+		swap1, swap2 := true, true // assume default of true
+		if m1 != nil {
+			swap1 = m1.Rewatch
+		}
+		if m2 != nil {
+			swap2 = m2.Rewatch
+		}
+
+		s1, ok1 := obj.state[v1]
+		s2, ok2 := obj.state[v2]
+		x1, x2 := false, false
+		if ok1 {
+			x1 = s1.workerErr != nil && swap1
+		}
+		if ok2 {
+			x2 = s2.workerErr != nil && swap2
+		}
+
+		if x1 || x2 {
+			// We swap, even if they're the same, so that we reload!
+			// This causes an add and remove of the "same" vertex...
+			return false, nil
+		}
+
+		return engine.VertexCmpFn(v1, v2) // do the normal cmp otherwise
+	}
+
 	// If GraphSync succeeds, it updates the receiver graph accordingly...
 	// Running the shutdown in vertexRemoveFn does not need to happen in a
 	// topologically sorted order because it already paused in that order.
 	obj.Logf("graph sync...")
-	if err := obj.graph.GraphSync(obj.nextGraph, engine.VertexCmpFn, vertexAddFn, vertexRemoveFn, engine.EdgeCmpFn); err != nil {
+	if err := obj.graph.GraphSync(obj.nextGraph, vertexCmpFn, vertexAddFn, vertexRemoveFn, engine.EdgeCmpFn); err != nil {
 		return errwrap.Wrapf(err, "error running graph sync")
 	}
-	// we run these afterwards, so that the state structs (that might get
-	// referenced) aren't destroyed while someone might poke or use one.
+	// We run these afterwards, so that we don't unnecessarily start anyone
+	// if GraphSync failed in some way. Otherwise we'd have to do clean up!
+	for _, fn := range start {
+		if err := fn(); err != nil {
+			return errwrap.Wrapf(err, "error running start fn")
+		}
+	}
+	// We run these afterwards, so that the state structs (that might get
+	// referenced) are not destroyed while someone might poke or use one.
 	for _, fn := range free {
 		if err := fn(); err != nil {
 			return errwrap.Wrapf(err, "error running free fn")
@@ -248,50 +318,28 @@ func (obj *Engine) Commit() error {
 	return nil
 }
 
-// Start runs the currently active graph. It also un-pauses the graph if it was
-// paused.
-func (obj *Engine) Start() error {
+// Resume runs the currently active graph. It also un-pauses the graph if it was
+// paused. Very little that is interesting should happen here. It all happens in
+// the Commit method. After Commit, new things are already started, but we still
+// need to Resume any pre-existing resources.
+func (obj *Engine) Resume() error {
+	if !obj.paused {
+		return fmt.Errorf("already resumed")
+	}
+
 	topoSort, err := obj.graph.TopologicalSort()
 	if err != nil {
 		return err
 	}
-	indegree := obj.graph.InDegree() // compute all of the indegree's
+	//indegree := obj.graph.InDegree() // compute all of the indegree's
 	reversed := pgraph.Reverse(topoSort)
 
 	for _, vertex := range reversed {
-		state := obj.state[vertex]
-		state.starter = (indegree[vertex] == 0)
-		var unpause = true // assume true
-
-		if !state.working { // if not running...
-			state.working = true
-			unpause = false // doesn't need unpausing if starting
-			obj.wg.Add(1)
-			obj.waits[vertex].Add(1)
-			go func(v pgraph.Vertex) {
-				defer obj.wg.Done()
-				defer obj.waits[vertex].Done()
-				defer func() {
-					obj.state[v].working = false
-				}()
-
-				obj.Logf("Worker(%s)", v)
-				// contains the Watch and CheckApply loops
-				err := obj.Worker(v)
-				obj.Logf("Worker(%s): Exited(%+v)", v, err)
-			}(vertex)
-		}
-
-		select {
-		case <-state.started:
-		case <-state.stopped: // we failed on Watch start
-		}
-
-		if unpause { // unpause (if needed)
-			obj.state[vertex].Event(event.Start)
-		}
+		//obj.state[vertex].starter = (indegree[vertex] == 0)
+		obj.state[vertex].Resume() // doesn't error
 	}
 	// we wait for everyone to start before exiting!
+	obj.paused = false
 	return nil
 }
 
@@ -302,22 +350,32 @@ func (obj *Engine) Start() error {
 // This is because once you've started a fast pause, some dependencies might
 // have been skipped when fast pausing, and future resources might have missed a
 // poke. In general this is only called when you're trying to hurry up the exit.
+// XXX: Not implemented
 func (obj *Engine) SetFastPause() {
 	obj.fastPause = true
 }
 
-// Pause the active, running graph. At the moment this cannot error.
-func (obj *Engine) Pause(fastPause bool) {
+// Pause the active, running graph.
+func (obj *Engine) Pause(fastPause bool) error {
+	if obj.paused {
+		return fmt.Errorf("already paused")
+	}
+
 	obj.fastPause = fastPause
 	topoSort, _ := obj.graph.TopologicalSort()
 	for _, vertex := range topoSort { // squeeze out the events...
 		// The Event is sent to an unbuffered channel, so this event is
 		// synchronous, and as a result it blocks until it is received.
-		obj.state[vertex].Event(event.Pause)
+		if err := obj.state[vertex].Pause(); err != nil && err != engine.ErrClosed {
+			return err
+		}
 	}
 
+	obj.paused = true
+
 	// we are now completely paused...
 	obj.fastPause = false // reset
+	return nil
 }
 
 // Close triggers a shutdown. Engine must be already paused before this is run.

engine/graph/graph_test.go

@@ -0,0 +1,37 @@
+// Mgmt
+// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+// +build !root
+
+package graph
+
+import (
+	"fmt"
+	"testing"
+
+	multierr "github.com/hashicorp/go-multierror"
+)
+
+func TestMultiErr(t *testing.T) {
+	var err error
+	e := fmt.Errorf("some error")
+	err = multierr.Append(err, e) // build an error from a nil base
+	// ensure that this lib allows us to append to a nil
+	if err == nil {
+		t.Errorf("missing error")
+	}
+}

engine/graph/state.go

@@ -19,14 +19,11 @@ package graph
 
 import (
 	"fmt"
-	"os"
-	"path"
 	"sync"
 	"time"
 
 	"github.com/purpleidea/mgmt/converger"
 	"github.com/purpleidea/mgmt/engine"
-	"github.com/purpleidea/mgmt/engine/event"
 	"github.com/purpleidea/mgmt/pgraph"
 	"github.com/purpleidea/mgmt/util"
 
@@ -51,7 +48,7 @@ type State struct {
 	// created if needed.
 	Prefix string
 
-	//Converger converger.Converger
+	//Converger *converger.Coordinator
 
 	// Debug turns on additional output and behaviours.
 	Debug bool
@@ -61,49 +58,62 @@ type State struct {
 
 	timestamp int64 // last updated timestamp
 	isStateOK bool  // is state OK or do we need to run CheckApply ?
+	workerErr error // did the Worker error?
 
-	// events is a channel of incoming events which is read by the Watch
-	// loop for that resource. It receives events like pause, start, and
-	// poke. The channel shuts down to signal for Watch to exit.
-	eventsChan chan *event.Msg // incoming to resource
-	eventsLock *sync.Mutex     // lock around sending and closing of events channel
-	eventsDone bool            // is channel closed?
+	// doneChan closes when Watch should shut down. When any of the
+	// following channels close, it causes this to close.
+	doneChan chan struct{}
 
-	// outputChan is the channel that the engine listens on for events from
+	// processDone is closed when the Process/CheckApply function fails
+	// permanently, and wants to cause Watch to exit.
+	processDone chan struct{}
+	// watchDone is closed when the Watch function fails permanently, and we
+	// close this to signal we should definitely exit. (Often redundant.)
+	watchDone chan struct{} // could be shared with limitDone
+	// limitDone is closed when the Watch function fails permanently, and we
+	// close this to signal we should definitely exit. This happens inside
+	// of the limit loop of the Process section of Worker.
+	limitDone chan struct{} // could be shared with watchDone
+	// removeDone is closed when the vertexRemoveFn method asks for an exit.
+	// This happens when we're switching graphs. The switch to an "empty" is
+	// the equivalent of asking for a final shutdown.
+	removeDone chan struct{}
+	// eventsDone is closed when we shutdown the Process loop because we
+	// closed without error. In theory this shouldn't happen, but it could
+	// if Watch returns without error for some reason.
+	eventsDone chan struct{}
+
+	// eventsChan is the channel that the engine listens on for events from
 	// the Watch loop for that resource. The event is nil normally, except
 	// when events are sent on this channel from the engine. This only
 	// happens as a signaling mechanism when Watch has shutdown and we want
 	// to notify the Process loop which reads from this.
-	outputChan chan error // outgoing from resource
+	eventsChan chan error // outgoing from resource
 
-	wg   *sync.WaitGroup
-	exit *util.EasyExit
+	// pokeChan is a separate channel that the Process loop listens on to
+	// know when we might need to run Process. It never closes, and is safe
+	// to send on since it is buffered.
+	pokeChan chan struct{} // outgoing from resource
 
-	started chan struct{} // closes when it's started
-	stopped chan struct{} // closes when it's stopped
+	// paused represents if this particular res is paused or not.
+	paused bool
+	// pauseSignal closes to request a pause of this resource.
+	pauseSignal chan struct{}
+	// resumeSignal closes to request a resume of this resource.
+	resumeSignal chan struct{}
+	// pausedAck is used to send an ack message saying that we've paused.
+	pausedAck *util.EasyAck
 
-	starter bool // do we have an indegree of 0 ?
-	working bool // is the Main() loop running ?
+	wg *sync.WaitGroup // used for all vertex specific processes
 
-	cuid converger.UID // primary converger
-	tuid converger.UID // secondary converger
+	cuid *converger.UID // primary converger
+	tuid *converger.UID // secondary converger
 
 	init *engine.Init // a copy of the init struct passed to res Init
 }
 
 // Init initializes structures like channels.
 func (obj *State) Init() error {
-	obj.eventsChan = make(chan *event.Msg)
-	obj.eventsLock = &sync.Mutex{}
-
-	obj.outputChan = make(chan error)
-
-	obj.wg = &sync.WaitGroup{}
-	obj.exit = util.NewEasyExit()
-
-	obj.started = make(chan struct{})
-	obj.stopped = make(chan struct{})
-
 	res, isRes := obj.Vertex.(engine.Res)
 	if !isRes {
 		return fmt.Errorf("vertex is not a Res")
@@ -121,6 +131,25 @@ func (obj *State) Init() error {
 		return fmt.Errorf("the Logf function is missing")
 	}
 
+	obj.doneChan = make(chan struct{})
+
+	obj.processDone = make(chan struct{})
+	obj.watchDone = make(chan struct{})
+	obj.limitDone = make(chan struct{})
+	obj.removeDone = make(chan struct{})
+	obj.eventsDone = make(chan struct{})
+
+	obj.eventsChan = make(chan error)
+
+	obj.pokeChan = make(chan struct{}, 1) // must be buffered
+
+	//obj.paused = false // starts off as started
+	obj.pauseSignal = make(chan struct{})
+	//obj.resumeSignal = make(chan struct{}) // happens on pause
+	//obj.pausedAck = util.NewEasyAck() // happens on pause
+
+	obj.wg = &sync.WaitGroup{}
+
 	//obj.cuid = obj.Converger.Register() // gets registered in Worker()
 	//obj.tuid = obj.Converger.Register() // gets registered in Worker()
 
@@ -129,24 +158,9 @@ func (obj *State) Init() error {
 		Hostname: obj.Hostname,
 
 		// Watch:
-		Running: func() error {
-			obj.tuid.StopTimer()
-			close(obj.started)    // this is reset in the reset func
-			obj.isStateOK = false // assume we're initially dirty
-			// optimization: skip the initial send if not a starter
-			// because we'll get poked from a starter soon anyways!
-			if !obj.starter {
-				return nil
-			}
-			return obj.event()
-		},
-		Event:  obj.event,
-		Events: obj.eventsChan,
-		Read:   obj.read,
-		Dirty: func() { // TODO: should we rename this SetDirty?
-			obj.tuid.StopTimer()
-			obj.isStateOK = false
-		},
+		Running: obj.event,
+		Event:   obj.event,
+		Done:    obj.doneChan,
 
 		// CheckApply:
 		Refresh: func() bool {
@@ -231,187 +245,91 @@ func (obj *State) Close() error {
 	return err
 }
 
-// reset is run to reset the state so that Watch can run a second time. Thus is
-// needed for the Watch retry in particular.
-func (obj *State) reset() {
-	obj.started = make(chan struct{})
-	obj.stopped = make(chan struct{})
-}
-
-// Poke sends a nil message on the outputChan. This channel is used by the
-// resource to signal a possible change. This will cause the Process loop to
-// run if it can.
+// Poke sends a notification on the poke channel. This channel is used to notify
+// the Worker to run the Process/CheckApply when it can. This is used when there
+// is a need to schedule or reschedule some work which got postponed or dropped.
+// This doesn't contain any internal synchronization primitives or wait groups,
+// callers are expected to make sure that they don't leave any of these running
+// by the time the Worker() shuts down.
 func (obj *State) Poke() {
-	// add a wait group on the vertex we're poking!
-	obj.wg.Add(1)
-	defer obj.wg.Done()
-
-	// now that we've added to the wait group, obj.outputChan won't close...
-	// so see if there's an exit signal before we release the wait group!
-	// XXX: i don't think this is necessarily happening, but maybe it is?
-	// XXX: re-write some of the engine to ensure that: "the sender closes"!
-	select {
-	case <-obj.exit.Signal():
-		return // skip sending the poke b/c we're closing
-	default:
-	}
+	// redundant
+	//if len(obj.pokeChan) > 0 {
+	//	return
+	//}
 
 	select {
-	case obj.outputChan <- nil:
-
-	case <-obj.exit.Signal():
+	case obj.pokeChan <- struct{}{}:
+	default: // if chan is now full because more than one poke happened...
 	}
 }
 
-// Event sends a Pause or Start event to the resource. It can also be used to
-// send Poke events, but it's much more efficient to send them directly instead
-// of passing them through the resource.
-func (obj *State) Event(msg *event.Msg) {
-	// TODO: should these happen after the lock?
-	obj.wg.Add(1)
-	defer obj.wg.Done()
+// Pause pauses this resource. It should not be called on any already paused
+// resource. It will block until the resource pauses with an acknowledgment, or
+// until an exit for that resource is seen. If the latter happens it will error.
+// It is NOT thread-safe with the Resume() method so only call either one at a
+// time.
+func (obj *State) Pause() error {
+	if obj.paused {
+		return fmt.Errorf("already paused")
+	}
 
-	obj.eventsLock.Lock()
-	defer obj.eventsLock.Unlock()
+	obj.pausedAck = util.NewEasyAck()
+	obj.resumeSignal = make(chan struct{}) // build the resume signal
+	close(obj.pauseSignal)
+	obj.Poke() // unblock and notice the pause if necessary
 
-	if obj.eventsDone { // closing, skip events...
+	// wait for ack (or exit signal)
+	select {
+	case <-obj.pausedAck.Wait(): // we got it!
+		// we're paused
+	case <-obj.doneChan:
+		return engine.ErrClosed
+	}
+	obj.paused = true
+
+	return nil
+}
+
+// Resume unpauses this resource. It can be safely called on a brand-new
+// resource that has just started running without incident. It is NOT
+// thread-safe with the Pause() method, so only call either one at a time.
+func (obj *State) Resume() {
+	// TODO: do we need a mutex around Resume?
+	if !obj.paused { // no need to unpause brand-new resources
 		return
 	}
 
-	if msg.Kind == event.KindExit { // set this so future events don't deadlock
-		obj.Logf("exit event...")
-		obj.eventsDone = true
-		close(obj.eventsChan) // causes resource Watch loop to close
-		obj.exit.Done(nil)    // trigger exit signal to unblock some cases
-		return
-	}
+	obj.pauseSignal = make(chan struct{}) // rebuild for next pause
+	close(obj.resumeSignal)
+	//obj.Poke() // not needed, we're already waiting for resume
+
+	obj.paused = false
+
+	// no need to wait for it to resume
+	//return // implied
+}
+
+// event is a helper function to send an event to the CheckApply process loop.
+// It can be used for the initial `running` event, or any regular event. You
+// should instead use Poke() to "schedule" a new Process/CheckApply loop when
+// one might be needed. This method will block until we're unpaused and ready to
+// receive on the events channel.
+func (obj *State) event() {
+	obj.setDirty() // assume we're initially dirty
 
 	select {
-	case obj.eventsChan <- msg:
-
-	case <-obj.exit.Signal():
+	case obj.eventsChan <- nil:
+		// send!
 	}
+
+	//return // implied
 }
 
-// read is a helper function used inside the main select statement of resources.
-// If it returns an error, then this is a signal for the resource to exit.
-func (obj *State) read(msg *event.Msg) error {
-	switch msg.Kind {
-	case event.KindPoke:
-		return obj.event() // a poke needs to cause an event...
-	case event.KindStart:
-		return fmt.Errorf("unexpected start")
-	case event.KindPause:
-		// pass
-	case event.KindExit:
-		return engine.ErrSignalExit
-
-	default:
-		return fmt.Errorf("unhandled event: %+v", msg.Kind)
-	}
-
-	// we're paused now
-	select {
-	case msg, ok := <-obj.eventsChan:
-		if !ok {
-			return engine.ErrWatchExit
-		}
-		switch msg.Kind {
-		case event.KindPoke:
-			return fmt.Errorf("unexpected poke")
-		case event.KindPause:
-			return fmt.Errorf("unexpected pause")
-		case event.KindStart:
-			// resumed
-			return nil
-		case event.KindExit:
-			return engine.ErrSignalExit
-
-		default:
-			return fmt.Errorf("unhandled event: %+v", msg.Kind)
-		}
-	}
-}
-
-// event is a helper function to send an event from the resource Watch loop. It
-// can be used for the initial `running` event, or any regular event. If it
-// returns an error, then the Watch loop must return this error and shutdown.
-func (obj *State) event() error {
-	// loop until we sent on obj.outputChan or exit with error
-	for {
-		select {
-		// send "activity" event
-		case obj.outputChan <- nil:
-			return nil // sent event!
-
-		// make sure to keep handling incoming
-		case msg, ok := <-obj.eventsChan:
-			if !ok {
-				return engine.ErrWatchExit
-			}
-			switch msg.Kind {
-			case event.KindPoke:
-				// we're trying to send an event, so swallow the
-				// poke: it's what we wanted to have happen here
-				continue
-			case event.KindStart:
-				return fmt.Errorf("unexpected start")
-			case event.KindPause:
-				// pass
-			case event.KindExit:
-				return engine.ErrSignalExit
-
-			default:
-				return fmt.Errorf("unhandled event: %+v", msg.Kind)
-			}
-		}
-
-		// we're paused now
-		select {
-		case msg, ok := <-obj.eventsChan:
-			if !ok {
-				return engine.ErrWatchExit
-			}
-			switch msg.Kind {
-			case event.KindPoke:
-				return fmt.Errorf("unexpected poke")
-			case event.KindPause:
-				return fmt.Errorf("unexpected pause")
-			case event.KindStart:
-				// resumed
-			case event.KindExit:
-				return engine.ErrSignalExit
-
-			default:
-				return fmt.Errorf("unhandled event: %+v", msg.Kind)
-			}
-		}
-	}
-}
-
-// varDir returns the path to a working directory for the resource. It will try
-// and create the directory first, and return an error if this failed. The dir
-// should be cleaned up by the resource on Close if it wishes to discard the
-// contents. If it does not, then a future resource with the same kind and name
-// may see those contents in that directory. The resource should clean up the
-// contents before use if it is important that nothing exist. It is always
-// possible that contents could remain after an abrupt crash, so do not store
-// overly sensitive data unless you're aware of the risks.
-func (obj *State) varDir(extra string) (string, error) {
-	// Using extra adds additional dirs onto our namespace. An empty extra
-	// adds no additional directories.
-	if obj.Prefix == "" { // safety
-		return "", fmt.Errorf("the VarDir prefix is empty")
-	}
-
-	// an empty string at the end has no effect
-	p := fmt.Sprintf("%s/", path.Join(obj.Prefix, extra))
-	if err := os.MkdirAll(p, 0770); err != nil {
-		return "", errwrap.Wrapf(err, "can't create prefix in: %s", p)
-	}
-
-	// returns with a trailing slash as per the mgmt file res convention
-	return p, nil
+// setDirty marks the resource state as dirty. This signals to the engine that
+// CheckApply will have some work to do in order to converge it.
+func (obj *State) setDirty() {
+	obj.tuid.StopTimer()
+	obj.isStateOK = false
 }
 
 // poll is a replacement for Watch when the Poll metaparameter is used.
@@ -420,34 +338,17 @@ func (obj *State) poll(interval uint32) error {
 	ticker := time.NewTicker(time.Duration(interval) * time.Second)
 	defer ticker.Stop()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
-	var send = false // send event?
 	for {
 		select {
 		case <-ticker.C: // received the timer event
 			obj.init.Logf("polling...")
-			send = true
-			obj.init.Dirty() // dirty
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // signal for shutdown request
+			return nil
 		}
 
-		// do all our event sending all together to avoid duplicate msgs
-		if send {
-			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
-		}
+		obj.init.Event() // notify engine of an event (this can block)
 	}
 }

engine/graph/vardir.go

@@ -0,0 +1,51 @@
+// Mgmt
+// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package graph
+
+import (
+	"fmt"
+	"os"
+	"path"
+
+	errwrap "github.com/pkg/errors"
+)
+
+// varDir returns the path to a working directory for the resource. It will try
+// and create the directory first, and return an error if this failed. The dir
+// should be cleaned up by the resource on Close if it wishes to discard the
+// contents. If it does not, then a future resource with the same kind and name
+// may see those contents in that directory. The resource should clean up the
+// contents before use if it is important that nothing exist. It is always
+// possible that contents could remain after an abrupt crash, so do not store
+// overly sensitive data unless you're aware of the risks.
+func (obj *State) varDir(extra string) (string, error) {
+	// Using extra adds additional dirs onto our namespace. An empty extra
+	// adds no additional directories.
+	if obj.Prefix == "" { // safety
+		return "", fmt.Errorf("the VarDir prefix is empty")
+	}
+
+	// an empty string at the end has no effect
+	p := fmt.Sprintf("%s/", path.Join(obj.Prefix, extra))
+	if err := os.MkdirAll(p, 0770); err != nil {
+		return "", errwrap.Wrapf(err, "can't create prefix in: %s", p)
+	}
+
+	// returns with a trailing slash as per the mgmt file res convention
+	return p, nil
+}

engine/metaparams.go

@@ -37,6 +37,8 @@ var DefaultMetaParams = &MetaParams{
 	Limit: rate.Inf, // defaults to no limit
 	Burst: 0,        // no burst needed on an infinite rate
 	//Sema:  []string{},
+	Rewatch: true,
+	Realize: false, // true would be more awesome, but unexpected for users
 }
 
 // MetaRes is the interface a resource must implement to support meta params.
@@ -81,6 +83,24 @@ type MetaParams struct {
 	// has a count equal to 1, is different from a sema named `foo:1` which
 	// also has a count equal to 1, but is a different semaphore.
 	Sema []string `yaml:"sema"`
+
+	// Rewatch specifies whether we re-run the Watch worker during a swap
+	// if it has errored. When doing a GraphCmp to swap the graphs, if this
+	// is true, and this particular worker has errored, then we'll remove it
+	// and add it back as a new vertex, thus causing it to run again. This
+	// is different from the Retry metaparam which applies during the normal
+	// execution. It is only when this is exhausted that we're in permanent
+	// worker failure, and only then can we rely on this metaparam.
+	Rewatch bool `yaml:"rewatch"`
+
+	// Realize ensures that the resource is guaranteed to converge at least
+	// once before a potential graph swap removes or changes it. This
+	// guarantee is useful for fast changing graphs, to ensure that the
+	// brief creation of a resource is seen. This guarantee does not prevent
+	// against the engine quitting normally, and it can't guarantee it if
+	// the resource is blocked because of a failed pre-requisite resource.
+	// XXX: Not implemented!
+	Realize bool `yaml:"realize"`
 }
 
 // Cmp compares two AutoGroupMeta structs and determines if they're equivalent.
@@ -118,6 +138,13 @@ func (obj *MetaParams) Cmp(meta *MetaParams) error {
 		return errwrap.Wrapf(err, "values for Sema are different")
 	}
 
+	if obj.Rewatch != meta.Rewatch {
+		return fmt.Errorf("values for Rewatch are different")
+	}
+	if obj.Realize != meta.Realize {
+		return fmt.Errorf("values for Realize are different")
+	}
+
 	return nil
 }
 
@@ -147,13 +174,15 @@ func (obj *MetaParams) Copy() *MetaParams {
 		copy(sema, obj.Sema)
 	}
 	return &MetaParams{
-		Noop:  obj.Noop,
-		Retry: obj.Retry,
-		Delay: obj.Delay,
-		Poll:  obj.Poll,
-		Limit: obj.Limit, // FIXME: can we copy this type like this? test me!
-		Burst: obj.Burst,
-		Sema:  sema,
+		Noop:    obj.Noop,
+		Retry:   obj.Retry,
+		Delay:   obj.Delay,
+		Poll:    obj.Poll,
+		Limit:   obj.Limit, // FIXME: can we copy this type like this? test me!
+		Burst:   obj.Burst,
+		Sema:    sema,
+		Rewatch: obj.Rewatch,
+		Realize: obj.Realize,
 	}
 }
 

engine/resources.go

@@ -21,8 +21,6 @@ import (
 	"encoding/gob"
 	"fmt"
 
-	"github.com/purpleidea/mgmt/engine/event"
-
 	errwrap "github.com/pkg/errors"
 	"gopkg.in/yaml.v2"
 )
@@ -93,22 +91,14 @@ type Init struct {
 	// Called from within Watch:
 
 	// Running must be called after your watches are all started and ready.
-	Running func() error
+	Running func()
 
 	// Event sends an event notifying the engine of a possible state change.
-	Event func() error
+	Event func()
 
-	// Events returns a channel that we must watch for messages from the
-	// engine. When it closes, this is a signal to shutdown.
-	Events chan *event.Msg
-
-	// Read processes messages that come in from the Events channel. It is a
-	// helper method that knows how to handle the pause mechanism correctly.
-	Read func(*event.Msg) error
-
-	// Dirty marks the resource state as dirty. This signals to the engine
-	// that CheckApply will have some work to do in order to converge it.
-	Dirty func()
+	// Done returns a channel that will close to signal to us that it's time
+	// for us to shutdown.
+	Done chan struct{}
 
 	// Called from within CheckApply:
 

engine/resources/augeas.go

@@ -135,10 +135,7 @@ func (obj *AugeasRes) Watch() error {
 	}
 	defer obj.recWatcher.Close()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -158,23 +155,15 @@ func (obj *AugeasRes) Watch() error {
 				obj.init.Logf("Event(%s): %v", event.Body.Name, event.Body.Op)
 			}
 			send = true
-			obj.init.Dirty() // dirty
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/aws_ec2.go

@@ -423,9 +423,7 @@ func (obj *AwsEc2Res) longpollWatch() error {
 
 	// We tell the engine that we're running right away. This is not correct,
 	// but the api doesn't have a way to signal when the waiters are ready.
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	// cancellable context used for exiting cleanly
 	ctx, cancel := context.WithCancel(context.TODO())
@@ -488,14 +486,6 @@ func (obj *AwsEc2Res) longpollWatch() error {
 	// process events from the goroutine
 	for {
 		select {
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
-
 		case msg, ok := <-obj.awsChan:
 			if !ok {
 				return nil
@@ -509,15 +499,16 @@ func (obj *AwsEc2Res) longpollWatch() error {
 				continue
 			default:
 				obj.init.Logf("State: %v", msg.state)
-				obj.init.Dirty() // dirty
 				send = true
 			}
+
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
+
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }
@@ -587,14 +578,6 @@ func (obj *AwsEc2Res) snsWatch() error {
 	// process events
 	for {
 		select {
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
-
 		case msg, ok := <-obj.awsChan:
 			if !ok {
 				return nil
@@ -607,20 +590,19 @@ func (obj *AwsEc2Res) snsWatch() error {
 			// is confirmed, we are ready to receive events, so we
 			// can notify the engine that we're running.
 			if msg.event == awsEc2EventWatchReady {
-				if err := obj.init.Running(); err != nil {
-					return err // exit if requested
-				}
+				obj.init.Running() // when started, notify engine that we're running
 				continue
 			}
 			obj.init.Logf("State: %v", msg.event)
-			obj.init.Dirty() // dirty
 			send = true
+
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
+
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/cron.go

@@ -271,10 +271,7 @@ func (obj *CronRes) Watch() error {
 	}
 	defer obj.recWatcher.Close()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -285,7 +282,7 @@ func (obj *CronRes) Watch() error {
 				obj.init.Logf("%+v", event)
 			}
 			send = true
-			obj.init.Dirty() // dirty
+
 		case event, ok := <-obj.recWatcher.Events():
 			// process unit file recwatch events
 			if !ok { // channel shutdown
@@ -298,21 +295,14 @@ func (obj *CronRes) Watch() error {
 				obj.init.Logf("Event(%s): %v", event.Body.Name, event.Body.Op)
 			}
 			send = true
-			obj.init.Dirty() // dirty
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/docker_container.go

@@ -168,10 +168,7 @@ func (obj *DockerContainerRes) Watch() error {
 
 	eventChan, errChan := obj.client.Events(ctx, types.EventsOptions{})
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -184,27 +181,21 @@ func (obj *DockerContainerRes) Watch() error {
 				obj.init.Logf("%+v", event)
 			}
 			send = true
-			obj.init.Dirty() // dirty
+
 		case err, ok := <-errChan:
 			if !ok {
 				return nil
 			}
 			return err
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/exec.go

@@ -20,6 +20,7 @@ package resources
 import (
 	"bufio"
 	"bytes"
+	"context"
 	"fmt"
 	"os/exec"
 	"os/user"
@@ -47,11 +48,14 @@ type ExecRes struct {
 	init *engine.Init
 
 	Cmd        string  `yaml:"cmd"`        // the command to run
+	Cwd        string  `yaml:"cwd"`        // the dir to run the command in (empty means use `pwd` of command)
 	Shell      string  `yaml:"shell"`      // the (optional) shell to use to run the cmd
 	Timeout    int     `yaml:"timeout"`    // the cmd timeout in seconds
 	WatchCmd   string  `yaml:"watchcmd"`   // the watch command to run
+	WatchCwd   string  `yaml:"watchcwd"`   // the dir to run the watch command in (empty means use `pwd` of command)
 	WatchShell string  `yaml:"watchshell"` // the (optional) shell to use to run the watch cmd
 	IfCmd      string  `yaml:"ifcmd"`      // the if command to run
+	IfCwd      string  `yaml:"ifcwd"`      // the dir to run the if command in (empty means use `pwd` of command)
 	IfShell    string  `yaml:"ifshell"`    // the (optional) shell to use to run the if cmd
 	User       string  `yaml:"user"`       // the (optional) user to use to execute the command
 	Group      string  `yaml:"group"`      // the (optional) group to use to execute the command
@@ -122,7 +126,7 @@ func (obj *ExecRes) Watch() error {
 			cmdArgs = []string{"-c", obj.WatchCmd}
 		}
 		cmd := exec.Command(cmdName, cmdArgs...)
-		//cmd.Dir = "" // look for program in pwd ?
+		cmd.Dir = obj.WatchCwd // run program in pwd if ""
 		// ignore signals sent to parent process (we're in our own group)
 		cmd.SysProcAttr = &syscall.SysProcAttr{
 			Setpgid: true,
@@ -151,13 +155,12 @@ func (obj *ExecRes) Watch() error {
 			return errwrap.Wrapf(err, "error starting Cmd")
 		}
 
-		ioChan = obj.bufioChanScanner(scanner)
+		ctx, cancel := context.WithCancel(context.Background())
+		defer cancel() // unblock and cleanup
+		ioChan = obj.bufioChanScanner(ctx, scanner)
 	}
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -177,24 +180,16 @@ func (obj *ExecRes) Watch() error {
 			obj.init.Logf("watch output: %s", data.text)
 			if data.text != "" {
 				send = true
-				obj.init.Dirty() // dirty
 			}
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }
@@ -208,7 +203,6 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 	// have a chance to execute, and all without the check of obj.Refresh()!
 
 	if obj.IfCmd != "" { // if there is no onlyif check, we should just run
-
 		var cmdName string
 		var cmdArgs []string
 		if obj.IfShell == "" {
@@ -224,6 +218,7 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 			cmdArgs = []string{"-c", obj.IfCmd}
 		}
 		cmd := exec.Command(cmdName, cmdArgs...)
+		cmd.Dir = obj.IfCwd // run program in pwd if ""
 		// ignore signals sent to parent process (we're in our own group)
 		cmd.SysProcAttr = &syscall.SysProcAttr{
 			Setpgid: true,
@@ -266,7 +261,7 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 		cmdArgs = []string{"-c", obj.Cmd}
 	}
 	cmd := exec.Command(cmdName, cmdArgs...)
-	//cmd.Dir = "" // look for program in pwd ?
+	cmd.Dir = obj.Cwd // run program in pwd if ""
 	// ignore signals sent to parent process (we're in our own group)
 	cmd.SysProcAttr = &syscall.SysProcAttr{
 		Setpgid: true,
@@ -373,6 +368,9 @@ func (obj *ExecRes) Compare(r engine.Res) bool {
 	if obj.Cmd != res.Cmd {
 		return false
 	}
+	if obj.Cwd != res.Cwd {
+		return false
+	}
 	if obj.Shell != res.Shell {
 		return false
 	}
@@ -382,12 +380,18 @@ func (obj *ExecRes) Compare(r engine.Res) bool {
 	if obj.WatchCmd != res.WatchCmd {
 		return false
 	}
+	if obj.WatchCwd != res.WatchCwd {
+		return false
+	}
 	if obj.WatchShell != res.WatchShell {
 		return false
 	}
 	if obj.IfCmd != res.IfCmd {
 		return false
 	}
+	if obj.IfCwd != res.IfCwd {
+		return false
+	}
 	if obj.IfShell != res.IfShell {
 		return false
 	}
@@ -535,18 +539,26 @@ type bufioOutput struct {
 }
 
 // bufioChanScanner wraps the scanner output in a channel.
-func (obj *ExecRes) bufioChanScanner(scanner *bufio.Scanner) chan *bufioOutput {
+func (obj *ExecRes) bufioChanScanner(ctx context.Context, scanner *bufio.Scanner) chan *bufioOutput {
 	ch := make(chan *bufioOutput)
 	obj.wg.Add(1)
 	go func() {
 		defer obj.wg.Done()
 		defer close(ch)
 		for scanner.Scan() {
-			ch <- &bufioOutput{text: scanner.Text()} // blocks here ?
+			select {
+			case ch <- &bufioOutput{text: scanner.Text()}: // blocks here ?
+			case <-ctx.Done():
+				return
+			}
 		}
 		// on EOF, scanner.Err() will be nil
 		if err := scanner.Err(); err != nil {
-			ch <- &bufioOutput{err: err} // send any misc errors we encounter
+			select {
+			case ch <- &bufioOutput{err: err}: // send any misc errors we encounter
+			case <-ctx.Done():
+				return
+			}
 		}
 	}()
 	return ch

engine/resources/exec_test.go

@@ -31,9 +31,6 @@ func fakeInit(t *testing.T) *engine.Init {
 		t.Logf("test: "+format, v...)
 	}
 	return &engine.Init{
-		Running: func() error {
-			return nil
-		},
 		Debug: debug,
 		Logf:  logf,
 	}

engine/resources/file.go

@@ -194,10 +194,7 @@ func (obj *FileRes) Watch() error {
 	}
 	defer obj.recWatcher.Close()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -217,23 +214,15 @@ func (obj *FileRes) Watch() error {
 				obj.init.Logf("Event(%s): %v", event.Body.Name, event.Body.Op)
 			}
 			send = true
-			obj.init.Dirty() // dirty
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }
@@ -248,7 +237,7 @@ func (obj *FileRes) fileCheckApply(apply bool, src io.ReadSeeker, dst string, sh
 	// TODO: does it make sense to switch dst to an io.Writer ?
 	// TODO: use obj.Force when dealing with symlinks and other file types!
 	if obj.init.Debug {
-		obj.init.Logf("fileCheckApply: %s -> %s", src, dst)
+		obj.init.Logf("fileCheckApply: %v -> %s", src, dst)
 	}
 
 	srcFile, isFile := src.(*os.File)
@@ -345,7 +334,7 @@ func (obj *FileRes) fileCheckApply(apply bool, src io.ReadSeeker, dst string, sh
 		return sha256sum, false, nil
 	}
 	if obj.init.Debug {
-		obj.init.Logf("fileCheckApply: Apply: %s -> %s", src, dst)
+		obj.init.Logf("fileCheckApply: Apply: %v -> %s", src, dst)
 	}
 
 	dstClose() // unlock file usage so we can write to it
@@ -366,7 +355,7 @@ func (obj *FileRes) fileCheckApply(apply bool, src io.ReadSeeker, dst string, sh
 
 	// TODO: should we offer a way to cancel the copy on ^C ?
 	if obj.init.Debug {
-		obj.init.Logf("fileCheckApply: Copy: %s -> %s", src, dst)
+		obj.init.Logf("fileCheckApply: Copy: %v -> %s", src, dst)
 	}
 	if n, err := io.Copy(dstFile, src); err != nil {
 		return sha256sum, false, err

engine/resources/group.go

@@ -85,10 +85,7 @@ func (obj *GroupRes) Watch() error {
 	}
 	defer obj.recWatcher.Close()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -108,23 +105,15 @@ func (obj *GroupRes) Watch() error {
 				obj.init.Logf("Event(%s): %v", event.Body.Name, event.Body.Op)
 			}
 			send = true
-			obj.init.Dirty() // dirty
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/hostname.go

@@ -127,33 +127,22 @@ func (obj *HostnameRes) Watch() error {
 	signals := make(chan *dbus.Signal, 10) // closed by dbus package
 	bus.Signal(signals)
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
 		select {
 		case <-signals:
 			send = true
-			obj.init.Dirty() // dirty
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/kv.go

@@ -102,11 +102,7 @@ func (obj *KVRes) Close() error {
 
 // Watch is the primary listener for this resource and it outputs events.
 func (obj *KVRes) Watch() error {
-
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	ch := obj.init.World.StrMapWatch(obj.Key) // get possible events!
 
@@ -125,23 +121,15 @@ func (obj *KVRes) Watch() error {
 				obj.init.Logf("Event!")
 			}
 			send = true
-			obj.init.Dirty() // dirty
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/mount.go

@@ -224,10 +224,7 @@ func (obj *MountRes) Watch() error {
 	// close the recwatcher when we're done
 	defer recWatcher.Close()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // bubble up a NACK...
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send bool
 	var done bool
@@ -248,7 +245,6 @@ func (obj *MountRes) Watch() error {
 				obj.init.Logf("event(%s): %v", event.Body.Name, event.Body.Op)
 			}
 
-			obj.init.Dirty()
 			send = true
 
 		case event, ok := <-ch:
@@ -263,24 +259,16 @@ func (obj *MountRes) Watch() error {
 				obj.init.Logf("event: %+v", event)
 			}
 
-			obj.init.Dirty()
 			send = true
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/mount_linux_test.go

@@ -0,0 +1,76 @@
+// Mgmt
+// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+// +build !root !darwin
+
+package resources
+
+import (
+	"io/ioutil"
+	"os"
+	"testing"
+
+	fstab "github.com/deniswernert/go-fstab"
+)
+
+func TestMountExists(t *testing.T) {
+	const procMock1 = `/tmp/mount0 /mnt/proctest ext4 rw,seclabel,relatime,data=ordered 0 0` + "\n"
+
+	var mountExistsTests = []struct {
+		procMock []byte
+		in       *fstab.Mount
+		out      bool
+	}{
+		{
+			[]byte(procMock1),
+			&fstab.Mount{
+				Spec:    "/tmp/mount0",
+				File:    "/mnt/proctest",
+				VfsType: "ext4",
+				MntOps:  map[string]string{"defaults": ""},
+				Freq:    1,
+				PassNo:  1,
+			},
+			true,
+		},
+	}
+
+	file, err := ioutil.TempFile("", "proc")
+	if err != nil {
+		t.Errorf("error creating temp file: %v", err)
+		return
+	}
+	defer os.Remove(file.Name())
+	for _, test := range mountExistsTests {
+		if err := ioutil.WriteFile(file.Name(), test.procMock, 0664); err != nil {
+			t.Errorf("error writing proc file: %s: %v", file.Name(), err)
+			return
+		}
+		if err := ioutil.WriteFile(test.in.Spec, []byte{}, 0664); err != nil {
+			t.Errorf("error writing fstab file: %s: %v", file.Name(), err)
+			return
+		}
+		result, err := mountExists(file.Name(), test.in)
+		if err != nil {
+			t.Errorf("error checking if fstab entry %s exists: %v", test.in.String(), err)
+			return
+		}
+		if result != test.out {
+			t.Errorf("mountExistsTests test wanted: %t, got: %t", test.out, result)
+		}
+	}
+}

engine/resources/mount_test.go

@@ -29,8 +29,6 @@ import (
 
 const fstabMock1 = `UUID=ef5726f2-615c-4350-b0ab-f106e5fc90ad / ext4 defaults 1 1` + "\n"
 
-const procMock1 = `/tmp/mount0 /mnt/proctest ext4 rw,seclabel,relatime,data=ordered 0 0` + "\n"
-
 var fstabWriteTests = []struct {
 	in fstab.Mounts
 }{
@@ -295,49 +293,3 @@ func TestMountCompare(t *testing.T) {
 		}
 	}
 }
-
-var mountExistsTests = []struct {
-	procMock []byte
-	in       *fstab.Mount
-	out      bool
-}{
-	{
-		[]byte(procMock1),
-		&fstab.Mount{
-			Spec:    "/tmp/mount0",
-			File:    "/mnt/proctest",
-			VfsType: "ext4",
-			MntOps:  map[string]string{"defaults": ""},
-			Freq:    1,
-			PassNo:  1,
-		},
-		true,
-	},
-}
-
-func TestMountExists(t *testing.T) {
-	file, err := ioutil.TempFile("", "proc")
-	if err != nil {
-		t.Errorf("error creating temp file: %v", err)
-		return
-	}
-	defer os.Remove(file.Name())
-	for _, test := range mountExistsTests {
-		if err := ioutil.WriteFile(file.Name(), test.procMock, 0664); err != nil {
-			t.Errorf("error writing proc file: %s: %v", file.Name(), err)
-			return
-		}
-		if err := ioutil.WriteFile(test.in.Spec, []byte{}, 0664); err != nil {
-			t.Errorf("error writing fstab file: %s: %v", file.Name(), err)
-			return
-		}
-		result, err := mountExists(file.Name(), test.in)
-		if err != nil {
-			t.Errorf("error checking if fstab entry %s exists: %v", test.in.String(), err)
-			return
-		}
-		if result != test.out {
-			t.Errorf("mountExistsTests test wanted: %t, got: %t", test.out, result)
-		}
-	}
-}

engine/resources/msg.go

@@ -94,30 +94,20 @@ func (obj *MsgRes) Close() error {
 
 // Watch is the primary listener for this resource and it outputs events.
 func (obj *MsgRes) Watch() error {
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
-	var send = false // send event?
+	//var send = false // send event?
 	for {
 		select {
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
-		if send {
-			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
-		}
+		//if send {
+		//	send = false
+		//	obj.init.Event() // notify engine of an event (this can block)
+		//}
 	}
 }
 
@@ -137,7 +127,7 @@ func (obj *MsgRes) isAllStateOK() bool {
 func (obj *MsgRes) updateStateOK() {
 	// XXX: this resource doesn't entirely make sense to me at the moment.
 	if !obj.isAllStateOK() {
-		obj.init.Dirty()
+		//obj.init.Dirty() // XXX: removed with API cleanup
 	}
 }
 

engine/resources/net.go

@@ -34,6 +34,7 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/socketset"
 
 	multierr "github.com/hashicorp/go-multierror"
 	errwrap "github.com/pkg/errors"
@@ -190,16 +191,20 @@ func (obj *NetRes) Close() error {
 // TODO: currently gets events from ALL interfaces, would be nice to reject
 // events from other interfaces.
 func (obj *NetRes) Watch() error {
-	// waitgroup for netlink receive goroutine
-	wg := &sync.WaitGroup{}
-	defer wg.Wait()
-
 	// create a netlink socket for receiving network interface events
-	conn, err := newSocketSet(rtmGrps, obj.socketFile)
+	conn, err := socketset.NewSocketSet(rtmGrps, obj.socketFile, unix.NETLINK_ROUTE)
 	if err != nil {
 		return errwrap.Wrapf(err, "error creating socket set")
 	}
-	defer conn.shutdown() // close the netlink socket and unblock conn.receive()
+
+	// waitgroup for netlink receive goroutine
+	wg := &sync.WaitGroup{}
+	defer conn.Close()
+	// We must wait for the Shutdown() AND the select inside of SocketSet to
+	// complete before we Close, since the unblocking in SocketSet is not a
+	// synchronous operation.
+	defer wg.Wait()
+	defer conn.Shutdown() // close the netlink socket and unblock conn.receive()
 
 	// watch the systemd-networkd configuration file
 	recWatcher, err := recwatch.NewRecWatcher(obj.unitFilePath, false)
@@ -219,11 +224,10 @@ func (obj *NetRes) Watch() error {
 	wg.Add(1)
 	go func() {
 		defer wg.Done()
-		defer conn.close() // close the pipe when we're done with it
 		defer close(nlChan)
 		for {
 			// receive messages from the socket set
-			msgs, err := conn.receive()
+			msgs, err := conn.ReceiveNetlinkMessages()
 			if err != nil {
 				select {
 				case nlChan <- &nlChanStruct{
@@ -243,10 +247,7 @@ func (obj *NetRes) Watch() error {
 		}
 	}()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	var done bool
@@ -268,7 +269,6 @@ func (obj *NetRes) Watch() error {
 			}
 
 			send = true
-			obj.init.Dirty() // dirty
 
 		case event, ok := <-recWatcher.Events():
 			if !ok {
@@ -286,23 +286,15 @@ func (obj *NetRes) Watch() error {
 			}
 
 			send = true
-			obj.init.Dirty() // dirty
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }
@@ -768,122 +760,3 @@ func (obj *iface) addrApplyAdd(objAddrs []string) error {
 	}
 	return nil
 }
-
-// socketSet is used to receive events from a socket and shut it down cleanly
-// when asked. It contains a socket for events and a pipe socket to unblock
-// receive on shutdown.
-type socketSet struct {
-	fdEvents int
-	fdPipe   int
-	pipeFile string
-}
-
-// newSocketSet returns a socketSet, initialized with the given parameters.
-func newSocketSet(groups uint32, file string) (*socketSet, error) {
-	// make a netlink socket file descriptor
-	fdEvents, err := unix.Socket(unix.AF_NETLINK, unix.SOCK_RAW, unix.NETLINK_ROUTE)
-	if err != nil {
-		return nil, errwrap.Wrapf(err, "error creating netlink socket")
-	}
-	// bind to the socket and add add the netlink groups we need to get events
-	if err := unix.Bind(fdEvents, &unix.SockaddrNetlink{
-		Family: unix.AF_NETLINK,
-		Groups: groups,
-	}); err != nil {
-		return nil, errwrap.Wrapf(err, "error binding netlink socket")
-	}
-
-	// create a pipe socket to unblock unix.Select when we close
-	fdPipe, err := unix.Socket(unix.AF_UNIX, unix.SOCK_RAW, unix.PROT_NONE)
-	if err != nil {
-		return nil, errwrap.Wrapf(err, "error creating pipe socket")
-	}
-	// bind the pipe to a file
-	if err = unix.Bind(fdPipe, &unix.SockaddrUnix{
-		Name: file,
-	}); err != nil {
-		return nil, errwrap.Wrapf(err, "error binding pipe socket")
-	}
-	return &socketSet{
-		fdEvents: fdEvents,
-		fdPipe:   fdPipe,
-		pipeFile: file,
-	}, nil
-}
-
-// shutdown closes the event file descriptor and unblocks receive by sending
-// a message to the pipe file descriptor. It must be called before close, and
-// should only be called once.
-func (obj *socketSet) shutdown() error {
-	// close the event socket so no more events are produced
-	if err := unix.Close(obj.fdEvents); err != nil {
-		return err
-	}
-	// send a message to the pipe to unblock select
-	return unix.Sendto(obj.fdPipe, nil, 0, &unix.SockaddrUnix{
-		Name: path.Join(obj.pipeFile),
-	})
-}
-
-// close closes the pipe file descriptor. It must only be called after
-// shutdown has closed fdEvents, and unblocked receive. It should only be
-// called once.
-func (obj *socketSet) close() error {
-	return unix.Close(obj.fdPipe)
-}
-
-// receive waits for bytes from fdEvents and parses them into a slice of
-// netlink messages. It will block until an event is produced, or shutdown
-// is called.
-func (obj *socketSet) receive() ([]syscall.NetlinkMessage, error) {
-	// Select will return when any fd in fdSet (fdEvents and fdPipe) is ready
-	// to read.
-	_, err := unix.Select(obj.nfd(), obj.fdSet(), nil, nil, nil)
-	if err != nil {
-		// if a system interrupt is caught
-		if err == unix.EINTR { // signal interrupt
-			return nil, nil
-		}
-		return nil, errwrap.Wrapf(err, "error selecting on fd")
-	}
-	// receive the message from the netlink socket into b
-	b := make([]byte, os.Getpagesize())
-	n, _, err := unix.Recvfrom(obj.fdEvents, b, unix.MSG_DONTWAIT) // non-blocking receive
-	if err != nil {
-		// if fdEvents is closed
-		if err == unix.EBADF { // bad file descriptor
-			return nil, nil
-		}
-		return nil, errwrap.Wrapf(err, "error receiving messages")
-	}
-	// if we didn't get enough bytes for a header, something went wrong
-	if n < unix.NLMSG_HDRLEN {
-		return nil, fmt.Errorf("received short header")
-	}
-	b = b[:n] // truncate b to message length
-	// use syscall to parse, as func does not exist in x/sys/unix
-	return syscall.ParseNetlinkMessage(b)
-}
-
-// nfd returns one more than the highest fd value in the struct, for use as as
-// the nfds parameter in select. It represents the file descriptor set maximum
-// size. See man select for more info.
-func (obj *socketSet) nfd() int {
-	if obj.fdEvents > obj.fdPipe {
-		return obj.fdEvents + 1
-	}
-	return obj.fdPipe + 1
-}
-
-// fdSet returns a bitmask representation of the integer values of fdEvents
-// and fdPipe. See man select for more info.
-func (obj *socketSet) fdSet() *unix.FdSet {
-	fdSet := &unix.FdSet{}
-	// Generate the bitmask representing the file descriptors in the socketSet.
-	// The rightmost bit corresponds to file descriptor zero, and each bit to
-	// the left represents the next file descriptor number in the sequence of
-	// all real numbers. E.g. the FdSet containing containing 0 and 4 is 10001.
-	fdSet.Bits[obj.fdEvents/64] |= 1 << uint(obj.fdEvents)
-	fdSet.Bits[obj.fdPipe/64] |= 1 << uint(obj.fdPipe)
-	return fdSet
-}

engine/resources/net_test.go

@@ -15,14 +15,14 @@
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+// +build !darwin
+
 package resources
 
 import (
 	"bytes"
 	"strings"
 	"testing"
-
-	"golang.org/x/sys/unix"
 )
 
 // test cases for NetRes.unitFileContents()
@@ -82,85 +82,3 @@ func TestUnitFileContents(t *testing.T) {
 		}
 	}
 }
-
-// test cases for socketSet.fdSet()
-var fdSetTests = []struct {
-	in  *socketSet
-	out *unix.FdSet
-}{
-	{
-		&socketSet{
-			fdEvents: 3,
-			fdPipe:   4,
-		},
-		&unix.FdSet{
-			Bits: [16]int64{0x18}, // 11000
-		},
-	},
-	{
-		&socketSet{
-			fdEvents: 12,
-			fdPipe:   8,
-		},
-		&unix.FdSet{
-			Bits: [16]int64{0x1100}, // 1000100000000
-		},
-	},
-	{
-		&socketSet{
-			fdEvents: 9,
-			fdPipe:   21,
-		},
-		&unix.FdSet{
-			Bits: [16]int64{0x200200}, // 1000000000001000000000
-		},
-	},
-}
-
-// test socketSet.fdSet()
-func TestFdSet(t *testing.T) {
-	for _, test := range fdSetTests {
-		result := test.in.fdSet()
-		if *result != *test.out {
-			t.Errorf("fdSet test wanted: %b, got: %b", *test.out, *result)
-		}
-	}
-}
-
-// test cases for socketSet.nfd()
-var nfdTests = []struct {
-	in  *socketSet
-	out int
-}{
-	{
-		&socketSet{
-			fdEvents: 3,
-			fdPipe:   4,
-		},
-		5,
-	},
-	{
-		&socketSet{
-			fdEvents: 8,
-			fdPipe:   4,
-		},
-		9,
-	},
-	{
-		&socketSet{
-			fdEvents: 90,
-			fdPipe:   900,
-		},
-		901,
-	},
-}
-
-// test socketSet.nfd()
-func TestNfd(t *testing.T) {
-	for _, test := range nfdTests {
-		result := test.in.nfd()
-		if result != test.out {
-			t.Errorf("nfd test wanted: %d, got: %d", test.out, result)
-		}
-	}
-}

engine/resources/noop.go

@@ -63,31 +63,15 @@ func (obj *NoopRes) Close() error {
 
 // Watch is the primary listener for this resource and it outputs events.
 func (obj *NoopRes) Watch() error {
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
+	obj.init.Running() // when started, notify engine that we're running
+
+	select {
+	case <-obj.init.Done: // closed by the engine to signal shutdown
 	}
 
-	var send = false // send event?
-	for {
-		select {
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
-		}
+	//obj.init.Event() // notify engine of an event (this can block)
 
-		// do all our event sending all together to avoid duplicate msgs
-		if send {
-			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
-		}
-	}
+	return nil
 }
 
 // CheckApply method for Noop resource. Does nothing, returns happy!

engine/resources/nspawn.go

@@ -167,10 +167,7 @@ func (obj *NspawnRes) Watch() error {
 	bus.Signal(busChan)
 	defer bus.RemoveSignal(busChan) // not needed here, but nice for symmetry
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -187,24 +184,16 @@ func (obj *NspawnRes) Watch() error {
 					return fmt.Errorf("unknown event: %s", event.Name)
 				}
 				send = true
-				obj.init.Dirty() // dirty
 			}
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/password.go

@@ -182,10 +182,7 @@ func (obj *PasswordRes) Watch() error {
 	}
 	defer obj.recWatcher.Close()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -199,23 +196,15 @@ func (obj *PasswordRes) Watch() error {
 				return errwrap.Wrapf(err, "unknown %s watcher error", obj)
 			}
 			send = true
-			obj.init.Dirty() // dirty
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/pkg.go

@@ -67,7 +67,7 @@ type PkgRes struct {
 // Default returns some sensible defaults for this resource.
 func (obj *PkgRes) Default() engine.Res {
 	return &PkgRes{
-		State: PkgStateInstalled, // i think this is preferable to "latest"
+		State: PkgStateInstalled, // this *is* preferable to "newest"
 	}
 }
 
@@ -76,6 +76,9 @@ func (obj *PkgRes) Validate() error {
 	if obj.State == "" {
 		return fmt.Errorf("state cannot be empty")
 	}
+	if obj.State == "latest" {
+		return fmt.Errorf("state is invalid, did you mean `newest` ?")
+	}
 
 	return nil
 }
@@ -118,10 +121,7 @@ func (obj *PkgRes) Watch() error {
 		return errwrap.Wrapf(err, "error adding signal match")
 	}
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -143,20 +143,15 @@ func (obj *PkgRes) Watch() error {
 			}
 
 			send = true
-			obj.init.Dirty() // dirty
 
-		case event := <-obj.init.Events:
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }
@@ -203,7 +198,7 @@ func (obj *PkgRes) pkgMappingHelper(bus *packagekit.Conn) (map[string]*packageki
 	packageMap[obj.Name()] = obj.State     // key is pkg name, value is pkg state
 	var filter uint64                      // initializes at the "zero" value of 0
 	filter += packagekit.PkFilterEnumArch  // always search in our arch (optional!)
-	// we're requesting latest version, or to narrow down install choices!
+	// we're requesting newest version, or to narrow down install choices!
 	if obj.State == PkgStateNewest || obj.State == PkgStateInstalled {
 		// if we add this, we'll still see older packages if installed
 		// this is an optimization, and is *optional*, this logic is
@@ -218,7 +213,7 @@ func (obj *PkgRes) pkgMappingHelper(bus *packagekit.Conn) (map[string]*packageki
 	}
 	result, err := bus.PackagesToPackageIDs(packageMap, filter)
 	if err != nil {
-		return nil, errwrap.Wrapf(err, "Can't run PackagesToPackageIDs")
+		return nil, errwrap.Wrapf(err, "can't run PackagesToPackageIDs")
 	}
 	return result, nil
 }
@@ -249,6 +244,10 @@ func (obj *PkgRes) populateFileList() error {
 	if !ok || !data.Found {
 		return fmt.Errorf("can't find package named '%s'", obj.Name())
 	}
+	if data.PackageID == "" {
+		// this can happen if you specify a bad version like "latest"
+		return fmt.Errorf("empty PackageID found for '%s'", obj.Name())
+	}
 
 	packageIDs := []string{data.PackageID} // just one for now
 	filesMap, err := bus.GetFilesByPackageID(packageIDs)

engine/resources/print.go

@@ -66,31 +66,15 @@ func (obj *PrintRes) Close() error {
 
 // Watch is the primary listener for this resource and it outputs events.
 func (obj *PrintRes) Watch() error {
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
+	obj.init.Running() // when started, notify engine that we're running
+
+	select {
+	case <-obj.init.Done: // closed by the engine to signal shutdown
 	}
 
-	var send = false // send event?
-	for {
-		select {
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
-		}
+	//obj.init.Event() // notify engine of an event (this can block)
 
-		// do all our event sending all together to avoid duplicate msgs
-		if send {
-			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
-		}
-	}
+	return nil
 }
 
 // CheckApply method for Print resource. Does nothing, returns happy!

engine/resources/resources_test.go

@@ -0,0 +1,457 @@
+// Mgmt
+// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+// +build !root
+
+package resources
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/purpleidea/mgmt/engine"
+	"github.com/purpleidea/mgmt/util"
+)
+
+// TODO: consider providing this as a lib so that we can add tests into the
+// specific _test.go file of each resource.
+
+// makeRes is a helper function to build a res. It should only be called in
+// tests, because it panics if something goes wrong.
+func makeRes(kind, name string) engine.Res {
+	res, err := engine.NewNamedResource(kind, name)
+	if err != nil {
+		panic(fmt.Sprintf("could not create resource: %+v", err))
+	}
+	return res
+}
+
+// Step is used for the timeline in tests.
+type Step interface {
+	Action() error
+	Expect() error
+}
+
+type manualStep struct {
+	action func() error
+	expect func() error
+}
+
+func (obj *manualStep) Action() error {
+	return obj.action()
+}
+func (obj *manualStep) Expect() error {
+	return obj.expect()
+}
+
+// NewManualStep creates a new manual step with an action and an expect test.
+func NewManualStep(action, expect func() error) Step {
+	return &manualStep{
+		action: action,
+		expect: expect,
+	}
+}
+
+type startupStep struct {
+	ms uint
+	ch chan struct{} // set by test harness
+}
+
+func (obj *startupStep) Action() error {
+	select {
+	case <-obj.ch: // called by Running() in Watch
+	case <-time.After(time.Duration(obj.ms) * time.Millisecond):
+		return fmt.Errorf("took too long to startup")
+	}
+	return nil
+}
+func (obj *startupStep) Expect() error { return nil }
+
+// NewStartupStep waits up to this many ms for the Watch function to startup.
+func NewStartupStep(ms uint) Step {
+	return &startupStep{
+		ms: ms,
+	}
+}
+
+type changedStep struct {
+	ms     uint
+	expect bool      // what checkOK value we're expecting
+	ch     chan bool // set by test harness, filled with checkOK values
+}
+
+func (obj *changedStep) Action() error {
+	select {
+	case checkOK, ok := <-obj.ch: // from CheckApply() in test Process loop
+		if !ok {
+			return fmt.Errorf("channel closed unexpectedly")
+		}
+		if checkOK != obj.expect {
+			return fmt.Errorf("got unexpected checkOK value of: %t", checkOK)
+		}
+	case <-time.After(time.Duration(obj.ms) * time.Millisecond):
+		return fmt.Errorf("took too long to startup")
+	}
+	return nil
+}
+func (obj *changedStep) Expect() error { return nil }
+
+// NewChangedStep waits up to this many ms for a CheckApply action to occur. Watch function to startup.
+func NewChangedStep(ms uint, expect bool) Step {
+	return &changedStep{
+		ms:     ms,
+		expect: expect,
+	}
+}
+
+type clearChangedStep struct {
+	ms uint
+	ch chan bool // set by test harness, filled with checkOK values
+}
+
+func (obj *clearChangedStep) Action() error {
+	// read all pending events...
+	for {
+		select {
+		case _, ok := <-obj.ch: // from CheckApply() in test Process loop
+			if !ok {
+				return fmt.Errorf("channel closed unexpectedly")
+			}
+		case <-time.After(time.Duration(obj.ms) * time.Millisecond):
+			return nil // done waiting
+		}
+	}
+}
+func (obj *clearChangedStep) Expect() error { return nil }
+
+// NewClearChangedStep waits up to this many ms for additional CheckApply
+// actions to occur, and flushes them all so that a future NewChangedStep won't
+// see unwanted events.
+func NewClearChangedStep(ms uint) Step {
+	return &clearChangedStep{
+		ms: ms,
+	}
+}
+
+func TestResources1(t *testing.T) {
+	type test struct { // an individual test
+		name      string
+		res       engine.Res // a resource
+		fail      bool
+		experr    error        // expected error if fail == true (nil ignores it)
+		experrstr string       // expected error prefix
+		timeline  []Step       // TODO: this could be a generator that keeps pushing out steps until it's done!
+		expect    func() error // function to check for expected state
+		cleanup   func() error // function to run as cleanup
+	}
+
+	// helpers
+	// TODO: make a series of helps to orchestrate the resources (eg: edit
+	// file, wait for event w/ timeout, run command w/ timeout, etc...)
+	sleep := func(ms uint) Step {
+		return &manualStep{
+			action: func() error {
+				time.Sleep(time.Duration(ms) * time.Millisecond)
+				return nil
+			},
+			expect: func() error { return nil },
+		}
+	}
+	fileExpect := func(p, s string) Step { // path & string
+		return &manualStep{
+			action: func() error { return nil },
+			expect: func() error {
+				content, err := ioutil.ReadFile(p)
+				if err != nil {
+					return err
+				}
+				if string(content) != s {
+					return fmt.Errorf("contents did not match in %s", p)
+				}
+				return nil
+			},
+		}
+	}
+	fileWrite := func(p, s string) Step { // path & string
+		return &manualStep{
+			action: func() error {
+				// TODO: apparently using 0666 is equivalent to respecting the current umask
+				const umask = 0666
+				return ioutil.WriteFile(p, []byte(s), umask)
+			},
+			expect: func() error { return nil },
+		}
+	}
+
+	testCases := []test{}
+	{
+		res := makeRes("file", "r1")
+		r := res.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/whatever"
+		s := "hello, world\n"
+		r.Path = p
+		contents := s
+		r.Content = &contents
+
+		timeline := []Step{
+			NewStartupStep(1000 * 60),          // startup
+			NewChangedStep(1000*60, false),     // did we do something?
+			fileExpect(p, s),                   // check initial state
+			NewClearChangedStep(1000 * 15),     // did we do something?
+			fileWrite(p, "this is whatever\n"), // change state
+			NewChangedStep(1000*60, false),     // did we do something?
+			fileExpect(p, s),                   // check again
+			sleep(1),                           // we can sleep too!
+		}
+
+		testCases = append(testCases, test{
+			name:     "simple res",
+			res:      res,
+			fail:     false,
+			timeline: timeline,
+			expect:   func() error { return nil },
+			cleanup:  func() error { return os.Remove(p) },
+		})
+	}
+
+	names := []string{}
+	for index, tc := range testCases { // run all the tests
+		if tc.name == "" {
+			t.Errorf("test #%d: not named", index)
+			continue
+		}
+		if util.StrInList(tc.name, names) {
+			t.Errorf("test #%d: duplicate sub test name of: %s", index, tc.name)
+			continue
+		}
+		names = append(names, tc.name)
+		t.Run(fmt.Sprintf("test #%d (%s)", index, tc.name), func(t *testing.T) {
+			res, fail, experr, experrstr, timeline, expect, cleanup := tc.res, tc.fail, tc.experr, tc.experrstr, tc.timeline, tc.expect, tc.cleanup
+
+			t.Logf("\n\ntest #%d: Res: %+v\n", index, res)
+			defer t.Logf("test #%d: done!", index)
+
+			// run validate!
+			err := res.Validate()
+
+			if !fail && err != nil {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: could not validate Res: %+v", index, err)
+				return
+			}
+			if fail && err == nil {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: validate passed, expected fail", index)
+				return
+			}
+			if fail && experr != nil && err != experr { // test for specific error!
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: expected validate fail, got wrong error", index)
+				t.Errorf("test #%d: got error: %+v", index, err)
+				t.Errorf("test #%d: exp error: %+v", index, experr)
+				return
+			}
+			// test for specific error string!
+			if fail && experrstr != "" && !strings.HasPrefix(err.Error(), experrstr) {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: expected validate fail, got wrong error", index)
+				t.Errorf("test #%d: got error: %s", index, err.Error())
+				t.Errorf("test #%d: exp error: %s", index, experrstr)
+				return
+			}
+			if fail && err != nil {
+				t.Logf("test #%d: err: %+v", index, err)
+			}
+
+			changedChan := make(chan bool, 1) // buffered!
+			readyChan := make(chan struct{})
+			eventChan := make(chan struct{})
+			doneChan := make(chan struct{})
+			debug := testing.Verbose() // set via the -test.v flag to `go test`
+			logf := func(format string, v ...interface{}) {
+				t.Logf(fmt.Sprintf("test #%d: Res: ", index)+format, v...)
+			}
+			init := &engine.Init{
+				Running: func() {
+					close(readyChan)
+					select { // this always sends one!
+					case eventChan <- struct{}{}:
+
+					}
+				},
+				// Watch runs this to send a changed event.
+				Event: func() {
+					select {
+					case eventChan <- struct{}{}:
+
+					}
+				},
+
+				// Watch listens on this for close/pause events.
+				Done:  doneChan,
+				Debug: debug,
+				Logf:  logf,
+
+				// unused
+				Recv: func() map[string]*engine.Send {
+					return map[string]*engine.Send{}
+				},
+			}
+
+			// run init
+			t.Logf("test #%d: running Init", index)
+			err = res.Init(init)
+			defer func() {
+				t.Logf("test #%d: running cleanup()", index)
+				if err := cleanup(); err != nil {
+					t.Errorf("test #%d: FAIL", index)
+					t.Errorf("test #%d: could not cleanup: %+v", index, err)
+				}
+			}()
+			closeFn := func() {
+				// run close (we don't ever expect an error on close!)
+				t.Logf("test #%d: running Close", index)
+				if err := res.Close(); err != nil {
+					t.Errorf("test #%d: FAIL", index)
+					t.Errorf("test #%d: could not close Res: %+v", index, err)
+					//return
+				}
+			}
+
+			if !fail && err != nil {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: could not init Res: %+v", index, err)
+				return
+			}
+			if fail && err == nil {
+				closeFn() // close if Init didn't fail
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: init passed, expected fail", index)
+				return
+			}
+			if fail && experr != nil && err != experr { // test for specific error!
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: expected init fail, got wrong error", index)
+				t.Errorf("test #%d: got error: %+v", index, err)
+				t.Errorf("test #%d: exp error: %+v", index, experr)
+				return
+			}
+			// test for specific error string!
+			if fail && experrstr != "" && !strings.HasPrefix(err.Error(), experrstr) {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: expected init fail, got wrong error", index)
+				t.Errorf("test #%d: got error: %s", index, err.Error())
+				t.Errorf("test #%d: exp error: %s", index, experrstr)
+				return
+			}
+			if fail && err != nil {
+				t.Logf("test #%d: err: %+v", index, err)
+			}
+			defer closeFn()
+
+			// run watch
+			wg := &sync.WaitGroup{}
+			defer wg.Wait() // if we return early
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				t.Logf("test #%d: running Watch", index)
+				if err := res.Watch(); err != nil {
+					t.Errorf("test #%d: FAIL", index)
+					t.Errorf("test #%d: Watch failed: %s", index, err.Error())
+				}
+				close(eventChan) // done with this part
+			}()
+
+			// TODO: can we block here if the test fails early?
+			select {
+			case <-readyChan: // called by Running() in Watch
+			}
+			wg.Add(1)
+			go func() { // run timeline
+				t.Logf("test #%d: executing timeline", index)
+				defer wg.Done()
+				for ix, step := range timeline {
+
+					// magic setting of important values...
+					if s, ok := step.(*startupStep); ok {
+						s.ch = readyChan
+					}
+					if s, ok := step.(*changedStep); ok {
+						s.ch = changedChan
+					}
+					if s, ok := step.(*clearChangedStep); ok {
+						s.ch = changedChan
+					}
+
+					t.Logf("test #%d: step(%d)...", index, ix)
+					if err := step.Action(); err != nil {
+						t.Errorf("test #%d: FAIL", index)
+						t.Errorf("test #%d: step(%d) action failed: %s", index, ix, err.Error())
+						break
+					}
+					if err := step.Expect(); err != nil {
+						t.Errorf("test #%d: FAIL", index)
+						t.Errorf("test #%d: step(%d) expect failed: %s", index, ix, err.Error())
+						break
+					}
+				}
+				t.Logf("test #%d: shutting down Watch", index)
+				close(doneChan) // send Watch shutdown command
+			}()
+		Loop:
+			for {
+				select {
+				case _, ok := <-eventChan: // from Watch()
+					if !ok {
+						//t.Logf("test #%d: break!", index)
+						break Loop
+					}
+				}
+
+				t.Logf("test #%d: running CheckApply", index)
+				checkOK, err := res.CheckApply(true) // no noop!
+				if err != nil {
+					t.Errorf("test #%d: FAIL", index)
+					t.Errorf("test #%d: CheckApply failed: %s", index, err.Error())
+					return
+				}
+				select {
+				// send a msg if we can, but never block
+				case changedChan <- checkOK:
+				default:
+				}
+			}
+
+			t.Logf("test #%d: waiting for shutdown", index)
+			wg.Wait()
+
+			if err := expect(); err != nil {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: expect failed: %s", index, err.Error())
+				return
+			}
+
+			// all done!
+		})
+	}
+}

engine/resources/svc.go

@@ -120,10 +120,7 @@ func (obj *SvcRes) Watch() error {
 	bus.Signal(buschan)
 	defer bus.RemoveSignal(buschan) // not needed here, but nice for symmetry
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var svc = fmt.Sprintf("%s.service", obj.Name()) // systemd name
 	var send = false                                // send event?
@@ -161,7 +158,6 @@ func (obj *SvcRes) Watch() error {
 
 		if previous != invalid { // if invalid changed, send signal
 			send = true
-			obj.init.Dirty() // dirty
 		}
 
 		if invalid {
@@ -176,10 +172,8 @@ func (obj *SvcRes) Watch() error {
 				// loop so that we can see the changed invalid signal
 				obj.init.Logf("daemon reload")
 
-			case event := <-obj.init.Events:
-				if err := obj.init.Read(event); err != nil {
-					return err
-				}
+			case <-obj.init.Done: // closed by the engine to signal shutdown
+				return nil
 			}
 		} else {
 			if !activeSet {
@@ -217,23 +211,18 @@ func (obj *SvcRes) Watch() error {
 					obj.init.Logf("stopped")
 				}
 				send = true
-				obj.init.Dirty() // dirty
 
 			case err := <-subErrors:
 				return errwrap.Wrapf(err, "unknown %s error", obj)
 
-			case event := <-obj.init.Events:
-				if err := obj.init.Read(event); err != nil {
-					return err
-				}
+			case <-obj.init.Done: // closed by the engine to signal shutdown
+				return nil
 			}
 		}
 
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }
@@ -343,7 +332,12 @@ func (obj *SvcRes) CheckApply(apply bool) (checkOK bool, err error) {
 	if &status == nil {
 		return false, fmt.Errorf("systemd service action result is nil")
 	}
-	if status != "done" {
+	switch status {
+	case "done":
+		// pass
+	case "failed":
+		return false, fmt.Errorf("svc failed (selinux?)")
+	default:
 		return false, fmt.Errorf("unknown systemd return string: %v", status)
 	}
 

engine/resources/test.go

@@ -125,31 +125,15 @@ func (obj *TestRes) Close() error {
 
 // Watch is the primary listener for this resource and it outputs events.
 func (obj *TestRes) Watch() error {
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
+	obj.init.Running() // when started, notify engine that we're running
+
+	select {
+	case <-obj.init.Done: // closed by the engine to signal shutdown
 	}
 
-	var send = false // send event?
-	for {
-		select {
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
-		}
+	//obj.init.Event() // notify engine of an event (this can block)
 
-		// do all our event sending all together to avoid duplicate msgs
-		if send {
-			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
-		}
-	}
+	return nil
 }
 
 // CheckApply method for Test resource. Does nothing, returns happy!

engine/resources/timer.go

@@ -75,10 +75,7 @@ func (obj *TimerRes) Watch() error {
 	obj.ticker = obj.newTicker()
 	defer obj.ticker.Stop()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -87,20 +84,13 @@ func (obj *TimerRes) Watch() error {
 			send = true
 			obj.init.Logf("received tick")
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/user.go

@@ -119,10 +119,7 @@ func (obj *UserRes) Watch() error {
 	}
 	defer obj.recWatcher.Close()
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -142,23 +139,15 @@ func (obj *UserRes) Watch() error {
 				obj.init.Logf("Event(%s): %v", event.Body.Name, event.Body.Op)
 			}
 			send = true
-			obj.init.Dirty() // dirty
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }

engine/resources/virt.go

@@ -71,24 +71,24 @@ type VirtRes struct {
 
 	init *engine.Init
 
-	URI        string             `yaml:"uri"`       // connection uri, eg: qemu:///session
-	State      string             `yaml:"state"`     // running, paused, shutoff
-	Transient  bool               `yaml:"transient"` // defined (false) or undefined (true)
-	CPUs       uint               `yaml:"cpus"`
-	MaxCPUs    uint               `yaml:"maxcpus"`
-	Memory     uint64             `yaml:"memory"` // in KBytes
-	OSInit     string             `yaml:"osinit"` // init used by lxc
-	Boot       []string           `yaml:"boot"`   // boot order. values: fd, hd, cdrom, network
-	Disk       []diskDevice       `yaml:"disk"`
-	CDRom      []cdRomDevice      `yaml:"cdrom"`
-	Network    []networkDevice    `yaml:"network"`
-	Filesystem []filesystemDevice `yaml:"filesystem"`
-	Auth       *VirtAuth          `yaml:"auth"`
+	URI        string             `lang:"uri" yaml:"uri"`             // connection uri, eg: qemu:///session
+	State      string             `lang:"state" yaml:"state"`         // running, paused, shutoff
+	Transient  bool               `lang:"transient" yaml:"transient"` // defined (false) or undefined (true)
+	CPUs       uint               `lang:"cpus" yaml:"cpus"`
+	MaxCPUs    uint               `lang:"maxcpus" yaml:"maxcpus"`
+	Memory     uint64             `lang:"memory" yaml:"memory"` // in KBytes
+	OSInit     string             `lang:"osinit" yaml:"osinit"` // init used by lxc
+	Boot       []string           `lang:"boot" yaml:"boot"`     // boot order. values: fd, hd, cdrom, network
+	Disk       []DiskDevice       `lang:"disk" yaml:"disk"`
+	CDRom      []CDRomDevice      `lang:"cdrom" yaml:"cdrom"`
+	Network    []NetworkDevice    `lang:"network" yaml:"network"`
+	Filesystem []FilesystemDevice `lang:"filesystem" yaml:"filesystem"`
+	Auth       *VirtAuth          `lang:"auth" yaml:"auth"`
 
-	HotCPUs bool `yaml:"hotcpus"` // allow hotplug of cpus?
+	HotCPUs bool `lang:"hotcpus" yaml:"hotcpus"` // allow hotplug of cpus?
 	// FIXME: values here should be enum's!
-	RestartOnDiverge string `yaml:"restartondiverge"` // restart policy: "ignore", "ifneeded", "error"
-	RestartOnRefresh bool   `yaml:"restartonrefresh"` // restart on refresh?
+	RestartOnDiverge string `lang:"restartondiverge" yaml:"restartondiverge"` // restart policy: "ignore", "ifneeded", "error"
+	RestartOnRefresh bool   `lang:"restartonrefresh" yaml:"restartonrefresh"` // restart on refresh?
 
 	wg                  *sync.WaitGroup
 	conn                *libvirt.Connect
@@ -103,8 +103,8 @@ type VirtRes struct {
 
 // VirtAuth is used to pass credentials to libvirt.
 type VirtAuth struct {
-	Username string `yaml:"username"`
-	Password string `yaml:"password"`
+	Username string `lang:"username" yaml:"username"`
+	Password string `lang:"password" yaml:"password"`
 }
 
 // Default returns some sensible defaults for this resource.
@@ -326,10 +326,7 @@ func (obj *VirtRes) Watch() error {
 	}
 	defer obj.conn.DomainEventDeregister(gaCallbackID)
 
-	// notify engine that we're running
-	if err := obj.init.Running(); err != nil {
-		return err // exit if requested
-	}
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -340,31 +337,26 @@ func (obj *VirtRes) Watch() error {
 			switch event {
 			case libvirt.DOMAIN_EVENT_DEFINED:
 				if obj.Transient {
-					obj.init.Dirty() // dirty
 					send = true
 				}
 			case libvirt.DOMAIN_EVENT_UNDEFINED:
 				if !obj.Transient {
-					obj.init.Dirty() // dirty
 					send = true
 				}
 			case libvirt.DOMAIN_EVENT_STARTED:
 				fallthrough
 			case libvirt.DOMAIN_EVENT_RESUMED:
 				if obj.State != "running" {
-					obj.init.Dirty() // dirty
 					send = true
 				}
 			case libvirt.DOMAIN_EVENT_SUSPENDED:
 				if obj.State != "paused" {
-					obj.init.Dirty() // dirty
 					send = true
 				}
 			case libvirt.DOMAIN_EVENT_STOPPED:
 				fallthrough
 			case libvirt.DOMAIN_EVENT_SHUTDOWN:
 				if obj.State != "shutoff" {
-					obj.init.Dirty() // dirty
 					send = true
 				}
 				processExited = true
@@ -375,7 +367,6 @@ func (obj *VirtRes) Watch() error {
 				// verify, detect and patch appropriately!
 				fallthrough
 			case libvirt.DOMAIN_EVENT_CRASHED:
-				obj.init.Dirty() // dirty
 				send = true
 				processExited = true // FIXME: is this okay for PMSUSPENDED ?
 			}
@@ -390,7 +381,6 @@ func (obj *VirtRes) Watch() error {
 
 			if state == libvirt.CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_STATE_CONNECTED {
 				obj.guestAgentConnected = true
-				obj.init.Dirty() // dirty
 				send = true
 				obj.init.Logf("Guest agent connected")
 
@@ -409,21 +399,14 @@ func (obj *VirtRes) Watch() error {
 		case err := <-errorChan:
 			return fmt.Errorf("unknown %s libvirt error: %s", obj, err)
 
-		case event, ok := <-obj.init.Events:
-			if !ok {
-				return nil
-			}
-			if err := obj.init.Read(event); err != nil {
-				return err
-			}
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+			return nil
 		}
 
 		// do all our event sending all together to avoid duplicate msgs
 		if send {
 			send = false
-			if err := obj.init.Event(); err != nil {
-				return err // exit if requested
-			}
+			obj.init.Event() // notify engine of an event (this can block)
 		}
 	}
 }
@@ -970,44 +953,51 @@ type virtDevice interface {
 	GetXML(idx int) string
 }
 
-type diskDevice struct {
-	Source string `yaml:"source"`
-	Type   string `yaml:"type"`
+// DiskDevice represents a disk that is attached to the virt machine.
+type DiskDevice struct {
+	Source string `lang:"source" yaml:"source"`
+	Type   string `lang:"type" yaml:"type"`
 }
 
-type cdRomDevice struct {
-	Source string `yaml:"source"`
-	Type   string `yaml:"type"`
+// CDRomDevice represents a CDRom device that is attached to the virt machine.
+type CDRomDevice struct {
+	Source string `lang:"source" yaml:"source"`
+	Type   string `lang:"type" yaml:"type"`
 }
 
-type networkDevice struct {
-	Name string `yaml:"name"`
-	MAC  string `yaml:"mac"`
+// NetworkDevice represents a network card that is attached to the virt machine.
+type NetworkDevice struct {
+	Name string `lang:"name" yaml:"name"`
+	MAC  string `lang:"mac" yaml:"mac"`
 }
 
-type filesystemDevice struct {
-	Access   string `yaml:"access"`
-	Source   string `yaml:"source"`
-	Target   string `yaml:"target"`
-	ReadOnly bool   `yaml:"read_only"`
+// FilesystemDevice represents a filesystem that is attached to the virt
+// machine.
+type FilesystemDevice struct {
+	Access   string `lang:"access" yaml:"access"`
+	Source   string `lang:"source" yaml:"source"`
+	Target   string `lang:"target" yaml:"target"`
+	ReadOnly bool   `lang:"read_only" yaml:"read_only"`
 }
 
-func (d *diskDevice) GetXML(idx int) string {
-	source, _ := util.ExpandHome(d.Source) // TODO: should we handle errors?
+// GetXML returns the XML representation of this device.
+func (obj *DiskDevice) GetXML(idx int) string {
+	source, _ := util.ExpandHome(obj.Source) // TODO: should we handle errors?
 	var b string
 	b += "<disk type='file' device='disk'>"
-	b += fmt.Sprintf("<driver name='qemu' type='%s'/>", d.Type)
+	b += fmt.Sprintf("<driver name='qemu' type='%s'/>", obj.Type)
 	b += fmt.Sprintf("<source file='%s'/>", source)
 	b += fmt.Sprintf("<target dev='vd%s' bus='virtio'/>", util.NumToAlpha(idx))
 	b += "</disk>"
 	return b
 }
 
-func (d *cdRomDevice) GetXML(idx int) string {
-	source, _ := util.ExpandHome(d.Source) // TODO: should we handle errors?
+// GetXML returns the XML representation of this device.
+func (obj *CDRomDevice) GetXML(idx int) string {
+	source, _ := util.ExpandHome(obj.Source) // TODO: should we handle errors?
 	var b string
 	b += "<disk type='file' device='cdrom'>"
-	b += fmt.Sprintf("<driver name='qemu' type='%s'/>", d.Type)
+	b += fmt.Sprintf("<driver name='qemu' type='%s'/>", obj.Type)
 	b += fmt.Sprintf("<source file='%s'/>", source)
 	b += fmt.Sprintf("<target dev='hd%s' bus='ide'/>", util.NumToAlpha(idx))
 	b += "<readonly/>"
@@ -1015,29 +1005,31 @@ func (d *cdRomDevice) GetXML(idx int) string {
 	return b
 }
 
-func (d *networkDevice) GetXML(idx int) string {
-	if d.MAC == "" {
-		d.MAC = randMAC()
+// GetXML returns the XML representation of this device.
+func (obj *NetworkDevice) GetXML(idx int) string {
+	if obj.MAC == "" {
+		obj.MAC = randMAC()
 	}
 	var b string
 	b += "<interface type='network'>"
-	b += fmt.Sprintf("<mac address='%s'/>", d.MAC)
-	b += fmt.Sprintf("<source network='%s'/>", d.Name)
+	b += fmt.Sprintf("<mac address='%s'/>", obj.MAC)
+	b += fmt.Sprintf("<source network='%s'/>", obj.Name)
 	b += "</interface>"
 	return b
 }
 
-func (d *filesystemDevice) GetXML(idx int) string {
-	source, _ := util.ExpandHome(d.Source) // TODO: should we handle errors?
+// GetXML returns the XML representation of this device.
+func (obj *FilesystemDevice) GetXML(idx int) string {
+	source, _ := util.ExpandHome(obj.Source) // TODO: should we handle errors?
 	var b string
 	b += "<filesystem" // open
-	if d.Access != "" {
-		b += fmt.Sprintf(" accessmode='%s'", d.Access)
+	if obj.Access != "" {
+		b += fmt.Sprintf(" accessmode='%s'", obj.Access)
 	}
 	b += ">" // close
 	b += fmt.Sprintf("<source dir='%s'/>", source)
-	b += fmt.Sprintf("<target dir='%s'/>", d.Target)
-	if d.ReadOnly {
+	b += fmt.Sprintf("<target dir='%s'/>", obj.Target)
+	if obj.ReadOnly {
 		b += "<readonly/>"
 	}
 	b += "</filesystem>"

engine/reverse.go

@@ -0,0 +1,65 @@
+// Mgmt
+// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package engine
+
+import (
+	"fmt"
+)
+
+// ReversibleRes is an interface that a resource can implement if it wants to
+// have some resource run when it disappears. A disappearance happens when a
+// resource is defined in one instance of the graph, and is gone in the
+// subsequent one. This is helpful for building robust programs with the engine.
+// Default implementations for most of the methods declared in this interface
+// can be obtained for your resource by anonymously adding the traits.Reversible
+// struct to your resource implementation.
+type ReversibleRes interface {
+	Res
+
+	// ReversibleMeta lets you get or set meta params for the reversible
+	// trait.
+	ReversibleMeta() *ReversibleMeta
+
+	// SetReversibleMeta lets you set all of the meta params for the
+	// reversible trait in a single call.
+	SetReversibleMeta(*ReversibleMeta)
+
+	// Reversed returns the "reverse" or "reciprocal" resource. This is used
+	// to "clean" up after a previously defined resource has been removed.
+	// Interestingly, this returns the core Res interface instead of a
+	// ReversibleRes, because there is no requirement that the reverse of a
+	// Res be the same kind of Res, and the reverse might not be reversible!
+	Reversed() (Res, error)
+}
+
+// ReversibleMeta provides some parameters specific to reversible resources.
+type ReversibleMeta struct {
+	// Disabled specifies that reversing should be disabled for this
+	// resource.
+	Disabled bool
+
+	// TODO: add options here, including whether to reverse edges, etc...
+}
+
+// Cmp compares two ReversibleMeta structs and determines if they're equivalent.
+func (obj *ReversibleMeta) Cmp(rm *ReversibleMeta) error {
+	if obj.Disabled != rm.Disabled {
+		return fmt.Errorf("values for Disabled are different")
+	}
+	return nil
+}

engine/traits/reverse.go

@@ -0,0 +1,48 @@
+// Mgmt
+// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package traits
+
+import (
+	"github.com/purpleidea/mgmt/engine"
+)
+
+// Reversible contains a general implementation with most of the properties and
+// methods needed to support reversing resources. It may be used as a starting
+// point to avoid re-implementing the straightforward methods.
+type Reversible struct {
+	meta *engine.ReversibleMeta
+
+	// Bug5819 works around issue https://github.com/golang/go/issues/5819
+	Bug5819 interface{} // XXX: workaround
+}
+
+// ReversibleMeta lets you get or set meta params for the reversing trait.
+func (obj *Reversible) ReversibleMeta() *engine.ReversibleMeta {
+	if obj.meta == nil { // set the defaults if previously empty
+		obj.meta = &engine.ReversibleMeta{
+			Disabled: true, // by default we're disabled
+		}
+	}
+	return obj.meta
+}
+
+// SetReversibleMeta lets you set all of the meta params for the reversing trait
+// in a single call.
+func (obj *Reversible) SetReversibleMeta(meta *engine.ReversibleMeta) {
+	obj.meta = meta
+}

etcd/etcd.go

@@ -194,6 +194,7 @@ type EmbdEtcd struct { // EMBeddeD etcd
 	advertiseClientURLs etcdtypes.URLs    // client urls to advertise
 	advertiseServerURLs etcdtypes.URLs    // server urls to advertise
 	noServer            bool              // disable all server peering if true
+	noNetwork           bool              // use unix:// sockets instead of TCP for clients/servers
 
 	// local tracked state
 	nominated        etcdtypes.URLsMap // copy of who's nominated to locally track state
@@ -209,8 +210,8 @@ type EmbdEtcd struct { // EMBeddeD etcd
 	txnq    chan *TN // txn queue
 
 	flags     Flags
-	prefix    string              // folder prefix to use for misc storage
-	converger converger.Converger // converged tracking
+	prefix    string                 // folder prefix to use for misc storage
+	converger *converger.Coordinator // converged tracking
 
 	// etcd server related
 	serverwg    sync.WaitGroup // wait for server to shutdown
@@ -220,7 +221,7 @@ type EmbdEtcd struct { // EMBeddeD etcd
 }
 
 // NewEmbdEtcd creates the top level embedded etcd struct client and server obj.
-func NewEmbdEtcd(hostname string, seeds, clientURLs, serverURLs, advertiseClientURLs, advertiseServerURLs etcdtypes.URLs, noServer bool, idealClusterSize uint16, flags Flags, prefix string, converger converger.Converger) *EmbdEtcd {
+func NewEmbdEtcd(hostname string, seeds, clientURLs, serverURLs, advertiseClientURLs, advertiseServerURLs etcdtypes.URLs, noServer bool, noNetwork bool, idealClusterSize uint16, flags Flags, prefix string, converger *converger.Coordinator) *EmbdEtcd {
 	endpoints := make(etcdtypes.URLsMap)
 	if hostname == seedSentinel { // safety
 		return nil
@@ -229,6 +230,15 @@ func NewEmbdEtcd(hostname string, seeds, clientURLs, serverURLs, advertiseClient
 		log.Printf("Etcd: need at least one seed if running with --no-server!")
 		return nil
 	}
+	if noNetwork {
+		if len(clientURLs) != 0 || len(serverURLs) != 0 || len(seeds) != 0 {
+			log.Printf("--no-network is mutual exclusive with --seeds, --client-urls and --server-urls")
+			return nil
+		}
+		clientURLs, _ = etcdtypes.NewURLs([]string{"unix://clients.sock:0"})
+		serverURLs, _ = etcdtypes.NewURLs([]string{"unix://servers.sock:0"})
+	}
+
 	if len(seeds) > 0 {
 		endpoints[seedSentinel] = seeds
 		idealClusterSize = 0 // unset, get from running cluster
@@ -253,6 +263,7 @@ func NewEmbdEtcd(hostname string, seeds, clientURLs, serverURLs, advertiseClient
 		advertiseClientURLs: advertiseClientURLs,
 		advertiseServerURLs: advertiseServerURLs,
 		noServer:            noServer,
+		noNetwork:           noNetwork,
 
 		idealClusterSize: idealClusterSize,
 		converger:        converger,
@@ -304,7 +315,7 @@ func (obj *EmbdEtcd) GetConfig() etcd.Config {
 	// XXX: filter out any urls which wouldn't resolve here ?
 	for _, eps := range obj.endpoints { // flatten map
 		for _, u := range eps {
-			endpoints = append(endpoints, u.Host) // remove http:// prefix
+			endpoints = append(endpoints, u.String()) // use full url including scheme
 		}
 	}
 	sort.Strings(endpoints) // sort for determinism
@@ -753,7 +764,6 @@ func (obj *EmbdEtcd) CbLoop() {
 	obj.exitwg.Add(1)
 	defer obj.exitwg.Done()
 	cuid := obj.converger.Register()
-	cuid.SetName("Etcd: CbLoop")
 	defer cuid.Unregister()
 	if e := obj.Connect(false); e != nil {
 		return // fatal
@@ -822,7 +832,6 @@ func (obj *EmbdEtcd) Loop() {
 	obj.exitwg.Add(1) // TODO: add these to other go routines?
 	defer obj.exitwg.Done()
 	cuid := obj.converger.Register()
-	cuid.SetName("Etcd: Loop")
 	defer cuid.Unregister()
 	if e := obj.Connect(false); e != nil {
 		return // fatal
@@ -1692,8 +1701,12 @@ func (obj *EmbdEtcd) LocalhostClientURLs() etcdtypes.URLs {
 	// look through obj.clientURLs and return the localhost ones
 	urls := etcdtypes.URLs{}
 	for _, x := range obj.clientURLs {
-		// "localhost" or anything in 127.0.0.0/8 is valid!
-		if s := x.Host; strings.HasPrefix(s, "localhost") || strings.HasPrefix(s, "127.") {
+		// "localhost", ::1 or anything in 127.0.0.0/8 is valid!
+		if s := x.Host; strings.HasPrefix(s, "localhost") || strings.HasPrefix(s, "127.") || strings.HasPrefix(s, "[::1]") {
+			urls = append(urls, x)
+		}
+		// or local unix domain socket
+		if x.Scheme == "unix" {
 			urls = append(urls, x)
 		}
 	}

etcd/etcd_test.go

@@ -31,7 +31,7 @@ func TestNewEmbdEtcd(t *testing.T) {
 	noServer := false
 	var flags Flags
 
-	obj := NewEmbdEtcd("", nil, nil, nil, nil, nil, noServer, 0, flags, "", nil)
+	obj := NewEmbdEtcd("", nil, nil, nil, nil, nil, noServer, false, 0, flags, "", nil)
 	if obj == nil {
 		t.Fatal("failed to create server object")
 	}
@@ -44,7 +44,7 @@ func TestNewEmbdEtcdConfigValidation(t *testing.T) {
 	noServer := true
 	var flags Flags
 
-	obj := NewEmbdEtcd("", seeds, nil, nil, nil, nil, noServer, 0, flags, "", nil)
+	obj := NewEmbdEtcd("", seeds, nil, nil, nil, nil, noServer, false, 0, flags, "", nil)
 	if obj != nil {
 		t.Fatal("server initialization should fail on invalid configuration")
 	}

examples/hcl/graph0.hcl

@@ -1,14 +0,0 @@
-resource "file" "file1" {
-  path = "/tmp/mgmt-hello-world"
-  content = "hello, world"
-  state = "exists"
-  depends_on = ["noop.noop1", "exec.sleep"]
-}
-
-resource "noop" "noop1" {
-  test = "nil"
-}
-
-resource "exec" "sleep" {
- cmd = "sleep 10s"
-}

examples/hcl/graph1.hcl

@@ -1,4 +0,0 @@
-resource "exec" "exec1" {
-  cmd = "cat /tmp/mgmt-hello-world"
-  state =  "present"
-}

examples/hcl/hil.hcl

@@ -1,9 +0,0 @@
-resource "file" "file1" {
-  path = "/tmp/mgmt-hello-world"
-  content = "${exec.sleep.Output}"
-  state = "exists"
-}
-
-resource "exec" "sleep" {
- cmd = "echo hello"
-}

examples/lang/autoedges1.mcl

@@ -0,0 +1,30 @@
+pkg "drbd-utils" {
+	state => "installed",
+
+	Meta:autoedge => true,
+	Meta:noop => true,
+}
+
+file "/etc/drbd.conf" {
+	content => "this is an mgmt test",
+	state => "exists",
+
+	Meta:autoedge => true,
+	Meta:noop => true,
+}
+
+file "/etc/drbd.d/" {
+	source => "/dev/null",
+	state => "exists",
+
+	Meta:autoedge => true,
+	Meta:noop => true,
+}
+
+# note that the autoedges between the files and the svc don't exist yet :(
+svc "drbd" {
+	state => "stopped",
+
+	Meta:autoedge => true,
+	Meta:noop => true,
+}

examples/lang/autogroup1.mcl

@@ -0,0 +1,17 @@
+pkg "powertop" {
+	state => "installed",
+
+	Meta:autogroup => true,
+}
+
+pkg "sl" {
+	state => "installed",
+
+	Meta:autogroup => true,
+}
+
+pkg "cowsay" {
+	state => "installed",
+
+	Meta:autogroup => true,
+}

examples/lang/class-include.mcl

@@ -0,0 +1,22 @@
+import "fmt"
+class foo {
+	print "foo1" {
+		msg => "inside foo",
+
+		Meta:autogroup => false,
+	}
+}
+
+class bar($a, $b) { # a parameterized class
+	print "bar-"+ $a {
+		msg => fmt.printf("inside bar: %s", $b),
+
+		Meta:autogroup => false,
+	}
+}
+
+include foo
+include foo			# duplicate
+include bar("b1", "hello")
+include bar("b2", "world")
+include bar("b2", "world")	# duplicate

examples/lang/cron0.mcl

@@ -1,9 +1,9 @@
 cron "purpleidea-oneshot" {
-    session => true,
-    trigger => "OnBootSec",
-    time => "60",
+	session => true,
+	trigger => "OnBootSec",
+	time => "60",
 }
 
 svc "purpleidea-oneshot" {
-    session => true,
+	session => true,
 }

examples/lang/cron1.mcl

@@ -1,3 +1,3 @@
 cron "purpleidea-oneshot" {
-    state => "absent",
+	state => "absent",
 }

examples/lang/cron2.mcl

@@ -1,6 +1,6 @@
 cron "purpleidea-oneshot" {
-    trigger => "OnUnitActiveSec",
-    time => "2minutes",
+	trigger => "OnUnitActiveSec",
+	time => "2minutes",
 }
 
 svc "purpleidea-oneshot" {}

examples/lang/cron3.mcl

@@ -1,13 +1,13 @@
 $home = getenv("HOME")
 
 cron "purpleidea-oneshot" {
-    session => true,
-    trigger => "OnCalendar",
-    time => "*:*:0",
+	session => true,
+	trigger => "OnCalendar",
+	time => "*:*:0",
 }
 
 svc "purpleidea-oneshot" {
-    session => true,
+	session => true,
 }
 
 file printf("%s/.config/systemd/user/purpleidea-oneshot.service", $home) {}

examples/lang/cron4.mcl

@@ -1,17 +1,17 @@
 $home = getenv("HOME")
 
 cron "purpleidea-oneshot" {
-    state => "absent",
-    session => true,
-    trigger => "OnCalendar",
-    time => "*:*:0",
+	state => "absent",
+	session => true,
+	trigger => "OnCalendar",
+	time => "*:*:0",
 }
 
 svc "purpleidea-oneshot" {
-    state => "stopped",
-    session => true,
+	state => "stopped",
+	session => true,
 }
 
 file printf("%s/.config/systemd/user/purpleidea-oneshot.service", $home) {
-    state => "absent",
+	state => "absent",
 }

examples/lang/datetime3.mcl

@@ -1,5 +1,6 @@
 import "datetime"
 import "sys"
+import "example"
 
 $secplusone = datetime.now() + $ayear
 
@@ -10,7 +11,7 @@ $tmplvalues = struct{year => $secplusone, load => $theload, vumeter => $vumeter,
 
 $theload = structlookup(sys.load(), "x1")
 
-$vumeter = vumeter("====", 10, 0.9)
+$vumeter = example.vumeter("====", 10, 0.9)
 
 file "/tmp/mgmt/datetime" {
 	content => template("Now + 1 year is: {{ .year }} seconds, aka: {{ datetime_print .year }}\n\nload average: {{ .load }}\n\nvu: {{ .vumeter }}\n", $tmplvalues),

examples/lang/docker_container0.mcl

@@ -1,6 +1,6 @@
 docker:container "mgmt-nginx" {
-    state => "running",
-    image => "nginx",
-    cmd => ["nginx", "-g", "daemon off;",],
-    ports => {"tcp" => {80 => 8080,},},
+	state => "running",
+	image => "nginx",
+	cmd => ["nginx", "-g", "daemon off;",],
+	ports => {"tcp" => {80 => 8080,},},
 }

examples/lang/duplicate-error.mcl

@@ -0,0 +1,8 @@
+# this combination should error
+pkg "cowsay" {
+        state => "uninstalled",
+}
+
+pkg "cowsay" {
+        state => "installed",
+}

examples/lang/duplicate.mcl

@@ -0,0 +1,7 @@
+pkg "cowsay" {
+        state => "newest",
+}
+
+pkg "cowsay" {
+        state => "installed",
+}

examples/lang/env-bad.mcl

@@ -0,0 +1,8 @@
+import "fmt"
+import "sys"
+
+$x = sys.getenv("TEST", "321")
+
+print "print1" {
+	msg => fmt.printf("TEST is: %s", $x),
+}

examples/lang/exchange0.mcl

@@ -6,9 +6,10 @@
 # time ./mgmt run --hostname h4 --seeds http://127.0.0.1:2379 --client-urls http://127.0.0.1:2385 --server-urls http://127.0.0.1:2386 --tmp-prefix --no-pgp lang --lang examples/lang/exchange0.mcl
 
 import "sys"
+import "world"
 
 $rand = random1(8)
-$exchanged = exchange("keyns", $rand)
+$exchanged = world.exchange("keyns", $rand)
 
 file "/tmp/mgmt/exchange-${sys.hostname()}" {
 	content => template("Found: {{ . }}\n", $exchanged),

examples/lang/hysteresis1.mcl

@@ -1,10 +1,12 @@
+import "sys"
+
 file "/tmp/mgmt/systemload" {
 	content => template("load average: {{ .load }} threshold: {{ .threshold }}\n", $tmplvalues),
 }
 
 $tmplvalues = struct{load => $theload, threshold => $threshold,}
 
-$theload = structlookup(load(), "x1")
+$theload = structlookup(sys.load(), "x1")
 $threshold = 1.5	# change me if you like
 
 # simple hysteresis implementation

examples/lang/iteration1.mcl

@@ -0,0 +1,21 @@
+# single resource
+print "name" {}
+
+# single resource, defined by list variable
+$names = ["hey", "there",]
+print $names {
+	Meta:autogroup => false,
+}
+
+# multiples resources, defined by list
+print ["hello", "world",] {
+	Meta:autogroup => false,
+	Depend => Print[$names],
+}
+
+$morenames = ["wow", "cool", "amazing",]
+print $morenames {
+	Meta:autogroup => false,
+}
+
+Print[$names] -> Print[$morenames]

examples/lang/modules/badexample1/main/h2g2.mcl

@@ -0,0 +1,3 @@
+import "third.mcl"
+
+$answer = 42 + $third.three

examples/lang/modules/badexample1/main/hello.mcl

@@ -0,0 +1,20 @@
+import "fmt"
+
+import "h2g2.mcl"
+
+import "mod1/"
+
+# imports as example1
+import "git://github.com/purpleidea/mgmt-example1/"
+
+$answer = $h2g2.answer
+
+test "hello" {
+	anotherstr => fmt.printf("the answer is: %d", $answer),
+}
+test "hello2" {
+	anotherstr => fmt.printf("i imported local: %s", $mod1.name),
+}
+test "hello3" {
+	anotherstr => fmt.printf("i imported remote: %s", $example1.name),
+}

examples/lang/modules/badexample1/main/mod1/main.mcl

@@ -0,0 +1 @@
+$name = "this is module mod1"

examples/lang/modules/badexample1/main/third.mcl

@@ -0,0 +1 @@
+$three = 3

examples/lang/modules/badexample1/path/github.com/purpleidea/mgmt-example1/main.mcl

@@ -0,0 +1,2 @@
+# this is a pretty lame module!
+$name = "i am github.com/purpleidea/mgmt-example1/"

examples/lang/modules/badexample1/path/github.com/purpleidea/mgmt-example2/main.mcl

@@ -0,0 +1,6 @@
+# this is a pretty lame module!
+$name = "i am github.com/purpleidea/mgmt-example2/" # + $hmmm
+
+# import another module
+import "git://github.com/purpleidea/mgmt-example1/"
+$hmmm = $example1.name

examples/lang/nspawn0.mcl

@@ -0,0 +1,31 @@
+# setenforce Permissive
+
+import "fmt"
+
+$codename = "stretch"
+$baserepo = "https://deb.debian.org/debian/"
+$rootpath = "/var/lib/machines/"
+
+pkg "debootstrap" {
+	state => "newest",
+}
+
+$dir = $codename + "-" + "nspawn" # dir name
+$cmd = fmt.printf("debootstrap --include=systemd-container %s %s %s", $codename, $dir, $baserepo)
+exec "debootstrap-" + $codename {
+	cwd => $rootpath,
+
+	shell => "/bin/bash",
+	cmd => $cmd,
+
+	ifshell => "/bin/bash",
+	ifcmd => fmt.printf("test ! -d %s", $rootpath),
+
+	Depend => Pkg["debootstrap"],
+}
+
+nspawn $dir {
+	state => "running",
+
+	Depend => Exec["debootstrap-" + $codename],
+}

examples/lang/nspawn1.mcl

@@ -0,0 +1,3 @@
+nspawn "sid-chroot" {
+	state => "running",
+}

examples/lang/nspawn2.mcl

@@ -0,0 +1,3 @@
+nspawn "Fedora-Cloud-Base-27-1.6.x86_64" {
+	state => "running",
+}

examples/lang/nspawn3.mcl

@@ -0,0 +1,4 @@
+# setenforce Permissive
+nspawn "stretch-nspawn-1" {
+	state => "running",
+}

examples/lang/readfile1.mcl

@@ -0,0 +1,6 @@
+import "os"
+
+# this copies the contents from /tmp/input and puts them in /tmp/output
+file "/tmp/output" {
+	content => os.readfile("/tmp/input"),
+}

examples/lang/schedule0.mcl

@@ -1,4 +1,5 @@
 import "sys"
+import "world"
 
 # here are all the possible options:
 #$opts = struct{strategy => "rr", max => 3, reuse => false, ttl => 10,}
@@ -10,10 +11,10 @@ import "sys"
 $opts = struct{strategy => "rr", max => 2, ttl => 10,}
 
 # schedule in a particular namespace with options:
-$set = schedule("xsched", $opts)
+$set = world.schedule("xsched", $opts)
 
 # and if you want, you can omit the options entirely:
-#$set = schedule("xsched")
+#$set = world.schedule("xsched")
 
 file "/tmp/mgmt/scheduled-${sys.hostname()}" {
 	content => template("set: {{ . }}\n", $set),

examples/lang/sendrecv3.mcl

@@ -1,7 +1,8 @@
 import "fmt"
+import "world"
 
 $ns = "estate"
-$exchanged = kvlookup($ns)
+$exchanged = world.kvlookup($ns)
 $state = maplookup($exchanged, $hostname, "default")
 
 exec "exec0" {

examples/lang/states0.mcl

@@ -1,5 +1,7 @@
+import "world"
+
 $ns = "estate"
-$exchanged = kvlookup($ns)
+$exchanged = world.kvlookup($ns)
 
 $state = maplookup($exchanged, $hostname, "default")
 

examples/lang/uptime0.mcl

@@ -0,0 +1,8 @@
+import "fmt"
+import "sys"
+
+$uptime = sys.uptime()
+
+print "print1" {
+	msg => fmt.printf("uptime: %d", $uptime),
+}

examples/lang/virt2.mcl

@@ -0,0 +1,53 @@
+# qemu-img create -b fedora-23.qcow2 -f qcow2 fedora-23-scratch.qcow2
+
+import "fmt"
+import "os"
+import "strings"
+import "example"
+
+$input = example.str2int(strings.trim_space(os.readfile("/tmp/cpu-count")))
+$count = if $input > 8 {
+	8
+} else {
+	if $input < 1 {
+		1
+	} else {
+		$input
+	}
+}
+
+file "/tmp/output" {
+	content => fmt.printf("requesting: %d cpus\n", $count),
+}
+
+virt "mgmt4" {
+	uri => "qemu:///session",
+	cpus => $count,
+	maxcpus => 8,
+	memory => 524288,
+	state => "running",
+	transient => false,
+	boot => ["hd", ],
+	# can't add this part until we fix the unification bug
+	#disk => [
+	#	struct{
+	#		source => "~/.local/share/libvirt/images/fedora-23-scratch.qcow2",
+	#		type => "qcow2",
+	#	},
+	#],
+	# add the rest for unification bug
+	#osinit => "",
+	#cdrom => [
+	#],
+	#network => [
+	#],
+	#filesystem => [
+	#],
+	#auth => struct{
+	#	username => "",
+	#	password => "",
+	#},
+	#hotcpus => true,	# this is the default
+	#restartondiverge => "",
+	#restartonrefresh => false,
+}

integration/instance.go

@@ -315,10 +315,11 @@ func (obj *Instance) Wait(ctx context.Context) error {
 			if err := event.Error; err != nil {
 				return errwrap.Wrapf(err, "error event received")
 			}
-
+			startup = nil
 			// send event...
 
 		case <-ctx.Done():
+			startup = nil
 			return ctx.Err()
 		}
 

lang/Makefile

@@ -25,13 +25,15 @@ all: build
 build: lexer.nn.go y.go
 
 clean:
-	rm lexer.nn.go y.go y.output || true
+	@rm -f lexer.nn.go y.go y.output || true
 
 lexer.nn.go: lexer.nex
+	@echo "Generating: lexer..."
 	nex -e lexer.nex
 	@ROOT="$$( cd "$$( dirname "$${BASH_SOURCE[0]}" )" && cd .. && pwd )" && $$ROOT/misc/header.sh 'lexer.nn.go'
 
 y.go: parser.y
+	@echo "Generating: parser..."
 ifneq ($(OLDGOYACC),)
 	go tool yacc parser.y
 else

lang/funcs/core/.gitignore

@@ -0,0 +1,2 @@
+generated_funcs.go
+generated_funcs_test.go

lang/funcs/core/core.go

@@ -19,9 +19,12 @@ package core
 
 import (
 	// import so the funcs register
-	_ "github.com/purpleidea/mgmt/lang/funcs/core/coredatetime"
-	_ "github.com/purpleidea/mgmt/lang/funcs/core/coreexample"
-	_ "github.com/purpleidea/mgmt/lang/funcs/core/corefmt"
-	_ "github.com/purpleidea/mgmt/lang/funcs/core/coremath"
-	_ "github.com/purpleidea/mgmt/lang/funcs/core/coresys"
+	_ "github.com/purpleidea/mgmt/lang/funcs/core/datetime"
+	_ "github.com/purpleidea/mgmt/lang/funcs/core/example"
+	_ "github.com/purpleidea/mgmt/lang/funcs/core/fmt"
+	_ "github.com/purpleidea/mgmt/lang/funcs/core/math"
+	_ "github.com/purpleidea/mgmt/lang/funcs/core/os"
+	_ "github.com/purpleidea/mgmt/lang/funcs/core/strings"
+	_ "github.com/purpleidea/mgmt/lang/funcs/core/sys"
+	_ "github.com/purpleidea/mgmt/lang/funcs/core/world"
 )

lang/funcs/core/example/str2int_func.go

@@ -0,0 +1,40 @@
+// Mgmt
+// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package coreexample
+
+import (
+	"strconv"
+
+	"github.com/purpleidea/mgmt/lang/funcs/simple"
+	"github.com/purpleidea/mgmt/lang/types"
+)
+
+func init() {
+	simple.ModuleRegister(moduleName, "str2int", &types.FuncValue{
+		T: types.NewType("func(a str) int"),
+		V: func(input []types.Value) (types.Value, error) {
+			var i int64
+			if val, err := strconv.ParseInt(input[0].Str(), 10, 64); err == nil {
+				i = val
+			}
+			return &types.IntValue{
+				V: i,
+			}, nil
+		},
+	})
+}

lang/funcs/core/funcgen.yaml

@@ -0,0 +1,55 @@
+# This file is used by github.com/purpleidea/mgmt/lang/funcs/funcgen/ to
+# generate mcl functions.
+functions:
+- mgmtName: to_upper
+  mgmtPackage: strings
+  help: turns a string to uppercase.
+  goPackage: strings
+  goFunc: ToUpper
+  args: [{name: a, type: string}]
+  return: [{type: string}]
+  tests:
+    - args: [{type: string, value: "Hello"}]
+      return: [{type: string, value: "HELLO"}]
+    - args: [{type: string, value: "HELLO 22"}]
+      return: [{type: string, value: "HELLO 22"}]
+- mgmtName: trim
+  mgmtPackage: strings
+  help: returns a slice of the string s with all leading and trailing Unicode code points contained in cutset removed.
+  goPackage: strings
+  goFunc: Trim
+  args: [{name: s, type: string}, {name: cutset, type: string}]
+  return: [{type: string}]
+  tests:
+    - args: [{type: string, value: "??Hello.."}, {type: string, value: "?."}]
+      return: [{type: string, value: "Hello"}]
+- mgmtName: trim_left
+  mgmtPackage: strings
+  help: returns a slice of the string s with all leading Unicode code points contained in cutset removed.
+  goPackage: strings
+  goFunc: TrimLeft
+  args: [{name: s, type: string}, {name: cutset, type: string}]
+  return: [{type: string}]
+  tests:
+    - args: [{type: string, value: "??Hello.."}, {type: string, value: "?."}]
+      return: [{type: string, value: "Hello.."}]
+- mgmtName: trim_space
+  mgmtPackage: strings
+  help: returns a slice of the string s, with all leading and trailing white space removed, as defined by Unicode.
+  goPackage: strings
+  goFunc: TrimSpace
+  args: [{name: s, type: string}]
+  return: [{type: string}]
+  tests:
+    - args: [{type: string, value: "Hello 2    "}]
+      return: [{type: string, value: "Hello 2"}]
+- mgmtName: trim_right
+  mgmtPackage: strings
+  help: returns a slice of the string s with all trailing Unicode code points contained in cutset removed.
+  goPackage: strings
+  goFunc: TrimRight
+  args: [{name: s, type: string}, {name: cutset, type: string}]
+  return: [{type: string}]
+  tests:
+    - args: [{type: string, value: "??Hello.."}, {type: string, value: "?."}]
+      return: [{type: string, value: "??Hello"}]