From b54977d61be3be492ebe68a99686371931f1c2f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20M=C3=ADchal?= Date: Thu, 25 Jun 2020 15:11:00 +0200 Subject: [PATCH 1/2] pkg/podman: Add and use package-wide error variables Using Golang errors with dynamic strings is not very good because it's very hard to check them. It is much easier to declare package-wide errors based on which printed error messages are generated. This makes out job to check for expected/unexpected errors much easier (even though there's a bit more code for checking and acting upon the different errors). --- src/cmd/rm.go | 15 +++++++- src/cmd/rmi.go | 15 +++++++- src/cmd/root.go | 4 +-- src/pkg/podman/podman.go | 76 +++++++++++++++++++++++++++------------- 4 files changed, 82 insertions(+), 28 deletions(-) diff --git a/src/cmd/rm.go b/src/cmd/rm.go index f20cc8b49..baa9aba7e 100644 --- a/src/cmd/rm.go +++ b/src/cmd/rm.go @@ -111,8 +111,21 @@ func rm(cmd *cobra.Command, args []string) error { } for _, container := range args { + if exists, err := podman.ContainerExists(container); !exists { + if errors.Is(err, podman.ErrContainerNotExist) { + fmt.Fprintf(os.Stderr, "Error: container %s does not exist\n", container) + } else { + fmt.Fprintf(os.Stderr, "Error: %s\n", err) + } + continue + } + if _, err := podman.IsToolboxContainer(container); err != nil { - fmt.Fprintf(os.Stderr, "Error: %s\n", err) + if errors.Is(err, podman.ErrContainerNotToolbox) { + fmt.Fprintf(os.Stderr, "Error: container %s is not a toolbox container\n", container) + } else { + fmt.Fprintf(os.Stderr, "Error: %s\n", err) + } continue } diff --git a/src/cmd/rmi.go b/src/cmd/rmi.go index b7a40c0ee..186ca8e40 100644 --- a/src/cmd/rmi.go +++ b/src/cmd/rmi.go @@ -113,8 +113,21 @@ func rmi(cmd *cobra.Command, args []string) error { } for _, image := range args { + if exists, err := podman.ImageExists(image); !exists { + if errors.Is(err, podman.ErrImageNotExist) { + fmt.Fprintf(os.Stderr, "Error: image %s does not exist\n", image) + } else { + fmt.Fprintf(os.Stderr, "Error: %s\n", err) + } + continue + } + if _, err := podman.IsToolboxImage(image); err != nil { - fmt.Fprintf(os.Stderr, "Error: %s\n", err) + if errors.Is(err, podman.ErrImageNotToolbox) { + fmt.Fprintf(os.Stderr, "Error: image %s is not a toolbox image\n", image) + } else { + fmt.Fprintf(os.Stderr, "Error: %s\n", err) + } continue } diff --git a/src/cmd/root.go b/src/cmd/root.go index 1735aa472..15ac706fa 100644 --- a/src/cmd/root.go +++ b/src/cmd/root.go @@ -216,7 +216,7 @@ func migrate() error { podmanVersion, err := podman.GetVersion() if err != nil { - return fmt.Errorf("failed to get the Podman version") + return fmt.Errorf("failed to get the Podman version: %w", err) } logrus.Debugf("Current Podman version is %s", podmanVersion) @@ -274,7 +274,7 @@ func migrate() error { } if err = podman.SystemMigrate(""); err != nil { - return fmt.Errorf("failed to migrate containers") + return fmt.Errorf("failed to migrate containers: %w", err) } logrus.Debugf("Migration to Podman version %s was ok", podmanVersion) diff --git a/src/pkg/podman/podman.go b/src/pkg/podman/podman.go index ea46e7899..bde771964 100644 --- a/src/pkg/podman/podman.go +++ b/src/pkg/podman/podman.go @@ -19,6 +19,7 @@ package podman import ( "bytes" "encoding/json" + "errors" "fmt" "io" @@ -33,6 +34,33 @@ var ( var ( LogLevel = logrus.ErrorLevel + + // ErrContainerIsRunning means a running container cannot be removed + ErrContainerIsRunning = errors.New("can't remove a running container") + // ErrGetContainers means 'podman ps' failed to get all containers + ErrGetContainers = errors.New("failed to get containers") + // ErrGetImages means 'podman images' failed to get all images + ErrGetImages = errors.New("failed to get images") + // ErrGetVersion means 'podman version' failed to get version of Podman + ErrGetVersion = errors.New("failed to get version of Podman") + // ErrImageHasChildren means an image with dependent children cannot be removed + ErrImageHasChildren = errors.New("can't remove an image with dependent children") + // ErrInspect means 'podman inspect' failed to inspect a container/image + ErrInspect = errors.New("failed to inspect container/image") + // ErrNotExist means a container/image does not exist + ErrNotExist = errors.New("failed to find container/image") + // ErrNotToolbox means the selected container/image is not a toolbox container/image + ErrNotToolbox = errors.New("container/image is not a toolbox container/image") + // ErrPullImage means 'podman pull' failed to pull an image + ErrPullImage = errors.New("failed to pull an image") + // ErrRemoveContainer means 'podman rm' failed to remove a container + ErrRemoveContainer = errors.New("failed to remove container") + // ErrRemoveImage means 'podman rmi' failed to remove an image + ErrRemoveImage = errors.New("failed to remove image") + // ErrStartContainer means a container failed to start when started with 'podman start' + ErrStartContainer = errors.New("failed to start a container") + // ErrSystemMigrate means 'podman system migrate' failed + ErrSystemMigrate = errors.New("system migration failed") ) // CheckVersion compares provided version with the version of Podman. @@ -58,7 +86,7 @@ func ContainerExists(container string) (bool, error) { exitCode, err := shell.RunWithExitCode("podman", nil, nil, nil, args...) if exitCode != 0 && err == nil { - err = fmt.Errorf("failed to find container %s", container) + err = ErrNotExist } if err != nil { @@ -82,14 +110,14 @@ func GetContainers(args ...string) ([]map[string]interface{}, error) { args = append([]string{"--log-level", logLevelString, "ps", "--format", "json"}, args...) if err := shell.Run("podman", nil, &stdout, nil, args...); err != nil { - return nil, err + return nil, fmt.Errorf("%s: %w", ErrGetContainers, err) } output := stdout.Bytes() var containers []map[string]interface{} if err := json.Unmarshal(output, &containers); err != nil { - return nil, err + return nil, fmt.Errorf("%s: %w", ErrGetContainers, err) } return containers, nil @@ -108,14 +136,14 @@ func GetImages(args ...string) ([]map[string]interface{}, error) { logLevelString := LogLevel.String() args = append([]string{"--log-level", logLevelString, "images", "--format", "json"}, args...) if err := shell.Run("podman", nil, &stdout, nil, args...); err != nil { - return nil, err + return nil, fmt.Errorf("%s: %w", ErrGetImages, err) } output := stdout.Bytes() var images []map[string]interface{} if err := json.Unmarshal(output, &images); err != nil { - return nil, err + return nil, fmt.Errorf("%s: %w", ErrGetImages, err) } return images, nil @@ -133,13 +161,13 @@ func GetVersion() (string, error) { args := []string{"--log-level", logLevelString, "version", "--format", "json"} if err := shell.Run("podman", nil, &stdout, nil, args...); err != nil { - return "", err + return "", fmt.Errorf("%s: %w", ErrGetVersion, err) } output := stdout.Bytes() var jsonoutput map[string]interface{} if err := json.Unmarshal(output, &jsonoutput); err != nil { - return "", err + return "", fmt.Errorf("%s: %w", ErrGetVersion, err) } podmanClientInfoInterface := jsonoutput["Client"] @@ -161,7 +189,7 @@ func ImageExists(image string) (bool, error) { exitCode, err := shell.RunWithExitCode("podman", nil, nil, nil, args...) if exitCode != 0 && err == nil { - err = fmt.Errorf("failed to find image %s", image) + err = ErrNotExist } if err != nil { @@ -181,14 +209,14 @@ func Inspect(typearg string, target string) (map[string]interface{}, error) { args := []string{"--log-level", logLevelString, "inspect", "--format", "json", "--type", typearg, target} if err := shell.Run("podman", nil, &stdout, nil, args...); err != nil { - return nil, err + return nil, fmt.Errorf("%s: %w", ErrInspect, err) } output := stdout.Bytes() var info []map[string]interface{} if err := json.Unmarshal(output, &info); err != nil { - return nil, err + return nil, fmt.Errorf("%s: %w", ErrInspect, err) } return info[0], nil @@ -197,13 +225,13 @@ func Inspect(typearg string, target string) (map[string]interface{}, error) { func IsToolboxContainer(container string) (bool, error) { info, err := Inspect("container", container) if err != nil { - return false, fmt.Errorf("failed to inspect container %s", container) + return false, err } labels, _ := info["Config"].(map[string]interface{})["Labels"].(map[string]interface{}) if labels["com.redhat.component"] != "fedora-toolbox" && labels["com.github.debarshiray.toolbox"] != "true" { - return false, fmt.Errorf("%s is not a toolbox container", container) + return false, ErrNotToolbox } return true, nil @@ -212,17 +240,17 @@ func IsToolboxContainer(container string) (bool, error) { func IsToolboxImage(image string) (bool, error) { info, err := Inspect("image", image) if err != nil { - return false, fmt.Errorf("failed to inspect image %s", image) + return false, fmt.Errorf("failed to inspect image %s: %w", image, err) } if info["Labels"] == nil { - return false, fmt.Errorf("%s is not a toolbox image", image) + return false, ErrNotToolbox } labels := info["Labels"].(map[string]interface{}) if labels["com.redhat.component"] != "fedora-toolbox" && labels["com.github.debarshiray.toolbox"] != "true" { - return false, fmt.Errorf("%s is not a toolbox image", image) + return false, ErrNotToolbox } return true, nil @@ -234,7 +262,7 @@ func Pull(imageName string) error { args := []string{"--log-level", logLevelString, "pull", imageName} if err := shell.Run("podman", nil, nil, nil, args...); err != nil { - return err + return fmt.Errorf("%s: %w", ErrPullImage, err) } return nil @@ -259,11 +287,11 @@ func RemoveContainer(container string, forceDelete bool) error { panic("unexpected error: 'podman rm' finished successfully") } case 1: - err = fmt.Errorf("container %s does not exist", container) + err = ErrNotExist case 2: - err = fmt.Errorf("container %s is running", container) + err = ErrContainerIsRunning default: - err = fmt.Errorf("failed to remove container %s", container) + err = ErrRemoveContainer } if err != nil { @@ -292,11 +320,11 @@ func RemoveImage(image string, forceDelete bool) error { panic("unexpected error: 'podman rmi' finished successfully") } case 1: - err = fmt.Errorf("image %s does not exist", image) + err = ErrNotExist case 2: - err = fmt.Errorf("image %s has dependent children", image) + err = ErrImageHasChildren default: - err = fmt.Errorf("failed to remove image %s", image) + err = ErrRemoveImage } if err != nil { @@ -315,7 +343,7 @@ func Start(container string, stderr io.Writer) error { args := []string{"--log-level", logLevelString, "start", container} if err := shell.Run("podman", nil, nil, stderr, args...); err != nil { - return err + return fmt.Errorf("%s: %w", ErrStartContainer, err) } return nil @@ -329,7 +357,7 @@ func SystemMigrate(ociRuntimeRequired string) error { } if err := shell.Run("podman", nil, nil, nil, args...); err != nil { - return err + return fmt.Errorf("%s: %w", ErrSystemMigrate, err) } return nil From ce6d1138b4a3562945322d963b9c4f51c876a9a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20M=C3=ADchal?= Date: Fri, 26 Jun 2020 10:46:21 +0200 Subject: [PATCH 2/2] cmd: Wrap errors to preserve information When testing for regressions introduced by Podman V2 I encountered an error with getting information about current user. I had to patch the toolbox binary to see the underlying error because we do not wrap errors in most places. Such problems could show again in different places. --- src/cmd/initContainer.go | 30 +++++++++++++++--------------- src/cmd/root.go | 26 +++++++++++++------------- src/pkg/utils/utils.go | 8 ++++---- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/cmd/initContainer.go b/src/cmd/initContainer.go index 0307ee80a..edefafa80 100644 --- a/src/cmd/initContainer.go +++ b/src/cmd/initContainer.go @@ -137,7 +137,7 @@ func initContainer(cmd *cobra.Command, args []string) error { toolboxEnvFile, err := os.Create("/run/.toolboxenv") if err != nil { - return errors.New("failed to create /run/.toolboxenv") + return fmt.Errorf("failed to create /run/.toolboxenv: %w", err) } defer toolboxEnvFile.Close() @@ -241,7 +241,7 @@ func initContainer(cmd *cobra.Command, args []string) error { sudoGroup, err := utils.GetGroupForSudo() if err != nil { - return fmt.Errorf("failed to add user %s: %s", initContainerFlags.user, err) + return fmt.Errorf("failed to add user %s: %w", initContainerFlags.user, err) } logrus.Debugf("Adding user %s with UID %d:", initContainerFlags.user, initContainerFlags.uid) @@ -269,13 +269,13 @@ func initContainer(cmd *cobra.Command, args []string) error { logrus.Debugf("Removing password for user %s", initContainerFlags.user) if err := shell.Run("passwd", nil, nil, nil, "--delete", initContainerFlags.user); err != nil { - return fmt.Errorf("failed to remove password for user %s", initContainerFlags.user) + return fmt.Errorf("failed to remove password for user %s: %w", initContainerFlags.user, err) } logrus.Debug("Removing password for user root") if err := shell.Run("passwd", nil, nil, nil, "--delete", "root"); err != nil { - return errors.New("failed to remove password for root") + return fmt.Errorf("failed to remove password for root: %w", err) } } @@ -295,7 +295,7 @@ func initContainer(cmd *cobra.Command, args []string) error { if err := ioutil.WriteFile("/etc/krb5.conf.d/kcm_default_ccache", kcmConfigBytes, 0644); err != nil { - return errors.New("failed to set KCM as the defult Kerberos credential cache") + return fmt.Errorf("failed to set KCM as the defult Kerberos credential cache: %w", err) } } @@ -305,12 +305,12 @@ func initContainer(cmd *cobra.Command, args []string) error { logrus.Debugf("Creating runtime directory %s", toolboxRuntimeDirectory) if err := os.MkdirAll(toolboxRuntimeDirectory, 0700); err != nil { - return fmt.Errorf("failed to create runtime directory %s", toolboxRuntimeDirectory) + return fmt.Errorf("failed to create runtime directory %s: %w", toolboxRuntimeDirectory, err) } if err := os.Chown(toolboxRuntimeDirectory, initContainerFlags.uid, initContainerFlags.uid); err != nil { - return fmt.Errorf("failed to change ownership of the runtime directory %s", - toolboxRuntimeDirectory) + return fmt.Errorf("failed to change ownership of the runtime directory %s: %w", + toolboxRuntimeDirectory, err) } pid := os.Getpid() @@ -320,13 +320,13 @@ func initContainer(cmd *cobra.Command, args []string) error { initializedStampFile, err := os.Create(initializedStamp) if err != nil { - return errors.New("failed to create initialization stamp") + return fmt.Errorf("failed to create initialization stamp: %w", err) } defer initializedStampFile.Close() if err := initializedStampFile.Chown(initContainerFlags.uid, initContainerFlags.uid); err != nil { - return errors.New("failed to change ownership of initialization stamp") + return fmt.Errorf("failed to change ownership of initialization stamp: %w", err) } logrus.Debug("Going to sleep") @@ -337,14 +337,14 @@ func initContainer(cmd *cobra.Command, args []string) error { return errors.New("sleep(1) not found") } - return errors.New("failed to lookup sleep(1)") + return fmt.Errorf("failed to lookup sleep(1): %w", err) } sleepArgs := []string{"sleep", "+Inf"} env := os.Environ() if err := syscall.Exec(sleepBinary, sleepArgs, env); err != nil { - return errors.New("failed to invoke sleep(1)") + return fmt.Errorf("failed to invoke sleep(1): %w", err) } return nil @@ -378,13 +378,13 @@ func mountBind(containerPath, source, flags string) error { return nil } - return fmt.Errorf("failed to stat %s", source) + return fmt.Errorf("failed to stat %s: %w", source, err) } if fi.IsDir() { logrus.Debugf("Creating %s", containerPath) if err := os.MkdirAll(containerPath, 0755); err != nil { - return fmt.Errorf("failed to create %s", containerPath) + return fmt.Errorf("failed to create %s: %w", containerPath, err) } } @@ -401,7 +401,7 @@ func mountBind(containerPath, source, flags string) error { args = append(args, []string{source, containerPath}...) if err := shell.Run("mount", nil, nil, nil, args...); err != nil { - return fmt.Errorf("failed to bind %s to %s", containerPath, source) + return fmt.Errorf("failed to bind %s to %s: %w", containerPath, source, err) } return nil diff --git a/src/cmd/root.go b/src/cmd/root.go index 15ac706fa..3365866ba 100644 --- a/src/cmd/root.go +++ b/src/cmd/root.go @@ -223,13 +223,13 @@ func migrate() error { err = os.MkdirAll(toolboxConfigDir, 0775) if err != nil { - return fmt.Errorf("failed to create configuration directory") + return fmt.Errorf("failed to create configuration directory: %w", err) } runtimeDirectory := os.Getenv("XDG_RUNTIME_DIR") toolboxRuntimeDirectory := runtimeDirectory + "/toolbox" if err := os.MkdirAll(toolboxRuntimeDirectory, 0700); err != nil { - return fmt.Errorf("failed to create runtime directory %s", toolboxRuntimeDirectory) + return fmt.Errorf("failed to create runtime directory %s: %w", toolboxRuntimeDirectory, err) } lockFile := toolboxRuntimeDirectory + "/migrate.lock" @@ -238,20 +238,20 @@ func migrate() error { syscall.O_CREAT|syscall.O_WRONLY, syscall.S_IRUSR|syscall.S_IWUSR|syscall.S_IRGRP|syscall.S_IWGRP|syscall.S_IROTH) if err != nil { - return fmt.Errorf("failed to open migration lock file") + return fmt.Errorf("failed to open migration lock file: %w", err) } defer syscall.Close(lockFD) err = syscall.Flock(lockFD, syscall.LOCK_EX) if err != nil { - return fmt.Errorf("failed to acquire migration lock") + return fmt.Errorf("failed to acquire migration lock: %w", err) } stampBytes, err := ioutil.ReadFile(stampPath) if err != nil { if !os.IsNotExist(err) { - return fmt.Errorf("failed to read migration stamp file") + return fmt.Errorf("failed to read migration stamp file: %w", err) } } else { stampString := string(stampBytes) @@ -283,7 +283,7 @@ func migrate() error { podmanVersionBytes := []byte(podmanVersion + "\n") err = ioutil.WriteFile(stampPath, podmanVersionBytes, 0664) if err != nil { - return fmt.Errorf("failed to update Podman version in migration stamp file") + return fmt.Errorf("failed to update Podman version in migration stamp file: %w", err) } return nil @@ -305,30 +305,30 @@ func setUpGlobals() error { if !utils.IsInsideContainer() { cgroupsVersion, err = utils.GetCgroupsVersion() if err != nil { - return errors.New("failed to get the cgroups version") + return fmt.Errorf("failed to get the cgroups version: %w", err) } } currentUser, err = user.Current() if err != nil { - return errors.New("failed to get the current user") + return fmt.Errorf("failed to get the current user: %w", err) } executable, err = os.Executable() if err != nil { - return errors.New("failed to get the path to the executable") + return fmt.Errorf("failed to get the path to the executable: %w", err) } executable, err = filepath.EvalSymlinks(executable) if err != nil { - return errors.New("failed to resolve absolute path to the executable") + return fmt.Errorf("failed to resolve absolute path to the executable: %w", err) } executableBase = filepath.Base(executable) workingDirectory, err = os.Getwd() if err != nil { - return errors.New("failed to get the working directory") + return fmt.Errorf("failed to get the working directory: %w", err) } return nil @@ -346,7 +346,7 @@ func setUpLoggers() error { logLevel, err := logrus.ParseLevel(rootFlags.logLevel) if err != nil { - return errors.New("failed to parse log-level") + return fmt.Errorf("failed to parse log-level: %w", err) } logrus.SetLevel(logLevel) @@ -365,7 +365,7 @@ func setUpLoggers() error { func validateSubIDFile(path string) (bool, error) { file, err := os.Open(path) if err != nil { - return false, fmt.Errorf("failed to open %s", path) + return false, fmt.Errorf("failed to open %s: %w", path, err) } scanner := bufio.NewScanner(file) diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index 634d981e8..b135d69bd 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -130,7 +130,7 @@ func CallFlatpakSessionHelper() (string, error) { connection, err := dbus.SessionBus() if err != nil { - return "", errors.New("failed to connect to the D-Bus session instance") + return "", fmt.Errorf("failed to connect to the D-Bus session instance: %w", err) } sessionHelper := connection.Object("org.freedesktop.Flatpak", "/org/freedesktop/Flatpak/SessionHelper") @@ -146,7 +146,7 @@ func CallFlatpakSessionHelper() (string, error) { pathVariant := result["path"] pathVariantSignature := pathVariant.Signature().String() if pathVariantSignature != "s" { - return "", errors.New("unknown reply from org.freedesktop.Flatpak.SessionHelper.RequestSession") + return "", fmt.Errorf("unknown reply from org.freedesktop.Flatpak.SessionHelper.RequestSession: %w", err) } pathValue := pathVariant.Value() @@ -559,11 +559,11 @@ func ShowManual(manual string) error { stdoutFd := os.Stdout.Fd() stdoutFdInt := int(stdoutFd) if err := syscall.Dup3(stdoutFdInt, stderrFdInt, 0); err != nil { - return errors.New("failed to redirect standard error to standard output") + return fmt.Errorf("failed to redirect standard error to standard output: %w", err) } if err := syscall.Exec(manBinary, manualArgs, env); err != nil { - return errors.New("failed to invoke man(1)") + return fmt.Errorf("failed to invoke man(1): %w", err) } return nil