Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor error handling #303

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactor error handling #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f9b045
commands: Rework newQemuHelper to bubble-up error instead of panicing
obbardc Jan 7, 2022
a3fbde3
Rework error handling to check context.State
obbardc Jul 26, 2023
64d043d
Fix typo in stage name Postmachine→PostMachine
obbardc Jan 5, 2022
aa1e5a0
debos: Improve error messages
obbardc Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 54 additions & 52 deletions cmd/debos/debos.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mention "this paradigm is quite fragile" but all this change seems to do is rather then having a local variable track the error state (in the form of error code) you moved it to marking it in the context. I'm not really sure why that's less fragile ;)

it does make the code a big more clear though so seems reasonable

Original file line number Diff line number Diff line change
Expand Up @@ -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)
obbardc marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand All @@ -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) {
Expand All @@ -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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these don't look like typo fixes

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"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more spurious changes that aren't typo fixes

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",
Expand All @@ -89,11 +88,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")
Expand All @@ -105,20 +105,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
}

Expand All @@ -141,12 +141,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
}

Expand All @@ -163,14 +163,14 @@ 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 */
if options.Backend == "auto" {
runInFakeMachine = false
} else {
exitcode = 1
context.State = debos.Failed
return
}
}
Expand Down Expand Up @@ -234,7 +234,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
}
}
Expand All @@ -253,8 +253,8 @@ func main() {
}
memsize, err := units.RAMInBytes(options.Memory)
if err != nil {
fmt.Printf("Couldn't parse memory size: %v\n", err)
exitcode = 1
log.Printf("Couldn't parse memory size: %v\n", err)
obbardc marked this conversation as resolved.
Show resolved Hide resolved
context.State = debos.Failed
return
}
m.SetMemory(int(memsize / 1024 / 1024))
Expand All @@ -268,8 +268,8 @@ func main() {
if options.ScratchSize != "" {
size, err := units.FromHumanSize(options.ScratchSize)
if err != nil {
fmt.Printf("Couldn't parse scratch size: %v\n", err)
exitcode = 1
log.Printf("Couldn't parse scratch size: %v\n", err)
context.State = debos.Failed
return
}
m.SetScratch(size, "")
Expand Down Expand Up @@ -311,25 +311,27 @@ 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)
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
}

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
}
}
Expand All @@ -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
}
}
Expand All @@ -354,20 +356,20 @@ 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
log.Printf("Couldn't create rootdir: %v\n", err)
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
}
}
Expand Down
14 changes: 9 additions & 5 deletions commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down