Skip to content

Commit

Permalink
Add a timeout of 1s to the 'podman version' command
Browse files Browse the repository at this point in the history
This command is called at dependency injection time to initialize a (nil-able) Podman client,
even if users won't use Podman at all.
As discussed, this command is supposed to be
quite fast to return, hence this timeout of 1 second.

Initially, we were using cmd.Output to get the command output,
but as reported in [1], cmd.Output does not respect the context timeout.
This explains the workaround of reading from both stdout and stderr pipes,
*and* relying on cmd.Wait() to close those pipes properly when the program exits
(either as expected or when the timeout is reached).

[1] golang/go#57129
  • Loading branch information
rm3l committed May 10, 2023
1 parent 4b99e43 commit 610bc52
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 15 deletions.
4 changes: 3 additions & 1 deletion pkg/podman/interface.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package podman

import (
"context"

corev1 "k8s.io/api/core/v1"

"github.com/redhat-developer/odo/pkg/api"
Expand Down Expand Up @@ -36,5 +38,5 @@ type Client interface {

ListAllComponents() ([]api.ComponentAbstract, error)

Version() (SystemVersionReport, error)
Version(ctx context.Context) (SystemVersionReport, error)
}
8 changes: 4 additions & 4 deletions pkg/podman/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/podman/podman.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func NewPodmanCli(ctx context.Context) (*PodmanCli, error) {
containerRunGlobalExtraArgs: envcontext.GetEnvConfig(ctx).OdoContainerBackendGlobalArgs,
containerRunExtraArgs: envcontext.GetEnvConfig(ctx).OdoContainerRunArgs,
}
version, err := cli.Version()
version, err := cli.Version(ctx)
if err != nil {
return nil, err
}
Expand Down
78 changes: 70 additions & 8 deletions pkg/podman/version.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package podman

import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"os/exec"
"time"

"k8s.io/klog"
)
Expand All @@ -23,20 +27,78 @@ type SystemVersionReport struct {
Client *Version `json:",omitempty"`
}

func (o *PodmanCli) Version() (SystemVersionReport, error) {
cmd := exec.Command(o.podmanCmd, "version", "--format", "json")
// Version returns the version of the Podman client.
func (o *PodmanCli) Version(ctx context.Context) (SystemVersionReport, error) {
// Because Version is used at the very beginning of odo, when resolving and injecting dependencies (for commands that might require the Podman client),
// it is expected to return in a timely manny (hence this timeout of 1 second).
// This is to avoid situations like the one described in https://github.com/redhat-developer/odo/issues/6575
// (where a podman that takes too long to respond affects the "odo dev" command, even if the user did not intend to use the Podman platform).
ctxWithTimeout, cancel := context.WithTimeout(ctx, 1*time.Second)
defer cancel()

cmd := exec.CommandContext(ctxWithTimeout, o.podmanCmd, "version", "--format", "json")
klog.V(3).Infof("executing %v", cmd.Args)
out, err := cmd.Output()

// Because cmd.Output() does not respect the context timeout (see https://github.com/golang/go/issues/57129),
// we are reading from the connected pipes instead.
stdoutPipe, err := cmd.StdoutPipe()
if err != nil {
if exiterr, ok := err.(*exec.ExitError); ok {
err = fmt.Errorf("%s: %s", err, string(exiterr.Stderr))
}
return SystemVersionReport{}, err
}
var result SystemVersionReport
err = json.Unmarshal(out, &result)
stderrPipe, err := cmd.StderrPipe()
if err != nil {
return SystemVersionReport{}, err
}

err = cmd.Start()
if err != nil {
return SystemVersionReport{}, err
}

var result SystemVersionReport
go func() {
// Reading from the pipe is a blocking call, hence this goroutine.
// The goroutine will exit after the pipe is closed or the command exits;
// these will be triggered by cmd.Wait() either after the timeout expires or the command finished.
err = json.NewDecoder(stdoutPipe).Decode(&result)
if err != nil {
klog.V(3).Infof("unable to decode output: %v", err)
}
}()

var stderr string
go func() {
var buf bytes.Buffer
_, rErr := buf.ReadFrom(stderrPipe)
if rErr != nil {
klog.V(7).Infof("unable to read from stderr pipe: %v", rErr)
}
stderr = buf.String()
}()

// Wait will block until the timeout expires or the command exits. It will then close all resources associated with cmd,
// including the stdout and stderr pipes above, which will in turn terminate the goroutines spawned above.
wErr := cmd.Wait()
if wErr != nil {
ctxErr := ctxWithTimeout.Err()
if ctxErr != nil {
msg := "error"
if errors.Is(ctxErr, context.DeadlineExceeded) {
msg = "timeout"
}
wErr = fmt.Errorf("%s while waiting for Podman version: %s: %w", msg, ctxErr, wErr)
}
if exitErr, ok := wErr.(*exec.ExitError); ok {
wErr = fmt.Errorf("%s: %s", wErr, string(exitErr.Stderr))
}
if err != nil {
wErr = fmt.Errorf("%s: (%w)", wErr, err)
}
if stderr != "" {
wErr = fmt.Errorf("%w: %s", wErr, stderr)
}
return SystemVersionReport{}, fmt.Errorf("%v. Please check the output of the following command: %v", wErr, cmd.Args)
}

return result, nil
}
2 changes: 1 addition & 1 deletion pkg/segment/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func setPlatformCluster(ctx context.Context, client kclient.ClientInterface) {

func setPlatformPodman(ctx context.Context, client podman.Client) {
setContextProperty(ctx, Platform, "podman")
version, err := client.Version()
version, err := client.Version(ctx)
if err != nil {
klog.V(3).Info(fmt.Errorf("unable to get podman version: %w", err))
return
Expand Down

0 comments on commit 610bc52

Please sign in to comment.