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) }