resources: Improve AutoEdge API and pkg breakage

I previously broke the pkg auto edges because the package list wasn't
available by the time it was called. This fixes the pkg resource so that
it gets the necessary list of packages when needed. Since this means
that a possible failure could happen, we also update the AutoEdges API
to support errors. Errors can only be generated at AutoEdge struct
creation, once the struct has been returned (right before modification
of the graph structure) there is no possibility to return any errors.

It's important to remember that the AutoEdges stuff gets called because
the Init of each resource, so make sure it doesn't depend on anything
that happens there or that gets cached as a result of Init.

This is all much nicer now and has a test too :)

docs/resource-guide.md

@@ -379,7 +379,7 @@ if another resource can match a dependency to this one.
 
 ### AutoEdges
 ```golang
-AutoEdges() AutoEdge
+AutoEdges() (AutoEdge, error)
 ```
 
 This returns a struct that implements the `AutoEdge` interface. This struct

lib/main.go

@@ -513,9 +513,22 @@ func (obj *Main) Run() error {
 				}
 				continue
 			}
+
+			// TODO: should we call each Res.Setup() here instead?
+
+			// add autoedges; modifies the graph only if no error
+			if err := resources.AutoEdges(oldGraph); err != nil {
+				log.Printf("Main: Error running auto edges: %v", err)
+				// unpause!
+				if !first {
+					graph.Start(first) // sync
+					converger.Start()  // after Start()
+				}
+				continue
+			}
+
 			graph.Update(oldGraph) // copy in structure of new graph
 
-			resources.AutoEdges(graph.Graph)                                      // add autoedges; modifies the graph
 			resources.AutoGroup(graph.Graph, &resources.NonReachabilityGrouper{}) // run autogroup; modifies the graph
 			// TODO: do we want to do a transitive reduction?
 			// FIXME: run a type checker that verifies all the send->recv relationships

resources/augeas.go

@@ -248,11 +248,6 @@ type AugeasUID struct {
 	name string
 }
 
