From 9a905cd9c48691fbc98196025d70a586fd2fc4e0 Mon Sep 17 00:00:00 2001 From: Drew Walters Date: Thu, 16 Apr 2020 22:53:14 +0000 Subject: [PATCH] Add validation to Redfish media ejection Currently, airshipctl relies on a wait period after ejecting media before trying to insert new media; however, this may not be safe if the sleep time is not long enough. This change replaces the sleep time with validation logic that polls the media status before attempting to insert new media. Change-Id: I64e8892686dbfa2fc780039331c341e14cc6752c Signed-off-by: Drew Walters --- pkg/remote/redfish/client.go | 36 ++++++++++++-- pkg/remote/redfish/client_test.go | 82 ++++++++++++++++++++++++++++++- pkg/remote/redfish/errors.go | 4 +- 3 files changed, 116 insertions(+), 6 deletions(-) diff --git a/pkg/remote/redfish/client.go b/pkg/remote/redfish/client.go index 6d997692c..95be9f5c2 100644 --- a/pkg/remote/redfish/client.go +++ b/pkg/remote/redfish/client.go @@ -30,7 +30,6 @@ import ( const ( // ClientType is used by other packages as the identifier of the Redfish client. ClientType string = "redfish" - mediaEjectDelay = 30 * time.Second systemActionRetries = 30 systemRebootDelay = 2 * time.Second ) @@ -58,7 +57,7 @@ func (c *Client) RebootSystem(ctx context.Context, systemID string) error { totalRetries = systemActionRetries } - for retry := 0; retry <= totalRetries; retry++ { + for retry := 0; retry < totalRetries; retry++ { system, httpResp, err := c.RedfishAPI.GetSystem(ctx, systemID) if err = ScreenRedfishError(httpResp, err); err != nil { return err @@ -68,7 +67,10 @@ func (c *Client) RebootSystem(ctx context.Context, systemID string) error { } time.Sleep(systemRebootDelay) } - return ErrOperationRetriesExceeded{} + return ErrOperationRetriesExceeded{ + What: fmt.Sprintf("reboot system %s", c.EphemeralNodeID()), + Retries: totalRetries, + } } resetReq := redfishClient.ResetRequestBody{} @@ -127,6 +129,28 @@ func (c *Client) SetEphemeralBootSourceByType(ctx context.Context) error { // SetVirtualMedia injects a virtual media device to an established virtual media ID. This assumes that isoPath is // accessible to the redfish server and virtualMedia device is either of type CD or DVD. func (c *Client) SetVirtualMedia(ctx context.Context, isoPath string) error { + waitForEjectMedia := func(managerID string, vMediaID string) error { + // Check if number of retries is defined in context + totalRetries, ok := ctx.Value("numRetries").(int) + if !ok { + totalRetries = systemActionRetries + } + + for retry := 0; retry < totalRetries; retry++ { + vMediaMgr, httpResp, err := c.RedfishAPI.GetManagerVirtualMedia(ctx, managerID, vMediaID) + if err = ScreenRedfishError(httpResp, err); err != nil { + return err + } + + if *vMediaMgr.Inserted == false { + log.Debugf("Successfully ejected virtual media.") + return nil + } + } + + return ErrOperationRetriesExceeded{What: fmt.Sprintf("eject media %s", vMediaID), Retries: totalRetries} + } + log.Debugf("Ephemeral Node System ID: '%s'", c.ephemeralNodeID) managerID, err := GetManagerID(ctx, c.RedfishAPI, c.ephemeralNodeID) @@ -148,13 +172,17 @@ func (c *Client) SetVirtualMedia(ctx context.Context, isoPath string) error { } if *vMediaMgr.Inserted == true { + log.Debugf("Manager %s media type %s inserted. Attempting to eject.", managerID, vMediaID) + var emptyBody map[string]interface{} _, httpResp, err = c.RedfishAPI.EjectVirtualMedia(ctx, managerID, vMediaID, emptyBody) if err = ScreenRedfishError(httpResp, err); err != nil { return err } - time.Sleep(mediaEjectDelay) + if err = waitForEjectMedia(managerID, vMediaID); err != nil { + return err + } } vMediaReq := redfishClient.InsertMediaRequestBody{} diff --git a/pkg/remote/redfish/client_test.go b/pkg/remote/redfish/client_test.go index 309eed1b9..c312d81a4 100644 --- a/pkg/remote/redfish/client_test.go +++ b/pkg/remote/redfish/client_test.go @@ -163,7 +163,7 @@ func TestRebootSystemTimeout(t *testing.T) { client.RedfishAPI = m err = client.RebootSystem(ctx, systemID) - assert.Equal(t, ErrOperationRetriesExceeded{}, err) + assert.Equal(t, ErrOperationRetriesExceeded{What: "reboot system ephemeral-node-id", Retries: 1}, err) } func TestSetEphemeralBootSourceByTypeGetSystemError(t *testing.T) { @@ -235,6 +235,46 @@ func TestSetEphemeralBootSourceByTypeBootSourceUnavailable(t *testing.T) { assert.True(t, ok) } +func TestSetVirtualMediaEjectVirtualMedia(t *testing.T) { + m := &redfishMocks.RedfishAPI{} + defer m.AssertExpectations(t) + + systemID := ephemeralNodeID + _, client, err := NewClient(systemID, isoPath, redfishURL, false, false, "", "") + assert.NoError(t, err) + + // Normal retries are 30. Limit them here for test time. + ctx := context.WithValue(context.Background(), "numRetries", 1) + + // Mark test media as inserted + inserted := true + testMedia := testutil.GetVirtualMedia([]string{"CD"}) + testMedia.Inserted = &inserted + + httpResp := &http.Response{StatusCode: 200} + m.On("GetSystem", ctx, client.ephemeralNodeID).Return(testutil.GetTestSystem(), httpResp, nil) + m.On("ListManagerVirtualMedia", ctx, testutil.ManagerID).Times(1). + Return(testutil.GetMediaCollection([]string{"Cd"}), httpResp, nil) + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Cd").Times(1). + Return(testMedia, httpResp, nil) + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Cd").Times(1). + Return(testMedia, httpResp, nil) + + // Verify retry logic + m.On("EjectVirtualMedia", ctx, testutil.ManagerID, "Cd", mock.Anything).Times(1). + Return(redfishClient.RedfishError{}, httpResp, nil) + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Cd").Times(1). + Return(testutil.GetVirtualMedia([]string{"Cd"}), httpResp, nil) + m.On("InsertVirtualMedia", ctx, testutil.ManagerID, "Cd", mock.Anything).Return( + redfishClient.RedfishError{}, httpResp, redfishClient.GenericOpenAPIError{}) + + // Replace normal API client with mocked API client + client.RedfishAPI = m + + err = client.SetVirtualMedia(ctx, client.isoPath) + assert.NoError(t, err) +} + func TestSetVirtualMediaGetSystemError(t *testing.T) { m := &redfishMocks.RedfishAPI{} defer m.AssertExpectations(t) @@ -253,6 +293,46 @@ func TestSetVirtualMediaGetSystemError(t *testing.T) { assert.Error(t, err) } +func TestSetVirtualMediaEjectVirtualMediaRetriesExceeded(t *testing.T) { + m := &redfishMocks.RedfishAPI{} + defer m.AssertExpectations(t) + + systemID := ephemeralNodeID + _, client, err := NewClient(systemID, isoPath, redfishURL, false, false, "", "") + assert.NoError(t, err) + + ctx := context.WithValue(context.Background(), "numRetries", 1) + + // Mark test media as inserted + inserted := true + testMedia := testutil.GetVirtualMedia([]string{"CD"}) + testMedia.Inserted = &inserted + + httpResp := &http.Response{StatusCode: 200} + m.On("GetSystem", ctx, client.ephemeralNodeID).Return(testutil.GetTestSystem(), httpResp, nil) + m.On("ListManagerVirtualMedia", ctx, testutil.ManagerID).Times(1). + Return(testutil.GetMediaCollection([]string{"Cd"}), httpResp, nil) + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Cd").Times(1). + Return(testMedia, httpResp, nil) + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Cd").Times(1). + Return(testMedia, httpResp, nil) + + // Verify retry logic + m.On("EjectVirtualMedia", ctx, testutil.ManagerID, "Cd", mock.Anything).Times(1). + Return(redfishClient.RedfishError{}, httpResp, nil) + + // Media still inserted on retry. Since retries are 1, this causes failure. + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Cd").Times(1). + Return(testMedia, httpResp, nil) + + // Replace normal API client with mocked API client + client.RedfishAPI = m + + err = client.SetVirtualMedia(ctx, client.isoPath) + _, ok := err.(ErrOperationRetriesExceeded) + assert.True(t, ok) +} + func TestSetVirtualMediaInsertVirtualMediaError(t *testing.T) { m := &redfishMocks.RedfishAPI{} defer m.AssertExpectations(t) diff --git a/pkg/remote/redfish/errors.go b/pkg/remote/redfish/errors.go index 2f2393140..0299a25ee 100644 --- a/pkg/remote/redfish/errors.go +++ b/pkg/remote/redfish/errors.go @@ -41,8 +41,10 @@ func (e ErrRedfishMissingConfig) Error() string { // ErrOperationRetriesExceeded raised if number of operation retries exceeded type ErrOperationRetriesExceeded struct { + What string + Retries int } func (e ErrOperationRetriesExceeded) Error() string { - return "maximum retries exceeded" + return fmt.Sprintf("operation %s failed. Maximum retries (%d) exceeded", e.What, e.Retries) }