From 0f9b04534e89d34737c81241d019d9a678dc3b86 Mon Sep 17 00:00:00 2001 From: Christopher Obbard Date: Fri, 7 Jan 2022 11:17:12 +0000 Subject: [PATCH 1/4] commands: Rework newQemuHelper to bubble-up error instead of panicing Rather than panicing on error, bubble up an error so that the execution may recover correctly and the error shown to the user. Fixes: #413 Signed-off-by: Christopher Obbard --- commands.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/commands.go b/commands.go index 201c62aa..c6ac809f 100644 --- a/commands.go +++ b/commands.go @@ -210,7 +210,11 @@ func (cmd *Command) restoreResolvConf(sum *[sha256.Size]byte) error { } func (cmd Command) Run(label string, cmdline ...string) error { - q := newQemuHelper(cmd) + q, err := newQemuHelper(cmd) + if err != nil { + return err + } + q.Setup() defer q.Cleanup() @@ -284,11 +288,11 @@ type qemuHelper struct { qemutarget string } -func newQemuHelper(c Command) qemuHelper { +func newQemuHelper(c Command) (*qemuHelper, error) { q := qemuHelper{} if c.Chroot == "" || c.Architecture == "" { - return q + return &q, nil } switch c.Architecture { @@ -323,14 +327,14 @@ func newQemuHelper(c Command) qemuHelper { q.qemusrc = "/usr/bin/qemu-x86_64-static" } default: - log.Panicf("Don't know qemu for Architecture %s", c.Architecture) + return nil, fmt.Errorf("Don't know qemu for architecture %s", c.Architecture) } if q.qemusrc != "" { q.qemutarget = path.Join(c.Chroot, q.qemusrc) } - return q + return &q, nil } func (q qemuHelper) Setup() error { From a3fbde3b9f58deb6f6d91e694700c6f46ff3981e Mon Sep 17 00:00:00 2001 From: Christopher Obbard Date: Wed, 26 Jul 2023 14:34:18 +0100 Subject: [PATCH 2/4] Rework error handling to check context.State Currently the debos error handling sets an exitcode during the run, then exits with that code once execution has finished. This paradigm is quite fragile, so let's switch around the error handling to use context.State to track errors and check the state of this variable after the run has completed. While we are here, rename the checkError function to handleError to better describe the function's intent and make it return a boolean to show that an error has been handled. Rework the do_run function to also return a boolean to show that an error occurred during the run. Signed-off-by: Christopher Obbard --- cmd/debos/debos.go | 57 +++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/cmd/debos/debos.go b/cmd/debos/debos.go index d7e1b5aa..07d46177 100644 --- a/cmd/debos/debos.go +++ b/cmd/debos/debos.go @@ -15,18 +15,18 @@ import ( "github.com/jessevdk/go-flags" ) -func checkError(context *debos.DebosContext, err error, a debos.Action, stage string) int { +func handleError(context *debos.DebosContext, err error, a debos.Action, stage string) bool { if err == nil { - return 0 + return false } context.State = debos.Failed log.Printf("Action `%s` failed at stage %s, error: %s", a, stage, err) debos.DebugShell(*context) - return 1 + return true } -func do_run(r actions.Recipe, context *debos.DebosContext) int { +func do_run(r actions.Recipe, context *debos.DebosContext) bool { for _, a := range r.Actions { log.Printf("==== %s ====\n", a) err := a.Run(context) @@ -36,12 +36,12 @@ func do_run(r actions.Recipe, context *debos.DebosContext) int { defer a.Cleanup(context) // Check the state of Run method - if exitcode := checkError(context, err, a, "Run"); exitcode != 0 { - return exitcode + if handleError(context, err, a, "Run") { + return false } } - return 0 + return true } func warnLocalhost(variable string, value string) { @@ -89,11 +89,12 @@ func main() { "no_proxy", } - var exitcode int = 0 // Allow to run all deferred calls prior to os.Exit() - defer func() { - os.Exit(exitcode) - }() + defer func(context debos.DebosContext) { + if context.State == debos.Failed { + os.Exit(1) + } + }(context) parser := flags.NewParser(&options, flags.Default) fakemachineBackends := parser.FindOptionByLongName("fakemachine-backend") @@ -105,20 +106,20 @@ func main() { if ok && flagsErr.Type == flags.ErrHelp { return } else { - exitcode = 1 + context.State = debos.Failed return } } if len(args) != 1 { log.Println("No recipe given!") - exitcode = 1 + context.State = debos.Failed return } if options.DisableFakeMachine && options.Backend != "auto" { log.Println("--disable-fakemachine and --fakemachine-backend are mutually exclusive") - exitcode = 1 + context.State = debos.Failed return } @@ -141,12 +142,12 @@ func main() { r := actions.Recipe{} if _, err := os.Stat(file); os.IsNotExist(err) { log.Println(err) - exitcode = 1 + context.State = debos.Failed return } if err := r.Parse(file, options.PrintRecipe, options.Verbose, options.TemplateVars); err != nil { log.Println(err) - exitcode = 1 + context.State = debos.Failed return } @@ -170,7 +171,7 @@ func main() { if options.Backend == "auto" { runInFakeMachine = false } else { - exitcode = 1 + context.State = debos.Failed return } } @@ -234,7 +235,7 @@ func main() { for _, a := range r.Actions { err = a.Verify(&context) - if exitcode = checkError(&context, err, a, "Verify"); exitcode != 0 { + if handleError(&context, err, a, "Verify") { return } } @@ -254,7 +255,7 @@ func main() { memsize, err := units.RAMInBytes(options.Memory) if err != nil { fmt.Printf("Couldn't parse memory size: %v\n", err) - exitcode = 1 + context.State = debos.Failed return } m.SetMemory(int(memsize / 1024 / 1024)) @@ -269,7 +270,7 @@ func main() { size, err := units.FromHumanSize(options.ScratchSize) if err != nil { fmt.Printf("Couldn't parse scratch size: %v\n", err) - exitcode = 1 + context.State = debos.Failed return } m.SetScratch(size, "") @@ -311,14 +312,15 @@ func main() { defer a.PostMachineCleanup(&context) err = a.PreMachine(&context, m, &args) - if exitcode = checkError(&context, err, a, "PreMachine"); exitcode != 0 { + if handleError(&context, err, a, "PreMachine") { return } } - exitcode, err = m.RunInMachineWithArgs(args) + exitcode, err := m.RunInMachineWithArgs(args) if err != nil { fmt.Println(err) + context.State = debos.Failed return } @@ -329,7 +331,7 @@ func main() { for _, a := range r.Actions { err = a.PostMachine(&context) - if exitcode = checkError(&context, err, a, "Postmachine"); exitcode != 0 { + if handleError(&context, err, a, "Postmachine") { return } } @@ -344,7 +346,7 @@ func main() { defer a.PostMachineCleanup(&context) err = a.PreNoMachine(&context) - if exitcode = checkError(&context, err, a, "PreNoMachine"); exitcode != 0 { + if handleError(&context, err, a, "PreNoMachine") { return } } @@ -354,20 +356,19 @@ func main() { if _, err = os.Stat(context.Rootdir); os.IsNotExist(err) { err = os.Mkdir(context.Rootdir, 0755) if err != nil && os.IsNotExist(err) { - exitcode = 1 + context.State = debos.Failed return } } - exitcode = do_run(r, &context) - if exitcode != 0 { + if !do_run(r, &context) { return } if !fakemachine.InMachine() { for _, a := range r.Actions { err = a.PostMachine(&context) - if exitcode = checkError(&context, err, a, "PostMachine"); exitcode != 0 { + if handleError(&context, err, a, "PostMachine") { return } } From 64d043de2c6be544e0014ddd64b7d80d01a69be6 Mon Sep 17 00:00:00 2001 From: Christopher Obbard Date: Wed, 5 Jan 2022 10:48:07 +0000 Subject: [PATCH 3/4] =?UTF-8?q?Fix=20typo=20in=20stage=20name=20Postmachin?= =?UTF-8?q?e=E2=86=92PostMachine?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Christopher Obbard --- cmd/debos/debos.go | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/cmd/debos/debos.go b/cmd/debos/debos.go index 07d46177..53c71475 100644 --- a/cmd/debos/debos.go +++ b/cmd/debos/debos.go @@ -50,37 +50,36 @@ func warnLocalhost(variable string, value string) { Consider using an address that is valid on your network.` if strings.Contains(value, "localhost") || - strings.Contains(value, "127.0.0.1") || - strings.Contains(value, "::1") { + strings.Contains(value, "127.0.0.1") || + strings.Contains(value, "::1") { log.Printf(message, variable) } } - func main() { - context := debos.DebosContext { &debos.CommonContext{}, "", "" } + context := debos.DebosContext{&debos.CommonContext{}, "", ""} var options struct { - Backend string `short:"b" long:"fakemachine-backend" description:"Fakemachine backend to use" default:"auto"` - ArtifactDir string `long:"artifactdir" description:"Directory for packed archives and ostree repositories (default: current directory)"` - InternalImage string `long:"internal-image" hidden:"true"` - TemplateVars map[string]string `short:"t" long:"template-var" description:"Template variables (use -t VARIABLE:VALUE syntax)"` - DebugShell bool `long:"debug-shell" description:"Fall into interactive shell on error"` - Shell string `short:"s" long:"shell" description:"Redefine interactive shell binary (default: bash)" optionsl:"" default:"/bin/bash"` - ScratchSize string `long:"scratchsize" description:"Size of disk backed scratch space"` - CPUs int `short:"c" long:"cpus" description:"Number of CPUs to use for build VM (default: 2)"` - Memory string `short:"m" long:"memory" description:"Amount of memory for build VM (default: 2048MB)"` - ShowBoot bool `long:"show-boot" description:"Show boot/console messages from the fake machine"` - EnvironVars map[string]string `short:"e" long:"environ-var" description:"Environment variables (use -e VARIABLE:VALUE syntax)"` - Verbose bool `short:"v" long:"verbose" description:"Verbose output"` - PrintRecipe bool `long:"print-recipe" description:"Print final recipe"` - DryRun bool `long:"dry-run" description:"Compose final recipe to build but without any real work started"` - DisableFakeMachine bool `long:"disable-fakemachine" description:"Do not use fakemachine."` + Backend string `short:"b" long:"fakemachine-backend" description:"Fakemachine backend to use" default:"auto"` + ArtifactDir string `long:"artifactdir" description:"Directory for packed archives and ostree repositories (default: current directory)"` + InternalImage string `long:"internal-image" hidden:"true"` + TemplateVars map[string]string `short:"t" long:"template-var" description:"Template variables (use -t VARIABLE:VALUE syntax)"` + DebugShell bool `long:"debug-shell" description:"Fall into interactive shell on error"` + Shell string `short:"s" long:"shell" description:"Redefine interactive shell binary (default: bash)" optionsl:"" default:"/bin/bash"` + ScratchSize string `long:"scratchsize" description:"Size of disk backed scratch space"` + CPUs int `short:"c" long:"cpus" description:"Number of CPUs to use for build VM (default: 2)"` + Memory string `short:"m" long:"memory" description:"Amount of memory for build VM (default: 2048MB)"` + ShowBoot bool `long:"show-boot" description:"Show boot/console messages from the fake machine"` + EnvironVars map[string]string `short:"e" long:"environ-var" description:"Environment variables (use -e VARIABLE:VALUE syntax)"` + Verbose bool `short:"v" long:"verbose" description:"Verbose output"` + PrintRecipe bool `long:"print-recipe" description:"Print final recipe"` + DryRun bool `long:"dry-run" description:"Compose final recipe to build but without any real work started"` + DisableFakeMachine bool `long:"disable-fakemachine" description:"Do not use fakemachine."` } // These are the environment variables that will be detected on the // host and propagated to fakemachine. These are listed lower case, but // they are detected and configured in both lower case and upper case. - var environ_vars = [...]string { + var environ_vars = [...]string{ "http_proxy", "https_proxy", "ftp_proxy", @@ -331,7 +330,7 @@ func main() { for _, a := range r.Actions { err = a.PostMachine(&context) - if handleError(&context, err, a, "Postmachine") { + if handleError(&context, err, a, "PostMachine") { return } } From aa1e5a0e91a43e3ebeddb7d02488ab3d0e2a261a Mon Sep 17 00:00:00 2001 From: Christopher Obbard Date: Wed, 9 Mar 2022 11:25:38 +0000 Subject: [PATCH 4/4] debos: Improve error messages Rework the error handling to show where an error occured during the run. Also print all error messages to the log rather than stdout. Signed-off-by: Christopher Obbard --- cmd/debos/debos.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cmd/debos/debos.go b/cmd/debos/debos.go index 53c71475..165245fa 100644 --- a/cmd/debos/debos.go +++ b/cmd/debos/debos.go @@ -163,7 +163,7 @@ func main() { // attempt to create a fakemachine m, err = fakemachine.NewMachineWithBackend(options.Backend) if err != nil { - log.Printf("error creating fakemachine: %v", err) + log.Printf("Couldn't create fakemachine: %v", err) /* fallback to running on the host unless the user has chosen * a specific backend */ @@ -253,7 +253,7 @@ func main() { } memsize, err := units.RAMInBytes(options.Memory) if err != nil { - fmt.Printf("Couldn't parse memory size: %v\n", err) + log.Printf("Couldn't parse memory size: %v\n", err) context.State = debos.Failed return } @@ -268,7 +268,7 @@ func main() { if options.ScratchSize != "" { size, err := units.FromHumanSize(options.ScratchSize) if err != nil { - fmt.Printf("Couldn't parse scratch size: %v\n", err) + log.Printf("Couldn't parse scratch size: %v\n", err) context.State = debos.Failed return } @@ -318,12 +318,13 @@ func main() { exitcode, err := m.RunInMachineWithArgs(args) if err != nil { - fmt.Println(err) + log.Printf("Couldn't start fakemachine: %v\n", err) context.State = debos.Failed return } if exitcode != 0 { + log.Printf("fakemachine failed with non-zero exitcode: %d\n", exitcode) context.State = debos.Failed return } @@ -355,6 +356,7 @@ func main() { if _, err = os.Stat(context.Rootdir); os.IsNotExist(err) { err = os.Mkdir(context.Rootdir, 0755) if err != nil && os.IsNotExist(err) { + log.Printf("Couldn't create rootdir: %v\n", err) context.State = debos.Failed return }