lang: unification: Improve type unification algorithm

The simple type unification algorithm suffered from some serious
performance and memory problems when used with certain code bases. This
adds some crucial optimizations that improve performance drastically.

.github/FUNDING.yml

@@ -0,0 +1,2 @@
+# You can add one username per supported platform and one custom link
+patreon: purpleidea

COPYRIGHT

@@ -1,5 +1,5 @@
 Mgmt
-Copyright (C) 2013-2018+ James Shubin and the project contributors
+Copyright (C) 2013-2019+ 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

Makefile

@@ -1,5 +1,5 @@
 # Mgmt
-# Copyright (C) 2013-2018+ James Shubin and the project contributors
+# Copyright (C) 2013-2019+ 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
@@ -188,8 +188,8 @@ $(addprefix test-shell-,${test_shell}): test-shell-%: build
 
 gofmt:
 	# TODO: remove gofmt once goimports has a -s option
-	find . -maxdepth 6 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec gofmt -s -w {} \;
-	find . -maxdepth 6 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec goimports -w {} \;
+	find . -maxdepth 9 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec gofmt -s -w {} \;
+	find . -maxdepth 9 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec goimports -w {} \;
 
 yamlfmt:
 	find . -maxdepth 3 -type f -name '*.yaml' -not -path './old/*' -not -path './tmp/*' -not -path './omv.yaml' -exec ruby -e "require 'yaml'; x=YAML.load_file('{}').to_yaml.each_line.map(&:rstrip).join(10.chr)+10.chr; File.open('{}', 'w').write x" \;

bindata/Makefile

@@ -1,5 +1,5 @@
 # Mgmt
-# Copyright (C) 2013-2018+ James Shubin and the project contributors
+# Copyright (C) 2013-2019+ 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

