Improve logging and output formatting with CommandSet

cmd/create.go

@@ -38,7 +38,7 @@ var createCmd = &cobra.Command{
 names or categories. Multiple arguments are supported.`,
 	Run: func(_ *cobra.Command, args []string) {
 		conts := config.Union(args, contMask)
-		getAndExecute("CREATE", conts, func(c *container.Container) command.Commands { return c.CreateCommands() })
+		getAndExecute(conts, func(c *container.Container) command.CommandSet { return c.CreateCommands() })
 	},
 }
 

cmd/execute.go

@@ -22,23 +22,26 @@ THE SOFTWARE.
 package cmd
 
 import (
-	"fmt"
 	"sync"
 
 	"gitea.elkins.co/Networking/ccl/internal/pkg/command"
 	"gitea.elkins.co/Networking/ccl/internal/pkg/container"
+	"github.com/sirupsen/logrus"
 )
 
-func getAndExecute(action string, tgts []container.Container, getCmds func(*container.Container) command.Commands) {
+func getAndExecute(tgts []container.Container, getSet func(*container.Container) command.CommandSet) {
 	var wg sync.WaitGroup
 	for i := range tgts {
-		fmt.Fprintln(output, action, tgts[i].Name)
 		wg.Add(1)
 		go func(cont *container.Container) {
 			defer wg.Done()
-			for _, cmd := range getCmds(cont) {
+			set := getSet(cont)
-				if err := cmd.Execute(output, fake); err != nil {
+			for _, cmd := range set.Commands {
-					cont.LogEntry().WithField("error", err).Errorln("Could not", action)
+				if err := cmd.Execute(output, fake, set.ID); err != nil {
+					cont.LogEntry().WithFields(logrus.Fields{
+						"error": err,
+						"action": set.ID,
+					}).Errorln("Failed")
 					return
 				}
 			}

cmd/pull.go

@@ -39,7 +39,7 @@ affected: the old image will still remain, though untagged, and any defined cont
 will still use it.`,
 	Run: func(cmd *cobra.Command, args []string) {
 		conts := config.Union(args, contMask)
-		getAndExecute("PULL", conts, func(c *container.Container) command.Commands { return c.PullCommands() })
+		getAndExecute(conts, func(c *container.Container) command.CommandSet { return c.PullCommands() })
 	},
 }
 

cmd/recreate.go

@@ -38,7 +38,7 @@ var recreateCmd = &cobra.Command{
 one or more container names or categories. If empty, "all" is assumed.`,
 	Run: func(_ *cobra.Command, args []string) {
 		conts := config.Union(args, contMask)
-		getAndExecute("RECREATE", conts, func(c *container.Container) command.Commands { return c.RecreateCommands() })
+		getAndExecute(conts, func(c *container.Container) command.CommandSet { return c.RecreateCommands() })
 	},
 }
 

cmd/restart.go

@@ -38,7 +38,7 @@ var restartCmd = &cobra.Command{
 one or more container names or categories. If empty, "all" is assumed.`,
 	Run: func(_ *cobra.Command, args []string) {
 		conts := config.Union(args, contMask)
-		getAndExecute("RESTART", conts, func(c *container.Container) command.Commands { return c.RestartCommands() })
+		getAndExecute(conts, func(c *container.Container) command.CommandSet { return c.RestartCommands() })
 	},
 }
 

cmd/rm.go

@@ -39,7 +39,7 @@ var rmCmd = &cobra.Command{
 If running, they will first be stopped.`,
 	Run: func(cmd *cobra.Command, args []string) {
 		conts := config.Union(args, contMask)
-		getAndExecute("REMOVE", conts, func(c *container.Container) command.Commands { return c.DestroyCommands() })
+		getAndExecute(conts, func(c *container.Container) command.CommandSet { return c.RemoveCommands() })
 	},
 }
 

cmd/start.go

@@ -38,7 +38,7 @@ var startCmd = &cobra.Command{
 one or more container names or categories. If empty, "all" is assumed.`,
 	Run: func(_ *cobra.Command, args []string)  {
 		conts := config.Union(args, contMask)
-		getAndExecute("START", conts, func(c *container.Container) command.Commands { return c.StartCommands() })
+		getAndExecute(conts, func(c *container.Container) command.CommandSet { return c.StartCommands() })
 	},
 }
 

cmd/stop.go

