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

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

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 {
+	if screenErr := ScreenRedfishError(httpResp, err); screenErr != nil {
-		return power.StatusUnknown, err
+		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)

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

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

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(),
+	return b.RemoteDirect(context.Background(), remoteDirectConfiguration.IsoURL)
-		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
 }

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("RemoteDirect").Times(1).Return(nil)
-
-	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)
 
 	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()
+	expectedErr := fmt.Errorf("remote direct error")
-
+	rMock.On("RemoteDirect").Times(1).Return(expectedErr)
-	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)
 
 	ephemeralHost := baremetalHost{
 		rMock,
@@ -128,97 +86,6 @@ func TestDoRemoteDirectRedfishNodePoweredOff(t *testing.T) {
 	}
 
 	settings := initSettings(t, withTestDataPath("base"))
-	err = ephemeralHost.DoRemoteDirect(settings)
+	actualErr := ephemeralHost.DoRemoteDirect(settings)
-	assert.NoError(t, err)
+	assert.Equal(t, expectedErr, actualErr)
-}
-
-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)
 }

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

testutil/redfishutils/mock_client.go

@@ -63,7 +63,7 @@ func (m *MockClient) EjectVirtualMedia(ctx context.Context) error {
 //
 //         err := client.RebootSystem(<args>)
 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(<args>)
 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(<args>)
 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(<args>)
 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(<args>)
 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(<args>)
 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,