Skip to content

Commit 7278624

Browse files
rm3lfeloy
andcommitted
Add a timeout of 1s to the 'podman version' command
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 Co-authored-by: Philippe Martin <[email protected]>
1 parent 4b99e43 commit 7278624

File tree

5 files changed

+79
-15
lines changed

5 files changed

+79
-15
lines changed

pkg/podman/interface.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package podman
22

33
import (
4+
"context"
5+
46
corev1 "k8s.io/api/core/v1"
57

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

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

39-
Version() (SystemVersionReport, error)
41+
Version(ctx context.Context) (SystemVersionReport, error)
4042
}

pkg/podman/mock.go

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/podman/podman.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func NewPodmanCli(ctx context.Context) (*PodmanCli, error) {
3333
containerRunGlobalExtraArgs: envcontext.GetEnvConfig(ctx).OdoContainerBackendGlobalArgs,
3434
containerRunExtraArgs: envcontext.GetEnvConfig(ctx).OdoContainerRunArgs,
3535
}
36-
version, err := cli.Version()
36+
version, err := cli.Version(ctx)
3737
if err != nil {
3838
return nil, err
3939
}

pkg/podman/version.go

Lines changed: 70 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
package podman
22

33
import (
4+
"bytes"
5+
"context"
46
"encoding/json"
7+
"errors"
58
"fmt"
69
"os/exec"
10+
"time"
711

812
"k8s.io/klog"
913
)
@@ -23,20 +27,78 @@ type SystemVersionReport struct {
2327
Client *Version `json:",omitempty"`
2428
}
2529

26-
func (o *PodmanCli) Version() (SystemVersionReport, error) {
27-
cmd := exec.Command(o.podmanCmd, "version", "--format", "json")
30+
// Version returns the version of the Podman client.
31+
func (o *PodmanCli) Version(ctx context.Context) (SystemVersionReport, error) {
32+
// Because Version is used at the very beginning of odo, when resolving and injecting dependencies (for commands that might require the Podman client),
33+
// it is expected to return in a timely manny (hence this timeout of 1 second).
34+
// This is to avoid situations like the one described in https://github.com/redhat-developer/odo/issues/6575
35+
// (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).
36+
ctxWithTimeout, cancel := context.WithTimeout(ctx, 1*time.Second)
37+
defer cancel()
38+
39+
cmd := exec.CommandContext(ctxWithTimeout, o.podmanCmd, "version", "--format", "json")
2840
klog.V(3).Infof("executing %v", cmd.Args)
29-
out, err := cmd.Output()
41+
42+
// Because cmd.Output() does not respect the context timeout (see https://github.com/golang/go/issues/57129),
43+
// we are reading from the connected pipes instead.
44+
stdoutPipe, err := cmd.StdoutPipe()
3045
if err != nil {
31-
if exiterr, ok := err.(*exec.ExitError); ok {
32-
err = fmt.Errorf("%s: %s", err, string(exiterr.Stderr))
33-
}
3446
return SystemVersionReport{}, err
3547
}
36-
var result SystemVersionReport
37-
err = json.Unmarshal(out, &result)
48+
stderrPipe, err := cmd.StderrPipe()
49+
if err != nil {
50+
return SystemVersionReport{}, err
51+
}
52+
53+
err = cmd.Start()
3854
if err != nil {
3955
return SystemVersionReport{}, err
4056
}
57+
58+
var result SystemVersionReport
59+
go func() {
60+
// Reading from the pipe is a blocking call, hence this goroutine.
61+
// The goroutine will exit after the pipe is closed or the command exits;
62+
// these will be triggered by cmd.Wait() either after the timeout expires or the command finished.
63+
err = json.NewDecoder(stdoutPipe).Decode(&result)
64+
if err != nil {
65+
klog.V(3).Infof("unable to decode output: %v", err)
66+
}
67+
}()
68+
69+
var stderr string
70+
go func() {
71+
var buf bytes.Buffer
72+
_, rErr := buf.ReadFrom(stderrPipe)
73+
if rErr != nil {
74+
klog.V(7).Infof("unable to read from stderr pipe: %v", rErr)
75+
}
76+
stderr = buf.String()
77+
}()
78+
79+
// Wait will block until the timeout expires or the command exits. It will then close all resources associated with cmd,
80+
// including the stdout and stderr pipes above, which will in turn terminate the goroutines spawned above.
81+
wErr := cmd.Wait()
82+
if wErr != nil {
83+
ctxErr := ctxWithTimeout.Err()
84+
if ctxErr != nil {
85+
msg := "error"
86+
if errors.Is(ctxErr, context.DeadlineExceeded) {
87+
msg = "timeout"
88+
}
89+
wErr = fmt.Errorf("%s while waiting for Podman version: %s: %w", msg, ctxErr, wErr)
90+
}
91+
if exitErr, ok := wErr.(*exec.ExitError); ok {
92+
wErr = fmt.Errorf("%s: %s", wErr, string(exitErr.Stderr))
93+
}
94+
if err != nil {
95+
wErr = fmt.Errorf("%s: (%w)", wErr, err)
96+
}
97+
if stderr != "" {
98+
wErr = fmt.Errorf("%w: %s", wErr, stderr)
99+
}
100+
return SystemVersionReport{}, fmt.Errorf("%v. Please check the output of the following command: %v", wErr, cmd.Args)
101+
}
102+
41103
return result, nil
42104
}

pkg/segment/context/context.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ func setPlatformCluster(ctx context.Context, client kclient.ClientInterface) {
150150

151151
func setPlatformPodman(ctx context.Context, client podman.Client) {
152152
setContextProperty(ctx, Platform, "podman")
153-
version, err := client.Version()
153+
version, err := client.Version(ctx)
154154
if err != nil {
155155
klog.V(3).Info(fmt.Errorf("unable to get podman version: %w", err))
156156
return

0 commit comments

Comments
 (0)