From 69e8c886fe15a501f401ce34b9632b2b013fcfa6 Mon Sep 17 00:00:00 2001 From: Drew Walters Date: Thu, 30 Apr 2020 20:42:50 +0000 Subject: [PATCH] Add strong typing for baremetal power statuses Baremetal host power statuses are handled as strings in airshipctl. Invoking remotedirect on a host that is powered off will result in a remotedirect failure; therefore, the power status of a host needs to be verified before remotedirect can begin. This change adds strict typing to baremetal power statuses so airshipctl can verify the status of a remote host before performing remotedirect regardless of the client type (e.g. redfish, other). Change-Id: I22f1784006add018ee1d67c18f94499aa9544752 Signed-off-by: Drew Walters --- pkg/remote/management.go | 12 ++---- pkg/remote/power/status.go | 45 ++++++++++++++++++++++ pkg/remote/power/status_test.go | 56 ++++++++++++++++++++++++++++ pkg/remote/redfish/client.go | 18 +++++++-- testutil/redfishutils/mock_client.go | 10 ++++- 5 files changed, 128 insertions(+), 13 deletions(-) create mode 100644 pkg/remote/power/status.go create mode 100644 pkg/remote/power/status_test.go diff --git a/pkg/remote/management.go b/pkg/remote/management.go index 577c6598a..6c45f4b52 100644 --- a/pkg/remote/management.go +++ b/pkg/remote/management.go @@ -22,6 +22,7 @@ import ( "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/environment" "opendev.org/airship/airshipctl/pkg/log" + "opendev.org/airship/airshipctl/pkg/remote/power" "opendev.org/airship/airshipctl/pkg/remote/redfish" redfishdell "opendev.org/airship/airshipctl/pkg/remote/redfish/vendors/dell" ) @@ -30,17 +31,12 @@ import ( // functions within client are used by power management commands and remote direct functionality. type Client interface { EjectVirtualMedia(context.Context) error + NodeID() string RebootSystem(context.Context) error + SetBootSourceByType(context.Context) error SystemPowerOff(context.Context) error SystemPowerOn(context.Context) error - - // TODO(drewwalters96): Should this be a string forever? We may want to define our own custom type, as the - // string format will be client dependent when we add new clients. - SystemPowerStatus(context.Context) (string, error) - - NodeID() string - - SetBootSourceByType(context.Context) error + SystemPowerStatus(context.Context) (power.Status, 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/power/status.go b/pkg/remote/power/status.go new file mode 100644 index 000000000..2e15862bb --- /dev/null +++ b/pkg/remote/power/status.go @@ -0,0 +1,45 @@ +/* + 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 power safely translates power information between different management clients. +package power + +const ( + // StatusUnknown indicates that a baremetal host is in an unknown power state. + StatusUnknown Status = iota + // StatusOn indicates that a baremetal host is powered on. + StatusOn + // StatusOff indicates that a baremetal host is not powered on. + StatusOff + // StatusPoweringOn indicates that a baremetal host is powering on. + StatusPoweringOn + // StatusPoweringOff indicates that a baremetal host is powering off. + StatusPoweringOff +) + +// Status indicates the power status of a baremetal host e.g. on, off, unknown. +type Status int + +var statusMap = [5]string{ + "UNKNOWN", + "ON", + "OFF", + "POWERING ON", + "POWERING OFF", +} + +// String provides a human-readable string value for a power status. +func (s Status) String() string { + return statusMap[s] +} diff --git a/pkg/remote/power/status_test.go b/pkg/remote/power/status_test.go new file mode 100644 index 000000000..10abc71b0 --- /dev/null +++ b/pkg/remote/power/status_test.go @@ -0,0 +1,56 @@ +/* + 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 power + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestStatus(t *testing.T) { + tests := []struct { + name string + status Status + }{ + { + name: "StatusUnknown", + status: StatusUnknown, + }, + { + name: "StatusOn", + status: StatusOn, + }, + { + name: "StatusOff", + status: StatusOff, + }, + { + name: "StatusPoweringOn", + status: StatusPoweringOn, + }, + { + name: "StatusPoweringOff", + status: StatusPoweringOff, + }, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, statusMap[test.status], test.status.String()) + }) + } +} diff --git a/pkg/remote/redfish/client.go b/pkg/remote/redfish/client.go index b4675918a..6a7fc8852 100644 --- a/pkg/remote/redfish/client.go +++ b/pkg/remote/redfish/client.go @@ -24,6 +24,7 @@ import ( redfishClient "opendev.org/airship/go-redfish/client" "opendev.org/airship/airshipctl/pkg/log" + "opendev.org/airship/airshipctl/pkg/remote/power" ) // contextKey is used by the redfish package as a unique key type in order to prevent collisions @@ -257,13 +258,24 @@ func (c *Client) SystemPowerOn(ctx context.Context) error { } // SystemPowerStatus retrieves the power status of a host as a human-readable string. -func (c *Client) SystemPowerStatus(ctx context.Context) (string, error) { +func (c *Client) SystemPowerStatus(ctx context.Context) (power.Status, error) { computerSystem, httpResp, err := c.RedfishAPI.GetSystem(ctx, c.nodeID) if err = ScreenRedfishError(httpResp, err); err != nil { - return "", err + return power.StatusUnknown, err } - return string(computerSystem.PowerState), nil + switch computerSystem.PowerState { + case redfishClient.POWERSTATE_ON: + return power.StatusOn, nil + case redfishClient.POWERSTATE_OFF: + return power.StatusOff, nil + case redfishClient.POWERSTATE_POWERING_ON: + return power.StatusPoweringOn, nil + case redfishClient.POWERSTATE_POWERING_OFF: + return power.StatusPoweringOff, nil + default: + return power.StatusUnknown, nil + } } // NewClient returns a client with the capability to make Redfish requests. diff --git a/testutil/redfishutils/mock_client.go b/testutil/redfishutils/mock_client.go index 333da719e..ac5829288 100644 --- a/testutil/redfishutils/mock_client.go +++ b/testutil/redfishutils/mock_client.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/mock" redfishClient "opendev.org/airship/go-redfish/client" + "opendev.org/airship/airshipctl/pkg/remote/power" "opendev.org/airship/airshipctl/pkg/remote/redfish" ) @@ -127,9 +128,14 @@ func (m *MockClient) SystemPowerOn(ctx context.Context) error { // client.On("SystemPowerStatus").Return() // // err := client.SystemPowerStatus() -func (m *MockClient) SystemPowerStatus(ctx context.Context) (string, error) { +func (m *MockClient) SystemPowerStatus(ctx context.Context) (power.Status, error) { args := m.Called(ctx) - return args.String(0), args.Error(1) + powerStatus, ok := args.Get(0).(power.Status) + if !ok { + return power.StatusUnknown, args.Error(2) + } + + return powerStatus, args.Error(1) } // NewClient returns a mocked Redfish client in order to test functions that use the Redfish client without making any