airship
/
airshipctl
Code Issues Proposed changes

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 <andrew.walters@att.com>
This commit is contained in:
Drew Walters 2020-04-16 22:53:14 +00:00
parent 2d65de45e9
commit 9a905cd9c4
3 changed files with 116 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
pkg/remote/redfish/client.go
View File

@ -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{}

82
pkg/remote/redfish/client_test.go
View File

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

4
pkg/remote/redfish/errors.go
View File

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