From 7eddbdee232b6dc2f39bba9586e4bc0b21a970a4 Mon Sep 17 00:00:00 2001 From: Joel Rebello Date: Tue, 11 Jul 2023 12:26:05 +0200 Subject: [PATCH 1/4] internal/redfishwrapper: add methods to override the HTTP client timeout value --- internal/redfishwrapper/client.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/redfishwrapper/client.go b/internal/redfishwrapper/client.go index a77632ce..f600631c 100644 --- a/internal/redfishwrapper/client.go +++ b/internal/redfishwrapper/client.go @@ -161,6 +161,16 @@ func (c *Client) SessionActive() error { return nil } +// Overrides the HTTP client timeout +func (c *Client) SetHttpClientTimeout(t time.Duration) { + c.client.HTTPClient.Timeout = t +} + +// retrieve the current HTTP client timeout +func (c *Client) HttpClientTimeout() time.Duration { + return c.client.HTTPClient.Timeout +} + // RunRawRequestWithHeaders wraps the gofish client method RunRawRequestWithHeaders func (c *Client) RunRawRequestWithHeaders(method, url string, payloadBuffer io.ReadSeeker, contentType string, customHeaders map[string]string) (*http.Response, error) { if err := c.SessionActive(); err != nil { From 7ebacd2cd7a7704a8d3859816710cfac3e420a41 Mon Sep 17 00:00:00 2001 From: Joel Rebello Date: Tue, 11 Jul 2023 12:26:41 +0200 Subject: [PATCH 2/4] redfish/FirmwareInstall: check ctx timeout and override before proceeding install --- providers/redfish/firmware.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/providers/redfish/firmware.go b/providers/redfish/firmware.go index b36fd821..3d5f432a 100644 --- a/providers/redfish/firmware.go +++ b/providers/redfish/firmware.go @@ -13,6 +13,7 @@ import ( "path/filepath" "strconv" "strings" + "time" "github.com/pkg/errors" gofishrf "github.com/stmcginnis/gofish/redfish" @@ -43,6 +44,12 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, "invalid applyAt parameter: "+applyAt) } + // expect atleast 30 minutes left in the deadline to proceed with the update + ctxDeadline, _ := ctx.Deadline() + if time.Until(ctxDeadline) < 30*time.Minute { + return "", errors.New("remaining context deadline insufficient to perform update: " + time.Until(ctxDeadline).String()) + } + // list redfish firmware install task if theres one present task, err := c.GetFirmwareInstallTaskQueued(ctx, component) if err != nil { @@ -77,6 +84,17 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, err.Error()) } + // override the gofish HTTP client timeout, since that is generally set to a much lower value, + // and we cannot pass a context deadline for the multipart upload. + // + // record the http client time to be restored + httpClientTimeout := c.redfishwrapper.HttpClientTimeout() + defer func() { + c.redfishwrapper.SetHttpClientTimeout(httpClientTimeout) + }() + + c.redfishwrapper.SetHttpClientTimeout(time.Until(ctxDeadline)) + payload := map[string]io.Reader{ "UpdateParameters": bytes.NewReader(updateParameters), "UpdateFile": reader, From 725e651bac57ba84a7ba09ed98188e178528c9fa Mon Sep 17 00:00:00 2001 From: Joel Rebello Date: Tue, 11 Jul 2023 15:16:25 +0200 Subject: [PATCH 3/4] redfish/FirmwareInstall: add test case and lower timeout value --- providers/redfish/firmware.go | 16 ++++++++---- providers/redfish/firmware_test.go | 41 +++++++++++++++++++++++------- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/providers/redfish/firmware.go b/providers/redfish/firmware.go index 3d5f432a..94112521 100644 --- a/providers/redfish/firmware.go +++ b/providers/redfish/firmware.go @@ -23,6 +23,10 @@ import ( "github.com/bmc-toolbox/bmclib/v2/internal" ) +var ( + errInsufficientCtxTimeout = errors.New("remaining context timeout insufficient to install firmware") +) + // SupportedFirmwareApplyAtValues returns the supported redfish firmware applyAt values func SupportedFirmwareApplyAtValues() []string { return []string{ @@ -44,10 +48,12 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, "invalid applyAt parameter: "+applyAt) } - // expect atleast 30 minutes left in the deadline to proceed with the update + // expect atleast 10 minutes left in the deadline to proceed with the update + // + // this gives the BMC enough time to have the firmware uploaded and return a response to the client. ctxDeadline, _ := ctx.Deadline() - if time.Until(ctxDeadline) < 30*time.Minute { - return "", errors.New("remaining context deadline insufficient to perform update: " + time.Until(ctxDeadline).String()) + if time.Until(ctxDeadline) < 10*time.Minute { + return "", errors.Wrap(errInsufficientCtxTimeout, " "+time.Until(ctxDeadline).String()) } // list redfish firmware install task if theres one present @@ -84,8 +90,8 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, err.Error()) } - // override the gofish HTTP client timeout, since that is generally set to a much lower value, - // and we cannot pass a context deadline for the multipart upload. + // override the gofish HTTP client timeout, + // since the context timeout is set at Open() and is at a lower value than required for this operation. // // record the http client time to be restored httpClientTimeout := c.redfishwrapper.HttpClientTimeout() diff --git a/providers/redfish/firmware_test.go b/providers/redfish/firmware_test.go index 9611d6d1..8e098e4d 100644 --- a/providers/redfish/firmware_test.go +++ b/providers/redfish/firmware_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/stretchr/testify/assert" @@ -70,19 +71,32 @@ func Test_FirmwareInstall(t *testing.T) { defer os.Remove(binPath) tests := []struct { - component string - applyAt string - forceInstall bool - reader io.Reader - expectTaskID string - expectErr error - expectErrSubStr string - testName string + component string + applyAt string + forceInstall bool + setRequiredTimeout bool + reader io.Reader + expectTaskID string + expectErr error + expectErrSubStr string + testName string }{ + { + common.SlugBIOS, + constants.FirmwareApplyOnReset, + false, + false, + nil, + "", + errInsufficientCtxTimeout, + "", + "remaining context deadline", + }, { common.SlugBIOS, "invalidApplyAt", false, + true, nil, "", bmclibErrs.ErrFirmwareInstall, @@ -93,6 +107,7 @@ func Test_FirmwareInstall(t *testing.T) { common.SlugBIOS, constants.FirmwareApplyOnReset, false, + true, fh, "467696020275", bmclibErrs.ErrFirmwareInstall, @@ -103,6 +118,7 @@ func Test_FirmwareInstall(t *testing.T) { common.SlugBIOS, constants.FirmwareApplyOnReset, true, + true, fh, "467696020275", nil, @@ -113,7 +129,12 @@ func Test_FirmwareInstall(t *testing.T) { for _, tc := range tests { t.Run(tc.testName, func(t *testing.T) { - taskID, err := mockClient.FirmwareInstall(context.TODO(), tc.component, tc.applyAt, tc.forceInstall, tc.reader) + ctx, cancel := context.WithTimeout(context.TODO(), 1*time.Second) + if tc.setRequiredTimeout { + ctx, cancel = context.WithTimeout(context.TODO(), 20*time.Minute) + } + + taskID, err := mockClient.FirmwareInstall(ctx, tc.component, tc.applyAt, tc.forceInstall, tc.reader) if tc.expectErr != nil { assert.ErrorIs(t, err, tc.expectErr) if tc.expectErrSubStr != "" { @@ -123,6 +144,8 @@ func Test_FirmwareInstall(t *testing.T) { assert.Nil(t, err) assert.Equal(t, tc.expectTaskID, taskID) } + + defer cancel() }) } From c13e347595e301068f220c49c004fb20c0a2e880 Mon Sep 17 00:00:00 2001 From: Joel Rebello Date: Fri, 14 Jul 2023 10:54:52 +0200 Subject: [PATCH 4/4] smc: lower required timeout value for update --- providers/supermicro/firmware.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/providers/supermicro/firmware.go b/providers/supermicro/firmware.go index 0c6a7fa7..4e94ba97 100644 --- a/providers/supermicro/firmware.go +++ b/providers/supermicro/firmware.go @@ -28,9 +28,9 @@ func (c *Client) FirmwareInstall(ctx context.Context, component, applyAt string, size = finfo.Size() } - // expect atleast 30 minutes left in the deadline to proceed with the update + // expect atleast 10 minutes left in the deadline to proceed with the update d, _ := ctx.Deadline() - if time.Until(d) < 30*time.Minute { + if time.Until(d) < 10*time.Minute { return "", errors.New("remaining context deadline insufficient to perform update: " + time.Until(d).String()) }