converger/converger.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -25,8 +25,7 @@ import (
 	"time"
 
 	"github.com/purpleidea/mgmt/util"
-
-	multierr "github.com/hashicorp/go-multierror"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // New builds a new converger coordinator.
@@ -323,9 +322,8 @@ func (obj *Coordinator) runStateFns(converged bool) error {
 	for _, name := range keys { // run in deterministic order
 		fn := obj.stateFns[name]
 		// call an arbitrary function
-		if e := fn(converged); e != nil {
-			err = multierr.Append(err, e) // list of errors
-		}
+		e := fn(converged)
+		err = errwrap.Append(err, e) // list of errors
 	}
 	return err
 }

converger/converger_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

debian/copyright

@@ -3,7 +3,7 @@ Upstream-Name: mgmt
 Source: <https://github.com/purpleidea/mgmt>
 
 Files: *
-Copyright: Copyright (C) 2013-2018+ James Shubin and the project contributors
+Copyright: Copyright (C) 2013-2019+ James Shubin and the project contributors
 License: GPL-3.0
 
 License: GPL-3.0

doc.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

docs/conf.py

@@ -51,7 +51,7 @@ master_doc = 'index'
 
 # General information about the project.
 project = u'mgmt'
-copyright = u'2013-2018+ James Shubin and the project contributors'
+copyright = u'2013-2019+ James Shubin and the project contributors'
 author = u'James Shubin'
 
 # The version info for the project you're documenting, acts as replacement for

docs/development.md

@@ -3,7 +3,7 @@
 This document contains some additional information and help regarding
 developing `mgmt`. Useful tools, conventions, etc.
 
-Be sure to read [quick start guide](docs/quick-start-guide.md) first.
+Be sure to read [quick start guide](quick-start-guide.md) first.
 
 ## Testing
 

docs/documentation.md

@@ -455,7 +455,7 @@ To report any bugs, please file a ticket at: [https://github.com/purpleidea/mgmt
 
 ## Authors
 
-Copyright (C) 2013-2018+ James Shubin and the project contributors
+Copyright (C) 2013-2019+ James Shubin and the project contributors
 
 Please see the
 [AUTHORS](https://github.com/purpleidea/mgmt/tree/master/AUTHORS) file

docs/faq.md

@@ -9,6 +9,18 @@ I wanted a next generation config management solution that didn't have all of
 the design flaws or limitations that the current generation of tools do, and no
 tool existed!
 
+### Why did you choose `golang` for the project?
+
+When I started working on the project, I needed to choose a language that
+already had an implementation of a distributed consensus algorithm available.
+That meant [Paxos](https://en.wikipedia.org/wiki/Paxos_(computer_science)) or
+[Raft](https://en.wikipedia.org/wiki/Raft_(computer_science)). Golang was one
+language that actually had two different Raft implementations, `etcd`, and
+`consul`. Other design requirements included something that was reasonably fast,
+typed and memory-safe, and suited for systems engineering. After a reasonably
+extensive search, I chose `golang`. I think it was the right decision. There are
+a number of other features of the language which helped influence the decision.
+
 ### How do I contribute to the project if I don't know `golang`?
 
 There are many different ways you can contribute to the project. They can be
@@ -125,6 +137,58 @@ The downside to this approach is that you won't benefit from the automatic
 elastic nature of the embedded etcd servers, and that you're responsible if you
 accidentally break your etcd cluster, or if you use an unsupported version.
 
+### In `mgmt` you talk about events. What is this referring to?
+
+Mgmt has two main concepts that involve "events":
+1. Events in the [resource primitive](resource-guide.md).
+2. Events in the [reactive language](language-guide.md).
+
+Each resource primitive in mgmt can test (check) and set (apply) the desired
+state that was requested of it. This is familiar to what is common with existing
+tools such as `Puppet`, `Ansible`, `Chef`, `Terraform`, etc... In addition,
+`mgmt` can also **watch** the state and detect changes. As a result, it never
+has to waste time and cpu resources by polling to test and set state, leading to
+a design which is algorithmically much faster than the existing generation of
+tools.
+
+To describe the set of resources to apply, mgmt describes this collection with a
+language. In order to model the time component of infrastructure, we use a
+special kind of language called an [FRP](https://en.wikipedia.org/wiki/Functional_reactive_programming).
+This language has a built-in concept that we call "events", and which means that
+we re-evaluate the relevant portions of the code whenever a value or function
+has an event that tells us that it changed. The `R` in `FRP` stands for
+reactive. This is similar to how a spreadsheet updates dependent cells when a
+pre-requisite value is modified. [This article](https://en.wikipedia.org/wiki/Reactive_programming)
+provides a bit more background.
+
+Whenever any of the streams of values in the language change, the program is
+partially re-evaluated. The output of any mgmt program is a [DAG](https://en.wikipedia.org/wiki/Directed_acyclic_graph)
+of resources, or more precisely, a stream of resource graphs. Since we have
+events per-resource, we can efficiently switch from one desired-state resource
+graph to the next without re-checking their individual states, since we've been
+monitoring them all along.
+
+One side-effect of all this, is that if a rogue systems administrator manually
+changes the state of any managed resource, mgmt will detect this and attempt to
+revert the change. This makes for excellent live demos, but is not the primary
+design goal. It is a consequence of tracking state so that graph changes are
+efficient. We implement the event detection via an intentional per-resource
+[main loop](https://en.wikipedia.org/wiki/Event_loop) which can enable other
+interesting functionality too!
+
+Make sure to get rid of your rogue sysadmin! ;)
+
+### Do I need to run `mgmt` as `root`?
+
+No and yes. It depends. Nothing in mgmt explicitly requires root in the design,
+however mgmt will require root only if the changes to your system that you want
+it to make require root.
+
+For example, if you use it to manage files that require root access to modify,
+then you'll need root. If you only use it to manage files and resources
+elsewhere, then it shouldn't need root. Many resources are perfectly usable
+without root, and virtually all of my live demos are done without root.
+
 ### How can I run `mgmt` on-demand, or in `cron`, instead of continuously?
 
 By default, `mgmt` will run continuously in an attempt to keep your machine in a
@@ -151,40 +215,38 @@ requires a number of seconds as an argument.
 ./mgmt run lang --lang examples/lang/hello0.mcl --converged-timeout=5
 ```
 
-### What does the error message about an inconsistent dataDir mean?
+### On startup `mgmt` hangs after: `etcd: server: starting...`.
 
 If you get an error message similar to:
 
 ```
-Etcd: Connect: CtxError...
-Etcd: CtxError: Reason: CtxDelayErr(5s): No endpoints available yet!
-Etcd: Connect: Endpoints: []
-Etcd: The dataDir (/var/lib/mgmt/etcd) might be inconsistent or corrupt.
+etcd: server: starting...
+etcd: server: start timeout of 1m0s reached
+etcd: server: close timeout of 15s reached
 ```
 
-This happens when there are a series of fatal connect errors in a row. This can
-happen when you start `mgmt` using a dataDir that doesn't correspond to the
-current cluster view. As a result, the embedded etcd server never finishes
-starting up, and as a result, a default endpoint never gets added. The solution
-is to either reconcile the mistake, and if there is no important data saved, you
-can remove the etcd dataDir. This is typically `/var/lib/mgmt/etcd/member/`.
+But nothing happens afterwards, this can be due to a corrupt etcd storage
+directory. Each etcd server embedded in mgmt must have a special directory where
+it stores local state. It must not be shared by more than one individual member.
+This dir is typically `/var/lib/mgmt/etcd/member/`. If you accidentally use it
+(for example during testing) with a different cluster view, then you can corrupt
+it. This can happen if you use it with more than one different hostname.
 
-### Why do resources have both a `Cmp` method and an `IFF` (on the UID) method?
+The solution is to avoid making this mistake, and if there is no important data
+saved, you can remove the etcd member dir and start over.
 
-The `Cmp()` methods are for determining if two resources are effectively the
-same, which is used to make graph change delta's efficient. This is when we want
-to change from the current running graph to a new graph, but preserve the common
-vertices. Since we want to make this process efficient, we only update the parts
-that are different, and leave everything else alone. This `Cmp()` method can
-tell us if two resources are the same. In case it is not obvious, `cmp` is an
-abbrev. for compare.
+### On running `make` to build a new version, it errors with: `Text file busy`.
 
-The `IFF()` method is part of the whole UID system, which is for discerning if a
-resource meets the requirements another expects for an automatic edge. This is
-because the automatic edge system assumes a unified UID pattern to test for
-equality. In the future it might be helpful or sane to merge the two similar
-comparison functions although for now they are separate because they are
-actually answer different questions.
+If you get an error like:
+
+```
+cp: cannot create regular file 'mgmt': Text file busy
+```
+
+This can happen if you ran `make build` (or just `make`) when there was already
+an instance of mgmt running, or if a related file locking issue occurred. To
+solve this, shutdown and running mgmt process, run `rm mgmt` to remove the file,
+and then get a new one by running `make` again.
 
 ### Does this support Windows? OSX? GNU Hurd?
 
@@ -262,9 +324,8 @@ which definitely existed before the band did.
 ### You didn't answer my question, or I have a question!
 
 It's best to ask on [IRC](https://webchat.freenode.net/?channels=#mgmtconfig)
-to see if someone can help you. Once we get a big enough community going, we'll
-add a mailing list. If you don't get any response from the above, you can
-contact me through my [technical blog](https://purpleidea.com/contact/)
-and I'll do my best to help. If you have a good question, please add it as a
-patch to this documentation. I'll merge your question, and add a patch with the
-answer!
+to see if someone can help you. If you don't get a response from IRC, you can
+contact me through my [technical blog](https://purpleidea.com/contact/) and I'll
+do my best to help. If you have a good question, please add it as a patch to
+this documentation. I'll merge your question, and add a patch with the answer!
+For news and updates, subscribe to the [mailing list](https://www.redhat.com/mailman/listinfo/mgmtconfig-list).

docs/resource-guide.md

@@ -96,8 +96,10 @@ Default() engine.Res
 ```
 
 This returns a populated resource struct as a `Res`. It shouldn't populate any
-values which already have the correct default as the golang zero value. In
+values which already get a good default as the respective golang zero value. In
 general it is preferable if the zero values make for the correct defaults.
+(This is to say, resources are designed to behave safely and intuitively
+when parameters take a zero value, whenever this is possible.)
 
 #### Example
 
@@ -754,6 +756,23 @@ Feel free to use this pattern if you're convinced it's necessary. Alternatively,
 if you think I got the `Res` API wrong and you have an improvement, please let
 us know!
 
+### Why do resources have both a `Cmp` method and an `IFF` (on the UID) method?
+
+The `Cmp()` methods are for determining if two resources are effectively the
+same, which is used to make graph change delta's efficient. This is when we want
+to change from the current running graph to a new graph, but preserve the common
+vertices. Since we want to make this process efficient, we only update the parts
+that are different, and leave everything else alone. This `Cmp()` method can
+tell us if two resources are the same. In case it is not obvious, `cmp` is an
+abbrev. for compare.
+
+The `IFF()` method is part of the whole UID system, which is for discerning if a
+resource meets the requirements another expects for an automatic edge. This is
+because the automatic edge system assumes a unified UID pattern to test for
+equality. In the future it might be helpful or sane to merge the two similar
+comparison functions although for now they are separate because they are
+actually answer different questions.
+
 ### What new resource primitives need writing?
 
 There are still many ideas for new resources that haven't been written yet. If

engine/autoedge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/autoedge_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/autogroup.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/cmp.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/copy.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -20,7 +20,7 @@ package engine
 import (
 	"fmt"
 
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // ResCopy copies a resource. This is the main entry point for copying a

engine/edge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/error.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/fs.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/graph/actions.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -25,9 +25,8 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/pgraph"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
 	"golang.org/x/time/rate"
 )
 
@@ -377,8 +376,8 @@ Loop:
 			// we then save so we can return it to the caller of us.
 			if err != nil {
 				failed = true
-				close(obj.state[vertex].watchDone)    // causes doneChan to close
-				reterr = multierr.Append(reterr, err) // permanent failure
+				close(obj.state[vertex].watchDone)   // causes doneChan to close
+				reterr = errwrap.Append(reterr, err) // permanent failure
 				continue
 			}
 			if obj.Debug {
@@ -458,8 +457,8 @@ Loop:
 					}
 					if e != nil {
 						failed = true
-						close(obj.state[vertex].limitDone)  // causes doneChan to close
-						reterr = multierr.Append(reterr, e) // permanent failure
+						close(obj.state[vertex].limitDone) // causes doneChan to close
+						reterr = errwrap.Append(reterr, e) // permanent failure
 						break LimitWait
 					}
 					if obj.Debug {
@@ -498,8 +497,8 @@ Loop:
 						}
 						if e != nil {
 							failed = true
-							close(obj.state[vertex].limitDone)  // causes doneChan to close
-							reterr = multierr.Append(reterr, e) // permanent failure
+							close(obj.state[vertex].limitDone) // causes doneChan to close
+							reterr = errwrap.Append(reterr, e) // permanent failure
 							break RetryWait
 						}
 						if obj.Debug {
@@ -546,8 +545,8 @@ Loop:
 			// 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
+			close(obj.state[vertex].processDone) // causes doneChan to close
+			reterr = errwrap.Append(reterr, err) // permanent failure
 			continue
 
 		} // retry loop

engine/graph/autoedge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/graph/autoedge/autoedge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -22,9 +22,7 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/pgraph"
-
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // AutoEdge adds the automatic edges to the graph.
@@ -49,7 +47,7 @@ func AutoEdge(graph *pgraph.Graph, debug bool, logf func(format string, v ...int
 	for _, res := range sorted { // for each vertexes autoedges
 		autoEdgeObj, e := res.AutoEdges()
 		if e != nil {
-			err = multierr.Append(err, e) // collect all errors
+			err = errwrap.Append(err, e) // collect all errors
 			continue
 		}
 		if autoEdgeObj == nil {

engine/graph/autogroup.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -24,8 +24,7 @@ import (
 	"github.com/purpleidea/mgmt/engine/graph/autogroup"
 	"github.com/purpleidea/mgmt/pgraph"
 	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // AutoGroup runs the auto grouping on the loaded graph.

engine/graph/autogroup/autogroup.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -22,8 +22,7 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/pgraph"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // AutoGroup is the mechanical auto group "runner" that runs the interface spec.

engine/graph/autogroup/autogroup_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -31,8 +31,7 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/pgraph"
 	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {

engine/graph/autogroup/base.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/graph/autogroup/nonreachability.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -19,8 +19,7 @@ package autogroup
 
 import (
 	"github.com/purpleidea/mgmt/pgraph"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // NonReachabilityGrouper is the most straight-forward algorithm for grouping.

engine/graph/autogroup/util.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -19,8 +19,7 @@ package autogroup
 
 import (
 	"github.com/purpleidea/mgmt/pgraph"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // VertexMerge merges v2 into v1 by reattaching the edges where appropriate,

engine/graph/engine.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -26,10 +26,8 @@ import (
 	"github.com/purpleidea/mgmt/converger"
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/pgraph"
+	"github.com/purpleidea/mgmt/util/errwrap"
 	"github.com/purpleidea/mgmt/util/semaphore"
-
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
 )
 
 // Engine encapsulates a generic graph and manages its operations.
@@ -64,6 +62,13 @@ type Engine struct {
 // If the struct does not validate, or it cannot initialize, then this errors.
 // Initially it will contain an empty graph.
 func (obj *Engine) Init() error {
+	if obj.Program == "" {
+		return fmt.Errorf("the Program is empty")
+	}
+	if obj.Hostname == "" {
+		return fmt.Errorf("the Hostname is empty")
+	}
+
 	var err error
 	if obj.graph, err = pgraph.NewGraph("graph"); err != nil {
 		return err
@@ -380,20 +385,16 @@ func (obj *Engine) Pause(fastPause bool) error {
 
 // Close triggers a shutdown. Engine must be already paused before this is run.
 func (obj *Engine) Close() error {
-	var reterr error
-
-	emptyGraph, err := pgraph.NewGraph("empty")
-	if err != nil {
-		reterr = multierr.Append(reterr, err) // list of errors
-	}
+	emptyGraph, reterr := pgraph.NewGraph("empty")
 
 	// this is a graph switch (graph sync) that switches to an empty graph!
 	if err := obj.Load(emptyGraph); err != nil { // copy in empty graph
-		reterr = multierr.Append(reterr, err)
+		reterr = errwrap.Append(reterr, err)
 	}
+	// FIXME: Do we want to run commit if Load failed? Does this even work?
 	// the commit will cause the graph sync to shut things down cleverly...
 	if err := obj.Commit(); err != nil {
-		reterr = multierr.Append(reterr, err)
+		reterr = errwrap.Append(reterr, err)
 	}
 
 	obj.wg.Wait() // for now, this doesn't need to be a separate Wait() method

engine/graph/graph_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -23,13 +23,13 @@ import (
 	"fmt"
 	"testing"
 
-	multierr "github.com/hashicorp/go-multierror"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 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
+	err = errwrap.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/refresh.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/graph/semaphore.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -23,9 +23,8 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/purpleidea/mgmt/util/errwrap"
 	"github.com/purpleidea/mgmt/util/semaphore"
-
-	multierr "github.com/hashicorp/go-multierror"
 )
 
 // SemaSep is the trailing separator to split the semaphore id from the size.
@@ -46,9 +45,8 @@ func (obj *Engine) semaLock(semas []string) error {
 		}
 		obj.slock.Unlock()
 
-		if err := sema.P(1); err != nil { // lock!
-			reterr = multierr.Append(reterr, err) // list of errors
-		}
+		err := sema.P(1)                     // lock!
+		reterr = errwrap.Append(reterr, err) // list of errors
 	}
 	return reterr
 }
@@ -65,9 +63,8 @@ func (obj *Engine) semaUnlock(semas []string) error {
 			panic(fmt.Sprintf("graph: sema: %s does not exist", id))
 		}
 
-		if err := sema.V(1); err != nil { // unlock!
-			reterr = multierr.Append(reterr, err) // list of errors
-		}
+		err := sema.V(1)                     // unlock!
+		reterr = errwrap.Append(reterr, err) // list of errors
 	}
 	return reterr
 }

engine/graph/semaphore_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/graph/sendrecv.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -22,9 +22,8 @@ import (
 	"reflect"
 
 	"github.com/purpleidea/mgmt/engine"
-
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
+	engineUtil "github.com/purpleidea/mgmt/engine/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // SendRecv pulls in the sent values into the receive slots. It is called by the
@@ -47,16 +46,51 @@ func (obj *Engine) SendRecv(res engine.RecvableRes) (map[string]bool, error) {
 			st = v.Res.Sent()
 		}
 
+		if st == nil {
+			e := fmt.Errorf("received nil value from: %s", v.Res)
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+
+		if e := engineUtil.StructFieldCompat(st, v.Key, res, k); e != nil {
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+
 		// send
+		m1, e := engineUtil.StructTagToFieldName(st)
+		if e != nil {
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+		key1, exists := m1[v.Key]
+		if !exists {
+			e := fmt.Errorf("requested key of `%s` not found in send struct", v.Key)
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+
 		obj1 := reflect.Indirect(reflect.ValueOf(st))
 		type1 := obj1.Type()
-		value1 := obj1.FieldByName(v.Key)
+		value1 := obj1.FieldByName(key1)
 		kind1 := value1.Kind()
 
 		// recv
+		m2, e := engineUtil.StructTagToFieldName(res)
+		if e != nil {
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+		key2, exists := m2[k]
+		if !exists {
+			e := fmt.Errorf("requested key of `%s` not found in recv struct", k)
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+
 		obj2 := reflect.Indirect(reflect.ValueOf(res)) // pass in full struct
 		type2 := obj2.Type()
-		value2 := obj2.FieldByName(k)
+		value2 := obj2.FieldByName(key2)
 		kind2 := value2.Kind()
 
 		if obj.Debug {
@@ -67,7 +101,7 @@ func (obj *Engine) SendRecv(res engine.RecvableRes) (map[string]bool, error) {
 		// i think we probably want the same kind, at least for now...
 		if kind1 != kind2 {
 			e := fmt.Errorf("kind mismatch between %s: %s and %s: %s", v.Res, kind1, res, kind2)
-			err = multierr.Append(err, e) // list of errors
+			err = errwrap.Append(err, e) // list of errors
 			continue
 		}
 
@@ -75,21 +109,21 @@ func (obj *Engine) SendRecv(res engine.RecvableRes) (map[string]bool, error) {
 		// FIXME: do we want to relax this for string -> *string ?
 		if e := TypeCmp(value1, value2); e != nil {
 			e := errwrap.Wrapf(e, "type mismatch between %s and %s", v.Res, res)
-			err = multierr.Append(err, e) // list of errors
+			err = errwrap.Append(err, e) // list of errors
 			continue
 		}
 
 		// if we can't set, then well this is pointless!
 		if !value2.CanSet() {
 			e := fmt.Errorf("can't set %s.%s", res, k)
-			err = multierr.Append(err, e) // list of errors
+			err = errwrap.Append(err, e) // list of errors
 			continue
 		}
 
 		// if we can't interface, we can't compare...
 		if !value1.CanInterface() || !value2.CanInterface() {
 			e := fmt.Errorf("can't interface %s.%s", res, k)
-			err = multierr.Append(err, e) // list of errors
+			err = errwrap.Append(err, e) // list of errors
 			continue
 		}
 

engine/graph/state.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -26,8 +26,7 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/pgraph"
 	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // State stores some state about the resource it is mapped to.

engine/graph/vardir.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -22,7 +22,7 @@ import (
 	"os"
 	"path"
 
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // varDir returns the path to a working directory for the resource. It will try

engine/metaparams.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -22,8 +22,8 @@ import (
 	"strconv"
 
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
-	errwrap "github.com/pkg/errors"
 	"golang.org/x/time/rate"
 )
 

engine/metaparams_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/refresh.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -21,7 +21,8 @@ import (
 	"encoding/gob"
 	"fmt"
 
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
+
 	"gopkg.in/yaml.v2"
 )
 

engine/resources/augeas.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -27,8 +27,8 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
-	errwrap "github.com/pkg/errors"
 	// FIXME: we vendor go/augeas because master requires augeas 1.6.0
 	// and libaugeas-dev-1.6.0 is not yet available in a PPA.
 	"honnef.co/go/augeas"

engine/resources/aws_ec2.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -34,6 +34,7 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
@@ -42,8 +43,6 @@ import (
 	cwe "github.com/aws/aws-sdk-go/service/cloudwatchevents"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/sns"
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
 )
 
 func init() {
@@ -393,17 +392,14 @@ func (obj *AwsEc2Res) Close() error {
 	// clean up sns objects created by Init/snsWatch
 	if obj.snsClient != nil {
 		// delete the topic and associated subscriptions
-		if err := obj.snsDeleteTopic(obj.snsTopicArn); err != nil {
-			errList = multierr.Append(errList, err)
-		}
+		e1 := obj.snsDeleteTopic(obj.snsTopicArn)
+		errList = errwrap.Append(errList, e1)
 		// remove the target
-		if err := obj.cweRemoveTarget(CweTargetID, CweRuleName); err != nil {
-			errList = multierr.Append(errList, err)
-		}
+		e2 := obj.cweRemoveTarget(CweTargetID, CweRuleName)
+		errList = errwrap.Append(errList, e2)
 		// delete the cloudwatch rule
-		if err := obj.cweDeleteRule(CweRuleName); err != nil {
-			errList = multierr.Append(errList, err)
-		}
+		e3 := obj.cweDeleteRule(CweRuleName)
+		errList = errwrap.Append(errList, e3)
 	}
 
 	return errList
@@ -608,7 +604,7 @@ func (obj *AwsEc2Res) snsWatch() error {
 }
 
 // CheckApply method for AwsEc2 resource.
-func (obj *AwsEc2Res) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *AwsEc2Res) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply(%t)", apply)
 
 	// find the instance we need to check

engine/resources/config_etcd.go

@@ -0,0 +1,250 @@
+// Mgmt
+// Copyright (C) 2013-2019+ 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 resources
+
+import (
+	"context"
+	"fmt"
+	"sync"
+	"time"
+
+	"github.com/purpleidea/mgmt/engine"
+	"github.com/purpleidea/mgmt/engine/traits"
+	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
+)
+
+func init() {
+	engine.RegisterResource("config:etcd", func() engine.Res { return &ConfigEtcdRes{} })
+}
+
+const (
+	sizeCheckApplyTimeout = 5 * time.Second
+)
+
+// ConfigEtcdRes is a resource that sets mgmt's etcd configuration.
+type ConfigEtcdRes struct {
+	traits.Base // add the base methods without re-implementation
+
+	init *engine.Init
+
+	// IdealClusterSize is the requested minimum size of the cluster. If you
+	// set this to zero, it will cause a cluster wide shutdown if
+	// AllowSizeShutdown is true. If it's not true, then it will cause a
+	// validation error.
+	IdealClusterSize uint16 `lang:"idealclustersize"`
+	// AllowSizeShutdown is a required safety flag that you must set to true
+	// if you want to allow causing a cluster shutdown by setting
+	// IdealClusterSize to zero.
+	AllowSizeShutdown bool `lang:"allow_size_shutdown"`
+
+	// sizeFlag determines whether sizeCheckApply already ran or not.
+	sizeFlag bool
+
+	interruptChan chan struct{}
+	wg            *sync.WaitGroup
+}
+
+// Default returns some sensible defaults for this resource.
+func (obj *ConfigEtcdRes) Default() engine.Res {
+	return &ConfigEtcdRes{}
+}
+
+// Validate if the params passed in are valid data.
+func (obj *ConfigEtcdRes) Validate() error {
+	if obj.IdealClusterSize < 0 {
+		return fmt.Errorf("the IdealClusterSize param must be positive")
+	}
+
+	if obj.IdealClusterSize == 0 && !obj.AllowSizeShutdown {
+		return fmt.Errorf("the IdealClusterSize can't be zero if AllowSizeShutdown is false")
+	}
+
+	return nil
+}
+
+// Init runs some startup code for this resource.
+func (obj *ConfigEtcdRes) Init(init *engine.Init) error {
+	obj.init = init // save for later
+
+	obj.interruptChan = make(chan struct{})
+	obj.wg = &sync.WaitGroup{}
+
+	return nil
+}
+
+// Close is run by the engine to clean up after the resource is done.
+func (obj *ConfigEtcdRes) Close() error {
+	obj.wg.Wait() // bonus
+	return nil
+}
+
+// Watch is the primary listener for this resource and it outputs events.
+func (obj *ConfigEtcdRes) Watch() error {
+	obj.wg.Add(1)
+	defer obj.wg.Done()
+	// FIXME: add timeout to context
+	// The obj.init.Done channel is closed by the engine to signal shutdown.
+	ctx, cancel := util.ContextWithCloser(context.Background(), obj.init.Done)
+	defer cancel()
+	ch, err := obj.init.World.IdealClusterSizeWatch(util.CtxWithWg(ctx, obj.wg))
+	if err != nil {
+		return errwrap.Wrapf(err, "could not watch ideal cluster size")
+	}
+
+	obj.init.Running() // when started, notify engine that we're running
+
+Loop:
+	for {
+		select {
+		case event, ok := <-ch:
+			if !ok {
+				break Loop
+			}
+			if obj.init.Debug {
+				obj.init.Logf("event: %+v", event)
+			}
+			// pass through and send an event
+
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+		}
+
+		obj.init.Event() // notify engine of an event (this can block)
+	}
+
+	return nil
+}
+
+// sizeCheckApply sets the IdealClusterSize parameter. If it sees a value change
+// to zero, then it *won't* try and change it away from zero, because it assumes
+// that someone has requested a shutdown. If the value is seen on first startup,
+// then it will change it, because it might be a zero from the previous cluster.
+func (obj *ConfigEtcdRes) sizeCheckApply(apply bool) (bool, error) {
+	wg := &sync.WaitGroup{}
+	defer wg.Wait() // this must be above the defer cancel() call
+	ctx, cancel := context.WithTimeout(context.Background(), sizeCheckApplyTimeout)
+	defer cancel()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		select {
+		case <-obj.interruptChan:
+			cancel()
+		case <-ctx.Done():
+			// let this exit
+		}
+	}()
+
+	val, err := obj.init.World.IdealClusterSizeGet(ctx)
+	if err != nil {
+		return false, errwrap.Wrapf(err, "could not get ideal cluster size")
+	}
+
+	// if we got a value of zero, and we've already run before, then it's ok
+	if obj.IdealClusterSize != 0 && val == 0 && obj.sizeFlag {
+		obj.init.Logf("impending cluster shutdown, not setting ideal cluster size")
+		return true, nil // impending shutdown, don't try and cancel it.
+	}
+	obj.sizeFlag = true
+
+	// must be done after setting the above flag
+	if obj.IdealClusterSize == val { // state is correct
+		return true, nil
+	}
+
+	if !apply {
+		return false, nil
+	}
+
+	// set!
+	// This is run as a transaction so we detect if we needed to change it.
+	changed, err := obj.init.World.IdealClusterSizeSet(ctx, obj.IdealClusterSize)
+	if err != nil {
+		return false, errwrap.Wrapf(err, "could not set ideal cluster size")
+	}
+	if !changed {
+		return true, nil // we lost a race, which means no change needed
+	}
+	obj.init.Logf("set dynamic cluster size to: %d", obj.IdealClusterSize)
+
+	return false, nil
+}
+
+// CheckApply method for Noop resource. Does nothing, returns happy!
+func (obj *ConfigEtcdRes) CheckApply(apply bool) (bool, error) {
+	checkOK := true
+
+	if c, err := obj.sizeCheckApply(apply); err != nil {
+		return false, err
+	} else if !c {
+		checkOK = false
+	}
+
+	// TODO: add more config settings management here...
+	//if c, err := obj.TODOCheckApply(apply); err != nil {
+	//	return false, err
+	//} else if !c {
+	//	checkOK = false
+	//}
+
+	return checkOK, nil // w00t
+}
+
+// Cmp compares two resources and returns an error if they are not equivalent.
+func (obj *ConfigEtcdRes) Cmp(r engine.Res) error {
+	// we can only compare ConfigEtcdRes to others of the same resource kind
+	res, ok := r.(*ConfigEtcdRes)
+	if !ok {
+		return fmt.Errorf("not a %s", obj.Kind())
+	}
+
+	if obj.IdealClusterSize != res.IdealClusterSize {
+		return fmt.Errorf("the IdealClusterSize param differs")
+	}
+	if obj.AllowSizeShutdown != res.AllowSizeShutdown {
+		return fmt.Errorf("the AllowSizeShutdown param differs")
+	}
+
+	return nil
+}
+
+// Interrupt is called to ask the execution of this resource to end early.
+func (obj *ConfigEtcdRes) Interrupt() error {
+	close(obj.interruptChan)
+	return nil
+}
+
+// UnmarshalYAML is the custom unmarshal handler for this struct.
+// It is primarily useful for setting the defaults.
+func (obj *ConfigEtcdRes) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	type rawRes ConfigEtcdRes // indirection to avoid infinite recursion
+
+	def := obj.Default()            // get the default
+	res, ok := def.(*ConfigEtcdRes) // put in the right format
+	if !ok {
+		return fmt.Errorf("could not convert to ConfigEtcdRes")
+	}
+	raw := rawRes(*res) // convert; the defaults go here
+
+	if err := unmarshal(&raw); err != nil {
+		return err
+	}
+
+	*obj = ConfigEtcdRes(raw) // restore from indirection with type conversion!
+	return nil
+}

engine/resources/cron.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -31,12 +31,12 @@ import (
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/recwatch"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	sdbus "github.com/coreos/go-systemd/dbus"
 	"github.com/coreos/go-systemd/unit"
 	systemdUtil "github.com/coreos/go-systemd/util"
 	"github.com/godbus/dbus"
-	errwrap "github.com/pkg/errors"
 )
 
 const (
@@ -310,28 +310,29 @@ func (obj *CronRes) Watch() error {
 // CheckApply is run to check the state and, if apply is true, to apply the
 // necessary changes to reach the desired state. This is run before Watch and
 // again if Watch finds a change occurring to the state.
-func (obj *CronRes) CheckApply(apply bool) (checkOK bool, err error) {
-	ok := true
+func (obj *CronRes) CheckApply(apply bool) (bool, error) {
+	checkOK := true
 	// use the embedded file resource to apply the correct state
 	if c, err := obj.file.CheckApply(apply); err != nil {
 		return false, errwrap.Wrapf(err, "nested file failed")
 	} else if !c {
-		ok = false
+		checkOK = false
 	}
 	// check timer state and apply the defined state if needed
 	if c, err := obj.unitCheckApply(apply); err != nil {
 		return false, errwrap.Wrapf(err, "unitCheckApply error")
 	} else if !c {
-		ok = false
+		checkOK = false
 	}
-	return ok, nil
+	return checkOK, nil
 }
 
 // unitCheckApply checks the state of the systemd-timer unit and, if apply is
 // true, applies the defined state.
-func (obj *CronRes) unitCheckApply(apply bool) (checkOK bool, err error) {
+func (obj *CronRes) unitCheckApply(apply bool) (bool, error) {
 	var conn *sdbus.Conn
 	var godbusConn *dbus.Conn
+	var err error
 
 	// this resource depends on systemd to ensure that it's running
 	if !systemdUtil.IsRunningSystemd() {

engine/resources/docker_container.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -30,13 +30,13 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/client"
 	"github.com/docker/go-connections/nat"
-	errwrap "github.com/pkg/errors"
 )
 
 const (
@@ -201,7 +201,7 @@ func (obj *DockerContainerRes) Watch() error {
 }
 
 // CheckApply method for Docker resource.
-func (obj *DockerContainerRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *DockerContainerRes) CheckApply(apply bool) (bool, error) {
 	var id string
 	var destroy bool
 

engine/resources/docker_container_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/exec.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -27,13 +27,12 @@ import (
 	"strings"
 	"sync"
 	"syscall"
+	"time"
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
-	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -44,26 +43,60 @@ func init() {
 type ExecRes struct {
 	traits.Base // add the base methods without re-implementation
 	traits.Edgeable
+	traits.Sendable
 
 	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
-	Output     *string // all cmd output, read only, do not set!
-	Stdout     *string // the cmd stdout, read only, do not set!
-	Stderr     *string // the cmd stderr, read only, do not set!
+	// Cmd is the command to run. If this is not specified, we use the name.
+	Cmd string `yaml:"cmd"`
+	// Args is a list of args to pass to Cmd. This can be used *instead* of
+	// passing the full command and args as a single string to Cmd. It can
+	// only be used when a Shell is *not* specified. The advantage of this
+	// is that you don't have to worry about escape characters.
+	Args []string `yaml:"args"`
+	// Cmd is the dir to run the command in. If empty, then this will use
+	// the working directory of the calling process. (This process is mgmt,
+	// not the process being run here.)
+	Cwd string `yaml:"cwd"`
+	// Shell is the (optional) shell to use to run the cmd. If you specify
+	// this, then you can't use the Args parameter.
+	Shell string `yaml:"shell"`
+	// Timeout is the number of seconds to wait before sending a Kill to the
+	// running command. If the Kill is received before the process exits,
+	// then this be treated as an error.
+	Timeout uint64 `yaml:"timeout"`
 
-	wg *sync.WaitGroup
+	// Watch is the command to run to detect event changes. Each line of
+	// output from this command is treated as an event.
+	WatchCmd string `yaml:"watchcmd"`
+	// WatchCwd is the Cwd for the WatchCmd. See the docs for Cwd.
+	WatchCwd string `yaml:"watchcwd"`
+	// WatchShell is the Shell for the WatchCmd. See the docs for Shell.
+	WatchShell string `yaml:"watchshell"`
+
+	// IfCmd is the command that runs to guard against running the Cmd. If
+	// this command succeeds, then Cmd *will* be run. If this command
+	// returns a non-zero result, then the Cmd will not be run. Any error
+	// scenario or timeout will cause the resource to error.
+	IfCmd string `yaml:"ifcmd"`
+	// IfCwd is the Cwd for the IfCmd. See the docs for Cwd.
+	IfCwd string `yaml:"ifcwd"`
+	// IfShell is the Shell for the IfCmd. See the docs for Shell.
+	IfShell string `yaml:"ifshell"`
+
+	// User is the (optional) user to use to execute the command. It is used
+	// for any command being run.
+	User string `yaml:"user"`
+	// Group is the (optional) group to use to execute the command. It is
+	// used for any command being run.
+	Group string `yaml:"group"`
+
+	output *string // all cmd output, read only, do not set!
+	stdout *string // the cmd stdout, read only, do not set!
+	stderr *string // the cmd stderr, read only, do not set!
+
+	interruptChan chan struct{}
+	wg            *sync.WaitGroup
 }
 
 // Default returns some sensible defaults for this resource.
@@ -71,10 +104,27 @@ func (obj *ExecRes) Default() engine.Res {
 	return &ExecRes{}
 }
 
+// getCmd returns the actual command to run. When Cmd is not specified, we use
+// the Name.
+func (obj *ExecRes) getCmd() string {
+	if obj.Cmd != "" {
+		return obj.Cmd
+	}
+	return obj.Name()
+}
+
 // Validate if the params passed in are valid data.
 func (obj *ExecRes) Validate() error {
-	if obj.Cmd == "" { // this is the only thing that is really required
-		return fmt.Errorf("command can't be empty")
+	if obj.getCmd() == "" { // this is the only thing that is really required
+		return fmt.Errorf("the Cmd can't be empty")
+	}
+
+	split := strings.Fields(obj.getCmd())
+	if len(obj.Args) > 0 && obj.Shell != "" {
+		return fmt.Errorf("the Args param can't be used with a Shell")
+	}
+	if len(obj.Args) > 0 && len(split) > 1 {
+		return fmt.Errorf("the Args param can't be used when Cmd has args")
 	}
 
 	// check that, if an user or a group is set, we're running as root
@@ -84,7 +134,7 @@ func (obj *ExecRes) Validate() error {
 			return errwrap.Wrapf(err, "error looking up current user")
 		}
 		if currentUser.Uid != "0" {
-			return errwrap.Errorf("running as root is required if you want to use exec with a different user/group")
+			return fmt.Errorf("running as root is required if you want to use exec with a different user/group")
 		}
 	}
 
@@ -95,6 +145,7 @@ func (obj *ExecRes) Validate() error {
 func (obj *ExecRes) Init(init *engine.Init) error {
 	obj.init = init // save for later
 
+	obj.interruptChan = make(chan struct{})
 	obj.wg = &sync.WaitGroup{}
 
 	return nil
@@ -107,7 +158,7 @@ func (obj *ExecRes) Close() error {
 
 // Watch is the primary listener for this resource and it outputs events.
 func (obj *ExecRes) Watch() error {
-	ioChan := make(chan *bufioOutput)
+	ioChan := make(chan *cmdOutput)
 	defer obj.wg.Wait()
 
 	if obj.WatchCmd != "" {
@@ -125,7 +176,10 @@ func (obj *ExecRes) Watch() error {
 			cmdName = obj.WatchShell // usually bash, or sh
 			cmdArgs = []string{"-c", obj.WatchCmd}
 		}
-		cmd := exec.Command(cmdName, cmdArgs...)
+
+		ctx, cancel := context.WithCancel(context.Background())
+		defer cancel()
+		cmd := exec.CommandContext(ctx, cmdName, cmdArgs...)
 		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{
@@ -139,25 +193,9 @@ func (obj *ExecRes) Watch() error {
 			return errwrap.Wrapf(err, "error while setting credential")
 		}
 
-		cmdReader, err := cmd.StdoutPipe()
-		if err != nil {
-			return errwrap.Wrapf(err, "error creating StdoutPipe for Cmd")
+		if ioChan, err = obj.cmdOutputRunner(ctx, cmd); err != nil {
+			return errwrap.Wrapf(err, "error starting WatchCmd")
 		}
-		scanner := bufio.NewScanner(cmdReader)
-
-		defer cmd.Wait() // wait for the command to exit before return!
-		defer func() {
-			// FIXME: without wrapping this in this func it panic's
-			// when running certain graphs... why?
-			cmd.Process.Kill() // shutdown the Watch command on exit
-		}()
-		if err := cmd.Start(); err != nil {
-			return errwrap.Wrapf(err, "error starting Cmd")
-		}
-
-		ctx, cancel := context.WithCancel(context.Background())
-		defer cancel() // unblock and cleanup
-		ioChan = obj.bufioChanScanner(ctx, scanner)
 	}
 
 	obj.init.Running() // when started, notify engine that we're running
@@ -172,12 +210,32 @@ func (obj *ExecRes) Watch() error {
 				return fmt.Errorf("reached EOF")
 			}
 			if err := data.err; err != nil {
-				// error reading input?
-				return errwrap.Wrapf(err, "unknown error")
+				// error reading input or cmd failure
+				exitErr, ok := err.(*exec.ExitError) // embeds an os.ProcessState
+				if !ok {
+					// command failed in some bad way
+					return errwrap.Wrapf(err, "unknown error")
+				}
+				pStateSys := exitErr.Sys() // (*os.ProcessState) Sys
+				wStatus, ok := pStateSys.(syscall.WaitStatus)
+				if !ok {
+					return errwrap.Wrapf(err, "error running cmd")
+				}
+				exitStatus := wStatus.ExitStatus()
+				obj.init.Logf("watchcmd exited with: %d", exitStatus)
+				if exitStatus != 0 {
+					return errwrap.Wrapf(err, "unexpected exit status of zero")
+				}
+				return err // i'm not sure if this could happen
 			}
 
 			// each time we get a line of output, we loop!
-			obj.init.Logf("watch output: %s", data.text)
+			if s := data.text; s == "" {
+				obj.init.Logf("watch output is empty!")
+			} else {
+				obj.init.Logf("watch output is:")
+				obj.init.Logf(s)
+			}
 			if data.text != "" {
 				send = true
 			}
@@ -231,11 +289,42 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 			return false, errwrap.Wrapf(err, "error while setting credential")
 		}
 
+		var out splitWriter
+		out.Init()
+		cmd.Stdout = out.Stdout
+		cmd.Stderr = out.Stderr
+
 		if err := cmd.Run(); err != nil {
-			// TODO: check exit value
+			exitErr, ok := err.(*exec.ExitError) // embeds an os.ProcessState
+			if !ok {
+				// command failed in some bad way
+				return false, err
+			}
+			pStateSys := exitErr.Sys() // (*os.ProcessState) Sys
+			wStatus, ok := pStateSys.(syscall.WaitStatus)
+			if !ok {
+				return false, errwrap.Wrapf(err, "error running cmd")
+			}
+			exitStatus := wStatus.ExitStatus()
+			if exitStatus == 0 {
+				return false, fmt.Errorf("unexpected exit status of zero")
+			}
+
+			obj.init.Logf("ifcmd exited with: %d", exitStatus)
+			if s := out.String(); s == "" {
+				obj.init.Logf("ifcmd output is empty!")
+			} else {
+				obj.init.Logf("ifcmd output is:")
+				obj.init.Logf(s)
+			}
 			return true, nil // don't run
 		}
-
+		if s := out.String(); s == "" {
+			obj.init.Logf("ifcmd output is empty!")
+		} else {
+			obj.init.Logf("ifcmd output is:")
+			obj.init.Logf(s)
+		}
 	}
 
 	// state is not okay, no work done, exit, but without error
@@ -251,16 +340,33 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 		// call without a shell
 		// FIXME: are there still whitespace splitting issues?
 		// TODO: we could make the split character user selectable...!
-		split := strings.Fields(obj.Cmd)
+		split := strings.Fields(obj.getCmd())
 		cmdName = split[0]
 		//d, _ := os.Getwd() // TODO: how does this ever error ?
 		//cmdName = path.Join(d, cmdName)
 		cmdArgs = split[1:]
+		if len(obj.Args) > 0 {
+			if len(split) != 1 { // should not happen
+				return false, fmt.Errorf("validation error")
+			}
+			cmdArgs = obj.Args
+		}
 	} else {
 		cmdName = obj.Shell // usually bash, or sh
-		cmdArgs = []string{"-c", obj.Cmd}
+		cmdArgs = []string{"-c", obj.getCmd()}
 	}
-	cmd := exec.Command(cmdName, cmdArgs...)
+
+	wg := &sync.WaitGroup{}
+	defer wg.Wait() // this must be above the defer cancel() call
+	var ctx context.Context
+	var cancel context.CancelFunc
+	if obj.Timeout > 0 { // cmd.Process.Kill() is called on timeout
+		ctx, cancel = context.WithTimeout(context.Background(), time.Duration(obj.Timeout)*time.Second)
+	} else { // zero timeout means no timer
+		ctx, cancel = context.WithCancel(context.Background())
+	}
+	defer cancel()
+	cmd := exec.CommandContext(ctx, cmdName, cmdArgs...)
 	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{
@@ -285,35 +391,32 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 		return false, errwrap.Wrapf(err, "error starting cmd")
 	}
 
-	timeout := obj.Timeout
-	if timeout == 0 { // zero timeout means no timer, so disable it
-		timeout = -1
-	}
-	done := make(chan error)
-	go func() { done <- cmd.Wait() }()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		select {
+		case <-obj.interruptChan:
+			cancel()
+		case <-ctx.Done():
+			// let this exit
+		}
+	}()
 
-	select {
-	case e := <-done:
-		err = e // store
-
-	case <-util.TimeAfterOrBlock(timeout):
-		cmd.Process.Kill() // TODO: check error?
-		return false, fmt.Errorf("timeout for cmd")
-	}
+	err = cmd.Wait() // we can unblock this with the timeout
 
 	// save in memory for send/recv
 	// we use pointers to strings to indicate if used or not
 	if out.Stdout.Activity || out.Stderr.Activity {
 		str := out.String()
-		obj.Output = &str
+		obj.output = &str
 	}
 	if out.Stdout.Activity {
 		str := out.Stdout.String()
-		obj.Stdout = &str
+		obj.stdout = &str
 	}
 	if out.Stderr.Activity {
 		str := out.Stderr.String()
-		obj.Stderr = &str
+		obj.stderr = &str
 	}
 
 	// process the err result from cmd, we process non-zero exits here too!
@@ -324,7 +427,18 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 		if !ok {
 			return false, errwrap.Wrapf(err, "error running cmd")
 		}
-		return false, fmt.Errorf("cmd error, exit status: %d", wStatus.ExitStatus())
+		exitStatus := wStatus.ExitStatus()
+		if !wStatus.Signaled() { // not a timeout or cancel (no signal)
+			return false, errwrap.Wrapf(err, "cmd error, exit status: %d", exitStatus)
+		}
+		sig := wStatus.Signal()
+
+		// we get this on timeout, because ctx calls cmd.Process.Kill()
+		if sig == syscall.SIGKILL {
+			return false, errwrap.Wrapf(err, "cmd timeout, exit status: %d", exitStatus)
+		}
+
+		return false, fmt.Errorf("unknown cmd error, signal: %s, exit status: %d", sig, exitStatus)
 
 	} else if err != nil {
 		return false, errwrap.Wrapf(err, "general cmd error")
@@ -334,11 +448,18 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 	// would be nice, but it would require terminal log output that doesn't
 	// interleave all the parallel parts which would mix it all up...
 	if s := out.String(); s == "" {
-		obj.init.Logf("Command output is empty!")
-
+		obj.init.Logf("command output is empty!")
 	} else {
-		obj.init.Logf("Command output is:")
-		obj.init.Logf(out.String())
+		obj.init.Logf("command output is:")
+		obj.init.Logf(s)
+	}
+
+	if err := obj.init.Send(&ExecSends{
+		Output: obj.output,
+		Stdout: obj.stdout,
+		Stderr: obj.stderr,
+	}); err != nil {
+		return false, err
 	}
 
 	// The state tracking is for exec resources that can't "detect" their
@@ -351,58 +472,67 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *ExecRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *ExecRes) Compare(r engine.Res) bool {
 	// we can only compare ExecRes to others of the same resource kind
 	res, ok := r.(*ExecRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.Cmd != res.Cmd {
-		return false
+		return fmt.Errorf("the Cmd differs")
+	}
+	if len(obj.Args) != len(res.Args) {
+		return fmt.Errorf("the Args differ")
+	}
+	for i, a := range obj.Args {
+		if a != res.Args[i] {
+			return fmt.Errorf("the Args differ at index: %d", i)
+		}
 	}
 	if obj.Cwd != res.Cwd {
-		return false
+		return fmt.Errorf("the Cwd differs")
 	}
 	if obj.Shell != res.Shell {
-		return false
+		return fmt.Errorf("the Shell differs")
 	}
 	if obj.Timeout != res.Timeout {
-		return false
-	}
-	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
-	}
-	if obj.User != res.User {
-		return false
-	}
-	if obj.Group != res.Group {
-		return false
+		return fmt.Errorf("the Timeout differs")
 	}
 
-	return true
+	if obj.WatchCmd != res.WatchCmd {
+		return fmt.Errorf("the WatchCmd differs")
+	}
+	if obj.WatchCwd != res.WatchCwd {
+		return fmt.Errorf("the WatchCwd differs")
+	}
+	if obj.WatchShell != res.WatchShell {
+		return fmt.Errorf("the WatchShell differs")
+	}
+
+	if obj.IfCmd != res.IfCmd {
+		return fmt.Errorf("the IfCmd differs")
+	}
+	if obj.IfCwd != res.IfCwd {
+		return fmt.Errorf("the IfCwd differs")
+	}
+	if obj.IfShell != res.IfShell {
+		return fmt.Errorf("the IfShell differs")
+	}
+
+	if obj.User != res.User {
+		return fmt.Errorf("the User differs")
+	}
+	if obj.Group != res.Group {
+		return fmt.Errorf("the Group differs")
+	}
+
+	return nil
+}
+
+// Interrupt is called to ask the execution of this resource to end early.
+func (obj *ExecRes) Interrupt() error {
+	close(obj.interruptChan)
+	return nil
 }
 
 // ExecUID is the UID struct for ExecRes.
@@ -453,13 +583,32 @@ func (obj *ExecRes) AutoEdges() (engine.AutoEdge, error) {
 func (obj *ExecRes) UIDs() []engine.ResUID {
 	x := &ExecUID{
 		BaseUID: engine.BaseUID{Name: obj.Name(), Kind: obj.Kind()},
-		Cmd:     obj.Cmd,
+		Cmd:     obj.getCmd(),
 		IfCmd:   obj.IfCmd,
 		// TODO: add more params here
 	}
 	return []engine.ResUID{x}
 }
 
+// ExecSends is the struct of data which is sent after a successful Apply.
+type ExecSends struct {
+	// Output is the combined stdout and stderr of the command.
+	Output *string `lang:"output"`
+	// Stdout is the stdout of the command.
+	Stdout *string `lang:"stdout"`
+	// Stderr is the stderr of the command.
+	Stderr *string `lang:"stderr"`
+}
+
+// Sends represents the default struct of values we can send using Send/Recv.
+func (obj *ExecRes) Sends() interface{} {
+	return &ExecSends{
+		Output: nil,
+		Stdout: nil,
+		Stderr: nil,
+	}
+}
+
 // UnmarshalYAML is the custom unmarshal handler for this struct.
 // It is primarily useful for setting the defaults.
 func (obj *ExecRes) UnmarshalYAML(unmarshal func(interface{}) error) error {
@@ -516,7 +665,7 @@ func (obj *ExecRes) cmdFiles() []string {
 	var paths []string
 	if obj.Shell != "" {
 		paths = append(paths, obj.Shell)
-	} else if cmdSplit := strings.Fields(obj.Cmd); len(cmdSplit) > 0 {
+	} else if cmdSplit := strings.Fields(obj.getCmd()); len(cmdSplit) > 0 {
 		paths = append(paths, cmdSplit[0])
 	}
 	if obj.WatchShell != "" {
@@ -532,36 +681,56 @@ func (obj *ExecRes) cmdFiles() []string {
 	return paths
 }
 
-// bufioOutput is the output struct of the bufioChanScanner channel output.
-type bufioOutput struct {
+// cmdOutput is the output struct of the cmdOutputRunner channel output. You
+// should always check the error first. If it's nil, then you can assume the
+// text data is good to use.
+type cmdOutput struct {
 	text string
 	err  error
 }
 
-// bufioChanScanner wraps the scanner output in a channel.
-func (obj *ExecRes) bufioChanScanner(ctx context.Context, scanner *bufio.Scanner) chan *bufioOutput {
-	ch := make(chan *bufioOutput)
+// cmdOutputRunner wraps the Cmd in with a StdoutPipe scanner and reads for
+// errors. It runs Start and Wait, and errors runtime things in the channel.
+// If it can't start up the command, it will fail early. Once it's running, it
+// will return the channel which can be used for the duration of the process.
+// Cancelling the context merely unblocks the sending on the output channel, it
+// does not Kill the cmd process. For that you must do it yourself elsewhere.
+func (obj *ExecRes) cmdOutputRunner(ctx context.Context, cmd *exec.Cmd) (chan *cmdOutput, error) {
+	cmdReader, err := cmd.StdoutPipe()
+	if err != nil {
+		return nil, errwrap.Wrapf(err, "error creating StdoutPipe for Cmd")
+	}
+	scanner := bufio.NewScanner(cmdReader)
+	if err := cmd.Start(); err != nil {
+		return nil, errwrap.Wrapf(err, "error starting Cmd")
+	}
+
+	ch := make(chan *cmdOutput)
 	obj.wg.Add(1)
 	go func() {
 		defer obj.wg.Done()
 		defer close(ch)
 		for scanner.Scan() {
 			select {
-			case ch <- &bufioOutput{text: scanner.Text()}: // blocks here ?
+			case ch <- &cmdOutput{text: scanner.Text()}: // blocks here ?
 			case <-ctx.Done():
 				return
 			}
 		}
+
 		// on EOF, scanner.Err() will be nil
-		if err := scanner.Err(); err != nil {
+		reterr := scanner.Err()
+		reterr = errwrap.Append(reterr, cmd.Wait()) // always run Wait()
+		// send any misc errors we encounter on the channel
+		if reterr != nil {
 			select {
-			case ch <- &bufioOutput{err: err}: // send any misc errors we encounter
+			case ch <- &cmdOutput{err: reterr}:
 			case <-ctx.Done():
 				return
 			}
 		}
 	}()
-	return ch
+	return ch, nil
 }
 
 // splitWriter mimics what the ssh.CombinedOutput command does, but stores the

engine/resources/exec_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -20,20 +20,34 @@
 package resources
 
 import (
+	"context"
+	"fmt"
+	"os/exec"
+	"syscall"
 	"testing"
+	"time"
 
 	"github.com/purpleidea/mgmt/engine"
 )
 
-func fakeInit(t *testing.T) *engine.Init {
+func fakeExecInit(t *testing.T) (*engine.Init, *ExecSends) {
 	debug := testing.Verbose() // set via the -test.v flag to `go test`
 	logf := func(format string, v ...interface{}) {
 		t.Logf("test: "+format, v...)
 	}
+	execSends := &ExecSends{}
 	return &engine.Init{
+		Send: func(st interface{}) error {
+			x, ok := st.(*ExecSends)
+			if !ok {
+				return fmt.Errorf("unable to send")
+			}
+			*execSends = *x // set
+			return nil
+		},
 		Debug: debug,
 		Logf:  logf,
-	}
+	}, execSends
 }
 
 func TestExecSendRecv1(t *testing.T) {
@@ -50,7 +64,8 @@ func TestExecSendRecv1(t *testing.T) {
 			t.Errorf("close failed with: %v", err)
 		}
 	}()
-	if err := r1.Init(fakeInit(t)); err != nil {
+	init, execSends := fakeExecInit(t)
+	if err := r1.Init(init); err != nil {
 		t.Errorf("init failed with: %v", err)
 	}
 	// run artificially without the entire engine
@@ -58,23 +73,23 @@ func TestExecSendRecv1(t *testing.T) {
 		t.Errorf("checkapply failed with: %v", err)
 	}
 
-	t.Logf("output is: %v", r1.Output)
-	if r1.Output != nil {
-		t.Logf("output is: %v", *r1.Output)
+	t.Logf("output is: %v", execSends.Output)
+	if execSends.Output != nil {
+		t.Logf("output is: %v", *execSends.Output)
 	}
-	t.Logf("stdout is: %v", r1.Stdout)
-	if r1.Stdout != nil {
-		t.Logf("stdout is: %v", *r1.Stdout)
+	t.Logf("stdout is: %v", execSends.Stdout)
+	if execSends.Stdout != nil {
+		t.Logf("stdout is: %v", *execSends.Stdout)
 	}
-	t.Logf("stderr is: %v", r1.Stderr)
-	if r1.Stderr != nil {
-		t.Logf("stderr is: %v", *r1.Stderr)
+	t.Logf("stderr is: %v", execSends.Stderr)
+	if execSends.Stderr != nil {
+		t.Logf("stderr is: %v", *execSends.Stderr)
 	}
 
-	if r1.Stdout == nil {
+	if execSends.Stdout == nil {
 		t.Errorf("stdout is nil")
 	} else {
-		if out := *r1.Stdout; out != "hello world\n" {
+		if out := *execSends.Stdout; out != "hello world\n" {
 			t.Errorf("got wrong stdout(%d): %s", len(out), out)
 		}
 	}
@@ -94,7 +109,8 @@ func TestExecSendRecv2(t *testing.T) {
 			t.Errorf("close failed with: %v", err)
 		}
 	}()
-	if err := r1.Init(fakeInit(t)); err != nil {
+	init, execSends := fakeExecInit(t)
+	if err := r1.Init(init); err != nil {
 		t.Errorf("init failed with: %v", err)
 	}
 	// run artificially without the entire engine
@@ -102,23 +118,23 @@ func TestExecSendRecv2(t *testing.T) {
 		t.Errorf("checkapply failed with: %v", err)
 	}
 
-	t.Logf("output is: %v", r1.Output)
-	if r1.Output != nil {
-		t.Logf("output is: %v", *r1.Output)
+	t.Logf("output is: %v", execSends.Output)
+	if execSends.Output != nil {
+		t.Logf("output is: %v", *execSends.Output)
 	}
-	t.Logf("stdout is: %v", r1.Stdout)
-	if r1.Stdout != nil {
-		t.Logf("stdout is: %v", *r1.Stdout)
+	t.Logf("stdout is: %v", execSends.Stdout)
+	if execSends.Stdout != nil {
+		t.Logf("stdout is: %v", *execSends.Stdout)
 	}
-	t.Logf("stderr is: %v", r1.Stderr)
-	if r1.Stderr != nil {
-		t.Logf("stderr is: %v", *r1.Stderr)
+	t.Logf("stderr is: %v", execSends.Stderr)
+	if execSends.Stderr != nil {
+		t.Logf("stderr is: %v", *execSends.Stderr)
 	}
 
-	if r1.Stderr == nil {
+	if execSends.Stderr == nil {
 		t.Errorf("stderr is nil")
 	} else {
-		if out := *r1.Stderr; out != "hello world\n" {
+		if out := *execSends.Stderr; out != "hello world\n" {
 			t.Errorf("got wrong stderr(%d): %s", len(out), out)
 		}
 	}
@@ -138,7 +154,8 @@ func TestExecSendRecv3(t *testing.T) {
 			t.Errorf("close failed with: %v", err)
 		}
 	}()
-	if err := r1.Init(fakeInit(t)); err != nil {
+	init, execSends := fakeExecInit(t)
+	if err := r1.Init(init); err != nil {
 		t.Errorf("init failed with: %v", err)
 	}
 	// run artificially without the entire engine
@@ -146,42 +163,97 @@ func TestExecSendRecv3(t *testing.T) {
 		t.Errorf("checkapply failed with: %v", err)
 	}
 
-	t.Logf("output is: %v", r1.Output)
-	if r1.Output != nil {
-		t.Logf("output is: %v", *r1.Output)
+	t.Logf("output is: %v", execSends.Output)
+	if execSends.Output != nil {
+		t.Logf("output is: %v", *execSends.Output)
 	}
-	t.Logf("stdout is: %v", r1.Stdout)
-	if r1.Stdout != nil {
-		t.Logf("stdout is: %v", *r1.Stdout)
+	t.Logf("stdout is: %v", execSends.Stdout)
+	if execSends.Stdout != nil {
+		t.Logf("stdout is: %v", *execSends.Stdout)
 	}
-	t.Logf("stderr is: %v", r1.Stderr)
-	if r1.Stderr != nil {
-		t.Logf("stderr is: %v", *r1.Stderr)
+	t.Logf("stderr is: %v", execSends.Stderr)
+	if execSends.Stderr != nil {
+		t.Logf("stderr is: %v", *execSends.Stderr)
 	}
 
-	if r1.Output == nil {
+	if execSends.Output == nil {
 		t.Errorf("output is nil")
 	} else {
 		// it looks like bash or golang race to the write, so whichever
 		// order they come out in is ok, as long as they come out whole
-		if out := *r1.Output; out != "hello world\ngoodbye world\n" && out != "goodbye world\nhello world\n" {
+		if out := *execSends.Output; out != "hello world\ngoodbye world\n" && out != "goodbye world\nhello world\n" {
 			t.Errorf("got wrong output(%d): %s", len(out), out)
 		}
 	}
 
-	if r1.Stdout == nil {
+	if execSends.Stdout == nil {
 		t.Errorf("stdout is nil")
 	} else {
-		if out := *r1.Stdout; out != "hello world\n" {
+		if out := *execSends.Stdout; out != "hello world\n" {
 			t.Errorf("got wrong stdout(%d): %s", len(out), out)
 		}
 	}
 
-	if r1.Stderr == nil {
+	if execSends.Stderr == nil {
 		t.Errorf("stderr is nil")
 	} else {
-		if out := *r1.Stderr; out != "goodbye world\n" {
+		if out := *execSends.Stderr; out != "goodbye world\n" {
 			t.Errorf("got wrong stderr(%d): %s", len(out), out)
 		}
 	}
 }
+
+func TestExecTimeoutBehaviour(t *testing.T) {
+	// cmd.Process.Kill() is called on timeout
+	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
+	defer cancel()
+	cmdName := "/bin/sleep"    // it's /usr/bin/sleep on modern distros
+	cmdArgs := []string{"300"} // 5 min in seconds
+	cmd := exec.CommandContext(ctx, cmdName, cmdArgs...)
+	// ignore signals sent to parent process (we're in our own group)
+	cmd.SysProcAttr = &syscall.SysProcAttr{
+		Setpgid: true,
+		Pgid:    0,
+	}
+
+	if err := cmd.Start(); err != nil {
+		t.Errorf("error starting cmd: %+v", err)
+		return
+	}
+
+	err := cmd.Wait() // we can unblock this with the timeout
+
+	if err == nil {
+		t.Errorf("expected error, got nil")
+		return
+	}
+
+	exitErr, ok := err.(*exec.ExitError) // embeds an os.ProcessState
+	if err != nil && ok {
+		pStateSys := exitErr.Sys() // (*os.ProcessState) Sys
+		wStatus, ok := pStateSys.(syscall.WaitStatus)
+		if !ok {
+			t.Errorf("error running cmd")
+			return
+		}
+		if !wStatus.Signaled() {
+			t.Errorf("did not get signal, exit status: %d", wStatus.ExitStatus())
+			return
+		}
+
+		// we get this on timeout, because ctx calls cmd.Process.Kill()
+		if sig := wStatus.Signal(); sig != syscall.SIGKILL {
+			t.Errorf("got wrong signal: %+v, exit status: %d", sig, wStatus.ExitStatus())
+			return
+		}
+
+		t.Logf("exit status: %d", wStatus.ExitStatus())
+		return
+
+	} else if err != nil {
+		t.Errorf("general cmd error")
+		return
+	}
+
+	// no error
+}

engine/resources/file.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -36,8 +36,7 @@ import (
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/recwatch"
 	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -54,23 +53,30 @@ type FileRes struct {
 
 	init *engine.Init
 
-	// Path variable, which usually defaults to the name, represents the
+	// Path, which defaults to the name if not specified, represents the
 	// destination path for the file or directory being managed. It must be
 	// an absolute path, and as a result must start with a slash.
-	Path     string  `yaml:"path"`
-	Dirname  string  `yaml:"dirname"`  // override the path dirname
-	Basename string  `yaml:"basename"` // override the path basename
-	Content  *string `yaml:"content"`  // nil to mark as undefined
-	Source   string  `yaml:"source"`   // file path for source content
-	State    string  `yaml:"state"`    // state: exists/present?, absent, (undefined?)
-	Owner    string  `yaml:"owner"`
-	Group    string  `yaml:"group"`
-	Mode     string  `yaml:"mode"`
-	Recurse  bool    `yaml:"recurse"`
-	Force    bool    `yaml:"force"`
+	Path     string `lang:"path" yaml:"path"`
+	Dirname  string `lang:"dirname" yaml:"dirname"`   // override the path dirname
+	Basename string `lang:"basename" yaml:"basename"` // override the path basename
+
+	// Content specifies the file contents to use. If this is nil, they are
+	// left undefined. It cannot be combined with Source.
+	Content *string `lang:"content" yaml:"content"`
+	// Source specifies the source contents for the file resource. It cannot
+	// be combined with the Content parameter.
+	Source string `lang:"source" yaml:"source"`
+	// State specifies the desired state of the file. It can be either
+	// `exists` or `absent`. If you do not specify this, it will be
+	// undefined, and determined based on the other parameters.
+	State string `lang:"state" yaml:"state"`
+
+	Owner   string `lang:"owner" yaml:"owner"`
+	Group   string `lang:"group" yaml:"group"`
+	Mode    string `lang:"mode" yaml:"mode"`
+	Recurse bool   `lang:"recurse" yaml:"recurse"`
+	Force   bool   `lang:"force" yaml:"force"`
 
-	path       string // computed path
-	isDir      bool   // computed isDir
 	sha256sum  string
 	recWatcher *recwatch.RecWatcher
 }
@@ -84,7 +90,7 @@ func (obj *FileRes) Default() engine.Res {
 
 // Validate reports any problems with the struct definition.
 func (obj *FileRes) Validate() error {
-	if obj.GetPath() == "" {
+	if obj.getPath() == "" {
 		return fmt.Errorf("path is empty")
 	}
 
@@ -96,7 +102,7 @@ func (obj *FileRes) Validate() error {
 		return fmt.Errorf("basename must not start with a slash")
 	}
 
-	if !strings.HasPrefix(obj.GetPath(), "/") {
+	if !strings.HasPrefix(obj.getPath(), "/") {
 		return fmt.Errorf("resultant path must be absolute")
 	}
 
@@ -104,7 +110,7 @@ func (obj *FileRes) Validate() error {
 		return fmt.Errorf("can't specify both Content and Source")
 	}
 
-	if obj.isDir && obj.Content != nil { // makes no sense
+	if obj.isDir() && obj.Content != nil { // makes no sense
 		return fmt.Errorf("can't specify Content when creating a Dir")
 	}
 
@@ -114,6 +120,16 @@ func (obj *FileRes) Validate() error {
 		}
 	}
 
+	if obj.Owner != "" || obj.Group != "" {
+		fileInfo, err := os.Stat("/") // pick root just to do this test
+		if err != nil {
+			return fmt.Errorf("can't stat root to get system information")
+		}
+		_, ok := fileInfo.Sys().(*syscall.Stat_t)
+		if !ok {
+			return fmt.Errorf("can't set Owner or Group on this platform")
+		}
+	}
 	if _, err := engineUtil.GetUID(obj.Owner); obj.Owner != "" && err != nil {
 		return err
 	}
@@ -123,7 +139,7 @@ func (obj *FileRes) Validate() error {
 	}
 
 	// XXX: should this specify that we create an empty directory instead?
-	//if obj.Source == "" && obj.isDir {
+	//if obj.Source == "" && obj.isDir() {
 	//	return fmt.Errorf("Can't specify an empty source when creating a Dir.")
 	//}
 
@@ -136,7 +152,7 @@ func (obj *FileRes) Validate() error {
 func (obj *FileRes) mode() (os.FileMode, error) {
 	m, err := strconv.ParseInt(obj.Mode, 8, 32)
 	if err != nil {
-		return os.FileMode(0), errwrap.Wrapf(err, "Mode should be an octal number (%s)", obj.Mode)
+		return os.FileMode(0), errwrap.Wrapf(err, "mode should be an octal number (%s)", obj.Mode)
 	}
 	return os.FileMode(m), nil
 }
@@ -146,8 +162,6 @@ func (obj *FileRes) Init(init *engine.Init) error {
 	obj.init = init // save for later
 
 	obj.sha256sum = ""
-	obj.path = obj.GetPath()                     // compute once
-	obj.isDir = strings.HasSuffix(obj.path, "/") // dirs have trailing slashes
 
 	return nil
 }
@@ -157,9 +171,10 @@ func (obj *FileRes) Close() error {
 	return nil
 }
 
-// GetPath returns the actual path to use for this resource. It computes this
+// getPath returns the actual path to use for this resource. It computes this
 // after analysis of the Path, Dirname and Basename values. Dirs end with slash.
-func (obj *FileRes) GetPath() string {
+// TODO: memoize the result if this seems important.
+func (obj *FileRes) getPath() string {
 	p := obj.Path
 	if obj.Path == "" { // use the name as the path default if missing
 		p = obj.Name()
@@ -180,6 +195,11 @@ func (obj *FileRes) GetPath() string {
 	return obj.Dirname + obj.Basename
 }
 
+// isDir is a helper function to specify whether the path should be a dir.
+func (obj *FileRes) isDir() bool {
+	return strings.HasSuffix(obj.getPath(), "/") // dirs have trailing slashes
+}
+
 // Watch is the primary listener for this resource and it outputs events.
 // This one is a file watcher for files and directories.
 // Modify with caution, it is probably important to write some test cases first!
@@ -188,7 +208,7 @@ func (obj *FileRes) GetPath() string {
 // FIXME: Also watch the source directory when using obj.Source !!!
 func (obj *FileRes) Watch() error {
 	var err error
-	obj.recWatcher, err = recwatch.NewRecWatcher(obj.path, obj.Recurse)
+	obj.recWatcher, err = recwatch.NewRecWatcher(obj.getPath(), obj.Recurse)
 	if err != nil {
 		return err
 	}
@@ -199,7 +219,7 @@ func (obj *FileRes) Watch() error {
 	var send = false // send event?
 	for {
 		if obj.init.Debug {
-			obj.init.Logf("Watching: %s", obj.path) // attempting to watch...
+			obj.init.Logf("watching: %s", obj.getPath()) // attempting to watch...
 		}
 
 		select {
@@ -211,7 +231,7 @@ func (obj *FileRes) Watch() error {
 				return errwrap.Wrapf(err, "unknown %s watcher error", obj)
 			}
 			if obj.init.Debug { // don't access event.Body if event.Error isn't nil
-				obj.init.Logf("Event(%s): %v", event.Body.Name, event.Body.Op)
+				obj.init.Logf("event(%s): %v", event.Body.Name, event.Body.Op)
 			}
 			send = true
 
@@ -288,7 +308,7 @@ func (obj *FileRes) fileCheckApply(apply bool, src io.ReadSeeker, dst string, sh
 		}
 		// FIXME: respect obj.Recurse here...
 		// there is a dir here, where we want a file...
-		obj.init.Logf("fileCheckApply: Removing (force): %s", cleanDst)
+		obj.init.Logf("fileCheckApply: removing (force): %s", cleanDst)
 		if err := os.RemoveAll(cleanDst); err != nil { // dangerous ;)
 			return "", false, err
 		}
@@ -334,7 +354,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: %v -> %s", src, dst)
+		obj.init.Logf("fileCheckApply: apply: %v -> %s", src, dst)
 	}
 
 	dstClose() // unlock file usage so we can write to it
@@ -355,12 +375,12 @@ 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: %v -> %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
 	} else if obj.init.Debug {
-		obj.init.Logf("fileCheckApply: Copied: %v", n)
+		obj.init.Logf("fileCheckApply: copied: %v", n)
 	}
 	return sha256sum, false, dstFile.Sync()
 }
@@ -368,16 +388,16 @@ func (obj *FileRes) fileCheckApply(apply bool, src io.ReadSeeker, dst string, sh
 // dirCheckApply is the CheckApply operation for an empty directory.
 func (obj *FileRes) dirCheckApply(apply bool) (bool, error) {
 	// check if the path exists and is a directory
-	st, err := os.Stat(obj.path)
+	fileInfo, err := os.Stat(obj.getPath())
 	if err != nil && !os.IsNotExist(err) {
 		return false, errwrap.Wrapf(err, "error checking file resource existence")
 	}
 
-	if err == nil && st.IsDir() {
+	if err == nil && fileInfo.IsDir() {
 		return true, nil // already a directory, nothing to do
 	}
-	if err == nil && !st.IsDir() && !obj.Force {
-		return false, fmt.Errorf("can't force file into dir: %s", obj.path)
+	if err == nil && !fileInfo.IsDir() && !obj.Force {
+		return false, fmt.Errorf("can't force file into dir: %s", obj.getPath())
 	}
 
 	if !apply {
@@ -386,9 +406,9 @@ func (obj *FileRes) dirCheckApply(apply bool) (bool, error) {
 
 	// the path exists and is not a directory
 	// delete the file if force is given
-	if err == nil && !st.IsDir() {
-		obj.init.Logf("dirCheckApply: Removing (force): %s", obj.path)
-		if err := os.Remove(obj.path); err != nil {
+	if err == nil && !fileInfo.IsDir() {
+		obj.init.Logf("dirCheckApply: removing (force): %s", obj.getPath())
+		if err := os.Remove(obj.getPath()); err != nil {
 			return false, err
 		}
 	}
@@ -407,10 +427,10 @@ func (obj *FileRes) dirCheckApply(apply bool) (bool, error) {
 	if obj.Force {
 		// FIXME: respect obj.Recurse here...
 		// TODO: add recurse limit here
-		return false, os.MkdirAll(obj.path, mode)
+		return false, os.MkdirAll(obj.getPath(), mode)
 	}
 
-	return false, os.Mkdir(obj.path, mode)
+	return false, os.Mkdir(obj.getPath(), mode)
 }
 
 // syncCheckApply is the CheckApply operation for a source and destination dir.
@@ -424,7 +444,7 @@ func (obj *FileRes) syncCheckApply(apply bool, src, dst string) (bool, error) {
 		return false, fmt.Errorf("the src and dst must not be empty")
 	}
 
-	var checkOK = true
+	checkOK := true
 	// TODO: handle ./ cases or ../ cases that need cleaning ?
 
 	srcIsDir := strings.HasSuffix(src, "/")
@@ -441,7 +461,7 @@ func (obj *FileRes) syncCheckApply(apply bool, src, dst string) (bool, error) {
 		fin, err := os.Open(src)
 		if err != nil {
 			if obj.init.Debug && os.IsNotExist(err) { // if we get passed an empty src
-				obj.init.Logf("syncCheckApply: Missing src: %s", src)
+				obj.init.Logf("syncCheckApply: missing src: %s", src)
 			}
 			return false, err
 		}
@@ -489,7 +509,7 @@ func (obj *FileRes) syncCheckApply(apply bool, src, dst string) (bool, error) {
 					if absCleanDst == "" || absCleanDst == "/" {
 						return false, fmt.Errorf("don't want to remove root") // safety
 					}
-					obj.init.Logf("syncCheckApply: Removing (force): %s", absCleanDst)
+					obj.init.Logf("syncCheckApply: removing (force): %s", absCleanDst)
 					if err := os.Remove(absCleanDst); err != nil {
 						return false, err
 					}
@@ -508,11 +528,11 @@ func (obj *FileRes) syncCheckApply(apply bool, src, dst string) (bool, error) {
 		}
 
 		if obj.init.Debug {
-			obj.init.Logf("syncCheckApply: Recurse: %s -> %s", absSrc, absDst)
+			obj.init.Logf("syncCheckApply: recurse: %s -> %s", absSrc, absDst)
 		}
 		if obj.Recurse {
 			if c, err := obj.syncCheckApply(apply, absSrc, absDst); err != nil { // recurse
-				return false, errwrap.Wrapf(err, "syncCheckApply: Recurse failed")
+				return false, errwrap.Wrapf(err, "syncCheckApply: recurse failed")
 			} else if !c { // don't let subsequent passes make this true
 				checkOK = false
 			}
@@ -541,7 +561,7 @@ func (obj *FileRes) syncCheckApply(apply bool, src, dst string) (bool, error) {
 		// think the symmetry is more elegant and correct here for now
 		// Avoiding this is also useful if we had a recurse limit arg!
 		if true { // switch
-			obj.init.Logf("syncCheckApply: Removing: %s", absCleanDst)
+			obj.init.Logf("syncCheckApply: removing: %s", absCleanDst)
 			if apply {
 				if err := os.RemoveAll(absCleanDst); err != nil { // dangerous ;)
 					return false, err
@@ -569,12 +589,58 @@ func (obj *FileRes) syncCheckApply(apply bool, src, dst string) (bool, error) {
 	return checkOK, nil
 }
 
+// state performs a CheckApply of the file state to create an empty file.
+func (obj *FileRes) stateCheckApply(apply bool) (bool, error) {
+	if obj.State == "" { // state is not specified
+		return true, nil
+	}
+
+	_, err := os.Stat(obj.getPath())
+
+	if err != nil && !os.IsNotExist(err) {
+		return false, errwrap.Wrapf(err, "could not stat file")
+	}
+
+	if obj.State == "absent" && os.IsNotExist(err) {
+		return true, nil
+	}
+
+	if obj.State == "exists" && err == nil {
+		return true, nil
+	}
+
+	// state is not okay, no work done, exit, but without error
+	if !apply {
+		return false, nil
+	}
+
+	if obj.State == "absent" {
+		return false, nil // defer the work to contentCheckApply
+	}
+
+	if obj.Content == nil && !obj.isDir() {
+		// Create an empty file to ensure one exists. Don't O_TRUNC it,
+		// in case one is magically created right after our exists test.
+		// The chmod used is what is used by the os.Create function.
+		// TODO: is using O_EXCL okay?
+		f, err := os.OpenFile(obj.getPath(), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0666)
+		if err != nil {
+			return false, errwrap.Wrapf(err, "problem creating empty file")
+		}
+		if err := f.Close(); err != nil {
+			return false, errwrap.Wrapf(err, "problem closing empty file")
+		}
+	}
+
+	return false, nil // defer the Content != nil and isDir work to later...
+}
+
 // contentCheckApply performs a CheckApply for the file existence and content.
-func (obj *FileRes) contentCheckApply(apply bool) (checkOK bool, _ error) {
+func (obj *FileRes) contentCheckApply(apply bool) (bool, error) {
 	obj.init.Logf("contentCheckApply(%t)", apply)
 
 	if obj.State == "absent" {
-		if _, err := os.Stat(obj.path); os.IsNotExist(err) {
+		if _, err := os.Stat(obj.getPath()); os.IsNotExist(err) {
 			// no such file or directory, but
 			// file should be missing, phew :)
 			return true, nil
@@ -589,17 +655,17 @@ func (obj *FileRes) contentCheckApply(apply bool) (checkOK bool, _ error) {
 		}
 
 		// apply portion
-		if obj.path == "" || obj.path == "/" {
+		if obj.getPath() == "" || obj.getPath() == "/" {
 			return false, fmt.Errorf("don't want to remove root") // safety
 		}
-		obj.init.Logf("contentCheckApply: Removing: %s", obj.path)
+		obj.init.Logf("contentCheckApply: removing: %s", obj.getPath())
 		// FIXME: respect obj.Recurse here...
 		// TODO: add recurse limit here
-		err := os.RemoveAll(obj.path) // dangerous ;)
-		return false, err             // either nil or not
+		err := os.RemoveAll(obj.getPath()) // dangerous ;)
+		return false, err                  // either nil or not
 	}
 
-	if obj.isDir && obj.Source == "" {
+	if obj.isDir() && obj.Source == "" {
 		return obj.dirCheckApply(apply)
 	}
 
@@ -610,7 +676,7 @@ func (obj *FileRes) contentCheckApply(apply bool) (checkOK bool, _ error) {
 
 	if obj.Source == "" { // do the obj.Content checks first...
 		bufferSrc := bytes.NewReader([]byte(*obj.Content))
-		sha256sum, checkOK, err := obj.fileCheckApply(apply, bufferSrc, obj.path, obj.sha256sum)
+		sha256sum, checkOK, err := obj.fileCheckApply(apply, bufferSrc, obj.getPath(), obj.sha256sum)
 		if sha256sum != "" { // empty values mean errored or didn't hash
 			// this can be valid even when the whole function errors
 			obj.sha256sum = sha256sum // cache value
@@ -622,7 +688,7 @@ func (obj *FileRes) contentCheckApply(apply bool) (checkOK bool, _ error) {
 		return checkOK, nil // success
 	}
 
-	checkOK, err := obj.syncCheckApply(apply, obj.Source, obj.path)
+	checkOK, err := obj.syncCheckApply(apply, obj.Source, obj.getPath())
 	if err != nil {
 		obj.init.Logf("syncCheckApply: Error: %v", err)
 		return false, err
@@ -632,16 +698,16 @@ func (obj *FileRes) contentCheckApply(apply bool) (checkOK bool, _ error) {
 }
 
 // chmodCheckApply performs a CheckApply for the file permissions.
-func (obj *FileRes) chmodCheckApply(apply bool) (checkOK bool, _ error) {
+func (obj *FileRes) chmodCheckApply(apply bool) (bool, error) {
 	obj.init.Logf("chmodCheckApply(%t)", apply)
 
 	if obj.State == "absent" {
-		// File is absent
+		// file is absent
 		return true, nil
 	}
 
 	if obj.Mode == "" {
-		// No mode specified, everything is ok
+		// no mode specified, everything is ok
 		return true, nil
 	}
 
@@ -657,36 +723,36 @@ func (obj *FileRes) chmodCheckApply(apply bool) (checkOK bool, _ error) {
 		return false, err
 	}
 
-	st, err := os.Stat(obj.path)
+	fileInfo, err := os.Stat(obj.getPath())
 	if err != nil {
 		return false, err
 	}
 
-	// Nothing to do
-	if st.Mode() == mode {
+	// nothing to do
+	if fileInfo.Mode() == mode {
 		return true, nil
 	}
 
-	// Not clean but don't apply
+	// not clean but don't apply
 	if !apply {
 		return false, nil
 	}
 
-	err = os.Chmod(obj.path, mode)
+	err = os.Chmod(obj.getPath(), mode)
 	return false, err
 }
 
 // chownCheckApply performs a CheckApply for the file ownership.
-func (obj *FileRes) chownCheckApply(apply bool) (checkOK bool, _ error) {
+func (obj *FileRes) chownCheckApply(apply bool) (bool, error) {
 	var expectedUID, expectedGID int
 	obj.init.Logf("chownCheckApply(%t)", apply)
 
 	if obj.State == "absent" {
-		// File is absent or no owner specified
+		// file is absent or no owner specified
 		return true, nil
 	}
 
-	st, err := os.Stat(obj.path)
+	fileInfo, err := os.Stat(obj.getPath())
 
 	// If the file does not exist and we are in
 	// noop mode, do not throw an error.
@@ -698,10 +764,10 @@ func (obj *FileRes) chownCheckApply(apply bool) (checkOK bool, _ error) {
 		return false, err
 	}
 
-	stUnix, ok := st.Sys().(*syscall.Stat_t)
-	if !ok {
-		// Not unix
-		panic("No support for your platform")
+	stUnix, ok := fileInfo.Sys().(*syscall.Stat_t)
+	if !ok { // this check is done in Validate, but it's done here again...
+		// not unix
+		return false, fmt.Errorf("can't set Owner or Group on this platform")
 	}
 
 	if obj.Owner != "" {
@@ -710,7 +776,7 @@ func (obj *FileRes) chownCheckApply(apply bool) (checkOK bool, _ error) {
 			return false, err
 		}
 	} else {
-		// Nothing specified, no changes to be made, expect same as actual
+		// nothing specified, no changes to be made, expect same as actual
 		expectedUID = int(stUnix.Uid)
 	}
 
@@ -720,27 +786,26 @@ func (obj *FileRes) chownCheckApply(apply bool) (checkOK bool, _ error) {
 			return false, err
 		}
 	} else {
-		// Nothing specified, no changes to be made, expect same as actual
+		// nothing specified, no changes to be made, expect same as actual
 		expectedGID = int(stUnix.Gid)
 	}
 
-	// Nothing to do
+	// nothing to do
 	if int(stUnix.Uid) == expectedUID && int(stUnix.Gid) == expectedGID {
 		return true, nil
 	}
 
-	// Not clean, but don't apply
+	// not clean, but don't apply
 	if !apply {
 		return false, nil
 	}
 
-	err = os.Chown(obj.path, expectedUID, expectedGID)
-	return false, err
+	return false, os.Chown(obj.getPath(), expectedUID, expectedGID)
 }
 
 // CheckApply checks the resource state and applies the resource if the bool
 // input is true. It returns error info and if the state check passed or not.
-func (obj *FileRes) CheckApply(apply bool) (checkOK bool, _ error) {
+func (obj *FileRes) CheckApply(apply bool) (bool, error) {
 	// NOTE: all send/recv change notifications *must* be processed before
 	// there is a possibility of failure in CheckApply. This is because if
 	// we fail (and possibly run again) the subsequent send->recv transfer
@@ -749,12 +814,18 @@ func (obj *FileRes) CheckApply(apply bool) (checkOK bool, _ error) {
 	// promptly, if they must not be lost, such as for cache invalidation.
 	if val, exists := obj.init.Recv()["Content"]; exists && val.Changed {
 		// if we received on Content, and it changed, invalidate the cache!
-		obj.init.Logf("contentCheckApply: Invalidating sha256sum of `Content`")
+		obj.init.Logf("contentCheckApply: invalidating sha256sum of `Content`")
 		obj.sha256sum = "" // invalidate!!
 	}
 
-	checkOK = true
+	checkOK := true
 
+	// always run stateCheckApply before contentCheckApply, they go together
+	if c, err := obj.stateCheckApply(apply); err != nil {
+		return false, err
+	} else if !c {
+		checkOK = false
+	}
 	if c, err := obj.contentCheckApply(apply); err != nil {
 		return false, err
 	} else if !c {
@@ -778,45 +849,52 @@ func (obj *FileRes) CheckApply(apply bool) (checkOK bool, _ error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *FileRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *FileRes) Compare(r engine.Res) bool {
 	// we can only compare FileRes to others of the same resource kind
 	res, ok := r.(*FileRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
-	if obj.path != res.path {
-		return false
+	// We don't need to compare Path, Dirname or Basename-- we only care if
+	// the resultant path is different or not.
+	if obj.getPath() != res.getPath() {
+		return fmt.Errorf("the Path differs")
 	}
 	if (obj.Content == nil) != (res.Content == nil) { // xor
-		return false
+		return fmt.Errorf("the Content differs")
 	}
 	if obj.Content != nil && res.Content != nil {
 		if *obj.Content != *res.Content { // compare the strings
-			return false
+			return fmt.Errorf("the contents of Content differ")
 		}
 	}
 	if obj.Source != res.Source {
-		return false
+		return fmt.Errorf("the Source differs")
 	}
 	if obj.State != res.State {
-		return false
-	}
-	if obj.Recurse != res.Recurse {
-		return false
-	}
-	if obj.Force != res.Force {
-		return false
+		return fmt.Errorf("the State differs")
 	}
 
-	return true
+	if obj.Owner != res.Owner {
+		return fmt.Errorf("the Owner differs")
+	}
+	if obj.Group != res.Group {
+		return fmt.Errorf("the Group differs")
+	}
+	// TODO: when we start to allow alternate representations for the mode,
+	// ensure that we compare in the same format. Eg: `ug=rw` == `0660`.
+	if obj.Mode != res.Mode {
+		return fmt.Errorf("the Mode differs")
+	}
+
+	if obj.Recurse != res.Recurse {
+		return fmt.Errorf("the Recurse option differs")
+	}
+	if obj.Force != res.Force {
+		return fmt.Errorf("the Force option differs")
+	}
+
+	return nil
 }
 
 // FileUID is the UID struct for FileRes.
@@ -877,8 +955,8 @@ func (obj *FileResAutoEdges) Test(input []bool) bool {
 // the bottom up!
 func (obj *FileRes) AutoEdges() (engine.AutoEdge, error) {
 	var data []engine.ResUID // store linear result chain here...
-	// build it, but don't use obj.path because this gets called before Init
-	values := util.PathSplitFullReversed(obj.GetPath())
+	// don't use any memoization run in Init (this gets called before Init)
+	values := util.PathSplitFullReversed(obj.getPath())
 	_, values = values[0], values[1:] // get rid of first value which is me!
 	for _, x := range values {
 		var reversed = true // cheat by passing a pointer
@@ -903,7 +981,7 @@ func (obj *FileRes) AutoEdges() (engine.AutoEdge, error) {
 func (obj *FileRes) UIDs() []engine.ResUID {
 	x := &FileUID{
 		BaseUID: engine.BaseUID{Name: obj.Name(), Kind: obj.Kind()},
-		path:    obj.GetPath(), // not obj.path b/c we didn't init yet!
+		path:    obj.getPath(),
 	}
 	return []engine.ResUID{x}
 }

engine/resources/file_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -119,6 +119,12 @@ func TestMiscEncodeDecode2(t *testing.T) {
 		t.Errorf("Can't create: %v", err)
 		return
 	}
+	// NOTE: Do not add this bit of code, because it would cause the path to
+	// get taken from the actual Path parameter, instead of using the name,
+	// and if we use the name, the Cmp function will detect if the name is
+	// stored properly or not.
+	//fileRes := input.(*FileRes) // must not panic
+	//fileRes.Path = "/tmp/whatever"
 
 	b64, err := engineUtil.ResToB64(input)
 	if err != nil {
@@ -142,11 +148,53 @@ func TestMiscEncodeDecode2(t *testing.T) {
 		t.Errorf("Output %v is not a Res", res2)
 		return
 	}
+	// this uses the standalone file cmp function
 	if err := res1.Cmp(res2); err != nil {
 		t.Errorf("The input and output Res values do not match: %+v", err)
 	}
 }
 
+func TestMiscEncodeDecode3(t *testing.T) {
+	var err error
+
+	// encode
+	input, err := engine.NewNamedResource("file", "file1")
+	if err != nil {
+		t.Errorf("Can't create: %v", err)
+		return
+	}
+	fileRes := input.(*FileRes) // must not panic
+	fileRes.Path = "/tmp/whatever"
+	// TODO: add other params/traits/etc here!
+
+	b64, err := engineUtil.ResToB64(input)
+	if err != nil {
+		t.Errorf("Can't encode: %v", err)
+		return
+	}
+
+	output, err := engineUtil.B64ToRes(b64)
+	if err != nil {
+		t.Errorf("Can't decode: %v", err)
+		return
+	}
+
+	res1, ok := input.(engine.Res)
+	if !ok {
+		t.Errorf("Input %v is not a Res", res1)
+		return
+	}
+	res2, ok := output.(engine.Res)
+	if !ok {
+		t.Errorf("Output %v is not a Res", res2)
+		return
+	}
+	// this uses the more complete, engine cmp function
+	if err := engine.ResCmp(res1, res2); err != nil {
+		t.Errorf("The input and output Res values do not match: %+v", err)
+	}
+}
+
 func TestFileAbsolute1(t *testing.T) {
 	// file resource paths should be absolute
 	f1 := &FileRes{

engine/resources/group.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -28,8 +28,7 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -119,7 +118,7 @@ func (obj *GroupRes) Watch() error {
 }
 
 // CheckApply method for Group resource.
-func (obj *GroupRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *GroupRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply(%t)", apply)
 
 	// check if the group exists

engine/resources/hostname.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -25,9 +25,9 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/godbus/dbus"
-	errwrap "github.com/pkg/errors"
 )
 
 func init() {
@@ -110,7 +110,7 @@ func (obj *HostnameRes) Watch() error {
 	// if we share the bus with others, we will get each others messages!!
 	bus, err := util.SystemBusPrivateUsable() // don't share the bus connection!
 	if err != nil {
-		return errwrap.Wrap(err, "Failed to connect to bus")
+		return errwrap.Wrapf(err, "failed to connect to bus")
 	}
 	defer bus.Close()
 	// watch the PropertiesChanged signal on the hostname1 dbus path
@@ -120,7 +120,7 @@ func (obj *HostnameRes) Watch() error {
 		dbusPropertiesIface,
 	)
 	if call := bus.BusObject().Call(engineUtil.DBusAddMatch, 0, args); call.Err != nil {
-		return errwrap.Wrap(call.Err, "Failed to subscribe to DBus events for hostname1")
+		return errwrap.Wrapf(call.Err, "failed to subscribe to DBus events for hostname1")
 	}
 	defer bus.BusObject().Call(engineUtil.DBusRemoveMatch, 0, args) // ignore the error
 
@@ -147,18 +147,18 @@ func (obj *HostnameRes) Watch() error {
 	}
 }
 
-func (obj *HostnameRes) updateHostnameProperty(object dbus.BusObject, expectedValue, property, setterName string, apply bool) (checkOK bool, err error) {
+func (obj *HostnameRes) updateHostnameProperty(object dbus.BusObject, expectedValue, property, setterName string, apply bool) (bool, error) {
 	propertyObject, err := object.GetProperty("org.freedesktop.hostname1." + property)
 	if err != nil {
 		return false, errwrap.Wrapf(err, "failed to get org.freedesktop.hostname1.%s", property)
 	}
 	if propertyObject.Value() == nil {
-		return false, errwrap.Errorf("Unexpected nil value received when reading property %s", property)
+		return false, fmt.Errorf("unexpected nil value received when reading property %s", property)
 	}
 
 	propertyValue, ok := propertyObject.Value().(string)
 	if !ok {
-		return false, fmt.Errorf("Received unexpected type as %s value, expected string got '%T'", property, propertyValue)
+		return false, fmt.Errorf("received unexpected type as %s value, expected string got '%T'", property, propertyValue)
 	}
 
 	// expected value and actual value match => checkOk
@@ -182,16 +182,16 @@ func (obj *HostnameRes) updateHostnameProperty(object dbus.BusObject, expectedVa
 }
 
 // CheckApply method for Hostname resource.
-func (obj *HostnameRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *HostnameRes) CheckApply(apply bool) (bool, error) {
 	conn, err := util.SystemBusPrivateUsable()
 	if err != nil {
-		return false, errwrap.Wrap(err, "Failed to connect to the private system bus")
+		return false, errwrap.Wrapf(err, "failed to connect to the private system bus")
 	}
 	defer conn.Close()
 
 	hostnameObject := conn.Object(hostname1Iface, hostname1Path)
 
-	checkOK = true
+	checkOK := true
 	if obj.PrettyHostname != "" {
 		propertyCheckOK, err := obj.updateHostnameProperty(hostnameObject, obj.PrettyHostname, "PrettyHostname", "SetPrettyHostname", apply)
 		if err != nil {

engine/resources/kv.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -18,13 +18,16 @@
 package resources
 
 import (
+	"context"
 	"fmt"
 	"strconv"
+	"sync"
+	"time"
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -40,6 +43,10 @@ const (
 	SkipCmpStyleString
 )
 
+const (
+	kvCheckApplyTimeout = 5 * time.Second
+)
+
 // KVRes is a resource which writes a key/value pair into cluster wide storage.
 // It will ensure that the key is set to the requested value. The one exception
 // is that if you use the SkipLessThan parameter, then it will only replace the
@@ -56,14 +63,32 @@ type KVRes struct {
 
 	init *engine.Init
 
-	// XXX: shouldn't the name be the key?
-	Key          string            `yaml:"key"`          // key to set
-	Value        *string           `yaml:"value"`        // value to set (nil to delete)
-	SkipLessThan bool              `yaml:"skiplessthan"` // skip updates as long as stored value is greater
-	SkipCmpStyle KVResSkipCmpStyle `yaml:"skipcmpstyle"` // how to do the less than cmp
+	// Key represents the key to set. If it is not specified, the Name value
+	// is used instead.
+	Key string `lang:"key" yaml:"key"`
+	// Value represents the string value to set. If this value is nil or,
+	// undefined, then this will delete that key.
+	Value *string `lang:"value" yaml:"value"`
+	// SkipLessThan causes the value to be updated as long as it is greater.
+	SkipLessThan bool `lang:"skiplessthan" yaml:"skiplessthan"`
+	// SkipCmpStyle is the type of compare function used when determining if
+	// the value is greater when using the SkipLessThan parameter.
+	SkipCmpStyle KVResSkipCmpStyle `lang:"skipcmpstyle" yaml:"skipcmpstyle"`
+
+	interruptChan chan struct{}
+
 	// TODO: does it make sense to have different backends here? (eg: local)
 }
 
+// getKey returns the key to be used for this resource. If the Key field is
+// specified, it will use that, otherwise it uses the Name.
+func (obj *KVRes) getKey() string {
+	if obj.Key != "" {
+		return obj.Key
+	}
+	return obj.Name()
+}
+
 // Default returns some sensible defaults for this resource.
 func (obj *KVRes) Default() engine.Res {
 	return &KVRes{}
@@ -71,7 +96,7 @@ func (obj *KVRes) Default() engine.Res {
 
 // Validate if the params passed in are valid data.
 func (obj *KVRes) Validate() error {
-	if obj.Key == "" {
+	if obj.getKey() == "" {
 		return fmt.Errorf("key must not be empty")
 	}
 	if obj.SkipLessThan {
@@ -92,6 +117,8 @@ func (obj *KVRes) Validate() error {
 func (obj *KVRes) Init(init *engine.Init) error {
 	obj.init = init // save for later
 
+	obj.interruptChan = make(chan struct{})
+
 	return nil
 }
 
@@ -102,9 +129,17 @@ func (obj *KVRes) Close() error {
 
 // Watch is the primary listener for this resource and it outputs events.
 func (obj *KVRes) Watch() error {
-	obj.init.Running() // when started, notify engine that we're running
+	// FIXME: add timeout to context
+	// The obj.init.Done channel is closed by the engine to signal shutdown.
+	ctx, cancel := util.ContextWithCloser(context.Background(), obj.init.Done)
+	defer cancel()
 
-	ch := obj.init.World.StrMapWatch(obj.Key) // get possible events!
+	ch, err := obj.init.World.StrMapWatch(ctx, obj.getKey()) // get possible events!
+	if err != nil {
+		return err
+	}
+
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -118,7 +153,7 @@ func (obj *KVRes) Watch() error {
 				return errwrap.Wrapf(err, "unknown %s watcher error", obj)
 			}
 			if obj.init.Debug {
-				obj.init.Logf("Event!")
+				obj.init.Logf("event!")
 			}
 			send = true
 
@@ -135,7 +170,7 @@ func (obj *KVRes) Watch() error {
 }
 
 // lessThanCheck checks for less than validity.
-func (obj *KVRes) lessThanCheck(value string) (checkOK bool, err error) {
+func (obj *KVRes) lessThanCheck(value string) (bool, error) {
 	v := *obj.Value
 	if value == v { // redundant check for safety
 		return true, nil
@@ -173,16 +208,31 @@ func (obj *KVRes) lessThanCheck(value string) (checkOK bool, err error) {
 }
 
 // CheckApply method for Password resource. Does nothing, returns happy!
-func (obj *KVRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *KVRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply(%t)", apply)
 
+	wg := &sync.WaitGroup{}
+	defer wg.Wait() // this must be above the defer cancel() call
+	ctx, cancel := context.WithTimeout(context.Background(), kvCheckApplyTimeout)
+	defer cancel()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		select {
+		case <-obj.interruptChan:
+			cancel()
+		case <-ctx.Done():
+			// let this exit
+		}
+	}()
+
 	if val, exists := obj.init.Recv()["Value"]; exists && val.Changed {
 		// if we received on Value, and it changed, wooo, nothing to do.
 		obj.init.Logf("CheckApply: `Value` was updated!")
 	}
 
 	hostname := obj.init.Hostname // me
-	keyMap, err := obj.init.World.StrMapGet(obj.Key)
+	keyMap, err := obj.init.World.StrMapGet(ctx, obj.getKey())
 	if err != nil {
 		return false, errwrap.Wrapf(err, "check error during StrGet")
 	}
@@ -202,7 +252,7 @@ func (obj *KVRes) CheckApply(apply bool) (checkOK bool, err error) {
 		return true, nil // nothing to delete, we're good!
 
 	} else if ok && obj.Value == nil { // delete
-		err := obj.init.World.StrMapDel(obj.Key)
+		err := obj.init.World.StrMapDel(ctx, obj.getKey())
 		return false, errwrap.Wrapf(err, "apply error during StrDel")
 	}
 
@@ -210,7 +260,7 @@ func (obj *KVRes) CheckApply(apply bool) (checkOK bool, err error) {
 		return false, nil
 	}
 
-	if err := obj.init.World.StrMapSet(obj.Key, *obj.Value); err != nil {
+	if err := obj.init.World.StrMapSet(ctx, obj.getKey(), *obj.Value); err != nil {
 		return false, errwrap.Wrapf(err, "apply error during StrSet")
 	}
 
@@ -219,39 +269,37 @@ func (obj *KVRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *KVRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *KVRes) Compare(r engine.Res) bool {
 	// we can only compare KVRes to others of the same resource kind
 	res, ok := r.(*KVRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
-	if obj.Key != res.Key {
-		return false
+	if obj.getKey() != res.getKey() {
+		return fmt.Errorf("the Key differs")
 	}
 	if (obj.Value == nil) != (res.Value == nil) { // xor
-		return false
+		return fmt.Errorf("the Value differs")
 	}
 	if obj.Value != nil && res.Value != nil {
 		if *obj.Value != *res.Value { // compare the strings
-			return false
+			return fmt.Errorf("the contents of Value differs")
 		}
 	}
 	if obj.SkipLessThan != res.SkipLessThan {
-		return false
+		return fmt.Errorf("the SkipLessThan param differs")
 	}
 	if obj.SkipCmpStyle != res.SkipCmpStyle {
-		return false
+		return fmt.Errorf("the SkipCmpStyle param differs")
 	}
 
-	return true
+	return nil
+}
+
+// Interrupt is called to ask the execution of this resource to end early.
+func (obj *KVRes) Interrupt() error {
+	close(obj.interruptChan)
+	return nil
 }
 
 // KVUID is the UID struct for KVRes.

engine/resources/mount.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -33,13 +33,13 @@ import (
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/recwatch"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	sdbus "github.com/coreos/go-systemd/dbus"
 	"github.com/coreos/go-systemd/unit"
 	systemdUtil "github.com/coreos/go-systemd/util"
 	fstab "github.com/deniswernert/go-fstab"
 	"github.com/godbus/dbus"
-	errwrap "github.com/pkg/errors"
 	"golang.org/x/sys/unix"
 )
 
@@ -275,7 +275,7 @@ func (obj *MountRes) Watch() error {
 
 // fstabCheckApply checks /etc/fstab for entries corresponding to the resource
 // definition, and adds or deletes the entry as needed.
-func (obj *MountRes) fstabCheckApply(apply bool) (checkOK bool, err error) {
+func (obj *MountRes) fstabCheckApply(apply bool) (bool, error) {
 	exists, err := fstabEntryExists(fstabPath, obj.mount)
 	if err != nil {
 		return false, errwrap.Wrapf(err, "error checking if fstab entry exists")
@@ -339,8 +339,8 @@ func (obj *MountRes) mountCheckApply(apply bool) (bool, error) {
 // CheckApply is run to check the state and, if apply is true, to apply the
 // necessary changes to reach the desired state. This is run before Watch and
 // again if Watch finds a change occurring to the state.
-func (obj *MountRes) CheckApply(apply bool) (checkOK bool, err error) {
-	checkOK = true
+func (obj *MountRes) CheckApply(apply bool) (bool, error) {
+	checkOK := true
 
 	if c, err := obj.fstabCheckApply(apply); err != nil {
 		return false, err
@@ -584,7 +584,7 @@ func mountReload() error {
 	}
 
 	// systemctl restart remote-fs.target
-	if err := restartUnit(conn, "local-fs.target"); err != nil {
+	if err := restartUnit(conn, "remote-fs.target"); err != nil {
 		return errwrap.Wrapf(err, "error restarting unit")
 	}
 

engine/resources/mount_linux_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/mount_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/msg.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/msg_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/net.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -34,10 +34,9 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 	"github.com/purpleidea/mgmt/util/socketset"
 
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
 	// XXX: Do NOT use subscribe methods from this lib, as they are racey and
 	// do not clean up spawned goroutines. Should be replaced when a suitable
 	// alternative is available.
@@ -179,9 +178,7 @@ func (obj *NetRes) Close() error {
 		return fmt.Errorf("socket file should not be the root path")
 	}
 	if obj.socketFile != "" { // safety
-		if err := os.Remove(obj.socketFile); err != nil {
-			errList = multierr.Append(errList, err)
-		}
+		errList = errwrap.Append(errList, os.Remove(obj.socketFile))
 	}
 
 	return errList
@@ -463,8 +460,8 @@ func (obj *NetRes) fileCheckApply(apply bool) (bool, error) {
 // CheckApply is run to check the state and, if apply is true, to apply the
 // necessary changes to reach the desired state. This is run before Watch and
 // again if Watch finds a change occurring to the state.
-func (obj *NetRes) CheckApply(apply bool) (checkOK bool, err error) {
-	checkOK = true
+func (obj *NetRes) CheckApply(apply bool) (bool, error) {
+	checkOK := true
 
 	// check the network device
 	if c, err := obj.ifaceCheckApply(apply); err != nil {

engine/resources/net_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/noop.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -75,7 +75,7 @@ func (obj *NoopRes) Watch() error {
 }
 
 // CheckApply method for Noop resource. Does nothing, returns happy!
-func (obj *NoopRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *NoopRes) CheckApply(apply bool) (bool, error) {
 	if obj.init.Refresh() {
 		obj.init.Logf("received a notification!")
 	}

engine/resources/noop_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/nspawn.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -27,12 +27,12 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	systemdDbus "github.com/coreos/go-systemd/dbus"
 	machined "github.com/coreos/go-systemd/machine1"
 	systemdUtil "github.com/coreos/go-systemd/util"
 	"github.com/godbus/dbus"
-	errwrap "github.com/pkg/errors"
 )
 
 const (
@@ -201,7 +201,7 @@ func (obj *NspawnRes) Watch() error {
 // CheckApply is run to check the state and, if apply is true, to apply the
 // necessary changes to reach the desired state. This is run before Watch and
 // again if Watch finds a change occurring to the state.
-func (obj *NspawnRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *NspawnRes) CheckApply(apply bool) (bool, error) {
 	// this resource depends on systemd to ensure that it's running
 	if !systemdUtil.IsRunningSystemd() {
 		return false, errors.New("systemd is not running")

engine/resources/packagekit/packagekit.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -27,10 +27,9 @@ import (
 
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/godbus/dbus"
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
 )
 
 // global tweaks of verbosity and code path
@@ -198,9 +197,8 @@ func (obj *Conn) matchSignal(ch chan *dbus.Signal, path dbus.ObjectPath, iface s
 	removeSignals := func() error {
 		var errList error
 		for i := len(argsList) - 1; i >= 0; i-- { // last in first out
-			if call := bus.Call(engineUtil.DBusRemoveMatch, 0, argsList[i]); call.Err != nil {
-				errList = multierr.Append(errList, call.Err)
-			}
+			call := bus.Call(engineUtil.DBusRemoveMatch, 0, argsList[i])
+			errList = errwrap.Append(errList, call.Err)
 		}
 		return errList
 	}

engine/resources/password.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -29,8 +29,7 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -210,7 +209,7 @@ func (obj *PasswordRes) Watch() error {
 }
 
 // CheckApply method for Password resource. Does nothing, returns happy!
-func (obj *PasswordRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *PasswordRes) CheckApply(apply bool) (bool, error) {
 	var refresh = obj.init.Refresh() // do we have a pending reload to apply?
 	var exists = true                // does the file (aka the token) exist?
 	var generate bool                // do we need to generate a new password?
@@ -344,7 +343,7 @@ func (obj *PasswordRes) UIDs() []engine.ResUID {
 // PasswordSends is the struct of data which is sent after a successful Apply.
 type PasswordSends struct {
 	// Password is the generated password being sent.
-	Password *string
+	Password *string `lang:"password"`
 	// Hashing is the algorithm used for this password. Empty is plain text.
 	Hashing string // TODO: implement me
 }

engine/resources/pkg.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -26,8 +26,7 @@ import (
 	"github.com/purpleidea/mgmt/engine/resources/packagekit"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -263,7 +262,7 @@ func (obj *PkgRes) populateFileList() error {
 
 // CheckApply checks the resource state and applies the resource if the bool
 // input is true. It returns error info and if the state check passed or not.
-func (obj *PkgRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *PkgRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("Check: %s", obj.fmtNames(obj.getNames()))
 
 	bus := packagekit.NewBus()

engine/resources/pkg_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/print.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -40,6 +40,10 @@ type PrintRes struct {
 	init *engine.Init
 
 	Msg string `lang:"msg" yaml:"msg"` // the message to display
+	// RefreshOnly is an option that causes the message to be printed only
+	// when notified by another resource. When set to true, this resource
+	// cannot be autogrouped.
+	RefreshOnly bool `lang:"refresh_only" yaml:"refresh_only"`
 }
 
 // Default returns some sensible defaults for this resource.
@@ -78,24 +82,32 @@ func (obj *PrintRes) Watch() error {
 }
 
 // CheckApply method for Print resource. Does nothing, returns happy!
-func (obj *PrintRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *PrintRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply: %t", apply)
 	if val, exists := obj.init.Recv()["Msg"]; exists && val.Changed {
 		// if we received on Msg, and it changed, log message
 		obj.init.Logf("CheckApply: Received `Msg` of: %s", obj.Msg)
 	}
 
-	if obj.init.Refresh() {
+	var refresh = obj.init.Refresh()
+	// we output if not RefreshOnly, or we are in refresh mode and RefreshOnly
+	var display = refresh || !obj.RefreshOnly
+
+	if refresh {
 		obj.init.Logf("Received a notification!")
 	}
-	obj.init.Logf("Msg: %s", obj.Msg)
+	if display {
+		obj.init.Logf("Msg: %s", obj.Msg)
+	}
 	if g := obj.GetGroup(); len(g) > 0 { // add any grouped elements
 		for _, x := range g {
 			print, ok := x.(*PrintRes) // convert from Res
 			if !ok {
 				panic(fmt.Sprintf("grouped member %v is not a %s", x, obj.Kind()))
 			}
-			obj.init.Logf("%s: Msg: %s", print, print.Msg)
+			if display {
+				obj.init.Logf("%s: Msg: %s", print, print.Msg)
+			}
 		}
 	}
 	return true, nil // state is always okay
@@ -141,10 +153,14 @@ func (obj *PrintRes) UIDs() []engine.ResUID {
 
 // GroupCmp returns whether two resources can be grouped together or not.
 func (obj *PrintRes) GroupCmp(r engine.GroupableRes) error {
-	_, ok := r.(*PrintRes)
+	res, ok := r.(*PrintRes)
 	if !ok {
 		return fmt.Errorf("resource is not the same kind")
 	}
+	// we don't group if it's RefreshOnly: only the notifier may trigger
+	if obj.RefreshOnly || res.RefreshOnly {
+		return fmt.Errorf("resource uses RefreshOnly, it cannot be merged")
+	}
 	return nil // grouped together if we were asked to
 }
 

engine/resources/resources_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -161,6 +161,7 @@ func TestResources1(t *testing.T) {
 		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
+		startup   func() error // function to run as startup
 		cleanup   func() error // function to run as cleanup
 	}
 
@@ -204,13 +205,13 @@ func TestResources1(t *testing.T) {
 
 	testCases := []test{}
 	{
-		res := makeRes("file", "r1")
-		r := res.(*FileRes) // if this panics, the test will panic
+		r := makeRes("file", "r1")
+		res := r.(*FileRes) // if this panics, the test will panic
 		p := "/tmp/whatever"
 		s := "hello, world\n"
-		r.Path = p
+		res.Path = p
 		contents := s
-		r.Content = &contents
+		res.Content = &contents
 
 		timeline := []Step{
 			NewStartupStep(1000 * 60),          // startup
@@ -224,11 +225,94 @@ func TestResources1(t *testing.T) {
 		}
 
 		testCases = append(testCases, test{
-			name:     "simple res",
+			name:     "simple file",
 			res:      res,
 			fail:     false,
 			timeline: timeline,
 			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return os.Remove(p) },
+		})
+	}
+	{
+		r := makeRes("exec", "x1")
+		res := r.(*ExecRes) // if this panics, the test will panic
+		s := "hello, world"
+		f := "/tmp/whatever"
+		res.Cmd = fmt.Sprintf("echo '%s' > '%s'", s, f)
+		res.Shell = "/bin/bash"
+		res.IfCmd = "! diff <(cat /tmp/whatever) <(echo hello, world)"
+		res.IfShell = "/bin/bash"
+		res.WatchCmd = fmt.Sprintf("/usr/bin/inotifywait -e modify -m %s", f)
+		//res.WatchShell = "/bin/bash"
+
+		timeline := []Step{
+			NewStartupStep(1000 * 60),        // startup
+			NewChangedStep(1000*60, false),   // did we do something?
+			fileExpect(f, s+"\n"),            // check initial state
+			NewClearChangedStep(1000 * 15),   // did we do something?
+			fileWrite(f, "this is stuff!\n"), // change state
+			NewChangedStep(1000*60, false),   // did we do something?
+			fileExpect(f, s+"\n"),            // check again
+			sleep(1),                         // we can sleep too!
+		}
+
+		testCases = append(testCases, test{
+			name:     "simple exec",
+			res:      res,
+			fail:     false,
+			timeline: timeline,
+			expect:   func() error { return nil },
+			// build file for inotifywait
+			startup: func() error { return ioutil.WriteFile(f, []byte("starting...\n"), 0666) },
+			cleanup: func() error { return os.Remove(f) },
+		})
+	}
+	{
+		r := makeRes("file", "r1")
+		res := r.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/emptyfile"
+		res.Path = p
+		res.State = "exists"
+
+		timeline := []Step{
+			NewStartupStep(1000 * 60),      // startup
+			NewChangedStep(1000*60, false), // did we do something?
+			fileExpect(p, ""),              // check initial state
+			NewClearChangedStep(1000 * 15), // did we do something?
+		}
+
+		testCases = append(testCases, test{
+			name:     "touch file",
+			res:      res,
+			fail:     false,
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return os.Remove(p) },
+		})
+	}
+	{
+		r := makeRes("file", "r1")
+		res := r.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/existingfile"
+		res.Path = p
+		res.State = "exists"
+		content := "some existing text\n"
+
+		timeline := []Step{
+			NewStartupStep(1000 * 60),     // startup
+			NewChangedStep(1000*60, true), // did we do something?
+			fileExpect(p, content),        // check initial state
+		}
+
+		testCases = append(testCases, test{
+			name:     "existing file",
+			res:      res,
+			fail:     false,
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return ioutil.WriteFile(p, []byte(content), 0666) },
 			cleanup:  func() error { return os.Remove(p) },
 		})
 	}
@@ -245,7 +329,7 @@ func TestResources1(t *testing.T) {
 		}
 		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
+			res, fail, experr, experrstr, timeline, expect, startup, cleanup := tc.res, tc.fail, tc.experr, tc.experrstr, tc.timeline, tc.expect, tc.startup, tc.cleanup
 
 			t.Logf("\n\ntest #%d: Res: %+v\n", index, res)
 			defer t.Logf("test #%d: done!", index)
@@ -312,11 +396,19 @@ func TestResources1(t *testing.T) {
 				Logf:  logf,
 
 				// unused
+				Send: func(st interface{}) error {
+					return nil
+				},
 				Recv: func() map[string]*engine.Send {
 					return map[string]*engine.Send{}
 				},
 			}
 
+			t.Logf("test #%d: running startup()", index)
+			if err := startup(); err != nil {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: could not startup: %+v", index, err)
+			}
 			// run init
 			t.Logf("test #%d: running Init", index)
 			err = res.Init(init)
@@ -435,6 +527,7 @@ func TestResources1(t *testing.T) {
 					t.Errorf("test #%d: CheckApply failed: %s", index, err.Error())
 					return
 				}
+				//t.Logf("test #%d: CheckApply(true) (%t, %+v)", index, checkOK, err)
 				select {
 				// send a msg if we can, but never block
 				case changedChan <- checkOK:

engine/resources/svc.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -28,11 +28,11 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	systemd "github.com/coreos/go-systemd/dbus" // change namespace
 	systemdUtil "github.com/coreos/go-systemd/util"
 	"github.com/godbus/dbus" // namespace collides with systemd wrapper
-	errwrap "github.com/pkg/errors"
 )
 
 func init() {
@@ -229,12 +229,13 @@ func (obj *SvcRes) Watch() error {
 
 // CheckApply checks the resource state and applies the resource if the bool
 // input is true. It returns error info and if the state check passed or not.
-func (obj *SvcRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *SvcRes) CheckApply(apply bool) (bool, error) {
 	if !systemdUtil.IsRunningSystemd() {
 		return false, fmt.Errorf("systemd is not running")
 	}
 
 	var conn *systemd.Conn
+	var err error
 	if obj.Session {
 		conn, err = systemd.NewUserConnection() // user session
 	} else {

engine/resources/test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -137,7 +137,7 @@ func (obj *TestRes) Watch() error {
 }
 
 // CheckApply method for Test resource. Does nothing, returns happy!
-func (obj *TestRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *TestRes) CheckApply(apply bool) (bool, error) {
 	for key, val := range obj.init.Recv() {
 		obj.init.Logf("CheckApply: Received `%s`, changed: %t", key, val.Changed)
 	}
@@ -401,8 +401,8 @@ func (obj *TestRes) GroupCmp(r engine.GroupableRes) error {
 // TestSends is the struct of data which is sent after a successful Apply.
 type TestSends struct {
 	// Hello is some value being sent.
-	Hello  *string
-	Answer int // some other value being sent
+	Hello  *string `lang:"hello"`
+	Answer int     `lang:"answer"` // some other value being sent
 }
 
 // Sends represents the default struct of values we can send using Send/Recv.

engine/resources/test_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/timer.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/resources/user.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -30,8 +30,7 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -153,7 +152,7 @@ func (obj *UserRes) Watch() error {
 }
 
 // CheckApply method for User resource.
-func (obj *UserRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *UserRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply(%t)", apply)
 
 	var exists = true

engine/resources/virt.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -29,11 +29,12 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
+	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/libvirt/libvirt-go"
 	libvirtxml "github.com/libvirt/libvirt-go-xml"
-	errwrap "github.com/pkg/errors"
 )
 
 func init() {
@@ -65,30 +66,53 @@ const (
 // VirtRes is a libvirt resource. A transient virt resource, which has its state
 // set to `shutoff` is one which does not exist. The parallel equivalent is a
 // file resource which removes a particular path.
+// TODO: some values inside here should be enum's!
 type VirtRes struct {
 	traits.Base // add the base methods without re-implementation
 	traits.Refreshable
 
 	init *engine.Init
 
-	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"`
+	// URI is the libvirt connection URI, eg: `qemu:///session`.
+	URI string `lang:"uri" yaml:"uri"`
+	// State is the desired vm state. Possible values include: `running`,
+	// `paused` and `shutoff`.
+	State string `lang:"state" yaml:"state"`
+	// Transient is whether the vm is defined (false) or undefined (true).
+	Transient bool `lang:"transient" yaml:"transient"`
 
-	HotCPUs bool `lang:"hotcpus" yaml:"hotcpus"` // allow hotplug of cpus?
-	// FIXME: values here should be enum's!
-	RestartOnDiverge string `lang:"restartondiverge" yaml:"restartondiverge"` // restart policy: "ignore", "ifneeded", "error"
-	RestartOnRefresh bool   `lang:"restartonrefresh" yaml:"restartonrefresh"` // restart on refresh?
+	// CPUs is the desired cpu count of the machine.
+	CPUs uint `lang:"cpus" yaml:"cpus"`
+	// MaxCPUs is the maximum number of cpus allowed in the machine. You
+	// need to set this so that on boot the `hardware` knows how many cpu
+	// `slots` it might need to make room for.
+	MaxCPUs uint `lang:"maxcpus" yaml:"maxcpus"`
+	// HotCPUs specifies whether we can hot plug and unplug cpus.
+	HotCPUs bool `lang:"hotcpus" yaml:"hotcpus"`
+	// Memory is the size in KBytes of memory to include in the machine.
+	Memory uint64 `lang:"memory" yaml:"memory"`
+
+	// OSInit is the init used by lxc.
+	OSInit string `lang:"osinit" yaml:"osinit"`
+	// Boot is the boot order. Values are `fd`, `hd`, `cdrom` and `network`.
+	Boot []string `lang:"boot" yaml:"boot"`
+	// Disk is the list of disk devices to include.
+	Disk []*DiskDevice `lang:"disk" yaml:"disk"`
+	// CdRom is the list of cdrom devices to include.
+	CDRom []*CDRomDevice `lang:"cdrom" yaml:"cdrom"`
+	// Network is the list of network devices to include.
+	Network []*NetworkDevice `lang:"network" yaml:"network"`
+	// Filesystem is the list of file system devices to include.
+	Filesystem []*FilesystemDevice `lang:"filesystem" yaml:"filesystem"`
+
+	// Auth points to the libvirt credentials to use if any are necessary.
+	Auth *VirtAuth `lang:"auth" yaml:"auth"`
+
+	// RestartOnDiverge is the restart policy, and can be: `ignore`,
+	// `ifneeded` or `error`.
+	RestartOnDiverge string `lang:"restartondiverge" yaml:"restartondiverge"`
+	// RestartOnRefresh specifies if we restart on refresh signal.
+	RestartOnRefresh bool `lang:"restartonrefresh" yaml:"restartonrefresh"`
 
 	wg                  *sync.WaitGroup
 	conn                *libvirt.Connect
@@ -107,6 +131,24 @@ type VirtAuth struct {
 	Password string `lang:"password" yaml:"password"`
 }
 
+// Cmp compares two VirtAuth structs. It errors if they are not identical.
+func (obj *VirtAuth) Cmp(auth *VirtAuth) error {
+	if (obj == nil) != (auth == nil) { // xor
+		return fmt.Errorf("the VirtAuth differs")
+	}
+	if obj == nil && auth == nil {
+		return nil
+	}
+
+	if obj.Username != auth.Username {
+		return fmt.Errorf("the Username differs")
+	}
+	if obj.Password != auth.Password {
+		return fmt.Errorf("the Password differs")
+	}
+	return nil
+}
+
 // Default returns some sensible defaults for this resource.
 func (obj *VirtRes) Default() engine.Res {
 	return &VirtRes{
@@ -138,7 +180,7 @@ func (obj *VirtRes) Init(init *engine.Init) error {
 	var u *url.URL
 	var err error
 	if u, err = url.Parse(obj.URI); err != nil {
-		return errwrap.Wrapf(err, "%s: Parsing URI failed: %s", obj, obj.URI)
+		return errwrap.Wrapf(err, "parsing URI (`%s`) failed", obj.URI)
 	}
 	switch u.Scheme {
 	case "lxc":
@@ -149,7 +191,7 @@ func (obj *VirtRes) Init(init *engine.Init) error {
 
 	obj.conn, err = obj.connect() // gets closed in Close method of Res API
 	if err != nil {
-		return errwrap.Wrapf(err, "%s: Connection to libvirt failed in init", obj)
+		return errwrap.Wrapf(err, "connection to libvirt failed in init")
 	}
 
 	// check for hard to change properties
@@ -157,14 +199,14 @@ func (obj *VirtRes) Init(init *engine.Init) error {
 	if err == nil {
 		defer dom.Free()
 	} else if !isNotFound(err) {
-		return errwrap.Wrapf(err, "%s: Could not lookup on init", obj)
+		return errwrap.Wrapf(err, "could not lookup on init")
 	}
 
 	if err == nil {
 		// maxCPUs, err := dom.GetMaxVcpus()
 		i, err := dom.GetVcpusFlags(libvirt.DOMAIN_VCPU_MAXIMUM)
 		if err != nil {
-			return errwrap.Wrapf(err, "%s: Could not lookup MaxCPUs on init", obj)
+			return errwrap.Wrapf(err, "could not lookup MaxCPUs on init")
 		}
 		maxCPUs := uint(i)
 		if obj.MaxCPUs != maxCPUs { // max cpu slots is hard to change
@@ -177,11 +219,11 @@ func (obj *VirtRes) Init(init *engine.Init) error {
 		// event handlers so that we don't miss any events via race?
 		xmlDesc, err := dom.GetXMLDesc(0) // 0 means no flags
 		if err != nil {
-			return errwrap.Wrapf(err, "%s: Could not GetXMLDesc on init", obj)
+			return errwrap.Wrapf(err, "could not GetXMLDesc on init")
 		}
 		domXML := &libvirtxml.Domain{}
 		if err := domXML.Unmarshal(xmlDesc); err != nil {
-			return errwrap.Wrapf(err, "%s: Could not unmarshal XML on init", obj)
+			return errwrap.Wrapf(err, "could not unmarshal XML on init")
 		}
 
 		// guest agent: domain->devices->channel->target->state == connected?
@@ -382,22 +424,22 @@ func (obj *VirtRes) Watch() error {
 			if state == libvirt.CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_STATE_CONNECTED {
 				obj.guestAgentConnected = true
 				send = true
-				obj.init.Logf("Guest agent connected")
+				obj.init.Logf("guest agent connected")
 
 			} else if state == libvirt.CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_STATE_DISCONNECTED {
 				obj.guestAgentConnected = false
 				// ignore CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_DOMAIN_STARTED
 				// events because they just tell you that guest agent channel was added
 				if reason == libvirt.CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL {
-					obj.init.Logf("Guest agent disconnected")
+					obj.init.Logf("guest agent disconnected")
 				}
 
 			} else {
-				return fmt.Errorf("unknown %s guest agent state: %v", obj, state)
+				return fmt.Errorf("unknown guest agent state: %v", state)
 			}
 
 		case err := <-errorChan:
-			return fmt.Errorf("unknown %s libvirt error: %s", obj, err)
+			return errwrap.Wrapf(err, "unknown libvirt error")
 
 		case <-obj.init.Done: // closed by the engine to signal shutdown
 			return nil
@@ -434,7 +476,7 @@ func (obj *VirtRes) domainCreate() (*libvirt.Domain, bool, error) {
 		if err != nil {
 			return dom, false, err // returned dom is invalid
 		}
-		obj.init.Logf("Domain transient %s", state)
+		obj.init.Logf("transient domain %s", state) // log the state
 		return dom, false, nil
 	}
 
@@ -442,20 +484,20 @@ func (obj *VirtRes) domainCreate() (*libvirt.Domain, bool, error) {
 	if err != nil {
 		return dom, false, err // returned dom is invalid
 	}
-	obj.init.Logf("Domain defined")
+	obj.init.Logf("domain defined")
 
 	if obj.State == "running" {
 		if err := dom.Create(); err != nil {
 			return dom, false, err
 		}
-		obj.init.Logf("Domain started")
+		obj.init.Logf("domain started")
 	}
 
 	if obj.State == "paused" {
 		if err := dom.CreateWithFlags(libvirt.DOMAIN_START_PAUSED); err != nil {
 			return dom, false, err
 		}
-		obj.init.Logf("Domain created paused")
+		obj.init.Logf("domain created paused")
 	}
 
 	return dom, false, nil
@@ -483,7 +525,7 @@ func (obj *VirtRes) stateCheckApply(apply bool, dom *libvirt.Domain) (bool, erro
 		}
 		if domInfo.State == libvirt.DOMAIN_BLOCKED {
 			// TODO: what should happen?
-			return false, fmt.Errorf("domain %s is blocked", obj.Name())
+			return false, fmt.Errorf("domain is blocked")
 		}
 		if !apply {
 			return false, nil
@@ -493,14 +535,14 @@ func (obj *VirtRes) stateCheckApply(apply bool, dom *libvirt.Domain) (bool, erro
 				return false, errwrap.Wrapf(err, "domain.Resume failed")
 			}
 			checkOK = false
-			obj.init.Logf("Domain resumed")
+			obj.init.Logf("domain resumed")
 			break
 		}
 		if err := dom.Create(); err != nil {
 			return false, errwrap.Wrapf(err, "domain.Create failed")
 		}
 		checkOK = false
-		obj.init.Logf("Domain created")
+		obj.init.Logf("domain created")
 
 	case "paused":
 		if domInfo.State == libvirt.DOMAIN_PAUSED {
@@ -514,14 +556,14 @@ func (obj *VirtRes) stateCheckApply(apply bool, dom *libvirt.Domain) (bool, erro
 				return false, errwrap.Wrapf(err, "domain.Suspend failed")
 			}
 			checkOK = false
-			obj.init.Logf("Domain paused")
+			obj.init.Logf("domain paused")
 			break
 		}
 		if err := dom.CreateWithFlags(libvirt.DOMAIN_START_PAUSED); err != nil {
 			return false, errwrap.Wrapf(err, "domain.CreateWithFlags failed")
 		}
 		checkOK = false
-		obj.init.Logf("Domain created paused")
+		obj.init.Logf("domain created paused")
 
 	case "shutoff":
 		if domInfo.State == libvirt.DOMAIN_SHUTOFF || domInfo.State == libvirt.DOMAIN_SHUTDOWN {
@@ -535,7 +577,7 @@ func (obj *VirtRes) stateCheckApply(apply bool, dom *libvirt.Domain) (bool, erro
 			return false, errwrap.Wrapf(err, "domain.Destroy failed")
 		}
 		checkOK = false
-		obj.init.Logf("Domain destroyed")
+		obj.init.Logf("domain destroyed")
 	}
 
 	return checkOK, nil
@@ -561,7 +603,7 @@ func (obj *VirtRes) attrCheckApply(apply bool, dom *libvirt.Domain) (bool, error
 		if err := dom.SetMemory(obj.Memory); err != nil {
 			return false, errwrap.Wrapf(err, "domain.SetMemory failed")
 		}
-		obj.init.Logf("Memory changed to %d", obj.Memory)
+		obj.init.Logf("memory changed to: %d", obj.Memory)
 	}
 
 	// check cpus
@@ -600,7 +642,7 @@ func (obj *VirtRes) attrCheckApply(apply bool, dom *libvirt.Domain) (bool, error
 				return false, errwrap.Wrapf(err, "domain.SetVcpus failed")
 			}
 			checkOK = false
-			obj.init.Logf("CPUs (hot) changed to %d", obj.CPUs)
+			obj.init.Logf("cpus (hot) changed to: %d", obj.CPUs)
 
 		case libvirt.DOMAIN_SHUTOFF, libvirt.DOMAIN_SHUTDOWN:
 			if !obj.Transient {
@@ -612,7 +654,7 @@ func (obj *VirtRes) attrCheckApply(apply bool, dom *libvirt.Domain) (bool, error
 					return false, errwrap.Wrapf(err, "domain.SetVcpus failed")
 				}
 				checkOK = false
-				obj.init.Logf("CPUs (cold) changed to %d", obj.CPUs)
+				obj.init.Logf("cpus (cold) changed to: %d", obj.CPUs)
 			}
 
 		default:
@@ -643,7 +685,7 @@ func (obj *VirtRes) attrCheckApply(apply bool, dom *libvirt.Domain) (bool, error
 				return false, errwrap.Wrapf(err, "domain.SetVcpus failed")
 			}
 			checkOK = false
-			obj.init.Logf("CPUs (guest) changed to %d", obj.CPUs)
+			obj.init.Logf("cpus (guest) changed to: %d", obj.CPUs)
 		}
 	}
 
@@ -667,7 +709,7 @@ func (obj *VirtRes) domainShutdownSync(apply bool, dom *libvirt.Domain) (bool, e
 			return false, errwrap.Wrapf(err, "domain.GetInfo failed")
 		}
 		if domInfo.State == libvirt.DOMAIN_SHUTOFF || domInfo.State == libvirt.DOMAIN_SHUTDOWN {
-			obj.init.Logf("Shutdown")
+			obj.init.Logf("shutdown")
 			break
 		}
 
@@ -679,7 +721,7 @@ func (obj *VirtRes) domainShutdownSync(apply bool, dom *libvirt.Domain) (bool, e
 			obj.processExitChan = make(chan struct{})
 			// if machine shuts down before we call this, we error;
 			// this isn't ideal, but it happened due to user error!
-			obj.init.Logf("Running shutdown")
+			obj.init.Logf("running shutdown")
 			if err := dom.Shutdown(); err != nil {
 				// FIXME: if machine is already shutdown completely, return early
 				return false, errwrap.Wrapf(err, "domain.Shutdown failed")
@@ -700,7 +742,7 @@ func (obj *VirtRes) domainShutdownSync(apply bool, dom *libvirt.Domain) (bool, e
 			// https://libvirt.org/formatdomain.html#elementsEvents
 			continue
 		case <-timeout:
-			return false, fmt.Errorf("%s: didn't shutdown after %d seconds", obj, MaxShutdownDelayTimeout)
+			return false, fmt.Errorf("didn't shutdown after %d seconds", MaxShutdownDelayTimeout)
 		}
 	}
 
@@ -710,8 +752,8 @@ func (obj *VirtRes) domainShutdownSync(apply bool, dom *libvirt.Domain) (bool, e
 // CheckApply checks the resource state and applies the resource if the bool
 // input is true. It returns error info and if the state check passed or not.
 func (obj *VirtRes) CheckApply(apply bool) (bool, error) {
-	if obj.conn == nil {
-		panic("virt: CheckApply is being called with nil connection")
+	if obj.conn == nil { // programming error?
+		return false, fmt.Errorf("got called with nil connection")
 	}
 	// if we do the restart, we must flip the flag back to false as evidence
 	var restart bool                                // do we need to do a restart?
@@ -772,7 +814,7 @@ func (obj *VirtRes) CheckApply(apply bool) (bool, error) {
 			if err := dom.Undefine(); err != nil {
 				return false, errwrap.Wrapf(err, "domain.Undefine failed")
 			}
-			obj.init.Logf("Domain undefined")
+			obj.init.Logf("domain undefined")
 		} else {
 			domXML, err := dom.GetXMLDesc(libvirt.DOMAIN_XML_INACTIVE)
 			if err != nil {
@@ -781,7 +823,7 @@ func (obj *VirtRes) CheckApply(apply bool) (bool, error) {
 			if _, err = obj.conn.DomainDefineXML(domXML); err != nil {
 				return false, errwrap.Wrapf(err, "conn.DomainDefineXML failed")
 			}
-			obj.init.Logf("Domain defined")
+			obj.init.Logf("domain defined")
 		}
 		checkOK = false
 	}
@@ -829,7 +871,7 @@ func (obj *VirtRes) CheckApply(apply bool) (bool, error) {
 
 	// we had to do a restart, we didn't, and we should error if it was needed
 	if obj.restartScheduled && restart == true && obj.RestartOnDiverge == "error" {
-		return false, fmt.Errorf("%s: needed restart but didn't! (RestartOnDiverge: %v)", obj, obj.RestartOnDiverge)
+		return false, fmt.Errorf("needed restart but didn't! (RestartOnDiverge: %s)", obj.RestartOnDiverge)
 	}
 
 	return checkOK, nil // w00t
@@ -959,27 +1001,6 @@ type DiskDevice struct {
 	Type   string `lang:"type" 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"`
-}
-
-// 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"`
-}
-
-// 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"`
-}
-
 // 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?
@@ -992,6 +1013,32 @@ func (obj *DiskDevice) GetXML(idx int) string {
 	return b
 }
 
+// Cmp compares two DiskDevice's and returns an error if they are not
+// equivalent.
+func (obj *DiskDevice) Cmp(dev *DiskDevice) error {
+	if (obj == nil) != (dev == nil) { // xor
+		return fmt.Errorf("the DiskDevice differs")
+	}
+	if obj == nil && dev == nil {
+		return nil
+	}
+
+	if obj.Source != dev.Source {
+		return fmt.Errorf("the Source differs")
+	}
+	if obj.Type != dev.Type {
+		return fmt.Errorf("the Type differs")
+	}
+
+	return nil
+}
+
+// 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"`
+}
+
 // 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?
@@ -1005,6 +1052,32 @@ func (obj *CDRomDevice) GetXML(idx int) string {
 	return b
 }
 
+// Cmp compares two CDRomDevice's and returns an error if they are not
+// equivalent.
+func (obj *CDRomDevice) Cmp(dev *CDRomDevice) error {
+	if (obj == nil) != (dev == nil) { // xor
+		return fmt.Errorf("the CDRomDevice differs")
+	}
+	if obj == nil && dev == nil {
+		return nil
+	}
+
+	if obj.Source != dev.Source {
+		return fmt.Errorf("the Source differs")
+	}
+	if obj.Type != dev.Type {
+		return fmt.Errorf("the Type differs")
+	}
+
+	return nil
+}
+
+// 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"`
+}
+
 // GetXML returns the XML representation of this device.
 func (obj *NetworkDevice) GetXML(idx int) string {
 	if obj.MAC == "" {
@@ -1018,6 +1091,35 @@ func (obj *NetworkDevice) GetXML(idx int) string {
 	return b
 }
 
+// Cmp compares two NetworkDevice's and returns an error if they are not
+// equivalent.
+func (obj *NetworkDevice) Cmp(dev *NetworkDevice) error {
+	if (obj == nil) != (dev == nil) { // xor
+		return fmt.Errorf("the NetworkDevice differs")
+	}
+	if obj == nil && dev == nil {
+		return nil
+	}
+
+	if obj.Name != dev.Name {
+		return fmt.Errorf("the Name differs")
+	}
+	if obj.MAC != dev.MAC {
+		return fmt.Errorf("the MAC differs")
+	}
+
+	return nil
+}
+
+// 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"`
+}
+
 // 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?
@@ -1036,39 +1138,61 @@ func (obj *FilesystemDevice) GetXML(idx int) string {
 	return b
 }
 
-// Cmp compares two resources and returns an error if they are not equivalent.
-func (obj *VirtRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
+// Cmp compares two FilesystemDevice's and returns an error if they are not
+// equivalent.
+func (obj *FilesystemDevice) Cmp(dev *FilesystemDevice) error {
+	if (obj == nil) != (dev == nil) { // xor
+		return fmt.Errorf("the FilesystemDevice differs")
 	}
+	if obj == nil && dev == nil {
+		return nil
+	}
+
+	if obj.Access != dev.Access {
+		return fmt.Errorf("the Access differs")
+	}
+	if obj.Source != dev.Source {
+		return fmt.Errorf("the Source differs")
+	}
+	if obj.Target != dev.Target {
+		return fmt.Errorf("the Target differs")
+	}
+	if obj.ReadOnly != dev.ReadOnly {
+		return fmt.Errorf("the ReadOnly differs")
+	}
+
 	return nil
 }
 
-// Compare two resources and return if they are equivalent.
-func (obj *VirtRes) Compare(r engine.Res) bool {
+// Cmp compares two resources and returns an error if they are not equivalent.
+func (obj *VirtRes) Cmp(r engine.Res) error {
 	// we can only compare VirtRes to others of the same resource kind
 	res, ok := r.(*VirtRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.URI != res.URI {
-		return false
+		return fmt.Errorf("the URI differs")
 	}
 	if obj.State != res.State {
-		return false
+		return fmt.Errorf("the State differs")
 	}
 	if obj.Transient != res.Transient {
-		return false
+		return fmt.Errorf("the Transient differs")
 	}
+
 	if obj.CPUs != res.CPUs {
-		return false
+		return fmt.Errorf("the CPUs differ")
 	}
 	// we can't change this property while machine is running!
 	// we do need to return false, so that a new struct gets built,
 	// which will cause at least one Init() & CheckApply() to run.
 	if obj.MaxCPUs != res.MaxCPUs {
-		return false
+		return fmt.Errorf("the MaxCPUs differ")
+	}
+	if obj.HotCPUs != res.HotCPUs {
+		return fmt.Errorf("the HotCPUs differ")
 	}
 	// TODO: can we skip the compare of certain properties such as
 	// Memory because this object (but with different memory) can be
@@ -1076,26 +1200,61 @@ func (obj *VirtRes) Compare(r engine.Res) bool {
 	// We would need to run some sort of "old struct update", to get
 	// the new values, but that's easy to add.
 	if obj.Memory != res.Memory {
-		return false
+		return fmt.Errorf("the Memory differs")
 	}
-	// TODO:
-	//if obj.Boot != res.Boot {
-	//	return false
-	//}
-	//if obj.Disk != res.Disk {
-	//	return false
-	//}
-	//if obj.CDRom != res.CDRom {
-	//	return false
-	//}
-	//if obj.Network != res.Network {
-	//	return false
-	//}
-	//if obj.Filesystem != res.Filesystem {
-	//	return false
-	//}
 
-	return true
+	if obj.OSInit != res.OSInit {
+		return fmt.Errorf("the OSInit differs")
+	}
+	if err := engineUtil.StrListCmp(obj.Boot, res.Boot); err != nil {
+		return errwrap.Wrapf(err, "the Boot differs")
+	}
+
+	if len(obj.Disk) != len(res.Disk) {
+		return fmt.Errorf("the Disk length differs")
+	}
+	for i := range obj.Disk {
+		if err := obj.Disk[i].Cmp(res.Disk[i]); err != nil {
+			return errwrap.Wrapf(err, "the Disk differs")
+		}
+	}
+	if len(obj.CDRom) != len(res.CDRom) {
+		return fmt.Errorf("the CDRom length differs")
+	}
+	for i := range obj.CDRom {
+		if err := obj.CDRom[i].Cmp(res.CDRom[i]); err != nil {
+			return errwrap.Wrapf(err, "the CDRom differs")
+		}
+	}
+	if len(obj.Network) != len(res.Network) {
+		return fmt.Errorf("the Network length differs")
+	}
+	for i := range obj.Network {
+		if err := obj.Network[i].Cmp(res.Network[i]); err != nil {
+			return errwrap.Wrapf(err, "the Network differs")
+		}
+	}
+	if len(obj.Filesystem) != len(res.Filesystem) {
+		return fmt.Errorf("the Filesystem length differs")
+	}
+	for i := range obj.Filesystem {
+		if err := obj.Filesystem[i].Cmp(res.Filesystem[i]); err != nil {
+			return errwrap.Wrapf(err, "the Filesystem differs")
+		}
+	}
+
+	if err := obj.Auth.Cmp(res.Auth); err != nil {
+		return errwrap.Wrapf(err, "the Auth differs")
+	}
+
+	if obj.RestartOnDiverge != res.RestartOnDiverge {
+		return fmt.Errorf("the RestartOnDiverge differs")
+	}
+	if obj.RestartOnRefresh != res.RestartOnRefresh {
+		return fmt.Errorf("the RestartOnRefresh differs")
+	}
+
+	return nil
 }
 
 // VirtUID is the UID struct for FileRes.

engine/reverse.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/sendrecv.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/traits/autoedge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -25,7 +25,9 @@ import (
 // methods needed to support autoedges on resources. It may be used as a start
 // point to avoid re-implementing the straightforward methods.
 type Edgeable struct {
-	meta *engine.AutoEdgeMeta
+	// Xmeta is the stored meta. It should be called `meta` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xmeta *engine.AutoEdgeMeta
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -33,16 +35,16 @@ type Edgeable struct {
 
 // AutoEdgeMeta lets you get or set meta params for the automatic edges trait.
 func (obj *Edgeable) AutoEdgeMeta() *engine.AutoEdgeMeta {
-	if obj.meta == nil { // set the defaults if previously empty
-		obj.meta = &engine.AutoEdgeMeta{
+	if obj.Xmeta == nil { // set the defaults if previously empty
+		obj.Xmeta = &engine.AutoEdgeMeta{
 			Disabled: false,
 		}
 	}
-	return obj.meta
+	return obj.Xmeta
 }
 
 // SetAutoEdgeMeta lets you set all of the meta params for the automatic edges
 // trait in a single call.
 func (obj *Edgeable) SetAutoEdgeMeta(meta *engine.AutoEdgeMeta) {
-	obj.meta = meta
+	obj.Xmeta = meta
 }

engine/traits/autogroup.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -27,7 +27,9 @@ import (
 // methods needed to support autogrouping on resources. It may be used as a
 // starting point to avoid re-implementing the straightforward methods.
 type Groupable struct {
-	meta *engine.AutoGroupMeta
+	// Xmeta is the stored meta. It should be called `meta` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xmeta *engine.AutoGroupMeta
 
 	isGrouped bool                  // am i contained within a group?
 	grouped   []engine.GroupableRes // list of any grouped resources
@@ -39,18 +41,18 @@ type Groupable struct {
 // AutoGroupMeta lets you get or set meta params for the automatic grouping
 // trait.
 func (obj *Groupable) AutoGroupMeta() *engine.AutoGroupMeta {
-	if obj.meta == nil { // set the defaults if previously empty
-		obj.meta = &engine.AutoGroupMeta{
+	if obj.Xmeta == nil { // set the defaults if previously empty
+		obj.Xmeta = &engine.AutoGroupMeta{
 			Disabled: false,
 		}
 	}
-	return obj.meta
+	return obj.Xmeta
 }
 
 // SetAutoGroupMeta lets you set all of the meta params for the automatic
 // grouping trait in a single call.
 func (obj *Groupable) SetAutoGroupMeta(meta *engine.AutoGroupMeta) {
-	obj.meta = meta
+	obj.Xmeta = meta
 }
 
 // GroupCmp compares two resources and decides if they're suitable for grouping.

engine/traits/base.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/traits/kind.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -17,11 +17,21 @@
 
 package traits
 
+import (
+	"encoding/gob"
+)
+
+func init() {
+	gob.Register(&Kinded{})
+}
+
 // Kinded contains a general implementation of the properties and methods needed
 // to support the resource kind. It should be used as a starting point to avoid
 // re-implementing the straightforward kind methods.
 type Kinded struct {
-	kind string
+	// Xkind is the stored kind. It should be called `kind` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xkind string
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -29,11 +39,11 @@ type Kinded struct {
 
 // Kind returns the string representation for the kind this resource is.
 func (obj *Kinded) Kind() string {
-	return obj.kind
+	return obj.Xkind
 }
 
 // SetKind sets the kind string for this resource. It must only be set by the
 // engine.
 func (obj *Kinded) SetKind(kind string) {
-	obj.kind = kind
+	obj.Xkind = kind
 }

engine/traits/meta.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -25,7 +25,9 @@ import (
 // to support meta parameters. It should be used as a starting point to avoid
 // re-implementing the straightforward meta methods.
 type Meta struct {
-	meta *engine.MetaParams
+	// Xmeta is the stored meta. It should be called `meta` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xmeta *engine.MetaParams
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -33,14 +35,14 @@ type Meta struct {
 
 // MetaParams lets you get or set meta params for this trait.
 func (obj *Meta) MetaParams() *engine.MetaParams {
-	if obj.meta == nil { // set the defaults if previously empty
-		obj.meta = engine.DefaultMetaParams.Copy()
+	if obj.Xmeta == nil { // set the defaults if previously empty
+		obj.Xmeta = engine.DefaultMetaParams.Copy()
 	}
-	return obj.meta
+	return obj.Xmeta
 }
 
 // SetMetaParams lets you set all of the meta params for the resource in a
 // single call.
 func (obj *Meta) SetMetaParams(meta *engine.MetaParams) {
-	obj.meta = meta
+	obj.Xmeta = meta
 }

engine/traits/named.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -21,7 +21,9 @@ package traits
 // to support named resources. It should be used as a starting point to avoid
 // re-implementing the straightforward name methods.
 type Named struct {
-	name string
+	// Xname is the stored name. It should be called `name` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xname string
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -30,11 +32,11 @@ type Named struct {
 // Name returns the unique name this resource has. It is only unique within its
 // own kind.
 func (obj *Named) Name() string {
-	return obj.name
+	return obj.Xname
 }
 
 // SetName sets the unique name for this resource. It must only be unique within
 // its own kind.
 func (obj *Named) SetName(name string) {
-	obj.name = name
+	obj.Xname = name
 }

engine/traits/refresh.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/traits/reverse.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -25,7 +25,9 @@ import (
 // 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
+	// Xmeta is the stored meta. It should be called `meta` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xmeta *engine.ReversibleMeta
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -33,16 +35,16 @@ type Reversible struct {
 
 // 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{
+	if obj.Xmeta == nil { // set the defaults if previously empty
+		obj.Xmeta = &engine.ReversibleMeta{
 			Disabled: true, // by default we're disabled
 		}
 	}
-	return obj.meta
+	return obj.Xmeta
 }
 
 // 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
+	obj.Xmeta = meta
 }

engine/traits/sendrecv.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -33,7 +33,7 @@ type Sendable struct {
 
 // Sends returns a struct containing the defaults of the type we send. This
 // needs to be implemented (overridden) by the struct with the Sendable trait to
-// be able to send any values. The public struct field names are the keys used.
+// be able to send any values. The field struct tag names are the keys used.
 func (obj *Sendable) Sends() interface{} {
 	return nil
 }

engine/util.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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

engine/util/cmp.go

@@ -0,0 +1,37 @@
+// Mgmt
+// Copyright (C) 2013-2019+ 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 util
+
+import (
+	"fmt"
+)
+
+// StrListCmp compares two lists of strings. If they are not the same length or
+// do not contain identical strings in the same order, then this errors.
+func StrListCmp(x, y []string) error {
+	if len(x) != len(y) {
+		return fmt.Errorf("length differs")
+	}
+	for i := range x {
+		if x[i] != y[i] {
+			return fmt.Errorf("the elements at position %d differed", i)
+		}
+	}
+
+	return nil
+}

engine/util/util.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -30,9 +30,9 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/lang/types"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/godbus/dbus"
-	errwrap "github.com/pkg/errors"
 )
 
 const (
@@ -94,10 +94,22 @@ func B64ToRes(str string) (engine.Res, error) {
 
 // StructTagToFieldName returns a mapping from recommended alias to actual field
 // name. It returns an error if it finds a collision. It uses the `lang` tags.
-func StructTagToFieldName(res engine.Res) (map[string]string, error) {
+// It must be passed a ptr to a struct or it will error.
+func StructTagToFieldName(stptr interface{}) (map[string]string, error) {
 	// TODO: fallback to looking up yaml tags, although harder to parse
 	result := make(map[string]string) // `lang` field tag -> field name
-	st := reflect.TypeOf(res).Elem()  // elem for ptr to res
+	if stptr == nil {
+		return nil, fmt.Errorf("got nil input instead of ptr to struct")
+	}
+	typ := reflect.TypeOf(stptr)
+	if k := typ.Kind(); k != reflect.Ptr { // we only look at *Struct's
+		return nil, fmt.Errorf("input is not a ptr, got: %+v", k)
+	}
+	st := typ.Elem()                         // elem for ptr to struct (dereference the pointer)
+	if k := st.Kind(); k != reflect.Struct { // this should be a struct now
+		return nil, fmt.Errorf("input doesn't point to a struct, got: %+v", k)
+	}
+
 	for i := 0; i < st.NumField(); i++ {
 		field := st.Field(i)
 		name := field.Name
@@ -115,6 +127,62 @@ func StructTagToFieldName(res engine.Res) (map[string]string, error) {
 	return result, nil
 }
 
+// StructFieldCompat returns whether a send struct and key is compatible with a
+// recv struct and key. This inputs must both be a ptr to a string, and a valid
+// key that can be found in the struct tag.
+// TODO: add a bool to decide if *string to string or string to *string is okay.
+func StructFieldCompat(st1 interface{}, key1 string, st2 interface{}, key2 string) error {
+	m1, err := StructTagToFieldName(st1)
+	if err != nil {
+		return err
+	}
+	k1, exists := m1[key1]
+	if !exists {
+		return fmt.Errorf("key not found in send struct")
+	}
+
+	m2, err := StructTagToFieldName(st2)
+	if err != nil {
+		return err
+	}
+	k2, exists := m2[key2]
+	if !exists {
+		return fmt.Errorf("key not found in recv struct")
+	}
+
+	obj1 := reflect.Indirect(reflect.ValueOf(st1))
+	//type1 := obj1.Type()
+	value1 := obj1.FieldByName(k1)
+	kind1 := value1.Kind()
+
+	obj2 := reflect.Indirect(reflect.ValueOf(st2))
+	//type2 := obj2.Type()
+	value2 := obj2.FieldByName(k2)
+	kind2 := value2.Kind()
+
+	if kind1 != kind2 {
+		return fmt.Errorf("kind mismatch between %s and %s", kind1, kind2)
+	}
+
+	if t1, t2 := value1.Type(), value2.Type(); t1 != t2 {
+		return fmt.Errorf("type mismatch between %s and %s", t1, t2)
+	}
+
+	if !value2.CanSet() { // if we can't set, then this is pointless!
+		return fmt.Errorf("can't set")
+	}
+
+	// if we can't interface, we can't compare...
+	if !value1.CanInterface() {
+		return fmt.Errorf("can't interface the send")
+	}
+	if !value2.CanInterface() {
+		return fmt.Errorf("can't interface the recv")
+	}
+
+	return nil
+}
+
 // LowerStructFieldNameToFieldName returns a mapping from the lower case version
 // of each field name to the actual field name. It only returns public fields.
 // It returns an error if it finds a collision.

engine/util/util_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -21,6 +21,7 @@ package util
 
 import (
 	"os/user"
+	"reflect"
 	"strconv"
 	"testing"
 )
@@ -105,3 +106,61 @@ func TestCurrentUserGroupById(t *testing.T) {
 		t.Errorf("gid didn't match current user's: %s vs %s", strconv.Itoa(gid), currentGID)
 	}
 }
+
+func TestStructTagToFieldName0(t *testing.T) {
+	type foo struct {
+		A string `lang:"aaa"`
+		B bool   `lang:"bbb"`
+		C int64  `lang:"ccc"`
+	}
+	f := &foo{ // a ptr!
+		A: "hello",
+		B: true,
+		C: 13,
+	}
+	m, err := StructTagToFieldName(f) // (map[string]string, error)
+	if err != nil {
+		t.Errorf("got error: %+v", err)
+		return
+	}
+	t.Logf("got output: %+v", m)
+	expected := map[string]string{
+		"aaa": "A",
+		"bbb": "B",
+		"ccc": "C",
+	}
+	if !reflect.DeepEqual(m, expected) {
+		t.Errorf("unexpected result")
+		return
+	}
+}
+
+func TestStructTagToFieldName1(t *testing.T) {
+	type foo struct {
+		A string `lang:"aaa"`
+		B bool   `lang:"bbb"`
+		C int64  `lang:"ccc"`
+	}
+	f := foo{ // not a ptr!
+		A: "hello",
+		B: true,
+		C: 13,
+	}
+	m, err := StructTagToFieldName(f) // (map[string]string, error)
+	if err == nil {
+		t.Errorf("expected error, got nil")
+		//return
+	}
+	t.Logf("got output: %+v", m)
+	t.Logf("got error: %+v", err)
+}
+
+func TestStructTagToFieldName2(t *testing.T) {
+	m, err := StructTagToFieldName(nil) // (map[string]string, error)
+	if err == nil {
+		t.Errorf("expected error, got nil")
+		//return
+	}
+	t.Logf("got output: %+v", m)
+	t.Logf("got error: %+v", err)
+}

engine/world.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -18,6 +18,8 @@
 package engine
 
 import (
+	"context"
+
 	"github.com/purpleidea/mgmt/etcd/scheduler"
 )
 
@@ -25,22 +27,26 @@ import (
 // the GAPI to store state and exchange information throughout the cluster. It
 // is the interface each machine uses to communicate with the rest of the world.
 type World interface { // TODO: is there a better name for this interface?
-	ResWatch() chan error
-	ResExport([]Res) error
+	ResWatch(context.Context) (chan error, error)
+	ResExport(context.Context, []Res) error
 	// FIXME: should this method take a "filter" data struct instead of many args?
-	ResCollect(hostnameFilter, kindFilter []string) ([]Res, error)
+	ResCollect(ctx context.Context, hostnameFilter, kindFilter []string) ([]Res, error)
 
-	StrWatch(namespace string) chan error
+	IdealClusterSizeWatch(context.Context) (chan error, error)
+	IdealClusterSizeGet(context.Context) (uint16, error)
+	IdealClusterSizeSet(context.Context, uint16) (bool, error)
+
+	StrWatch(ctx context.Context, namespace string) (chan error, error)
 	StrIsNotExist(error) bool
-	StrGet(namespace string) (string, error)
-	StrSet(namespace, value string) error
-	StrDel(namespace string) error
+	StrGet(ctx context.Context, namespace string) (string, error)
+	StrSet(ctx context.Context, namespace, value string) error
+	StrDel(ctx context.Context, namespace string) error
 
 	// XXX: add the exchange primitives in here directly?
-	StrMapWatch(namespace string) chan error
-	StrMapGet(namespace string) (map[string]string, error)
-	StrMapSet(namespace, value string) error
-	StrMapDel(namespace string) error
+	StrMapWatch(ctx context.Context, namespace string) (chan error, error)
+	StrMapGet(ctx context.Context, namespace string) (map[string]string, error)
+	StrMapSet(ctx context.Context, namespace, value string) error
+	StrMapDel(ctx context.Context, namespace string) error
 
 	Scheduler(namespace string, opts ...scheduler.Option) (*scheduler.Result, error)
 

etcd/callback.go

@@ -0,0 +1,497 @@
+// Mgmt
+// Copyright (C) 2013-2019+ 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 etcd
+
+import (
+	"context"
+	"fmt"
+	"sync"
+
+	"github.com/purpleidea/mgmt/etcd/interfaces"
+	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
+
+	etcd "github.com/coreos/etcd/clientv3" // "clientv3"
+	pb "github.com/coreos/etcd/etcdserver/etcdserverpb"
+)
+
+// nominateApply applies the changed watcher data onto our local caches.
+func (obj *EmbdEtcd) nominateApply(data *interfaces.WatcherData) error {
+	if data == nil { // ignore empty data
+		return nil
+	}
+
+	// If we tried to lookup the nominated members here (in etcd v3) this
+	// would sometimes block because we would lose the cluster leader once
+	// the current leader calls the MemberAdd API and it steps down trying
+	// to form a two host cluster. Instead, we can look at the event
+	// response data to read the nominated values! Since we only see what
+	// has *changed* in the response data, we have to keep track of the
+	// original state and apply the deltas. This must be idempotent in case
+	// it errors and is called again. If we're retrying and we get a data
+	// format error, it's probably not the end of the world.
+	nominated, err := applyDeltaEvents(data, obj.nominated) // map[hostname]URLs (URLsMap)
+	if err != nil && err != errInconsistentApply {          // allow missing deletes
+		return err // unexpected error, fail
+	}
+	// TODO: do we want to sort this if it becomes a list instead of a map?
+	//sort.Strings(nominated) // deterministic order
+	obj.nominated = nominated
+	return nil
+}
+
+// volunteerApply applies the changed watcher data onto our local caches.
+func (obj *EmbdEtcd) volunteerApply(data *interfaces.WatcherData) error {
+	if data == nil { // ignore empty data
+		return nil
+	}
+	volunteers, err := applyDeltaEvents(data, obj.volunteers) // map[hostname]URLs (URLsMap)
+	if err != nil && err != errInconsistentApply {            // allow missing deletes
+		return err // unexpected error, fail
+	}
+	// TODO: do we want to sort this if it becomes a list instead of a map?
+	//sort.Strings(volunteers) // deterministic order
+	obj.volunteers = volunteers
+	return nil
+}
+
+// endpointApply applies the changed watcher data onto our local caches. In this
+// particular apply function, it also sets our client with the new endpoints.
+func (obj *EmbdEtcd) endpointApply(data *interfaces.WatcherData) error {
+	if data == nil { // ignore empty data
+		return nil
+	}
+	endpoints, err := applyDeltaEvents(data, obj.endpoints) // map[hostname]URLs (URLsMap)
+	if err != nil && err != errInconsistentApply {          // allow missing deletes
+		return err // unexpected error, fail
+	}
+
+	// is the endpoint list different?
+	if err := cmpURLsMap(obj.endpoints, endpoints); err != nil {
+		obj.endpoints = endpoints // set
+		// can happen if a server drops out for example
+		obj.Logf("endpoint list changed to: %+v", endpoints)
+		obj.setEndpoints()
+	}
+	return nil
+}
+
+// nominateCb runs to respond to the nomination list change events.
+// Functionally, it controls the starting and stopping of the server process. If
+// a nominate message is received for this machine, then it means it is already
+// being added to the cluster with member add and the cluster is now waiting for
+// it to start up. When a nominate entry is removed, it's up to this function to
+// run the member remove right before it shuts its server down.
+func (obj *EmbdEtcd) nominateCb(ctx context.Context) error {
+	// Ensure that only one copy of this function is run simultaneously.
+	// This is because we don't want to cause runServer to race with
+	// destroyServer. Let us completely start up before we can cancel it. As
+	// a special case, destroyServer itself can race against itself. I don't
+	// think it's possible for contention on this mutex, but we'll leave it
+	// in for safety.
+	obj.nominatedMutex.Lock()
+	defer obj.nominatedMutex.Unlock()
+	// This ordering mutex is being added for safety, since there is no good
+	// reason for this function and volunteerCb to run simultaneously, and
+	// it might be preventing a race condition that was happening.
+	obj.orderingMutex.Lock()
+	defer obj.orderingMutex.Unlock()
+	if obj.Debug {
+		obj.Logf("nominateCb")
+		defer obj.Logf("nominateCb: done!")
+	}
+
+	// check if i have actually volunteered first of all...
+	if obj.NoServer || len(obj.ServerURLs) == 0 {
+		obj.Logf("inappropriately nominated, rogue or stale server?")
+		// TODO: should we un-nominate ourself?
+		return nil // we've done our job successfully
+	}
+
+	// This can happen when we're shutting down, build the nominated value.
+	if len(obj.nominated) == 0 {
+		obj.Logf("list of nominations is empty")
+		//return nil // don't exit, we might want to shutdown the server
+	} else {
+		obj.Logf("nominated: %v", obj.nominated)
+	}
+
+	// if there are no other peers, we create a new server
+	// TODO: do we need an || len(obj.nominated) == 0 if we're the first?
+	_, exists := obj.nominated[obj.Hostname] // am i nominated?
+	newCluster := len(obj.nominated) == 1 && exists
+	if obj.Debug {
+		obj.Logf("nominateCb: newCluster: %t; exists: %t; obj.server == nil: %t", newCluster, exists, obj.server == nil)
+	}
+
+	// TODO: server start retries should be handled inside of runServer...
+	if obj.serverAction(serverActionStart) { // start
+		// no server is running, but it should be
+		wg := &sync.WaitGroup{}
+		serverReady, ackReady := obj.ServerReady()    // must call ack!
+		serverExited, ackExited := obj.ServerExited() // must call ack!
+
+		var sendError = false
+		var serverErr error
+		obj.Logf("waiting for server...")
+		nominated, err := copyURLsMap(obj.nominated)
+		if err != nil {
+			return err
+		}
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			obj.errExitN = make(chan struct{})
+			defer close(obj.errExitN) // multi-signal for errChan close op
+			// blocks until server exits
+			serverErr = obj.runServer(newCluster, nominated)
+			// in case this exits on its own instead of with destroy
+			defer obj.destroyServer()          // run to reset some values
+			if sendError && serverErr != nil { // exited with an error
+				select {
+				case obj.errChan <- errwrap.Wrapf(serverErr, "runServer errored"):
+				}
+			}
+		}()
+
+		// block until either server is ready or an early exit occurs
+		select {
+		case <-serverReady:
+			// detach from our local return of errors from an early
+			// server exit (pre server ready) and switch to channel
+			sendError = true // gets set before the ackReady() does
+			ackReady()       // must be called
+			ackExited()      // must be called
+			// pass
+
+		case <-serverExited:
+			ackExited() // must be called
+			ackReady()  // must be called
+
+			wg.Wait() // wait for server to finish to get early err
+			return serverErr
+		}
+
+		// Once the server is online, we *must* publish this information
+		// so that (1) others know where to connect to us (2) we provide
+		// an "event" for member add since there is not any event that's
+		// currently built-in to etcd and (3) so we have a key to expire
+		// when we shutdown or crash to give us the member remove event.
+		// please see issue: https://github.com/coreos/etcd/issues/5277
+
+	} else if obj.serverAction(serverActionStop) { // stop?
+		// server is running, but it should not be
+
+		// i have been un-nominated, remove self and shutdown server!
+		// we don't need to do a member remove if i'm the last one...
+		if len(obj.nominated) != 0 { // don't call if nobody left but me!
+			// work around: https://github.com/coreos/etcd/issues/5482
+			// and it might make sense to avoid it if we're the last
+			obj.Logf("member remove: removing self: %d", obj.serverID)
+			resp, err := obj.memberRemove(ctx, obj.serverID)
+			if err != nil {
+				if obj.Debug {
+					obj.Logf("error with member remove: %v", err)
+				}
+				return errwrap.Wrapf(err, "member self remove error")
+			}
+			if resp != nil {
+				obj.Logf("member removed (self): %s (%d)", obj.Hostname, obj.serverID)
+				if err := obj.updateMemberState(resp.Members); err != nil {
+					return err
+				}
+			}
+		}
+
+		// FIXME: if we fail on destroy should we try to run some of the
+		// other cleanup tasks that usually afterwards (below) anyways ?
+		if err := obj.destroyServer(); err != nil { // sync until exited
+			return errwrap.Wrapf(err, "destroyServer errored")
+		}
+
+		// We close with this special sentinel only during destroy/exit.
+		if obj.closing {
+			return interfaces.ErrShutdown
+		}
+	}
+
+	return nil
+}
+
+// volunteerCb runs to respond to the volunteer list change events.
+// Functionally, it controls the nominating and adding of members. It typically
+// nominates a peer so that it knows it will get to be a server, which causes it
+// to start up its server. It also runs the member add operation so that the
+// cluster gets quorum safely. The member remove operation is typically run in
+// the nominateCb of that server when it is asked to shutdown. This occurs when
+// the nominate entry for that server is removed. If a server removes its
+// volunteer entry we must respond by removing the nomination so that it can
+// receive that message and shutdown.
+// FIXME: we might need to respond to member change/disconnect/shutdown events,
+// see: https://github.com/coreos/etcd/issues/5277
+// XXX: Don't allow this function to partially run if it is canceled part way
+// through... We don't want an inconsistent state where we did unnominate, but
+// didn't remove a member...
+// XXX: If the leader changes, do we need to kick the volunteerCb or anything
+// else that might have required a leader and which returned because it did not
+// have one, thus loosing an event?
+func (obj *EmbdEtcd) volunteerCb(ctx context.Context) error {
+	// Ensure that only one copy of this function is run simultaneously.
+	// It's not entirely clear if this can ever happen or if it's needed,
+	// but it's an inexpensive safety check that we can add in for now.
+	obj.volunteerMutex.Lock()
+	defer obj.volunteerMutex.Unlock()
+	// This ordering mutex is being added for safety, since there is no good
+	// reason for this function and nominateCb to run simultaneously, and it
+	// might be preventing a race condition that was happening.
+	obj.orderingMutex.Lock()
+	defer obj.orderingMutex.Unlock()
+	if obj.Debug {
+		obj.Logf("volunteerCb")
+		defer obj.Logf("volunteerCb: done!")
+	}
+
+	// FIXME: are there any situations where we don't want to short circuit
+	// here, such as if i'm the last node?
+	if obj.server == nil {
+		if obj.Debug {
+			obj.Logf("i'm not a server yet...")
+		}
+		return nil // if i'm not a server, i'm not a leader, return
+	}
+
+	// FIXME: Instead of checking this, assume yes, and use the
+	// `WithRequireLeader` wrapper, and just ignore the error from that if
+	// it's wrong... Combined with events that poke this volunteerCb when
+	// the leader changes, we shouldn't miss any events...
+	if isLeader, err := obj.isLeader(ctx); err != nil { // XXX: race!
+		return errwrap.Wrapf(err, "error determining leader")
+	} else if !isLeader {
+		if obj.Debug {
+			obj.Logf("we are not the leader...")
+		}
+		return nil
+	}
+	// i am the leader!
+
+	// Remember that the member* operations return the membership, so this
+	// means we don't need to run an extra memberList in those scenarios...
+	// However, this can get out of sync easily, so ensure that our member
+	// information is very recent.
+	if err := obj.memberStateFromList(ctx); err != nil {
+		return errwrap.Wrapf(err, "error during state sync")
+	}
+	// XXX: If we have any unstarted members here, do we want to reschedule
+	// this volunteerCb in a moment? Or will we get another event anyways?
+
+	// NOTE: There used to be an is_leader check right here...
+	// FIXME: Should we use WithRequireLeader instead? Here? Elsewhere?
+	// https://godoc.org/github.com/coreos/etcd/clientv3#WithRequireLeader
+
+	// FIXME: can this happen, and if so, is it an error or a pass-through?
+	if len(obj.volunteers) == 0 {
+		obj.Logf("list of volunteers is empty")
+		//return fmt.Errorf("volunteer list is empty")
+	} else {
+		obj.Logf("volunteers: %+v", obj.volunteers)
+	}
+
+	// TODO: do we really need to check these errors?
+	m, err := copyURLsMap(obj.membermap) // list of members...
+	if err != nil {
+		return err
+	}
+	v, err := copyURLsMap(obj.volunteers)
+	if err != nil {
+		return err
+	}
+	// Unnominate anyone that unvolunteers, so they can shutdown cleanly...
+	// FIXME: one step at a time... do we trigger subsequent steps somehow?
+	obj.Logf("chooser: (%+v)/(%+v)", m, v)
+	nominate, unnominate, err := obj.Chooser.Choose(m, v)
+	if err != nil {
+		return errwrap.Wrapf(err, "chooser error")
+	}
+
+	// Ensure that we are the *last* in the list if we're unnominating, and
+	// the *first* in the list if we're nominating. This way, we self-remove
+	// last, and we self-add first. This is least likely to hurt quorum.
+	headFn := func(x string) bool {
+		return x != obj.Hostname
+	}
+	tailFn := func(x string) bool {
+		return x == obj.Hostname
+	}
+	nominate = util.PriorityStrSliceSort(nominate, headFn)
+	unnominate = util.PriorityStrSliceSort(unnominate, tailFn)
+	obj.Logf("chooser result(+/-): %+v/%+v", nominate, unnominate)
+	var reterr error
+	leaderCtx := ctx // default ctx to use
+	if RequireLeaderCtx {
+		leaderCtx = etcd.WithRequireLeader(ctx) // FIXME: Is this correct?
+	}
+
+	for i := range nominate {
+		member := nominate[i]
+		peerURLs, exists := obj.volunteers[member] // comma separated list of urls
+		if !exists {
+			// if this happens, do we have an update race?
+			return fmt.Errorf("could not find member `%s` in volunteers map", member)
+		}
+
+		// NOTE: storing peerURLs when they're already in volunteers/ is
+		// redundant, but it seems to be necessary for a sane algorithm.
+		// nominate before we call the API so that members see it first!
+		if err := obj.nominate(leaderCtx, member, peerURLs); err != nil {
+			return errwrap.Wrapf(err, "error nominating: %s", member)
+		}
+		// XXX: can we add a ttl here, because once we nominate someone,
+		// we need to give them up to N seconds to start up after we run
+		// the MemberAdd API because if they don't, in some situations
+		// such as if we're adding the second node to the cluster, then
+		// we've lost quorum until a second member joins! If the TTL
+		// expires, we need to MemberRemove! In this special case, we
+		// need to forcefully remove the second member if we don't add
+		// them, because we'll be in a lack of quorum state and unable
+		// to do anything... As a result, we should always only add ONE
+		// member at a time!
+
+		// XXX: After we memberAdd, can we wait a timeout, and then undo
+		// the add if the member doesn't come up? We'd also need to run
+		// an unnominate too, and mark the node as temporarily failed...
+		obj.Logf("member add: %s: %v", member, peerURLs)
+		resp, err := obj.memberAdd(leaderCtx, peerURLs)
+		if err != nil {
+			// FIXME: On on error this function needs to run again,
+			// because we need to make sure to add the member here!
+			return errwrap.Wrapf(err, "member add error")
+		}
+		if resp != nil { // if we're already the right state, we get nil
+			obj.Logf("member added: %s (%d): %v", member, resp.Member.ID, peerURLs)
+			if err := obj.updateMemberState(resp.Members); err != nil {
+				return err
+			}
+			if resp.Member.Name == "" { // not started instantly ;)
+				obj.addMemberState(member, resp.Member.ID, peerURLs, nil)
+			}
+			// TODO: would this ever happen or be necessary?
+			//if member == obj.Hostname {
+			//	obj.addSelfState()
+			//}
+		}
+	}
+
+	// we must remove them from the members API or it will look like a crash
+	if l := len(unnominate); l > 0 {
+		obj.Logf("unnominated: shutting down %d members...", l)
+	}
+	for i := range unnominate {
+		member := unnominate[i]
+		memberID, exists := obj.memberIDs[member] // map[string]uint64
+		if !exists {
+			// if this happens, do we have an update race?
+			return fmt.Errorf("could not find member `%s` in memberIDs map", member)
+		}
+
+		// start a watcher to know if member was added
+		cancelCtx, cancel := context.WithCancel(leaderCtx)
+		defer cancel()
+		timeout := util.CloseAfter(cancelCtx, SelfRemoveTimeout) // chan closes
+		fn := func(members []*pb.Member) error {
+			for _, m := range members {
+				if m.Name == member || m.ID == memberID {
+					return fmt.Errorf("still present")
+				}
+			}
+
+			return nil // not found!
+		}
+		ch, err := obj.memberChange(cancelCtx, fn, MemberChangeInterval)
+		if err != nil {
+			return errwrap.Wrapf(err, "error watching for change of: %s", member)
+		}
+		if err := obj.nominate(leaderCtx, member, nil); err != nil { // unnominate
+			return errwrap.Wrapf(err, "error unnominating: %s", member)
+		}
+		// Once we issue the above unnominate, that peer will
+		// shutdown, and this might cause us to loose quorum,
+		// therefore, let that member remove itself, and then
+		// double check that it did happen in case delinquent.
+		// TODO: get built-in transactional member Add/Remove
+		// functionality to avoid a separate nominate list...
+
+		// If we're removing ourself, then let the (un)nominate callback
+		// do it. That way it removes itself cleanly on server shutdown.
+		if member == obj.Hostname { // remove in unnominate!
+			cancel()
+			obj.Logf("unnominate: removing self...")
+			continue
+		}
+
+		// cancel remove sleep and unblock early on event...
+		obj.Logf("waiting %s for %s to self remove...", SelfRemoveTimeout.String(), member)
+		select {
+		case <-timeout:
+			// pass
+		case err, ok := <-ch:
+			if ok {
+				select {
+				case <-timeout:
+					// wait until timeout finishes
+				}
+				reterr = errwrap.Append(reterr, err)
+			}
+			// removed quickly!
+		}
+		cancel()
+
+		// In case the removed member doesn't remove itself, do it!
+		resp, err := obj.memberRemove(leaderCtx, memberID)
+		if err != nil {
+			return errwrap.Wrapf(err, "member remove error")
+		}
+		if resp != nil {
+			obj.Logf("member removed (forced): %s (%d)", member, memberID)
+			if err := obj.updateMemberState(resp.Members); err != nil {
+				return err
+			}
+			// Do this I guess, but the TTL will eventually get it.
+			// Remove the other member to avoid client connections.
+			if err := obj.advertise(leaderCtx, member, nil); err != nil {
+				return err
+			}
+		}
+
+		// Remove the member from our lists to avoid blocking future
+		// possible MemberList calls which would try and connect to a
+		// missing member... The lists should get updated from the
+		// member exiting safely if it doesn't crash, but if it did
+		// and/or since it's a race to see if the update event will get
+		// seen before we need the new data, just do it now anyways.
+		// TODO: Is the above comment still true?
+		obj.rmMemberState(member) // proactively delete it
+
+		obj.Logf("member %s (%d) removed successfully!", member, memberID)
+	}
+
+	// NOTE: We could ensure that etcd reconnects here, but we can just wait
+	// for the endpoints callback which should see the state change instead.
+
+	obj.setEndpoints() // sync client with new endpoints
+	return reterr
+}

etcd/chooser/chooser.go

@@ -0,0 +1,98 @@
+// Mgmt
+// Copyright (C) 2013-2019+ 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 chooser
+
+import (
+	"context"
+
+	"github.com/purpleidea/mgmt/etcd/interfaces"
+
+	etcdtypes "github.com/coreos/etcd/pkg/types"
+)
+
+// Data represents the input data that is passed to the chooser.
+type Data struct {
+	// Hostname is the hostname running this chooser instance. It can be
+	// used as a unique key in the cluster.
+	Hostname string // ourself
+
+	Debug bool
+	Logf  func(format string, v ...interface{})
+}
+
+// Chooser represents the interface you must implement if you want to be able to
+// control which cluster members are added and removed. Remember that this can
+// get run from any peer (server) machine in the cluster, and that this may
+// change as different leaders are elected! Do not assume any state will remain
+// between invocations. If you want to maintain hysteresis or state, make sure
+// to synchronize it in etcd.
+type Chooser interface {
+	// Validate validates the chooser implementation to ensure the params
+	// represent a valid instantiation.
+	Validate() error
+
+	// Init initializes the chooser and passes in some useful data and
+	// handles.
+	Init(*Data) error
+
+	// Connect will be called with a client interfaces.Client that you can
+	// use if necessary to store some shared state between instances of this
+	// and watch for external changes. Sharing state between members should
+	// be avoided if possible, and there is no guarantee that your data
+	// won't be deleted in a disaster. There are no backups for this,
+	// regenerate anything you might need. Additionally, this may only be
+	// used inside the Chooser method, since Connect is only called after
+	// Init. This is however very useful for implementing special choosers.
+	// Since some operations can run on connect, it gets a context. If you
+	// cancel this context, then you might expect that Watch could die too.
+	// Both of these should get cancelled if you call Disconnect.
+	Connect(context.Context, interfaces.Client) error // we get given a namespaced client
+
+	// Disconnect tells us to cancel our use of the client interface that we
+	// got from the Connect method. We must not return until we're done.
+	Disconnect() error
+
+	// Watch is called by the engine to allow us to Watch for changes that
+	// might cause us to want to re-evaluate our nomination decision. It
+	// should error if it cannot startup. Once it is running, it should send
+	// a nil error on every event, and an error if things go wrong. When
+	// Disconnect is shutdown, then that should cause this to exit. When
+	// this sends events, Choose will usually eventually get called in
+	// response.
+	Watch() (chan error, error)
+
+	// Choose takes the current peer membership state, and the available
+	// volunteers, and produces a list of who we should add and who should
+	// quit. In general, it's best to only remove one member at a time, in
+	// particular because this will get called iteratively on future events,
+	// and it can remove subsequent members on the next iteration. One
+	// important note: when building a new cluster, we do assume that out of
+	// one available volunteer, and no members, that this first volunteer is
+	// selected. Make sure that any implementations of this function do this
+	// as well, since otherwise the hardcoded initial assumption would be
+	// proven wrong here!
+	// TODO: we could pass in two lists of hostnames instead of the full
+	// URLsMap here, but let's keep it more complicated now in case, and
+	// reduce it down later if needed...
+	// TODO: should we add a step arg here ?
+	Choose(membership, volunteers etcdtypes.URLsMap) (nominees, quitters []string, err error)
+
+	// Close runs some cleanup routines in case there is anything that you'd
+	// like to free after we're done.
+	Close() error
+}

etcd/chooser/dynamicsize.go

@@ -0,0 +1,285 @@
+// Mgmt
+// Copyright (C) 2013-2019+ 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 chooser
+
+import (
+	"context"
+	"fmt"
+	"strconv"
+	"sync"
+
+	"github.com/purpleidea/mgmt/etcd/interfaces"
+
+	etcd "github.com/coreos/etcd/clientv3"
+	etcdtypes "github.com/coreos/etcd/pkg/types"
+)
+
+// XXX: Test causing cluster shutdowns with:
+// ETCDCTL_API=3 etcdctl --endpoints 127.0.0.1:2379 put /_mgmt/chooser/dynamicsize/idealclustersize 0
+// It is currently broken.
+
+const (
+	// DefaultIdealDynamicSize is the default target ideal dynamic cluster
+	// size used for the initial cluster.
+	DefaultIdealDynamicSize = 5
+
+	// IdealDynamicSizePath is the path key used for the chooser. It usually
+	// gets used with a namespace prefix.
+	IdealDynamicSizePath = "/dynamicsize/idealclustersize"
+)
+
+// DynamicSize is a simple implementation of the Chooser interface. This helps
+// select which machines to add and remove as we elastically grow and shrink our
+// cluster.
+// TODO: think of a better name
+type DynamicSize struct {
+	// IdealClusterSize is the ideal target size for this cluster. If it is
+	// set to zero, then it will use DefaultIdealDynamicSize as the value.
+	IdealClusterSize uint16
+
+	data   *Data // save for later
+	client interfaces.Client
+
+	ctx    context.Context
+	cancel func()
+	wg     *sync.WaitGroup
+}
+
+// Validate validates the struct.
+func (obj *DynamicSize) Validate() error {
+	// TODO: if changed to zero, treat as a cluster shutdown signal
+	if obj.IdealClusterSize < 0 {
+		return fmt.Errorf("must choose a positive IdealClusterSize value")
+	}
+	return nil
+}
+
+// Init accepts some useful data and handles.
+func (obj *DynamicSize) Init(data *Data) error {
+	if data.Hostname == "" {
+		return fmt.Errorf("can't Init with empty Hostname value")
+	}
+	if data.Logf == nil {
+		return fmt.Errorf("no Logf function was specified")
+	}
+
+	if obj.IdealClusterSize == 0 {
+		obj.IdealClusterSize = DefaultIdealDynamicSize
+	}
+
+	obj.data = data
+	obj.wg = &sync.WaitGroup{}
+	return nil
+}
+
+// Close runs some cleanup routines.
+func (obj *DynamicSize) Close() error {
+	return nil
+}
+
+// Connect is called to accept an etcd.KV namespace that we can use.
+func (obj *DynamicSize) Connect(ctx context.Context, client interfaces.Client) error {
+	obj.client = client
+	obj.ctx, obj.cancel = context.WithCancel(ctx)
+	size, err := DynamicSizeGet(obj.ctx, obj.client)
+	if err == interfaces.ErrNotExist || (err == nil && size <= 0) {
+		// unset, set in running cluster
+		changed, err := DynamicSizeSet(obj.ctx, obj.client, obj.IdealClusterSize)
+		if err == nil && changed {
+			obj.data.Logf("set dynamic cluster size to: %d", obj.IdealClusterSize)
+		}
+		return err
+	} else if err == nil && size >= 1 {
+		// unset, get from running cluster (use the valid cluster value)
+		if obj.IdealClusterSize != size {
+			obj.data.Logf("using dynamic cluster size of: %d", size)
+		}
+		obj.IdealClusterSize = size // get from exiting cluster...
+	}
+
+	return err
+}
+
+// Disconnect is called to cancel our use of the etcd.KV connection.
+func (obj *DynamicSize) Disconnect() error {
+	if obj.client != nil { // if connect was not called, don't call this...
+		obj.cancel()
+	}
+	obj.wg.Wait()
+	return nil
+}
+
+// Watch is called to send events anytime we might want to change membership. It
+// is also used to watch for changes so that when we get an event, we know to
+// honour the change in Choose.
+func (obj *DynamicSize) Watch() (chan error, error) {
+	// NOTE: The body of this function is very similar to the logic in the
+	// simple client.Watcher implementation that wraps ComplexWatcher.
+	path := IdealDynamicSizePath
+	cancelCtx, cancel := context.WithCancel(obj.ctx)
+	info, err := obj.client.ComplexWatcher(cancelCtx, path)
+	if err != nil {
+		defer cancel()
+		return nil, err
+	}
+	ch := make(chan error)
+	obj.wg.Add(1) // hook in to global wait group
+	go func() {
+		defer obj.wg.Done()
+		defer close(ch)
+		defer cancel()
+		var data *interfaces.WatcherData
+		var ok bool
+		for {
+			select {
+			case data, ok = <-info.Events: // read
+				if !ok {
+					return
+				}
+			case <-cancelCtx.Done():
+				continue // wait for ch closure, but don't block
+			}
+
+			size := obj.IdealClusterSize
+			for _, event := range data.Events { // apply each event
+				if event.Type != etcd.EventTypePut {
+					continue
+				}
+				key := string(event.Kv.Key)
+				key = key[len(data.Path):] // remove path prefix
+				val := string(event.Kv.Value)
+				if val == "" {
+					continue // ignore empty values
+				}
+				i, err := strconv.Atoi(val)
+				if err != nil {
+					continue // ignore bad values
+				}
+				size = uint16(i) // save
+			}
+			if size == obj.IdealClusterSize {
+				continue // no change
+			}
+			// set before sending the signal
+			obj.IdealClusterSize = size
+
+			if size == 0 { // zero means shutdown
+				obj.data.Logf("impending cluster shutdown...")
+			} else {
+				obj.data.Logf("got new dynamic cluster size of: %d", size)
+			}
+
+			select {
+			case ch <- data.Err: // send (might be nil!)
+			case <-cancelCtx.Done():
+				continue // wait for ch closure, but don't block
+			}
+		}
+	}()
+	return ch, nil
+}
+
+// Choose accepts a list of current membership, and a list of volunteers. From
+// that we can decide who we should add and remove. We return a list of those
+// nominated, and unnominated users respectively.
+func (obj *DynamicSize) Choose(membership, volunteers etcdtypes.URLsMap) ([]string, []string, error) {
+	// Possible nominees include anyone that has volunteered, but that
+	// isn't a member.
+	if obj.data.Debug {
+		obj.data.Logf("goal: %d members", obj.IdealClusterSize)
+	}
+	nominees := []string{}
+	for hostname := range volunteers {
+		if _, exists := membership[hostname]; !exists {
+			nominees = append(nominees, hostname)
+		}
+	}
+
+	// Possible quitters include anyone that is a member, but that is not a
+	// volunteer. (They must have unvolunteered.)
+	quitters := []string{}
+	for hostname := range membership {
+		if _, exists := volunteers[hostname]; !exists {
+			quitters = append(quitters, hostname)
+		}
+	}
+
+	// What we want to know...
+	nominated := []string{}
+	unnominated := []string{}
+
+	// We should always only add ONE member at a time!
+	// TODO: is it okay to remove multiple members at the same time?
+	if len(nominees) > 0 && len(membership)-len(quitters) < int(obj.IdealClusterSize) {
+		//unnominated = []string{} // only do one operation at a time
+		nominated = []string{nominees[0]} // FIXME: use a better picker algorithm
+
+	} else if len(quitters) == 0 && len(membership) > int(obj.IdealClusterSize) { // too many members
+		//nominated = []string{} // only do one operation at a time
+		for kicked := range membership {
+			// don't kick ourself unless we are the only one left...
+			if kicked != obj.data.Hostname || (obj.IdealClusterSize == 0 && len(membership) == 1) {
+				unnominated = []string{kicked} // FIXME: use a better picker algorithm
+				break
+			}
+		}
+	} else if len(quitters) > 0 { // must do these before new unvolunteers
+		unnominated = quitters // get rid of the quitters
+	}
+
+	return nominated, unnominated, nil // perform these changes
+}
+
+// DynamicSizeGet gets the currently set dynamic size set in the cluster.
+func DynamicSizeGet(ctx context.Context, client interfaces.Client) (uint16, error) {
+	key := IdealDynamicSizePath
+	m, err := client.Get(ctx, key) // (map[string]string, error)
+	if err != nil {
+		return 0, err
+	}
+	val, exists := m[IdealDynamicSizePath]
+	if !exists {
+		return 0, interfaces.ErrNotExist
+	}
+	i, err := strconv.Atoi(val)
+	if err != nil {
+		return 0, fmt.Errorf("bad value")
+	}
+	return uint16(i), nil
+}
+
+// DynamicSizeSet sets the dynamic size in the cluster. It returns true if it
+// changed or set the value.
+func DynamicSizeSet(ctx context.Context, client interfaces.Client, size uint16) (bool, error) {
+	key := IdealDynamicSizePath
+	val := strconv.FormatUint(uint64(size), 10) // fmt.Sprintf("%d", size)
+
+	ifCmps := []etcd.Cmp{
+		etcd.Compare(etcd.Value(key), "=", val), // desired state
+	}
+	elseOps := []etcd.Op{etcd.OpPut(key, val)}
+
+	resp, err := client.Txn(ctx, ifCmps, nil, elseOps)
+	if err != nil {
+		return false, err
+	}
+	// succeeded is set to true if the compare evaluated to true
+	changed := !resp.Succeeded
+
+	return changed, err
+}

etcd/client.go

@@ -1,94 +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 etcd
-
-import (
-	"time"
-
-	etcd "github.com/coreos/etcd/clientv3" // "clientv3"
-	errwrap "github.com/pkg/errors"
-	context "golang.org/x/net/context"
-)
-
-// ClientEtcd provides a simple etcd client for deploy and status operations.
-type ClientEtcd struct {
-	Seeds []string // list of endpoints to try to connect
-
-	client *etcd.Client
-}
-
-// GetClient returns a handle to the raw etcd client object.
-func (obj *ClientEtcd) GetClient() *etcd.Client {
-	return obj.client
-}
-
-// GetConfig returns the config struct to be used for the etcd client connect.
-func (obj *ClientEtcd) GetConfig() etcd.Config {
-	cfg := etcd.Config{
-		Endpoints: obj.Seeds,
-		// RetryDialer chooses the next endpoint to use
-		// it comes with a default dialer if unspecified
-		DialTimeout: 5 * time.Second,
-	}
-	return cfg
-}
-
-// Connect connects the client to a server, and then builds the *API structs.
-// If reconnect is true, it will force a reconnect with new config endpoints.
-func (obj *ClientEtcd) Connect() error {
-	if obj.client != nil { // memoize
-		return nil
-	}
-
-	var err error
-	cfg := obj.GetConfig()
-	obj.client, err = etcd.New(cfg) // connect!
-	if err != nil {
-		return errwrap.Wrapf(err, "client connect error")
-	}
-	return nil
-}
-
-// Destroy cleans up the entire etcd client connection.
-func (obj *ClientEtcd) Destroy() error {
-	err := obj.client.Close()
-	//obj.wg.Wait()
-	return err
-}
-
-// Get runs a get on the client connection. This has the same signature as our
-// EmbdEtcd Get function.
-func (obj *ClientEtcd) Get(path string, opts ...etcd.OpOption) (map[string]string, error) {
-	resp, err := obj.client.Get(context.TODO(), path, opts...)
-	if err != nil || resp == nil {
-		return nil, err
-	}
-
-	// TODO: write a resp.ToMap() function on https://godoc.org/github.com/coreos/etcd/etcdserver/etcdserverpb#RangeResponse
-	result := make(map[string]string)
-	for _, x := range resp.Kvs {
-		result[string(x.Key)] = string(x.Value)
-	}
-	return result, nil
-}
-
-// Txn runs a transaction on the client connection. This has the same signature
-// as our EmbdEtcd Txn function.
-func (obj *ClientEtcd) Txn(ifcmps []etcd.Cmp, thenops, elseops []etcd.Op) (*etcd.TxnResponse, error) {
-	return obj.client.KV.Txn(context.TODO()).If(ifcmps...).Then(thenops...).Else(elseops...).Commit()
-}

etcd/client/resources/resources.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -15,60 +15,43 @@
 // 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 etcd
+package resources
 
 import (
+	"context"
 	"fmt"
-	"log"
 	"strings"
 
 	"github.com/purpleidea/mgmt/engine"
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
+	"github.com/purpleidea/mgmt/etcd/interfaces"
 	"github.com/purpleidea/mgmt/util"
 
 	etcd "github.com/coreos/etcd/clientv3"
 )
 
+const (
+	ns = "" // in case we want to add one back in
+)
+
 // WatchResources returns a channel that outputs events when exported resources
 // change.
 // TODO: Filter our watch (on the server side if possible) based on the
 // collection prefixes and filters that we care about...
-func WatchResources(obj *EmbdEtcd) chan error {
-	ch := make(chan error, 1) // buffer it so we can measure it
-	path := fmt.Sprintf("%s/exported/", NS)
-	callback := func(re *RE) error {
-		// TODO: is this even needed? it used to happen on conn errors
-		log.Printf("Etcd: Watch: Path: %v", path) // event
-		if re == nil || re.response.Canceled {
-			return fmt.Errorf("watch is empty") // will cause a CtxError+retry
-		}
-		// we normally need to check if anything changed since the last
-		// event, since a set (export) with no changes still causes the
-		// watcher to trigger and this would cause an infinite loop. we
-		// don't need to do this check anymore because we do the export
-		// transactionally, and only if a change is needed. since it is
-		// atomic, all the changes arrive together which avoids dupes!!
-		if len(ch) == 0 { // send event only if one isn't pending
-			// this check avoids multiple events all queueing up and then
-			// being released continuously long after the changes stopped
-			// do not block!
-			ch <- nil // event
-		}
-		return nil
-	}
-	_, _ = obj.AddWatcher(path, callback, true, false, etcd.WithPrefix()) // no need to check errors
-	return ch
+func WatchResources(ctx context.Context, client interfaces.Client) (chan error, error) {
+	path := fmt.Sprintf("%s/exported/", ns)
+	return client.Watcher(ctx, path, etcd.WithPrefix())
 }
 
 // SetResources exports all of the resources which we pass in to etcd.
-func SetResources(obj *EmbdEtcd, hostname string, resourceList []engine.Res) error {
+func SetResources(ctx context.Context, client interfaces.Client, hostname string, resourceList []engine.Res) error {
 	// key structure is $NS/exported/$hostname/resources/$uid = $data
 
 	var kindFilter []string // empty to get from everyone
 	hostnameFilter := []string{hostname}
 	// this is not a race because we should only be reading keys which we
 	// set, and there should not be any contention with other hosts here!
-	originals, err := GetResources(obj, hostnameFilter, kindFilter)
+	originals, err := GetResources(ctx, client, hostnameFilter, kindFilter)
 	if err != nil {
 		return err
 	}
@@ -81,10 +64,10 @@ func SetResources(obj *EmbdEtcd, hostname string, resourceList []engine.Res) err
 	ops := []etcd.Op{}  // list of ops in this transaction
 	for _, res := range resourceList {
 		if res.Kind() == "" {
-			log.Fatalf("Etcd: SetResources: Error: Empty kind: %v", res.Name())
+			return fmt.Errorf("empty kind: %s", res.Name())
 		}
 		uid := fmt.Sprintf("%s/%s", res.Kind(), res.Name())
-		path := fmt.Sprintf("%s/exported/%s/resources/%s", NS, hostname, uid)
+		path := fmt.Sprintf("%s/exported/%s/resources/%s", ns, hostname, uid)
 		if data, err := engineUtil.ResToB64(res); err == nil {
 			ifs = append(ifs, etcd.Compare(etcd.Value(path), "=", data)) // desired state
 			ops = append(ops, etcd.OpPut(path, data))
@@ -106,10 +89,10 @@ func SetResources(obj *EmbdEtcd, hostname string, resourceList []engine.Res) err
 	// delete old, now unused resources here...
 	for _, res := range originals {
 		if res.Kind() == "" {
-			log.Fatalf("Etcd: SetResources: Error: Empty kind: %v", res.Name())
+			return fmt.Errorf("empty kind: %s", res.Name())
 		}
 		uid := fmt.Sprintf("%s/%s", res.Kind(), res.Name())
-		path := fmt.Sprintf("%s/exported/%s/resources/%s", NS, hostname, uid)
+		path := fmt.Sprintf("%s/exported/%s/resources/%s", ns, hostname, uid)
 
 		if match(res, resourceList) { // if we match, no need to delete!
 			continue
@@ -124,9 +107,9 @@ func SetResources(obj *EmbdEtcd, hostname string, resourceList []engine.Res) err
 	// it's important to do this in one transaction, and atomically, because
 	// this way, we only generate one watch event, and only when it's needed
 	if hasDeletes { // always run, ifs don't matter
-		_, err = obj.Txn(nil, ops, nil) // TODO: does this run? it should!
+		_, err = client.Txn(ctx, nil, ops, nil) // TODO: does this run? it should!
 	} else {
-		_, err = obj.Txn(ifs, nil, ops) // TODO: do we need to look at response?
+		_, err = client.Txn(ctx, ifs, nil, ops) // TODO: do we need to look at response?
 	}
 	return err
 }
@@ -136,11 +119,11 @@ func SetResources(obj *EmbdEtcd, hostname string, resourceList []engine.Res) err
 // TODO: Expand this with a more powerful filter based on what we eventually
 // support in our collect DSL. Ideally a server side filter like WithFilter()
 // We could do this if the pattern was $NS/exported/$kind/$hostname/$uid = $data.
-func GetResources(obj *EmbdEtcd, hostnameFilter, kindFilter []string) ([]engine.Res, error) {
+func GetResources(ctx context.Context, client interfaces.Client, hostnameFilter, kindFilter []string) ([]engine.Res, error) {
 	// key structure is $NS/exported/$hostname/resources/$uid = $data
-	path := fmt.Sprintf("%s/exported/", NS)
+	path := fmt.Sprintf("%s/exported/", ns)
 	resourceList := []engine.Res{}
-	keyMap, err := obj.Get(path, etcd.WithPrefix(), etcd.WithSort(etcd.SortByKey, etcd.SortAscend))
+	keyMap, err := client.Get(ctx, path, etcd.WithPrefix(), etcd.WithSort(etcd.SortByKey, etcd.SortAscend))
 	if err != nil {
 		return nil, fmt.Errorf("could not get resources: %v", err)
 	}
@@ -160,7 +143,9 @@ func GetResources(obj *EmbdEtcd, hostnameFilter, kindFilter []string) ([]engine.
 		if kind == "" {
 			return nil, fmt.Errorf("unexpected kind chunk")
 		}
-
+		if name == "" { // TODO: should I check this?
+			return nil, fmt.Errorf("unexpected empty name")
+		}
 		// FIXME: ideally this would be a server side filter instead!
 		if len(hostnameFilter) > 0 && !util.StrInList(hostname, hostnameFilter) {
 			continue
@@ -171,9 +156,9 @@ func GetResources(obj *EmbdEtcd, hostnameFilter, kindFilter []string) ([]engine.
 			continue
 		}
 
-		if obj, err := engineUtil.B64ToRes(val); err == nil {
-			log.Printf("Etcd: Get: (Hostname, Kind, Name): (%s, %s, %s)", hostname, kind, name)
-			resourceList = append(resourceList, obj)
+		if res, err := engineUtil.B64ToRes(val); err == nil {
+			//obj.Logf("Get: (Hostname, Kind, Name): (%s, %s, %s)", hostname, kind, name)
+			resourceList = append(resourceList, res)
 		} else {
 			return nil, fmt.Errorf("can't convert from B64: %v", err)
 		}

etcd/client/simple.go

@@ -0,0 +1,484 @@
+// Mgmt
+// Copyright (C) 2013-2019+ 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 client
+
+import (
+	"context"
+	"fmt"
+	"sync"
+	"time"
+
+	"github.com/purpleidea/mgmt/etcd/interfaces"
+	"github.com/purpleidea/mgmt/util/errwrap"
+
+	etcd "github.com/coreos/etcd/clientv3" // "clientv3"
+	"github.com/coreos/etcd/clientv3/namespace"
+)
+
+// method represents the method we used to build the simple client.
+type method uint8
+
+const (
+	methodError method = iota
+	methodSeeds
+	methodClient
+	methodNamespace
+)
+
+// NewClientFromSeeds builds a new simple client by connecting to a list of
+// seeds.
+func NewClientFromSeeds(seeds []string) *Simple {
+	return &Simple{
+		method: methodSeeds,
+		wg:     &sync.WaitGroup{},
+
+		seeds: seeds,
+	}
+}
+
+// NewClientFromSeedsNamespace builds a new simple client by connecting to a
+// list of seeds and ensuring all key access is prefixed with a namespace.
+func NewClientFromSeedsNamespace(seeds []string, ns string) *Simple {
+	return &Simple{
+		method: methodSeeds,
+		wg:     &sync.WaitGroup{},
+
+		seeds:     seeds,
+		namespace: ns,
+	}
+}
+
+// NewClientFromClient builds a new simple client by taking an existing client
+// struct. It does not disconnect this when Close is called, as that is up to
+// the parent, which is the owner of that client input struct.
+func NewClientFromClient(client *etcd.Client) *Simple {
+	return &Simple{
+		method: methodClient,
+		wg:     &sync.WaitGroup{},
+
+		client: client,
+	}
+}
+
+// NewClientFromNamespaceStr builds a new simple client by taking an existing
+// client and a string namespace. Warning, this doesn't properly nest the
+// namespaces.
+func NewClientFromNamespaceStr(client *etcd.Client, ns string) *Simple {
+	if client == nil {
+		return &Simple{
+			method: methodError,
+			err:    fmt.Errorf("client is nil"),
+		}
+	}
+	kv := client.KV
+	w := client.Watcher
+	if ns != "" { // only layer if not empty
+		kv = namespace.NewKV(client.KV, ns)
+		w = namespace.NewWatcher(client.Watcher, ns)
+	}
+
+	return &Simple{
+		method: methodClient, // similar enough to this one to share it!
+		wg:     &sync.WaitGroup{},
+
+		client: client, // store for GetClient()
+		kv:     kv,
+		w:      w,
+	}
+}
+
+// NewClientFromSimple builds a simple client from an existing client interface
+// which must be a simple client. This awkward method is required so that
+// namespace nesting works properly, because the *etcd.Client doesn't directly
+// pass through the namespace. I'd love to nuke this function, but it's good
+// enough for now.
+func NewClientFromSimple(client interfaces.Client, ns string) *Simple {
+	if client == nil {
+		return &Simple{
+			method: methodError,
+			err:    fmt.Errorf("client is nil"),
+		}
+	}
+
+	simple, ok := client.(*Simple)
+	if !ok {
+		return &Simple{
+			method: methodError,
+			err:    fmt.Errorf("client is not simple"),
+		}
+	}
+	kv := simple.kv
+	w := simple.w
+	if ns != "" { // only layer if not empty
+		kv = namespace.NewKV(simple.kv, ns)
+		w = namespace.NewWatcher(simple.w, ns)
+	}
+
+	return &Simple{
+		method: methodNamespace,
+		wg:     &sync.WaitGroup{},
+
+		client: client.GetClient(), // store for GetClient()
+		kv:     kv,
+		w:      w,
+	}
+}
+
+// NewClientFromNamespace builds a new simple client by taking an existing set
+// of interface API's that we might use.
+func NewClientFromNamespace(client *etcd.Client, kv etcd.KV, w etcd.Watcher) *Simple {
+	return &Simple{
+		method: methodNamespace,
+		wg:     &sync.WaitGroup{},
+
+		client: client, // store for GetClient()
+		kv:     kv,
+		w:      w,
+	}
+}
+
+// Simple provides a simple etcd client for deploy and status operations. You
+// can set Debug and Logf after you've built this with one of the NewClient*
+// methods.
+type Simple struct {
+	Debug bool
+	Logf  func(format string, v ...interface{})
+
+	method method
+	wg     *sync.WaitGroup
+
+	// err is the error we set when using methodError
+	err error
+
+	// seeds is the list of endpoints to try to connect to.
+	seeds     []string
+	namespace string
+
+	// client is the etcd client connection.
+	client *etcd.Client
+
+	// kv and w are the namespaced interfaces that we got passed.
+	kv etcd.KV
+	w  etcd.Watcher
+}
+
+// logf is a safe wrapper around the Logf parameter that doesn't panic if the
+// user didn't pass a logger in.
+func (obj *Simple) logf(format string, v ...interface{}) {
+	if obj.Logf == nil {
+		return
+	}
+	obj.Logf(format, v...)
+}
+
+// config returns the config struct to be used for the etcd client connect.
+func (obj *Simple) config() etcd.Config {
+	cfg := etcd.Config{
+		Endpoints: obj.seeds,
+		// RetryDialer chooses the next endpoint to use
+		// it comes with a default dialer if unspecified
+		DialTimeout: 5 * time.Second,
+	}
+	return cfg
+}
+
+// connect connects the client to a server, and then builds the *API structs.
+func (obj *Simple) connect() error {
+	if obj.client != nil { // memoize
+		return nil
+	}
+
+	var err error
+	cfg := obj.config()
+	obj.client, err = etcd.New(cfg) // connect!
+	if err != nil {
+		return errwrap.Wrapf(err, "client connect error")
+	}
+	obj.kv = obj.client.KV
+	obj.w = obj.client.Watcher
+	if obj.namespace != "" { // bonus feature of seeds method
+		obj.kv = namespace.NewKV(obj.client.KV, obj.namespace)
+		obj.w = namespace.NewWatcher(obj.client.Watcher, obj.namespace)
+	}
+	return nil
+}
+
+// Init starts up the struct.
+func (obj *Simple) Init() error {
+	// By the end of this, we must have obj.kv and obj.w available for use.
+	switch obj.method {
+	case methodError:
+		return obj.err // use the error we set
+
+	case methodSeeds:
+		if len(obj.seeds) <= 0 {
+			return fmt.Errorf("zero seeds")
+		}
+		return obj.connect()
+
+	case methodClient:
+		if obj.client == nil {
+			return fmt.Errorf("no client")
+		}
+		if obj.kv == nil { // overwrite if not specified!
+			obj.kv = obj.client.KV
+		}
+		if obj.w == nil {
+			obj.w = obj.client.Watcher
+		}
+		return nil
+
+	case methodNamespace:
+		if obj.kv == nil || obj.w == nil {
+			return fmt.Errorf("empty namespace")
+		}
+		return nil
+	}
+
+	return fmt.Errorf("unknown method: %+v", obj.method)
+}
+
+// Close cleans up the struct after we're finished.
+func (obj *Simple) Close() error {
+	defer obj.wg.Wait()
+	switch obj.method {
+	case methodError: // for consistency
+		return fmt.Errorf("did not Init")
+
+	case methodSeeds:
+		return obj.client.Close()
+
+	case methodClient:
+		// we we're given a client, so we don't own it or close it
+		return nil
+
+	case methodNamespace:
+		return nil
+	}
+
+	return fmt.Errorf("unknown method: %+v", obj.method)
+}
+
+// GetClient returns a handle to an open etcd Client. This is needed for certain
+// upstream API's that don't support passing in KV and Watcher instead.
+func (obj *Simple) GetClient() *etcd.Client {
+	return obj.client
+}
+
+// Set runs a set operation. If you'd like more information about whether a
+// value changed or not, use Txn instead.
+func (obj *Simple) Set(ctx context.Context, key, value string, opts ...etcd.OpOption) error {
+	// key is the full key path
+	resp, err := obj.kv.Put(ctx, key, value, opts...)
+	if obj.Debug {
+		obj.logf("set(%s): %v", key, resp) // bonus
+	}
+	return err
+}
+
+// Get runs a get operation.
+func (obj *Simple) Get(ctx context.Context, path string, opts ...etcd.OpOption) (map[string]string, error) {
+	resp, err := obj.kv.Get(ctx, path, opts...)
+	if err != nil {
+		return nil, err
+	}
+	if resp == nil {
+		return nil, fmt.Errorf("empty response")
+	}
+
+	// TODO: write a resp.ToMap() function on https://godoc.org/github.com/coreos/etcd/etcdserver/etcdserverpb#RangeResponse
+	result := make(map[string]string)
+	for _, x := range resp.Kvs {
+		result[string(x.Key)] = string(x.Value)
+	}
+	return result, nil
+}
+
+// Del runs a delete operation.
+func (obj *Simple) Del(ctx context.Context, path string, opts ...etcd.OpOption) (int64, error) {
+	resp, err := obj.kv.Delete(ctx, path, opts...)
+	if err == nil {
+		return resp.Deleted, nil
+	}
+	return -1, err
+}
+
+// Txn runs a transaction.
+func (obj *Simple) Txn(ctx context.Context, ifCmps []etcd.Cmp, thenOps, elseOps []etcd.Op) (*etcd.TxnResponse, error) {
+	resp, err := obj.kv.Txn(ctx).If(ifCmps...).Then(thenOps...).Else(elseOps...).Commit()
+	if obj.Debug {
+		obj.logf("txn: %v", resp) // bonus
+	}
+	return resp, err
+}
+
+// Watcher is a watcher that returns a chan of error's instead of a chan with
+// all sorts of watcher data. This is useful when we only want an event signal,
+// but we don't care about the specifics.
+func (obj *Simple) Watcher(ctx context.Context, path string, opts ...etcd.OpOption) (chan error, error) {
+	cancelCtx, cancel := context.WithCancel(ctx)
+	info, err := obj.ComplexWatcher(cancelCtx, path, opts...)
+	if err != nil {
+		defer cancel()
+		return nil, err
+	}
+	ch := make(chan error)
+	obj.wg.Add(1) // hook in to global wait group
+	go func() {
+		defer obj.wg.Done()
+		defer close(ch)
+		defer cancel()
+		var data *interfaces.WatcherData
+		var ok bool
+		for {
+			select {
+			case data, ok = <-info.Events: // read
+				if !ok {
+					return
+				}
+			case <-cancelCtx.Done():
+				continue // wait for ch closure, but don't block
+			}
+
+			select {
+			case ch <- data.Err: // send (might be nil!)
+			case <-cancelCtx.Done():
+				continue // wait for ch closure, but don't block
+			}
+		}
+	}()
+	return ch, nil
+}
+
+// ComplexWatcher is a more capable watcher that also returns data information.
+// This starts a watch request. It writes on a channel that you can follow to
+// know when an event or an error occurs. It always sends one startup event. It
+// will not return until the watch has been started. If it cannot start, then it
+// will return an error. Remember to add the WithPrefix() option if you want to
+// watch recursively.
+// TODO: do we need to support retry and changed client connections?
+// XXX: do we need to track last successful revision and retry from there?
+// XXX: if so, use:
+// lastRev := response.Header.Revision // TODO: +1 ?
+// etcd.WithRev(rev)
+func (obj *Simple) ComplexWatcher(ctx context.Context, path string, opts ...etcd.OpOption) (*interfaces.WatcherInfo, error) {
+	if obj.client == nil { // catch bugs, this often means programming error
+		return nil, fmt.Errorf("client is nil") // extra safety!
+	}
+	cancelCtx, cancel := context.WithCancel(ctx)
+	eventsChan := make(chan *interfaces.WatcherData) // channel of runtime errors
+
+	var count uint8
+	wg := &sync.WaitGroup{}
+
+	// TODO: if we can detect the use of WithCreatedNotify, we don't need to
+	// hard-code it down below... https://github.com/coreos/etcd/issues/9689
+	// XXX: proof of concept patch: https://github.com/coreos/etcd/pull/9705
+	//for _, op := range opts {
+	//	//if op.Cmp(etcd.WithCreatedNotify()) == nil { // would be best
+	//	if etcd.OpOptionCmp(op, etcd.WithCreatedNotify()) == nil {
+	//		count++
+	//		wg.Add(1)
+	//		break
+	//	}
+	//}
+	count++
+	wg.Add(1)
+
+	wOpts := []etcd.OpOption{
+		etcd.WithCreatedNotify(),
+	}
+	wOpts = append(wOpts, opts...)
+	var err error
+
+	obj.wg.Add(1) // hook in to global wait group
+	go func() {
+		defer obj.wg.Done()
+		defer close(eventsChan)
+		defer cancel() // it's safe to cancel() more than once!
+		ch := obj.w.Watch(cancelCtx, path, wOpts...)
+		for {
+			var resp etcd.WatchResponse
+			var ok bool
+			var created bool
+			select {
+			case resp, ok = <-ch:
+				if !ok {
+					if count > 0 { // closed before startup
+						// set err in parent scope!
+						err = fmt.Errorf("watch closed")
+						count--
+						wg.Done()
+					}
+					return
+				}
+
+				// the watch is now running!
+				if count > 0 && resp.Created {
+					created = true
+					count--
+					wg.Done()
+				}
+
+				isCanceled := resp.Canceled || resp.Err() == context.Canceled
+				// TODO: this might not be needed
+				if resp.Header.Revision == 0 { // by inspection
+					if obj.Debug {
+						obj.logf("watch: received empty message") // switched client connection
+					}
+					isCanceled = true
+				}
+
+				if isCanceled {
+					data := &interfaces.WatcherData{
+						Err: context.Canceled,
+					}
+					select { // send the error
+					case eventsChan <- data:
+					case <-ctx.Done():
+						return
+					}
+					continue // channel should close shortly
+				}
+			}
+
+			// TODO: consider processing the response data into a
+			// more useful form for the callback...
+			data := &interfaces.WatcherData{
+				Created: created,
+				Path:    path,
+				Header:  resp.Header,
+				Events:  resp.Events,
+				Err:     resp.Err(),
+			}
+
+			select { // send the event
+			case eventsChan <- data:
+			case <-ctx.Done():
+				return
+			}
+		}
+	}()
+
+	wg.Wait() // wait for created event before we return
+
+	return &interfaces.WatcherInfo{
+		Cancel: cancel,
+		Events: eventsChan,
+	}, err
+}

etcd/client/str/str.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ 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
@@ -15,19 +15,22 @@
 // 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 etcd
+package str
 
 import (
-	"errors"
+	"context"
 	"fmt"
 
+	"github.com/purpleidea/mgmt/etcd/interfaces"
+	"github.com/purpleidea/mgmt/util/errwrap"
+
 	etcd "github.com/coreos/etcd/clientv3"
-	errwrap "github.com/pkg/errors"
+	etcdutil "github.com/coreos/etcd/clientv3/clientv3util"
 )
 
-// ErrNotExist is returned when GetStr can not find the requested key.
-// TODO: https://dave.cheney.net/2016/04/07/constant-errors
-var ErrNotExist = errors.New("errNotExist")
+const (
+	ns = "" // in case we want to add one back in
+)
 
 // WatchStr returns a channel which spits out events on key activity.
 // FIXME: It should close the channel when it's done, and spit out errors when
@@ -36,37 +39,23 @@ var ErrNotExist = errors.New("errNotExist")
 // done, does that mean we leak go-routines since it might still be running, but
 // perhaps even blocked??? Could this cause a dead-lock? Should we instead return
 // some sort of struct which has a close method with it to ask for a shutdown?
-func WatchStr(obj *EmbdEtcd, key string) chan error {
+func WatchStr(ctx context.Context, client interfaces.Client, key string) (chan error, error) {
 	// new key structure is $NS/strings/$key = $data
-	path := fmt.Sprintf("%s/strings/%s", NS, key)
-	ch := make(chan error, 1)
-	// FIXME: fix our API so that we get a close event on shutdown.
-	callback := func(re *RE) error {
-		// TODO: is this even needed? it used to happen on conn errors
-		//log.Printf("Etcd: Watch: Path: %v", path) // event
-		if re == nil || re.response.Canceled {
-			return fmt.Errorf("watch is empty") // will cause a CtxError+retry
-		}
-		if len(ch) == 0 { // send event only if one isn't pending
-			ch <- nil // event
-		}
-		return nil
-	}
-	_, _ = obj.AddWatcher(path, callback, true, false, etcd.WithPrefix()) // no need to check errors
-	return ch
+	path := fmt.Sprintf("%s/strings/%s", ns, key)
+	return client.Watcher(ctx, path)
 }
 
 // GetStr collects the string which matches a global namespace in etcd.
-func GetStr(obj *EmbdEtcd, key string) (string, error) {
+func GetStr(ctx context.Context, client interfaces.Client, key string) (string, error) {
 	// new key structure is $NS/strings/$key = $data
-	path := fmt.Sprintf("%s/strings/%s", NS, key)
-	keyMap, err := obj.Get(path, etcd.WithPrefix())
+	path := fmt.Sprintf("%s/strings/%s", ns, key)
+	keyMap, err := client.Get(ctx, path, etcd.WithPrefix())
 	if err != nil {
 		return "", errwrap.Wrapf(err, "could not get strings in: %s", key)
 	}
 
 	if len(keyMap) == 0 {
-		return "", ErrNotExist
+		return "", interfaces.ErrNotExist
 	}
 
 	if count := len(keyMap); count != 1 {
@@ -78,23 +67,21 @@ func GetStr(obj *EmbdEtcd, key string) (string, error) {
 		return "", fmt.Errorf("path `%s` is missing", path)
 	}
 
-	//log.Printf("Etcd: GetStr(%s): %s", key, val)
 	return val, nil
 }
 
 // SetStr sets a key and hostname pair to a certain value. If the value is
 // nil, then it deletes the key. Otherwise the value should point to a string.
 // TODO: TTL or delete disconnect?
-func SetStr(obj *EmbdEtcd, key string, data *string) error {
+func SetStr(ctx context.Context, client interfaces.Client, key string, data *string) error {
 	// key structure is $NS/strings/$key = $data
-	path := fmt.Sprintf("%s/strings/%s", NS, key)
+	path := fmt.Sprintf("%s/strings/%s", ns, key)
 	ifs := []etcd.Cmp{} // list matching the desired state
 	ops := []etcd.Op{}  // list of ops in this transaction (then)
 	els := []etcd.Op{}  // list of ops in this transaction (else)
 	if data == nil {    // perform a delete
-		// TODO: use https://github.com/coreos/etcd/pull/7417 if merged
-		//ifs = append(ifs, etcd.KeyExists(path))
-		ifs = append(ifs, etcd.Compare(etcd.Version(path), ">", 0))
+		ifs = append(ifs, etcdutil.KeyExists(path))
+		//ifs = append(ifs, etcd.Compare(etcd.Version(path), ">", 0))
 		ops = append(ops, etcd.OpDelete(path))
 	} else {
 		data := *data                                                // get the real value
@@ -104,6 +91,6 @@ func SetStr(obj *EmbdEtcd, key string, data *string) error {
 
 	// it's important to do this in one transaction, and atomically, because
 	// this way, we only generate one watch event, and only when it's needed
-	_, err := obj.Txn(ifs, ops, els) // TODO: do we need to look at response?
+	_, err := client.Txn(ctx, ifs, ops, els) // TODO: do we need to look at response?
 	return errwrap.Wrapf(err, "could not set strings in: %s", key)
 }