@@ -38,7 +38,7 @@ var stopCmd = &cobra.Command{
 one or more container names or categories. If empty, "all" is assumed.`,
 	Run: func(_ *cobra.Command, args []string) {
 		conts := config.Union(args, contMask)
-		getAndExecute("STOP", conts, func(c *container.Container) command.Commands { return c.StopCommands() })
+		getAndExecute(conts, func(c *container.Container) command.CommandSet { return c.StopCommands() })
 	},
 }
 

cmd/update.go

@@ -38,7 +38,7 @@ var updateCmd = &cobra.Command{
 one or more container names or categories. If empty, "all" is assumed.`,
 	Run: func(_ *cobra.Command, args []string) {
 		conts := config.Union(args, contMask)
-		getAndExecute("UPDATE", conts, func(c *container.Container) command.Commands { return c.UpdateCommands() })
+		getAndExecute(conts, func(c *container.Container) command.CommandSet { return c.UpdateCommands() })
 	},
 }
 

internal/pkg/command/command.go

@@ -25,15 +25,13 @@ import (
 	"fmt"
 	"io"
 	"os/exec"
-
-	log "github.com/sirupsen/logrus"
 )
 
 type CommandType int
 
 const (
 	CT_NOP CommandType = iota
-	CT_SH
+	CT_SHELL
 	CT_FUNC
 	CT_INDIRECT
 	CT_SET
@@ -45,7 +43,7 @@ func (ct CommandType) String() string {
 	switch ct {
 	case CT_NOP:
 		return "NOP"
-	case CT_SH:
+	case CT_SHELL:
 		return "SHELL"
 	case CT_FUNC:
 		return "FUNC"
@@ -68,6 +66,11 @@ type Command struct {
 }
 type Commands []Command
 
+type CommandSet struct{
+	ID string
+	Commands
+}
+
 type errFunc struct {
 	Name string
 	Func func() error
@@ -81,7 +84,7 @@ type conditional struct {
 }
 
 func NewShell(cmd string) Command {
-	return Command{CT_SH, cmd}
+	return Command{CT_SHELL, cmd}
 }
 
 func NewFunc(name string, f func() error) Command {
@@ -92,8 +95,8 @@ func NewIndirect(c Command) Command {
 	return Command{CT_INDIRECT, c}
 }
 
-func NewSet(cs []Command) Command {
+func NewSet(cs CommandSet) Command {
-	return Command{CT_SET, cs}
+	return Command{CT_SET, cs.Commands}
 }
 
 func NewDebug(msg string) Command {
@@ -113,33 +116,22 @@ func NewConditional(name string, ifPart func() bool, thenPart Command, elsePart
 	}}
 }
 
-func (c Command) GetShell() (string, error) {
+func (cmds CommandSet) Execute(output io.Writer, fake bool) error {
-	s, ok := c.Command.(string)
+	for _, c := range cmds.Commands {
-	if ok {
+		if err := c.Execute(output, fake, cmds.ID); err != nil {
-		return s, nil
-	}
-	return s, fmt.Errorf("Type error. Requested = %s, Type = %s", CT_SH, c.Type)
-}
-
-func (cmds Commands) Execute(output io.Writer, fake bool) error {
-	for _, c := range cmds {
-		if err := c.Execute(output, fake); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-func (c Command) Execute(output io.Writer, fake bool) error {
+func (c Command) Execute(output io.Writer, fake bool, commandSetID string) error {
 	switch c.Type {
 	case CT_NOP:
-		fmt.Fprintln(output, c.Type)
+		fmt.Fprintf(output, "%s: %s\n", commandSetID, c.Type)
-	case CT_SH:
+	case CT_SHELL:
-		cmd, err := c.GetShell()
+		cmd := c.Command.(string)
-		if err != nil {
+		fmt.Fprintf(output, "%s: %s sh -c \"%s\"\n", commandSetID, c.Type, cmd)
-			return err
-		}
-		fmt.Fprintf(output, "%s sh -c \"%s\"\n", c.Type, cmd)
 		if !fake {
 			out, err := exec.Command("sh", "-c", cmd).CombinedOutput()
 			fmt.Fprint(output, string(out))
@@ -147,11 +139,7 @@ func (c Command) Execute(output io.Writer, fake bool) error {
 		}
 	case CT_FUNC:
 		ef := c.Command.(errFunc)
-		log.WithFields(log.Fields{
+		fmt.Fprintf(output, "%s: %s %s\n", commandSetID, c.Type, ef.Name)
-			"type": CT_FUNC.String(),
-			"func": ef.Name,
-		}).Debugf("Calling")
-		fmt.Fprintln(output, c.Type, ef.Name)
 		if !fake {
 			if err := ef.Func(); err != nil {
 				return err
@@ -159,50 +147,39 @@ func (c Command) Execute(output io.Writer, fake bool) error {
 			return nil
 		}
 	case CT_INDIRECT:
-		ct, ok := c.Command.(Command)
+		ct := c.Command.(Command)
-		if !ok {
+		return ct.Execute(output, fake, commandSetID)
-			return fmt.Errorf("Type error: Requested = %s, Type = %s", CT_INDIRECT, c.Type)
-		}
-		return ct.Execute(output, fake)
 	case CT_SET:
-		cs, ok := c.Command.([]Command)
+		cs := c.Command.(Commands)
-		if !ok {
-			return fmt.Errorf("Type error: Requested = %s, Type = %s", CT_SET, c.Type)
-		}
 		for i := range cs {
-			err := cs[i].Execute(output, fake)
+			err := cs[i].Execute(output, fake, commandSetID)
 			if err != nil {
 				return err
 			}
 		}
 	case CT_CONDITIONAL:
-		cond, ok := c.Command.(conditional)
+		cond := c.Command.(conditional)
-		if !ok {
-			return fmt.Errorf("Type error: Requested = %s, Type = %s", CT_CONDITIONAL, c.Type)
-		}
 		if fake {
-			fmt.Fprintln(output, c.Type, cond.Name)
+			// in a fake setting, we don't know which branch will be followed,
-			fmt.Fprintln(output, "-- True branch")
+			// so show what we would do in either branch
-			cond.ThenCmd.Execute(output, fake)
+			fmt.Fprintf(output, "%s: %s %s\n", commandSetID, c.Type, cond.Name)
-			fmt.Fprintln(output, "-- False branch")
+			fmt.Fprintf(output, "%s: -- True branch\n", commandSetID)
-			cond.ElseCmd.Execute(output, fake)
+			cond.ThenCmd.Execute(output, fake, commandSetID)
-			fmt.Fprintln(output, "-- End conditional")
+			fmt.Fprintf(output, "%s: -- False branch\n", commandSetID)
-			return nil
+			cond.ElseCmd.Execute(output, fake, commandSetID)
+			fmt.Fprintf(output, "%s: -- End conditional\n", commandSetID)
 		} else {
 			branch := cond.Condition()
-			fmt.Fprintln(output, c.Type, cond.Name, ":", branch)
+			fmt.Fprintf(output, "%s: %s %s: %v\n", commandSetID, c.Type, cond.Name, branch)
 			if branch {
-				return cond.ThenCmd.Execute(output, fake)
+				return cond.ThenCmd.Execute(output, fake, commandSetID)
 			} else {
-				return cond.ElseCmd.Execute(output, fake)
+				return cond.ElseCmd.Execute(output, fake, commandSetID)
 			}
 		}
 	case CT_DEBUG:
-		msg, ok := c.Command.(string)
+		msg := c.Command.(string)
-		if !ok {
+		fmt.Fprintf(output, "%s: %s %s\n", commandSetID, c.Type, msg)
-			return fmt.Errorf("Type error: Requested = %s, Type = %s", CT_DEBUG, c.Type)
-		}
-		fmt.Fprintln(output, c.Type, msg)
 	}
 	return nil
 }

internal/pkg/container/container.go

@@ -27,7 +27,7 @@ import (
 	"net"
 	"os/exec"
 
-	"gitea.elkins.co/Networking/ccl/internal/pkg/command"
+	cmd "gitea.elkins.co/Networking/ccl/internal/pkg/command"
 	"gitea.elkins.co/Networking/ccl/internal/pkg/network"
 	"github.com/containers/common/libnetwork/types"
 	"github.com/containers/podman/v4/libpod/define"
@@ -44,7 +44,7 @@ type Container struct {
 	Name      string            `toml:"name"`
 	Image     string            `toml:"image"`
 	Hostname  string            `toml:"hostname,omitempty"`
-	Command   []string          `toml:"command,omitempty"`
+	Command   []string          `toml:"cmd,omitempty"`
 	Arguments string            `toml:"arguments,omitempty"`
 	Networks  []network.Network `toml:"networks,omitempty"`
 	Env       map[string]string `toml:"env,omitempty"`
@@ -114,21 +114,28 @@ func (c *Container) pull() error {
 	return err
 }
 
-func (c *Container) PullCommands() command.Commands {
+func (c *Container) newCommandSet(op string, cmds cmd.Commands) cmd.CommandSet {
-	return command.Commands{
+	return cmd.CommandSet {
-		command.NewFunc("do_pull", func() error {
+		ID: fmt.Sprintf("%s-%s", op, c.Name),
-			return c.pull()
+		Commands: cmds,
-		}),
 	}
 }
 
-func (c *Container) CreateCommands() command.Commands {
+func (c *Container) PullCommands() cmd.CommandSet {
+	return c.newCommandSet("PULL", cmd.Commands{
+		cmd.NewFunc("do_pull", func() error {
+			return c.pull()
+		}),
+	})
+}
+
+func (c *Container) CreateCommands() cmd.CommandSet {
 	if c.Image == "" {
-		return command.Commands{
+		return c.newCommandSet("CREATE", cmd.Commands{
-			command.NewFunc("image_error", func() error {
+			cmd.NewFunc("image_error", func() error {
 				return fmt.Errorf("Image not configured")
 			}),
-		}
+		})
 	}
 	sysctl := map[string]string{}
 	nets := map[string]types.PerNetworkOptions{}
@@ -176,8 +183,8 @@ func (c *Container) CreateCommands() command.Commands {
 		},
 	}
 
-	return command.Commands{
+	return c.newCommandSet("CREATE", cmd.Commands{
-		command.NewFunc("bail_if_exists", func() error {
+		cmd.NewFunc("bail_if_exists", func() error {
 			if ex, err := containers.Exists(c.conn, c.Name, &containers.ExistsOptions{}); err != nil || ex {
 				if err != nil {
 					return err
@@ -186,7 +193,7 @@ func (c *Container) CreateCommands() command.Commands {
 			}
 			return nil
 		}),
-		command.NewFunc("pull_if_necessary", func() error {
+		cmd.NewFunc("pull_if_necessary", func() error {
 			if ex, err := images.Exists(c.conn, c.Image, &images.ExistsOptions{}); err != nil || !ex {
 				if err != nil {
 					return err
@@ -195,51 +202,61 @@ func (c *Container) CreateCommands() command.Commands {
 			}
 			return nil
 		}),
-		command.NewFunc("validate_spec", spec.Validate),
+		cmd.NewFunc("validate_spec", spec.Validate),
-		command.NewFunc("do_create", func() error {
+		cmd.NewFunc("do_create", func() error {
 			_, err := containers.CreateWithSpec(c.conn, &spec, nil)
 			if err != nil {
 				return err
 			}
 			return c.populateCData()
 		}),
-	}
+	})
 }
 
-func (c *Container) RecreateCommands() command.Commands {
+func (c *Container) RecreateCommands() cmd.CommandSet {
 	wasRunning := false
-	return command.Commands{
+	return c.newCommandSet("RECREATE", cmd.Commands{
-		command.NewFunc("stash_run_state", func() error {
+		cmd.NewFunc("stash_run_state", func() error {
 			wasRunning = c.IsRunning()
 			return nil
 		}),
-		command.NewSet(c.DestroyCommands()),
+		cmd.NewSet(c.StopCommands()),
-		command.NewSet(c.CreateCommands()),
+		cmd.NewSet(c.removeCommands()),
-		command.NewConditional("start_if_was_running",
+		cmd.NewSet(c.CreateCommands()),
+		cmd.NewConditional("start_if_was_running",
 			func() bool { return wasRunning },
-			command.NewSet(c.StartCommands()),
+			cmd.NewSet(c.StartCommands()),
-			command.NewNop(),
+			cmd.NewNop(),
 		),
-	}
+	})
 }
 
-func (c *Container) DestroyCommands() command.Commands {
+func (c *Container) RemoveCommands() cmd.CommandSet {
-	cmds := c.StopCommands()
+	return c.newCommandSet("REMOVE", cmd.Commands{
-	cmds = append(cmds, command.NewFunc("remove_if_exists", func() error {
+		cmd.NewSet(c.StopCommands()),
-		if c.cdata.ID == "" {
+		cmd.NewSet(c.removeCommands()),
-			return nil
+	})
-		}
-		yes := true
-		_, err := containers.Remove(c.conn, c.cdata.ID, &containers.RemoveOptions{Force: &yes})
-		return err
-	}))
-	return cmds
 }
 
-func (c *Container) StartCommands() command.Commands {
+// unexported version just removes the container without attempting a stop.
-	return command.Commands{
+func (c *Container) removeCommands() cmd.CommandSet {
-		command.NewFunc("start_container", func() error {
+	return c.newCommandSet("remove", cmd.Commands{
-			if c.cdata.State != nil && c.cdata.State.Running {
+		cmd.NewFunc("remove_if_exists", func() error {
+			if c.cdata.ID == "" {
+				return nil
+			}
+			yes := true
+			_, err := containers.Remove(c.conn, c.cdata.ID, &containers.RemoveOptions{Force: &yes})
+			return err
+		}),
+	})
+}
+
+func (c *Container) StartCommands() cmd.CommandSet {
+	return c.newCommandSet("START", cmd.Commands{
+		cmd.NewFunc("start_container", func() error {
+			if c.IsRunning() {
+				c.LogEntry().Debugln("Container start was commanded but it is already running. Not a problem.")
 				return nil
 			}
 			err := containers.Start(c.conn, c.cdata.ID, nil)
@@ -256,18 +273,18 @@ func (c *Container) StartCommands() command.Commands {
 			}
 			err = c.assureNetNS()
 			if err != nil {
-				return err
+				c.LogEntry().WithField("error", err).Warnln("Failed to create network namespace")
 			}
 			return nil
 		}),
-	}
+	})
 }
 
-func (c *Container) RestartCommands() command.Commands {
+func (c *Container) RestartCommands() cmd.CommandSet {
-	return command.Commands{
+	return c.newCommandSet("RESTART", cmd.Commands{
-		command.NewSet(c.StopCommands()),
+		cmd.NewSet(c.StopCommands()),
-		command.NewSet(c.StartCommands()),
+		cmd.NewSet(c.StartCommands()),
-	}
+	})
 }
 
 func (c *Container) IsRunning() bool {
@@ -284,54 +301,51 @@ func (c *Container) IsCreated() bool {
 	return true
 }
 
-func (c *Container) UpdateCommands() command.Commands {
+func (c *Container) UpdateCommands() cmd.CommandSet {
 	wasRunning := false
-	return command.Commands{
+	return c.newCommandSet("UPDATE", cmd.Commands{
-		command.NewFunc("pull_and_stop", func() error {
+		cmd.NewFunc("pull_image", func() error {
 			err := c.pull()
 			if err != nil {
 				return err
 			}
 			wasRunning = c.cdata != nil && c.cdata.State != nil && c.cdata.State.Running
-			if wasRunning {
-				var timeout uint = 10
-				_ = containers.Stop(c.conn, c.cdata.ID, &containers.StopOptions{Timeout: &timeout})
-			}
 			return nil
 		}),
-		command.NewSet(c.DestroyCommands()),
+		cmd.NewSet(c.StopCommands()),
-		command.NewSet(c.CreateCommands()),
+		cmd.NewSet(c.removeCommands()),
-		command.NewConditional("restart_if_was_running",
+		cmd.NewSet(c.CreateCommands()),
+		cmd.NewConditional("restart_if_was_running",
 			func() bool { return wasRunning },
-			command.NewSet(c.StartCommands()),
+			cmd.NewSet(c.StartCommands()),
-			command.NewNop(),
+			cmd.NewNop(),
 		),
-	}
+	})
 }
 
-func (c *Container) StopCommands() command.Commands {
+func (c *Container) StopCommands() cmd.CommandSet {
-	return command.Commands{
+	return c.newCommandSet("STOP", cmd.Commands{
-		command.NewFunc("stop_if_running", func() error {
+		cmd.NewFunc("stop_if_running", func() error {
-			if c.IsRunning() {
+			if !c.IsRunning() {
-				var timeout uint = 10
+				c.LogEntry().Debugln("Container stop was commanded but it wasn't running. Not a problem.")
-				err := containers.Stop(c.conn, c.cdata.ID, &containers.StopOptions{Timeout: &timeout})
+				return nil
-				if err != nil {
-					return err
-				}
-				_, err = containers.Wait(c.conn, c.cdata.ID, &containers.WaitOptions{Condition: []define.ContainerStatus{define.ContainerStateExited}})
-				if err != nil {
-					return err
-				}
-				return c.populateCData()
 			}
-			c.LogEntry().Debugf("Container stopped but wasn't running. Not a problem.")
+			var timeout uint = 10
-			return nil
+			err := containers.Stop(c.conn, c.cdata.ID, &containers.StopOptions{Timeout: &timeout})
+			if err != nil {
+				return err
+			}
+			_, err = containers.Wait(c.conn, c.cdata.ID, &containers.WaitOptions{Condition: []define.ContainerStatus{define.ContainerStateExited}})
+			if err != nil {
+				return err
+			}
+			return c.populateCData()
 		}),
-	}
+	})
 }
 
 func (c *Container) populateCData() error {
-	// TODO: locking
+	// TODO: locking?
 	var err error
 	no := false
 	c.cdata, err = containers.Inspect(c.conn, c.Name, &containers.InspectOptions{Size: &no})