-// AutoEdges returns the AutoEdge interface. In this case no autoedges are used.
-func (obj *AugeasRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // UIDs includes all params to make a unique identification of this object.
 func (obj *AugeasRes) UIDs() []ResUID {
 	x := &AugeasUID{

resources/autoedge.go

@@ -23,6 +23,9 @@ import (
 
 	"github.com/purpleidea/mgmt/pgraph"
 	"github.com/purpleidea/mgmt/util"
+
+	multierr "github.com/hashicorp/go-multierror"
+	errwrap "github.com/pkg/errors"
 )
 
 // The AutoEdge interface is used to implement the autoedges feature.
@@ -56,7 +59,7 @@ func addEdgesByMatchingUIDS(g *pgraph.Graph, v pgraph.Vertex, uids []ResUID) []b
 				continue
 			}
 			if b, ok := g.Value("debug"); ok && util.Bool(b) {
-				log.Printf("Compile: AutoEdge: Match: %s with UID: %s", VtoR(vv).String(), uid)
+				log.Printf("Compile: AutoEdge: Match: %s with UID: %s", vv, uid)
 			}
 			// we must match to an effective UID for the resource,
 			// that is to say, the name value of a res is a helpful
@@ -65,12 +68,12 @@ func addEdgesByMatchingUIDS(g *pgraph.Graph, v pgraph.Vertex, uids []ResUID) []b
 			if UIDExistsInUIDs(uid, VtoR(vv).UIDs()) {
 				// add edge from: vv -> v
 				if uid.IsReversed() {
-					txt := fmt.Sprintf("AutoEdge: %s -> %s", VtoR(vv).String(), VtoR(v).String())
+					txt := fmt.Sprintf("AutoEdge: %s -> %s", vv, v)
 					log.Printf("Compile: Adding %s", txt)
 					edge := &Edge{Name: txt}
 					g.AddEdge(vv, v, edge)
 				} else { // edges go the "normal" way, eg: pkg resource
-					txt := fmt.Sprintf("AutoEdge: %s -> %s", VtoR(v).String(), VtoR(vv).String())
+					txt := fmt.Sprintf("AutoEdge: %s -> %s", v, vv)
 					log.Printf("Compile: Adding %s", txt)
 					edge := &Edge{Name: txt}
 					g.AddEdge(v, vv, edge)
@@ -85,22 +88,39 @@ func addEdgesByMatchingUIDS(g *pgraph.Graph, v pgraph.Vertex, uids []ResUID) []b
 }
 
 // AutoEdges adds the automatic edges to the graph.
-func AutoEdges(g *pgraph.Graph) {
+func AutoEdges(g *pgraph.Graph) error {
 	log.Println("Compile: Adding AutoEdges...")
-	for _, v := range g.Vertices() { // for each vertexes autoedges
+
+	// initially get all of the autoedges to seek out all possible errors
+	var err error
+	autoEdgeObjVertexMap := make(map[pgraph.Vertex]AutoEdge)
+
+	for _, v := range g.VerticesSorted() { // for each vertexes autoedges
 		if !VtoR(v).Meta().AutoEdge { // is the metaparam true?
 			continue
 		}
-		autoEdgeObj := VtoR(v).AutoEdges()
+		autoEdgeObj, e := VtoR(v).AutoEdges()
+		if e != nil {
+			err = multierr.Append(err, e) // collect all errors
+			continue
+		}
 		if autoEdgeObj == nil {
-			log.Printf("%s: Config: No auto edges were found!", VtoR(v).String())
+			log.Printf("%s: No auto edges were found!", v)
 			continue // next vertex
 		}
+		autoEdgeObjVertexMap[v] = autoEdgeObj // save for next loop
+	}
+	if err != nil {
+		return errwrap.Wrapf(err, "the auto edges had errors")
+	}
 
+	// now that we're guaranteed error free, we can modify the graph safely
+	// TODO: loop through this in a sorted order for stable log output...
+	for v, autoEdgeObj := range autoEdgeObjVertexMap {
 		for { // while the autoEdgeObj has more uids to add...
 			uids := autoEdgeObj.Next() // get some!
 			if uids == nil {
-				log.Printf("%s: Config: The auto edge list is empty!", VtoR(v).String())
+				log.Printf("%s: The auto edge list is empty!", v)
 				break // inner loop
 			}
 			if b, ok := g.Value("debug"); ok && util.Bool(b) {
@@ -119,4 +139,5 @@ func AutoEdges(g *pgraph.Graph) {
 			}
 		}
 	}
+	return nil
 }

resources/exec.go

@@ -335,10 +335,10 @@ type ExecUID struct {
 }
 
 // AutoEdges returns the AutoEdge interface. In this case no autoedges are used.
-func (obj *ExecRes) AutoEdges() AutoEdge {
+func (obj *ExecRes) AutoEdges() (AutoEdge, error) {
 	// TODO: parse as many exec params to look for auto edges, for example
 	// the path of the binary in the Cmd variable might be from in a pkg
-	return nil
+	return nil, nil
 }
 
 // UIDs includes all params to make a unique identification of this object.

resources/file.go

@@ -898,10 +898,11 @@ func (obj *FileResAutoEdges) Test(input []bool) bool {
 
 // AutoEdges generates a simple linear sequence of each parent directory from
 // the bottom up!
-func (obj *FileRes) AutoEdges() AutoEdge {
+func (obj *FileRes) AutoEdges() (AutoEdge, error) {
-	var data []ResUID                              // store linear result chain here...
+	var data []ResUID // store linear result chain here...
-	values := util.PathSplitFullReversed(obj.path) // build it
+	// build it, but don't use obj.path because this gets called before Init
-	_, values = values[0], values[1:]              // get rid of first value which is me!
+	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
 		data = append(data, &FileUID{
@@ -917,7 +918,7 @@ func (obj *FileRes) AutoEdges() AutoEdge {
 		data:    data,
 		pointer: 0,
 		found:   false,
-	}
+	}, nil
 }
 
 // UIDs includes all params to make a unique identification of this object.
@@ -925,7 +926,7 @@ func (obj *FileRes) AutoEdges() AutoEdge {
 func (obj *FileRes) UIDs() []ResUID {
 	x := &FileUID{
 		BaseUID: BaseUID{Name: obj.GetName(), Kind: obj.GetKind()},
-		path:    obj.path,
+		path:    obj.GetPath(), // not obj.path b/c we didn't init yet!
 	}
 	return []ResUID{x}
 }

resources/file_test.go

@@ -0,0 +1,79 @@
+// Mgmt
+// Copyright (C) 2013-2017+ 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 Affero 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 Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package resources
+
+import (
+	"testing"
+
+	"github.com/purpleidea/mgmt/pgraph"
+)
+
+func TestFileAutoEdge1(t *testing.T) {
+
+	g, err := pgraph.NewGraph("TestGraph")
+	if err != nil {
+		t.Errorf("error creating graph: %v", err)
+		return
+	}
+
+	r1 := &FileRes{
+		BaseRes: BaseRes{
+			Name: "file1",
+			Kind: "file",
+			MetaParams: MetaParams{
+				AutoEdge: true,
+			},
+		},
+		Path: "/tmp/a/b/", // some dir
+	}
+	r2 := &FileRes{
+		BaseRes: BaseRes{
+			Name: "file2",
+			Kind: "file",
+			MetaParams: MetaParams{
+				AutoEdge: true,
+			},
+		},
+		Path: "/tmp/a/", // some parent dir
+	}
+	r3 := &FileRes{
+		BaseRes: BaseRes{
+			Name: "file3",
+			Kind: "file",
+			MetaParams: MetaParams{
+				AutoEdge: true,
+			},
+		},
+		Path: "/tmp/a/b/c", // some child file
+	}
+	g.AddVertex(r1, r2, r3)
+
+	if i := g.NumEdges(); i != 0 {
+		t.Errorf("should have 0 edges instead of: %d", i)
+	}
+
+	// run artificially without the entire engine
+	if err := AutoEdges(g); err != nil {
+		t.Errorf("error running autoedges: %v", err)
+	}
+
+	// two edges should have been added
+	if i := g.NumEdges(); i != 2 {
+		t.Errorf("should have 2 edges instead of: %d", i)
+	}
+}

resources/graph.go

@@ -165,11 +165,6 @@ func (obj *GraphRes) UIDs() []ResUID {
 
 // XXX: hook up the autogrouping magic!
 
-// AutoEdges returns the AutoEdges. In this case none are used.
-func (obj *GraphRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // Compare two resources and return if they are equivalent.
 func (obj *GraphRes) Compare(r Res) bool {
 	// we can only compare GraphRes to others of the same resource kind

resources/hostname.go

@@ -228,11 +228,6 @@ type HostnameUID struct {
 	transientHostname string
 }
 
-// AutoEdges returns the AutoEdge interface. In this case no autoedges are used.
-func (obj *HostnameRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // UIDs includes all params to make a unique identification of this object.
 // Most resources only return one, although some resources can return multiple.
 func (obj *HostnameRes) UIDs() []ResUID {

resources/kv.go

@@ -226,11 +226,6 @@ type KVUID struct {
 	name string
 }
 
-// AutoEdges returns the AutoEdge interface. In this case no autoedges are used.
-func (obj *KVRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // UIDs includes all params to make a unique identification of this object.
 // Most resources only return one, although some resources can return multiple.
 func (obj *KVRes) UIDs() []ResUID {

resources/msg.go

@@ -204,11 +204,6 @@ func (obj *MsgRes) UIDs() []ResUID {
 	return []ResUID{x}
 }
 
-// AutoEdges returns the AutoEdges. In this case none are used.
-func (obj *MsgRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // Compare two resources and return if they are equivalent.
 func (obj *MsgRes) Compare(r Res) bool {
 	// we can only compare MsgRes to others of the same resource kind

resources/noop.go

@@ -94,11 +94,6 @@ type NoopUID struct {
 	name string
 }
 
-// AutoEdges returns the AutoEdge interface. In this case no autoedges are used.
-func (obj *NoopRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // UIDs includes all params to make a unique identification of this object.
 // Most resources only return one, although some resources can return multiple.
 func (obj *NoopRes) UIDs() []ResUID {

resources/nspawn.go

@@ -295,11 +295,6 @@ func (obj *NspawnRes) Compare(r Res) bool {
 	return true
 }
 
-// AutoEdges returns the AutoEdge interface. In this case no autoedges are used.
-func (obj *NspawnRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // UnmarshalYAML is the custom unmarshal handler for this struct.
 // It is primarily useful for setting the defaults.
 func (obj *NspawnRes) UnmarshalYAML(unmarshal func(interface{}) error) error {

resources/password.go

@@ -295,11 +295,6 @@ type PasswordUID struct {
 	name string
 }
 
-// AutoEdges returns the AutoEdge interface. In this case no autoedges are used.
-func (obj *PasswordRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // UIDs includes all params to make a unique identification of this object.
 // Most resources only return one, although some resources can return multiple.
 func (obj *PasswordRes) UIDs() []ResUID {

resources/pkg.go

@@ -72,31 +72,12 @@ func (obj *PkgRes) Init() error {
 		return err
 	}
 
-	bus := packagekit.NewBus()
+	if obj.fileList == nil {
-	if bus == nil {
+		if err := obj.populateFileList(); err != nil {
-		return fmt.Errorf("can't connect to PackageKit bus")
+			return errwrap.Wrapf(err, "error populating file list in init")
-	}
+		}
-	defer bus.Close()
-
-	result, err := obj.pkgMappingHelper(bus)
-	if err != nil {
-		return errwrap.Wrapf(err, "the pkgMappingHelper failed")
 	}
 
-	data, ok := result[obj.Name] // lookup single package (init does just one)
-	// package doesn't exist, this is an error!
-	if !ok || !data.Found {
-		return fmt.Errorf("can't find package named '%s'", obj.Name)
-	}
-
-	packageIDs := []string{data.PackageID} // just one for now
-	filesMap, err := bus.GetFilesByPackageID(packageIDs)
-	if err != nil {
-		return errwrap.Wrapf(err, "can't run GetFilesByPackageID")
-	}
-	if files, ok := filesMap[data.PackageID]; ok {
-		obj.fileList = util.DirifyFileList(files, false)
-	}
 	return nil
 }
 
@@ -223,6 +204,39 @@ func (obj *PkgRes) pkgMappingHelper(bus *packagekit.Conn) (map[string]*packageki
 	return result, nil
 }
 
+// populateFileList fills in the fileList structure with what is in the package.
+// TODO: should this work properly if pkg has been autogrouped ?
+func (obj *PkgRes) populateFileList() error {
+
+	bus := packagekit.NewBus()
+	if bus == nil {
+		return fmt.Errorf("can't connect to PackageKit bus")
+	}
+	defer bus.Close()
+
+	result, err := obj.pkgMappingHelper(bus)
+	if err != nil {
+		return errwrap.Wrapf(err, "the pkgMappingHelper failed")
+	}
+
+	data, ok := result[obj.Name] // lookup single package (init does just one)
+	// package doesn't exist, this is an error!
+	if !ok || !data.Found {
+		return fmt.Errorf("can't find package named '%s'", obj.Name)
+	}
+
+	packageIDs := []string{data.PackageID} // just one for now
+	filesMap, err := bus.GetFilesByPackageID(packageIDs)
+	if err != nil {
+		return errwrap.Wrapf(err, "can't run GetFilesByPackageID")
+	}
+	if files, ok := filesMap[data.PackageID]; ok {
+		obj.fileList = util.DirifyFileList(files, false)
+	}
+
+	return nil
+}
+
 // 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) {
@@ -419,10 +433,16 @@ func (obj *PkgResAutoEdges) Test(input []bool) bool {
 
 // AutoEdges produces an object which generates a minimal pkg file optimization
 // sequence of edges.
-func (obj *PkgRes) AutoEdges() AutoEdge {
+func (obj *PkgRes) AutoEdges() (AutoEdge, error) {
 	// in contrast with the FileRes AutoEdges() function which contains
 	// more of the mechanics, most of the AutoEdge mechanics for the PkgRes
-	// is contained in the Test() method! This design is completely okay!
+	// are contained in the Test() method! This design is completely okay!
+
+	if obj.fileList == nil {
+		if err := obj.populateFileList(); err != nil {
+			return nil, errwrap.Wrapf(err, "error populating file list for automatic edges")
+		}
+	}
 
 	// add matches for any svc resources found in pkg definition!
 	var svcUIDs []ResUID
@@ -444,7 +464,7 @@ func (obj *PkgRes) AutoEdges() AutoEdge {
 		testIsNext: false,         // start with Next() call
 		name:       obj.GetName(), // save data for PkgResAutoEdges obj
 		kind:       obj.GetKind(),
-	}
+	}, nil
 }
 
 // UIDs includes all params to make a unique identification of this object.
@@ -511,25 +531,6 @@ func (obj *PkgRes) Compare(r Res) bool {
 	return true
 }
 
-// ReturnSvcInFileList returns a list of svc names for matches like: `/usr/lib/systemd/system/*.service`.
-func ReturnSvcInFileList(fileList []string) []string {
-	result := []string{}
-	for _, x := range fileList {
-		dirname, basename := path.Split(path.Clean(x))
-		// TODO: do we also want to look for /etc/systemd/system/ ?
-		if dirname != "/usr/lib/systemd/system/" {
-			continue
-		}
-		if !strings.HasSuffix(basename, ".service") {
-			continue
-		}
-		if s := strings.TrimSuffix(basename, ".service"); !util.StrInList(s, result) {
-			result = append(result, s)
-		}
-	}
-	return result
-}
-
 // UnmarshalYAML is the custom unmarshal handler for this struct.
 // It is primarily useful for setting the defaults.
 func (obj *PkgRes) UnmarshalYAML(unmarshal func(interface{}) error) error {
@@ -549,3 +550,22 @@ func (obj *PkgRes) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	*obj = PkgRes(raw) // restore from indirection with type conversion!
 	return nil
 }
+
+// ReturnSvcInFileList returns a list of svc names for matches like: `/usr/lib/systemd/system/*.service`.
+func ReturnSvcInFileList(fileList []string) []string {
+	result := []string{}
+	for _, x := range fileList {
+		dirname, basename := path.Split(path.Clean(x))
+		// TODO: do we also want to look for /etc/systemd/system/ ?
+		if dirname != "/usr/lib/systemd/system/" {
+			continue
+		}
+		if !strings.HasSuffix(basename, ".service") {
+			continue
+		}
+		if s := strings.TrimSuffix(basename, ".service"); !util.StrInList(s, result) {
+			result = append(result, s)
+		}
+	}
+	return result
+}

resources/resources.go

@@ -157,7 +157,7 @@ type Res interface {
 	UIDs() []ResUID // most resources only return one
 	Watch() error   // send on channel to signal process() events
 	CheckApply(apply bool) (checkOK bool, err error)
-	AutoEdges() AutoEdge
+	AutoEdges() (AutoEdge, error)
 	Compare(Res) bool
 	CollectPattern(string) // XXX: temporary until Res collection is more advanced
 	//UnmarshalYAML(unmarshal func(interface{}) error) error // optional
@@ -463,6 +463,13 @@ func (obj *BaseRes) SetGroup(g []Res) {
 	obj.grouped = g
 }
 
+// AutoEdges returns the AutoEdge interface. By default, none are created. This
+// should be implemented by the specific resource to be used. This base method
+// does not need to be called by the resource specific implementing method.
+func (obj *BaseRes) AutoEdges() (AutoEdge, error) {
+	return nil, nil
+}
+
 // Compare is the base compare method, which also handles the metaparams cmp.
 func (obj *BaseRes) Compare(res Res) bool {
 	// TODO: should the AutoEdge values be compared?

resources/svc.go

@@ -380,7 +380,7 @@ func (obj *SvcResAutoEdges) Test(input []bool) bool {
 }
 
 // AutoEdges returns the AutoEdge interface. In this case the systemd units.
-func (obj *SvcRes) AutoEdges() AutoEdge {
+func (obj *SvcRes) AutoEdges() (AutoEdge, error) {
 	var data []ResUID
 	svcFiles := []string{
 		fmt.Sprintf("/etc/systemd/system/%s.service", obj.Name),     // takes precedence
@@ -401,7 +401,7 @@ func (obj *SvcRes) AutoEdges() AutoEdge {
 		data:    data,
 		pointer: 0,
 		found:   false,
-	}
+	}, nil
 }
 
 // UIDs includes all params to make a unique identification of this object.

resources/timer.go

@@ -129,11 +129,6 @@ func (obj *TimerRes) UIDs() []ResUID {
 	return []ResUID{x}
 }
 
-// AutoEdges returns the AutoEdge interface. In this case no autoedges are used.
-func (obj *TimerRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // Compare two resources and return if they are equivalent.
 func (obj *TimerRes) Compare(r Res) bool {
 	// we can only compare TimerRes to others of the same resource kind

resources/virt.go

@@ -1070,11 +1070,6 @@ func (obj *VirtRes) GroupCmp(r Res) bool {
 	return false // not possible atm
 }
 
-// AutoEdges returns the AutoEdge interface. In this case no autoedges are used.
-func (obj *VirtRes) AutoEdges() AutoEdge {
-	return nil
-}
-
 // Compare two resources and return if they are equivalent.
 func (obj *VirtRes) Compare(r Res) bool {
 	// we can only compare VirtRes to others of the same resource kind

yamlgraph/gconfig.go

@@ -128,6 +128,7 @@ func (c *GraphConfig) NewGraphFromConfig(hostname string, world resources.World,
 			if !ok {
 				return nil, fmt.Errorf("Config: Error: Can't convert: %v of type: %T to Res", x, x)
 			}
+			res.SetKind(kind) // cheap init
 			//if noop { // now done in mgmtmain
 			//	res.Meta().Noop = noop
 			//}
@@ -154,7 +155,6 @@ func (c *GraphConfig) NewGraphFromConfig(hostname string, world resources.World,
 			} else if !noop { // do not export any resources if noop
 				// store for addition to backend storage...
 				res.SetName(res.GetName()[2:]) //slice off @@
-				res.SetKind(kind)              // cheap init
 				resourceList = append(resourceList, res)
 			}
 		}