From decc12b0a9021d9f139fedeee1fbe59448ab0c41 Mon Sep 17 00:00:00 2001 From: Kostiantyn Kalynovskyi Date: Thu, 21 Jan 2021 22:33:37 +0000 Subject: [PATCH] Move remotedirect method to remote interface Please note that this is first commit in series and in next commits remote/remote_direct* files will be remove completely, so if you have any comments that address issues in these files, keep in mind that they will be remove in next patchsets. Change-Id: I52e9b8438a4f8e1a2395cbe90238b9cb654772d4 --- pkg/remote/ifc/ifc.go | 1 + pkg/remote/redfish/client.go | 49 ++++++++- pkg/remote/redfish/client_test.go | 71 +++++++++++++ pkg/remote/redfish/utils.go | 6 +- pkg/remote/remote_direct.go | 42 +------- pkg/remote/remote_direct_test.go | 151 ++------------------------- testutil/inventory/inventory.go | 80 ++++++++++++++ testutil/redfishutils/mock_client.go | 20 ++-- 8 files changed, 228 insertions(+), 192 deletions(-) create mode 100644 testutil/inventory/inventory.go diff --git a/pkg/remote/ifc/ifc.go b/pkg/remote/ifc/ifc.go index c44117a08..4c841eff7 100644 --- a/pkg/remote/ifc/ifc.go +++ b/pkg/remote/ifc/ifc.go @@ -30,6 +30,7 @@ type Client interface { SystemPowerOff(context.Context) error SystemPowerOn(context.Context) error SystemPowerStatus(context.Context) (power.Status, error) + RemoteDirect(context.Context, string) error // TODO(drewwalters96): This function is tightly coupled to Redfish. It should be combined with the // SetBootSource operation and removed from the client interface. diff --git a/pkg/remote/redfish/client.go b/pkg/remote/redfish/client.go index 773fe276d..04e9ec0fb 100644 --- a/pkg/remote/redfish/client.go +++ b/pkg/remote/redfish/client.go @@ -38,6 +38,7 @@ type Client struct { nodeID string username string password string + redfishURL string RedfishAPI redfishAPI.RedfishAPI RedfishCFG *redfishClient.Configuration systemActionRetries int @@ -254,8 +255,8 @@ func (c *Client) SystemPowerOn(ctx context.Context) error { func (c *Client) SystemPowerStatus(ctx context.Context) (power.Status, error) { ctx = SetAuth(ctx, c.username, c.password) computerSystem, httpResp, err := c.RedfishAPI.GetSystem(ctx, c.nodeID) - if err = ScreenRedfishError(httpResp, err); err != nil { - return power.StatusUnknown, err + if screenErr := ScreenRedfishError(httpResp, err); screenErr != nil { + return power.StatusUnknown, screenErr } switch computerSystem.PowerState { @@ -272,6 +273,49 @@ func (c *Client) SystemPowerStatus(ctx context.Context) (power.Status, error) { } } +// RemoteDirect implements remote direct interface +func (c *Client) RemoteDirect(ctx context.Context, isoURL string) error { + log.Debugf("Bootstrapping ephemeral host with ID '%s' and BMC Address '%s'.", c.NodeID(), + c.redfishURL) + + powerStatus, err := c.SystemPowerStatus(ctx) + if err != nil { + return err + } + + // Power on node if it is off + if powerStatus != power.StatusOn { + log.Debugf("Ephemeral node has power status '%s'. Attempting to power on.", powerStatus.String()) + if err = c.SystemPowerOn(ctx); err != nil { + return err + } + } + + // Perform remote direct operations + if isoURL == "" { + return ErrRedfishMissingConfig{What: "isoURL"} + } + + err = c.SetVirtualMedia(ctx, isoURL) + if err != nil { + return err + } + + err = c.SetBootSourceByType(ctx) + if err != nil { + return err + } + + err = c.RebootSystem(ctx) + if err != nil { + return err + } + + log.Printf("Successfully bootstrapped ephemeral host '%s'.", c.NodeID()) + + return nil +} + // NewClient returns a client with the capability to make Redfish requests. func NewClient(redfishURL string, insecure bool, @@ -330,6 +374,7 @@ func NewClient(redfishURL string, systemRebootDelay: systemRebootDelay, password: password, username: username, + redfishURL: redfishURL, Sleep: func(d time.Duration) { time.Sleep(d) diff --git a/pkg/remote/redfish/client_test.go b/pkg/remote/redfish/client_test.go index f6cc5026f..31ec0114b 100644 --- a/pkg/remote/redfish/client_test.go +++ b/pkg/remote/redfish/client_test.go @@ -914,3 +914,74 @@ func TestWaitForPowerStateDifferentPowerState(t *testing.T) { err = client.waitForPowerState(ctx, redfishClient.POWERSTATE_ON) assert.NoError(t, err) } + +func TestRemoteDirect(t *testing.T) { + m := &redfishMocks.RedfishAPI{} + + client, err := NewClient(redfishURL, false, false, "", "", systemActionRetries, systemRebootDelay) + require.NoError(t, err) + + client.RedfishAPI = m + + inserted := true + testMediaCD := testutil.GetVirtualMedia([]string{"CD"}) + testMediaCD.Inserted = &inserted + resetReq := redfishClient.ResetRequestBody{ + ResetType: redfishClient.RESETTYPE_FORCE_OFF, + } + httpResp := &http.Response{StatusCode: 200} + system := redfishClient.ComputerSystem{ + PowerState: redfishClient.POWERSTATE_ON, + Links: redfishClient.SystemLinks{ + ManagedBy: []redfishClient.IdRef{ + {OdataId: testutil.ManagerID}, + }, + }, + Boot: redfishClient.Boot{ + BootSourceOverrideTargetRedfishAllowableValues: []redfishClient.BootSource{ + redfishClient.BOOTSOURCE_CD, + }, + }} + + ctx := SetAuth(context.Background(), "", "") + + m.On("GetSystem", ctx, client.nodeID).Return(system, httpResp, nil).Times(6) + m.On("ListManagerVirtualMedia", ctx, testutil.ManagerID). + Return(testutil.GetMediaCollection([]string{"Cd", "DVD", "Floppy"}), httpResp, nil) + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Cd").Times(1). + Return(testMediaCD, httpResp, nil) + 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("GetManagerVirtualMedia", ctx, testutil.ManagerID, "DVD").Times(1). + Return(testutil.GetVirtualMedia([]string{"DVD"}), httpResp, nil) + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Floppy").Times(1). + Return(testutil.GetVirtualMedia([]string{"Floppy"}), httpResp, nil) + + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Cd").Times(1). + Return(testMediaCD, httpResp, nil) + + m.On("InsertVirtualMedia", ctx, testutil.ManagerID, "Cd", mock.Anything).Return( + redfishClient.RedfishError{}, httpResp, redfishClient.GenericOpenAPIError{}) + + m.On("GetManagerVirtualMedia", ctx, testutil.ManagerID, "Cd").Times(1). + Return(testMediaCD, httpResp, nil) + + m.On("SetSystem", ctx, client.nodeID, mock.Anything).Times(1).Return( + redfishClient.ComputerSystem{}, httpResp, nil) + + m.On("ResetSystem", ctx, client.nodeID, resetReq).Times(1).Return(redfishClient.RedfishError{}, httpResp, nil) + offSystem := system + offSystem.PowerState = redfishClient.POWERSTATE_OFF + m.On("GetSystem", ctx, client.nodeID).Return(offSystem, httpResp, nil).Times(1) + + m.On("ResetSystem", ctx, client.nodeID, redfishClient.ResetRequestBody{ + ResetType: redfishClient.RESETTYPE_ON, + }).Times(1).Return(redfishClient.RedfishError{}, httpResp, nil) + + m.On("GetSystem", ctx, client.nodeID).Return(system, httpResp, nil).Times(1) + + err = client.RemoteDirect(ctx, "http://some-url") + assert.NoError(t, err) +} diff --git a/pkg/remote/redfish/utils.go b/pkg/remote/redfish/utils.go index 8e08e3173..8e05f7421 100644 --- a/pkg/remote/redfish/utils.go +++ b/pkg/remote/redfish/utils.go @@ -187,8 +187,12 @@ func GetVirtualMediaID(ctx context.Context, api redfishAPI.RedfishAPI, systemID // responses and errors. func ScreenRedfishError(httpResp *http.Response, clientErr error) error { if httpResp == nil { + m := "HTTP request failed. Redfish may be temporarily unavailable. Please try again." + if clientErr != nil { + m += fmt.Sprintf(" original error %s", clientErr.Error()) + } return ErrRedfishClient{ - Message: "HTTP request failed. Redfish may be temporarily unavailable. Please try again.", + Message: m, } } diff --git a/pkg/remote/remote_direct.go b/pkg/remote/remote_direct.go index 4a972a9fe..ecce0c8d8 100644 --- a/pkg/remote/remote_direct.go +++ b/pkg/remote/remote_direct.go @@ -20,10 +20,8 @@ import ( api "opendev.org/airship/airshipctl/pkg/api/v1alpha1" "opendev.org/airship/airshipctl/pkg/config" "opendev.org/airship/airshipctl/pkg/document" - "opendev.org/airship/airshipctl/pkg/log" "opendev.org/airship/airshipctl/pkg/phase" "opendev.org/airship/airshipctl/pkg/phase/ifc" - "opendev.org/airship/airshipctl/pkg/remote/power" ) // DoRemoteDirect bootstraps the ephemeral node. @@ -64,43 +62,5 @@ func (b baremetalHost) DoRemoteDirect(cfg *config.Config) error { return err } - log.Debugf("Bootstrapping ephemeral host '%s' with ID '%s' and BMC Address '%s'.", b.HostName, b.NodeID(), - b.BMCAddress) - - powerStatus, err := b.SystemPowerStatus(context.Background()) - if err != nil { - return err - } - - // Power on node if it is off - if powerStatus != power.StatusOn { - log.Debugf("Ephemeral node has power status '%s'. Attempting to power on.", powerStatus.String()) - if err = b.SystemPowerOn(context.Background()); err != nil { - return err - } - } - - // Perform remote direct operations - if remoteDirectConfiguration.IsoURL == "" { - return ErrMissingOption{What: "isoURL"} - } - - err = b.SetVirtualMedia(context.Background(), remoteDirectConfiguration.IsoURL) - if err != nil { - return err - } - - err = b.SetBootSourceByType(context.Background()) - if err != nil { - return err - } - - err = b.RebootSystem(context.Background()) - if err != nil { - return err - } - - log.Printf("Successfully bootstrapped ephemeral host '%s'.", b.HostName) - - return nil + return b.RemoteDirect(context.Background(), remoteDirectConfiguration.IsoURL) } diff --git a/pkg/remote/remote_direct_test.go b/pkg/remote/remote_direct_test.go index 287a4d4ec..768a67753 100644 --- a/pkg/remote/remote_direct_test.go +++ b/pkg/remote/remote_direct_test.go @@ -15,20 +15,16 @@ package remote import ( - "context" "fmt" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" - "opendev.org/airship/airshipctl/pkg/remote/power" - "opendev.org/airship/airshipctl/pkg/remote/redfish" "opendev.org/airship/airshipctl/testutil/redfishutils" ) const ( - systemID = "System.Embedded.1" - isoURL = "https://localhost:8080/ubuntu.iso" redfishURL = "redfish+https://localhost:2344/Systems/System.Embedded.1" username = "admin" password = "password" @@ -55,42 +51,11 @@ func TestDoRemoteDirectMissingConfigOpts(t *testing.T) { assert.Error(t, err) } -func TestDoRemoteDirectMissingISOURL(t *testing.T) { - rMock, err := redfishutils.NewClient(redfishURL, false, false, username, password) - assert.NoError(t, err) - - ctx := context.Background() - - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("SystemPowerStatus", ctx).Times(1).Return(power.StatusOn, nil) - - ephemeralHost := baremetalHost{ - rMock, - redfishURL, - "doc-name", - username, - password, - } - - settings := initSettings(t, withTestDataPath("noisourl")) - err = ephemeralHost.DoRemoteDirect(settings) - expectedErrorMessage := `missing option: isoURL` - assert.Equal(t, expectedErrorMessage, fmt.Sprintf("%s", err)) - assert.Error(t, err) -} - func TestDoRemoteDirectRedfish(t *testing.T) { rMock, err := redfishutils.NewClient(redfishURL, false, false, username, password) - assert.NoError(t, err) + require.NoError(t, err) - ctx := context.Background() - - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("SystemPowerStatus", ctx).Times(1).Return(power.StatusOn, nil) - rMock.On("SetVirtualMedia", ctx, isoURL).Times(1).Return(nil) - rMock.On("SetBootSourceByType", ctx).Times(1).Return(nil) - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("RebootSystem", ctx).Times(1).Return(nil) + rMock.On("RemoteDirect").Times(1).Return(nil) ephemeralHost := baremetalHost{ rMock, @@ -105,19 +70,12 @@ func TestDoRemoteDirectRedfish(t *testing.T) { assert.NoError(t, err) } -func TestDoRemoteDirectRedfishNodePoweredOff(t *testing.T) { +func TestDoRemoteDirectError(t *testing.T) { rMock, err := redfishutils.NewClient(redfishURL, false, false, username, password) - assert.NoError(t, err) + require.NoError(t, err) - ctx := context.Background() - - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("SystemPowerStatus", ctx).Times(1).Return(power.StatusOff, nil) - rMock.On("SystemPowerOn", ctx).Times(1).Return(nil) - rMock.On("SetVirtualMedia", ctx, isoURL).Times(1).Return(nil) - rMock.On("SetBootSourceByType", ctx).Times(1).Return(nil) - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("RebootSystem", ctx).Times(1).Return(nil) + expectedErr := fmt.Errorf("remote direct error") + rMock.On("RemoteDirect").Times(1).Return(expectedErr) ephemeralHost := baremetalHost{ rMock, @@ -128,97 +86,6 @@ func TestDoRemoteDirectRedfishNodePoweredOff(t *testing.T) { } settings := initSettings(t, withTestDataPath("base")) - err = ephemeralHost.DoRemoteDirect(settings) - assert.NoError(t, err) -} - -func TestDoRemoteDirectRedfishVirtualMediaError(t *testing.T) { - rMock, err := redfishutils.NewClient(redfishURL, false, false, username, password) - assert.NoError(t, err) - - ctx := context.Background() - - expectedErr := redfish.ErrRedfishClient{Message: "Unable to set virtual media."} - - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("SystemPowerStatus", ctx).Times(1).Return(power.StatusOn, nil) - rMock.On("SetVirtualMedia", ctx, isoURL).Times(1).Return(expectedErr) - rMock.On("SetBootSourceByType", ctx).Times(1).Return(nil) - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("RebootSystem", ctx).Times(1).Return(nil) - - ephemeralHost := baremetalHost{ - rMock, - redfishURL, - "doc-name", - username, - password, - } - - settings := initSettings(t, withTestDataPath("base")) - - err = ephemeralHost.DoRemoteDirect(settings) - _, ok := err.(redfish.ErrRedfishClient) - assert.True(t, ok) -} - -func TestDoRemoteDirectRedfishBootSourceError(t *testing.T) { - rMock, err := redfishutils.NewClient(redfishURL, false, false, username, password) - assert.NoError(t, err) - - ctx := context.Background() - - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("SystemPowerStatus", ctx).Times(1).Return(power.StatusOn, nil) - rMock.On("SetVirtualMedia", ctx, isoURL).Times(1).Return(nil) - - expectedErr := redfish.ErrRedfishClient{Message: "Unable to set boot source."} - rMock.On("SetBootSourceByType", ctx).Times(1).Return(expectedErr) - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("RebootSystem", ctx).Times(1).Return(nil) - - ephemeralHost := baremetalHost{ - rMock, - redfishURL, - "doc-name", - username, - password, - } - - settings := initSettings(t, withTestDataPath("base")) - - err = ephemeralHost.DoRemoteDirect(settings) - _, ok := err.(redfish.ErrRedfishClient) - assert.True(t, ok) -} - -func TestDoRemoteDirectRedfishRebootError(t *testing.T) { - rMock, err := redfishutils.NewClient(redfishURL, false, false, username, password) - assert.NoError(t, err) - - ctx := context.Background() - - rMock.On("NodeID").Times(1).Return(systemID) - rMock.On("SystemPowerStatus", ctx).Times(1).Return(power.StatusOn, nil) - rMock.On("SetVirtualMedia", ctx, isoURL).Times(1).Return(nil) - rMock.On("SetVirtualMedia", ctx, isoURL).Times(1).Return(nil) - rMock.On("SetBootSourceByType", ctx).Times(1).Return(nil) - rMock.On("NodeID").Times(1).Return(systemID) - - expectedErr := redfish.ErrRedfishClient{Message: "Unable to set boot source."} - rMock.On("RebootSystem", ctx).Times(1).Return(expectedErr) - - ephemeralHost := baremetalHost{ - rMock, - redfishURL, - "doc-name", - username, - password, - } - - settings := initSettings(t, withTestDataPath("base")) - - err = ephemeralHost.DoRemoteDirect(settings) - _, ok := err.(redfish.ErrRedfishClient) - assert.True(t, ok) + actualErr := ephemeralHost.DoRemoteDirect(settings) + assert.Equal(t, expectedErr, actualErr) } diff --git a/testutil/inventory/inventory.go b/testutil/inventory/inventory.go new file mode 100644 index 000000000..5a002205c --- /dev/null +++ b/testutil/inventory/inventory.go @@ -0,0 +1,80 @@ +/* + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package inventory + +import ( + "context" + + "github.com/stretchr/testify/mock" + + "opendev.org/airship/airshipctl/pkg/inventory/ifc" + remoteifc "opendev.org/airship/airshipctl/pkg/remote/ifc" +) + +var _ ifc.Inventory = &MockInventory{} + +// MockInventory mocks ifc.Inventory interface +type MockInventory struct { + mock.Mock +} + +// BaremetalInventory mock +func (i *MockInventory) BaremetalInventory() (ifc.BaremetalInventory, error) { + args := i.Called() + err := args.Error(1) + bmhInv, ok := args.Get(0).(ifc.BaremetalInventory) + if !ok { + return nil, err + } + return bmhInv, err +} + +var _ ifc.BaremetalInventory = &MockBMHInventory{} + +// MockBMHInventory mocks ifc.BaremetalInventory +type MockBMHInventory struct { + mock.Mock +} + +// Select mock +func (i *MockBMHInventory) Select(ifc.BaremetalHostSelector) ([]remoteifc.Client, error) { + args := i.Called() + err := args.Error(1) + hosts, ok := args.Get(0).([]remoteifc.Client) + if !ok { + return nil, err + } + return hosts, nil +} + +// SelectOne mock +func (i *MockBMHInventory) SelectOne(ifc.BaremetalHostSelector) (remoteifc.Client, error) { + args := i.Called() + err := args.Error(1) + host, ok := args.Get(0).(remoteifc.Client) + if !ok { + return nil, err + } + return host, nil +} + +// RunOperation mock +func (i *MockBMHInventory) RunOperation( + context.Context, + ifc.BaremetalOperation, + ifc.BaremetalHostSelector, + ifc.BaremetalBatchRunOptions) error { + return i.Called().Error(0) +} diff --git a/testutil/redfishutils/mock_client.go b/testutil/redfishutils/mock_client.go index b7d67c079..a958f6675 100644 --- a/testutil/redfishutils/mock_client.go +++ b/testutil/redfishutils/mock_client.go @@ -63,7 +63,7 @@ func (m *MockClient) EjectVirtualMedia(ctx context.Context) error { // // err := client.RebootSystem() func (m *MockClient) RebootSystem(ctx context.Context) error { - args := m.Called(ctx) + args := m.Called() return args.Error(0) } @@ -76,7 +76,7 @@ func (m *MockClient) RebootSystem(ctx context.Context) error { // // err := client.SetBootSourceByType() func (m *MockClient) SetBootSourceByType(ctx context.Context) error { - args := m.Called(ctx) + args := m.Called() return args.Error(0) } @@ -89,7 +89,7 @@ func (m *MockClient) SetBootSourceByType(ctx context.Context) error { // // err := client.SetVirtualMedia() func (m *MockClient) SetVirtualMedia(ctx context.Context, isoPath string) error { - args := m.Called(ctx, isoPath) + args := m.Called() return args.Error(0) } @@ -102,7 +102,7 @@ func (m *MockClient) SetVirtualMedia(ctx context.Context, isoPath string) error // // err := client.SystemPowerOff() func (m *MockClient) SystemPowerOff(ctx context.Context) error { - args := m.Called(ctx) + args := m.Called() return args.Error(0) } @@ -115,7 +115,7 @@ func (m *MockClient) SystemPowerOff(ctx context.Context) error { // // err := client.SystemPowerOn() func (m *MockClient) SystemPowerOn(ctx context.Context) error { - args := m.Called(ctx) + args := m.Called() return args.Error(0) } @@ -128,7 +128,7 @@ func (m *MockClient) SystemPowerOn(ctx context.Context) error { // // err := client.SystemPowerStatus() func (m *MockClient) SystemPowerStatus(ctx context.Context) (power.Status, error) { - args := m.Called(ctx) + args := m.Called() powerStatus, ok := args.Get(0).(power.Status) if !ok { return power.StatusUnknown, args.Error(2) @@ -137,6 +137,14 @@ func (m *MockClient) SystemPowerStatus(ctx context.Context) (power.Status, error return powerStatus, args.Error(1) } +// RemoteDirect mocks remote client interface +func (m *MockClient) RemoteDirect(ctx context.Context, isoURL string) error { + if isoURL == "" { + return redfish.ErrRedfishMissingConfig{What: "isoURL"} + } + return m.Called().Error(0) +} + // NewClient returns a mocked Redfish client in order to test functions that use the Redfish client without making any // Redfish API calls. func NewClient(redfishURL string, insecure bool, useProxy bool, username string,