From be2973c312f126a4083360b4115eeae4b5f54104 Mon Sep 17 00:00:00 2001 From: Roman Penyaev Date: Mon, 9 Oct 2023 11:25:46 +0200 Subject: [PATCH] Revert "execwrapper: reduce general timeout to 100s" This reverts commit b453e5dd328129025eac97356f4cdf468ead08b4. --- pkg/pillar/base/execwrapper.go | 98 +++++++++++++++------------ pkg/pillar/base/execwrapper_test.go | 18 ----- pkg/pillar/cmd/domainmgr/domainmgr.go | 2 +- pkg/pillar/diskmetrics/diskmetrics.go | 3 +- pkg/pillar/netmonitor/linux.go | 2 +- 5 files changed, 57 insertions(+), 66 deletions(-) delete mode 100644 pkg/pillar/base/execwrapper_test.go diff --git a/pkg/pillar/base/execwrapper.go b/pkg/pillar/base/execwrapper.go index ba650ab9e60..fabc7b67c1a 100644 --- a/pkg/pillar/base/execwrapper.go +++ b/pkg/pillar/base/execwrapper.go @@ -17,57 +17,84 @@ import ( ) const ( - /* - * execution timeout is supposed to be less than the watchdog timeout, - * as otherwise the watchdog might fire and reboot the system before - * the timeout fires - * exceptions are when an executable is started from a different goroutine - * source of the error timeout and the watchdog timeout: - * error timeout: $ grep -r errorTime pkg/pillar/cmd/ | grep time - * watchdog timeout: $ grep hv_watchdog_timer pkg/grub/rootfs.cfg - */ - timeoutLimit = 3 * time.Minute - defaultTimeout = 100 * time.Second + // Will wait for 1000s max for a call instead of waiting forever + maxTimeOutLimit = 1000 + timeOutLimit = 100 ) // Command holds the necessary data to execute command type Command struct { - command *exec.Cmd - log *LogObject - agentName string - timeout time.Duration - buffer *bytes.Buffer - ctx context.Context - errorMonad error + command *exec.Cmd + log *LogObject + agentName string + timeout time.Duration + buffer *bytes.Buffer + ctx context.Context } // Output runs the command and returns its standard output. // Any returned error will usually be of type *ExitError. -// Waits for the exec call to finish for `defaultTimeout` after which timeout error is returned +// Waits for the exec call to finish for `maxTimeOutLimit` after which timeout error is returned func (c *Command) Output() ([]byte, error) { var buf bytes.Buffer c.command.Stdout = &buf c.buffer = &buf - c.timeout = defaultTimeout + c.timeout = maxTimeOutLimit return c.execCommand() } // CombinedOutput runs the command and returns its combined standard output and standard error. -// Waits for the exec call to finish for `defaultTimeout` after which timeout error is returned +// Waits for the exec call to finish for `maxTimeOutLimit` after which timeout error is returned func (c *Command) CombinedOutput() ([]byte, error) { var buf bytes.Buffer c.command.Stdout = &buf c.command.Stderr = &buf c.buffer = &buf - c.timeout = defaultTimeout + c.timeout = maxTimeOutLimit return c.execCommand() } -func (c *Command) execCommand() ([]byte, error) { - if c.errorMonad != nil { - return nil, c.errorMonad - } +// OutputWithTimeout waits for the exec call to finish for `timeOutLimit` +// after which timeout error is returned +func (c *Command) OutputWithTimeout() ([]byte, error) { + var buf bytes.Buffer + c.command.Stdout = &buf + c.buffer = &buf + c.timeout = timeOutLimit + return c.execCommand() +} +// CombinedOutputWithTimeout waits for the exec call to finish for `timeOutLimit` +// after which timeout error is returned +func (c *Command) CombinedOutputWithTimeout() ([]byte, error) { + var buf bytes.Buffer + c.command.Stdout = &buf + c.command.Stderr = &buf + c.buffer = &buf + c.timeout = timeOutLimit + return c.execCommand() +} + +// OutputWithCustomTimeout accepts a custom timeout limit to wait for the exec call. +func (c *Command) OutputWithCustomTimeout(timeout uint) ([]byte, error) { + var buf bytes.Buffer + c.command.Stdout = &buf + c.buffer = &buf + c.timeout = time.Duration(timeout) + return c.execCommand() +} + +// CombinedOutputWithCustomTimeout accepts a custom timeout limit to wait for the exec call. +func (c *Command) CombinedOutputWithCustomTimeout(timeout uint) ([]byte, error) { + var buf bytes.Buffer + c.command.Stdout = &buf + c.command.Stderr = &buf + c.buffer = &buf + c.timeout = time.Duration(timeout) + return c.execCommand() +} + +func (c *Command) execCommand() ([]byte, error) { if c.log != nil { c.log.Tracef("execCommand(%v)", c.command.Args) } @@ -81,7 +108,7 @@ func (c *Command) execCommand() ([]byte, error) { stillRunning := time.NewTicker(25 * time.Second) defer stillRunning.Stop() - waitTimer := time.NewTimer(c.timeout) + waitTimer := time.NewTimer(c.timeout * time.Second) defer waitTimer.Stop() if c.ctx == nil { @@ -107,23 +134,6 @@ func (c *Command) execCommand() ([]byte, error) { } } -// WithLimitedTimeout set custom timeout for command -func (c *Command) WithLimitedTimeout(timeout time.Duration) *Command { - c.timeout = timeout - if c.timeout > timeoutLimit { - c.errorMonad = fmt.Errorf("custom timeout (%v) is longer than watchdog timeout (%v)", c.timeout, defaultTimeout) - } - - return c -} - -// WithUnlimitedTimeout set custom timeout for command not bound to any limits for when run in a separate goroutine -func (c *Command) WithUnlimitedTimeout(timeout time.Duration) *Command { - c.timeout = timeout - - return c -} - // WithContext set context for command func (c *Command) WithContext(ctx context.Context) *Command { c.ctx = ctx diff --git a/pkg/pillar/base/execwrapper_test.go b/pkg/pillar/base/execwrapper_test.go deleted file mode 100644 index 311b0dd208e..00000000000 --- a/pkg/pillar/base/execwrapper_test.go +++ /dev/null @@ -1,18 +0,0 @@ -package base_test - -import ( - "testing" - "time" - - "github.com/lf-edge/eve/pkg/pillar/base" -) - -func TestTimeoutLongerThanLimit(t *testing.T) { - t.Parallel() - - timeout := 600 * time.Second - _, err := base.Exec(nil, "/bin/true").WithLimitedTimeout(timeout).CombinedOutput() - if err == nil { - t.Fatalf("Execution should fail with timeout too long, but succeeded") - } -} diff --git a/pkg/pillar/cmd/domainmgr/domainmgr.go b/pkg/pillar/cmd/domainmgr/domainmgr.go index f0df04edb28..2d7ef855bba 100644 --- a/pkg/pillar/cmd/domainmgr/domainmgr.go +++ b/pkg/pillar/cmd/domainmgr/domainmgr.go @@ -2658,7 +2658,7 @@ func mkisofs(output string, dir string) error { dir, } log.Functionf("Calling command %s %v\n", cmd, args) - stdoutStderr, err := base.Exec(log, cmd, args...).WithUnlimitedTimeout(15 * time.Minute).CombinedOutput() + stdoutStderr, err := base.Exec(log, cmd, args...).CombinedOutput() if err != nil { errStr := fmt.Sprintf("mkisofs failed: %s", string(stdoutStderr)) diff --git a/pkg/pillar/diskmetrics/diskmetrics.go b/pkg/pillar/diskmetrics/diskmetrics.go index a31722101c4..6463a4b8d8c 100644 --- a/pkg/pillar/diskmetrics/diskmetrics.go +++ b/pkg/pillar/diskmetrics/diskmetrics.go @@ -11,7 +11,6 @@ import ( "os" "strconv" "strings" - "time" "github.com/lf-edge/eve/pkg/pillar/base" "github.com/lf-edge/eve/pkg/pillar/types" @@ -94,7 +93,7 @@ func RolloutImgToBlock(ctx context.Context, log *base.LogObject, diskfile, outpu // writeback cache instead of default unsafe, out of order enabled, skip file creation // Timeout 2 hours args := []string{"convert", "--target-is-zero", "-t", "writeback", "-W", "-n", "-O", outputFormat, diskfile, outputFile} - output, err := base.Exec(log, "/usr/bin/qemu-img", args...).WithContext(ctx).WithUnlimitedTimeout(15 * time.Minute).CombinedOutput() + output, err := base.Exec(log, "/usr/bin/qemu-img", args...).WithContext(ctx).CombinedOutputWithCustomTimeout(432000) if err != nil { errStr := fmt.Sprintf("qemu-img failed: %s, %s\n", err, output) diff --git a/pkg/pillar/netmonitor/linux.go b/pkg/pillar/netmonitor/linux.go index 31eff967a42..d7ece812b91 100644 --- a/pkg/pillar/netmonitor/linux.go +++ b/pkg/pillar/netmonitor/linux.go @@ -257,7 +257,7 @@ func (m *LinuxNetworkMonitor) GetInterfaceDHCPInfo(ifIndex int) (info DHCPInfo, // XXX Getting error -1 unless we add argument -4. // XXX Add IPv6 support. m.Log.Functionf("Calling dhcpcd -U -4 %s\n", ifName) - stdoutStderr, err := base.Exec(m.Log, "dhcpcd", "-U", "-4", ifName).CombinedOutput() + stdoutStderr, err := base.Exec(m.Log, "dhcpcd", "-U", "-4", ifName).CombinedOutputWithTimeout() if err != nil { if strings.Contains(string(stdoutStderr), "dhcp_dump: No such file or directory") { // DHCP is not configured for this interface. Return empty DHCPInfo.