From 4fd4f9ac24b42153aa4c838fae2dddb330545256 Mon Sep 17 00:00:00 2001 From: Doctor Vince Date: Mon, 31 Jul 2023 15:26:11 -0400 Subject: [PATCH] make installing equal versions a no-op (#46) * make installing equal versions a no-op Here we introduce a "non-error" `sm.ErrNoAction` to signal to the task handler that there is nothing more to do (as though there were a real error condition), but that the task handler should continue processing additional firmware install tasks. The result of this change is that attempting to install firmware at a version already installed will not fail the task. This should avoid the need to force install a firmware set which may overwrite firmware already at the correct version and thus cost more time to apply a condition. * remove ErrNoAction but keep the tests --- go.mod | 1 + go.sum | 1 + internal/model/test/mock.go | 154 +++++++++++++ internal/outofband/action_handlers.go | 32 ++- internal/outofband/action_handlers_test.go | 240 +++++++++++++++++++++ internal/statemachine/actions.go | 26 +++ 6 files changed, 445 insertions(+), 9 deletions(-) create mode 100644 internal/model/test/mock.go diff --git a/go.mod b/go.mod index 7aaaea93..5e73a44b 100644 --- a/go.mod +++ b/go.mod @@ -62,6 +62,7 @@ require ( github.com/goccy/go-json v0.10.2 // indirect github.com/gofrs/uuid v4.4.0+incompatible // indirect github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect + github.com/golang/mock v1.6.0 // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/googleapis/gax-go/v2 v2.11.0 // indirect github.com/gosimple/slug v1.13.1 // indirect diff --git a/go.sum b/go.sum index 5d686f79..2a9800ce 100644 --- a/go.sum +++ b/go.sum @@ -1342,6 +1342,7 @@ github.com/golang/mock v1.4.1/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt github.com/golang/mock v1.4.3/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt3cw= github.com/golang/mock v1.4.4/go.mod h1:l3mdAwkq5BuhzHwde/uurv3sEJeZMXNpwsxVWU71h+4= github.com/golang/mock v1.5.0/go.mod h1:CWnOUgYIOo4TcNZ0wHX3YZCqsaM1I1Jvs6v3mP3KVu8= +github.com/golang/mock v1.6.0 h1:ErTB+efbowRARo13NNdxyJji2egdxLGQhRaY+DUumQc= github.com/golang/mock v1.6.0/go.mod h1:p6yTPP+5HYm5mzsMV8JkE6ZKdX+/wYM6Hr+LicevLPs= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= diff --git a/internal/model/test/mock.go b/internal/model/test/mock.go new file mode 100644 index 00000000..78f6ab38 --- /dev/null +++ b/internal/model/test/mock.go @@ -0,0 +1,154 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: internal/model/model.go + +// Package mock_model is a generated GoMock package. +package mock_model + +import ( + context "context" + os "os" + reflect "reflect" + + common "github.com/bmc-toolbox/common" + gomock "github.com/golang/mock/gomock" + model "github.com/metal-toolbox/flasher/internal/model" +) + +// MockDeviceQueryor is a mock of DeviceQueryor interface. +type MockDeviceQueryor struct { + ctrl *gomock.Controller + recorder *MockDeviceQueryorMockRecorder +} + +// MockDeviceQueryorMockRecorder is the mock recorder for MockDeviceQueryor. +type MockDeviceQueryorMockRecorder struct { + mock *MockDeviceQueryor +} + +// NewMockDeviceQueryor creates a new mock instance. +func NewMockDeviceQueryor(ctrl *gomock.Controller) *MockDeviceQueryor { + mock := &MockDeviceQueryor{ctrl: ctrl} + mock.recorder = &MockDeviceQueryorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockDeviceQueryor) EXPECT() *MockDeviceQueryorMockRecorder { + return m.recorder +} + +// Close mocks base method. +func (m *MockDeviceQueryor) Close(ctx context.Context) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Close", ctx) + ret0, _ := ret[0].(error) + return ret0 +} + +// Close indicates an expected call of Close. +func (mr *MockDeviceQueryorMockRecorder) Close(ctx interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockDeviceQueryor)(nil).Close), ctx) +} + +// FirmwareInstall mocks base method. +func (m *MockDeviceQueryor) FirmwareInstall(ctx context.Context, componentSlug string, force bool, file *os.File) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FirmwareInstall", ctx, componentSlug, force, file) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FirmwareInstall indicates an expected call of FirmwareInstall. +func (mr *MockDeviceQueryorMockRecorder) FirmwareInstall(ctx, componentSlug, force, file interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FirmwareInstall", reflect.TypeOf((*MockDeviceQueryor)(nil).FirmwareInstall), ctx, componentSlug, force, file) +} + +// FirmwareInstallStatus mocks base method. +func (m *MockDeviceQueryor) FirmwareInstallStatus(ctx context.Context, installVersion, componentSlug, bmcTaskID string) (model.ComponentFirmwareInstallStatus, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FirmwareInstallStatus", ctx, installVersion, componentSlug, bmcTaskID) + ret0, _ := ret[0].(model.ComponentFirmwareInstallStatus) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FirmwareInstallStatus indicates an expected call of FirmwareInstallStatus. +func (mr *MockDeviceQueryorMockRecorder) FirmwareInstallStatus(ctx, installVersion, componentSlug, bmcTaskID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FirmwareInstallStatus", reflect.TypeOf((*MockDeviceQueryor)(nil).FirmwareInstallStatus), ctx, installVersion, componentSlug, bmcTaskID) +} + +// Inventory mocks base method. +func (m *MockDeviceQueryor) Inventory(ctx context.Context) (*common.Device, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Inventory", ctx) + ret0, _ := ret[0].(*common.Device) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Inventory indicates an expected call of Inventory. +func (mr *MockDeviceQueryorMockRecorder) Inventory(ctx interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Inventory", reflect.TypeOf((*MockDeviceQueryor)(nil).Inventory), ctx) +} + +// Open mocks base method. +func (m *MockDeviceQueryor) Open(ctx context.Context) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Open", ctx) + ret0, _ := ret[0].(error) + return ret0 +} + +// Open indicates an expected call of Open. +func (mr *MockDeviceQueryorMockRecorder) Open(ctx interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Open", reflect.TypeOf((*MockDeviceQueryor)(nil).Open), ctx) +} + +// PowerStatus mocks base method. +func (m *MockDeviceQueryor) PowerStatus(ctx context.Context) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "PowerStatus", ctx) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// PowerStatus indicates an expected call of PowerStatus. +func (mr *MockDeviceQueryorMockRecorder) PowerStatus(ctx interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PowerStatus", reflect.TypeOf((*MockDeviceQueryor)(nil).PowerStatus), ctx) +} + +// ResetBMC mocks base method. +func (m *MockDeviceQueryor) ResetBMC(ctx context.Context) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResetBMC", ctx) + ret0, _ := ret[0].(error) + return ret0 +} + +// ResetBMC indicates an expected call of ResetBMC. +func (mr *MockDeviceQueryorMockRecorder) ResetBMC(ctx interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResetBMC", reflect.TypeOf((*MockDeviceQueryor)(nil).ResetBMC), ctx) +} + +// SetPowerState mocks base method. +func (m *MockDeviceQueryor) SetPowerState(ctx context.Context, state string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetPowerState", ctx, state) + ret0, _ := ret[0].(error) + return ret0 +} + +// SetPowerState indicates an expected call of SetPowerState. +func (mr *MockDeviceQueryorMockRecorder) SetPowerState(ctx, state interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetPowerState", reflect.TypeOf((*MockDeviceQueryor)(nil).SetPowerState), ctx, state) +} diff --git a/internal/outofband/action_handlers.go b/internal/outofband/action_handlers.go index bfa1df6f..2fb177ad 100644 --- a/internal/outofband/action_handlers.go +++ b/internal/outofband/action_handlers.go @@ -197,11 +197,25 @@ func (h *actionHandler) checkCurrentFirmware(a sw.StateSwitch, c sw.TransitionAr "vendor": action.Firmware.Vendor, "models": action.Firmware.Models, "err": ErrComponentNotFound, - }).Error("none matched given slug, vendor, model attributes") + }).Error("no component found for given component/vendor/model") - return errors.Wrap(ErrComponentNotFound, "no match based on given slug, vendor, model attributes") + return errors.Wrap(ErrComponentNotFound, + fmt.Sprintf("component: %s, vendor: %s, model: %s", action.Firmware.Component, + action.Firmware.Vendor, + action.Firmware.Models, + ), + ) } + tctx.Logger.WithFields( + logrus.Fields{ + "component": component.Slug, + "vendor": component.Vendor, + "model": component.Model, + "serial": component.Serial, + "firmware.version": component.FirmwareInstalled, + }).Debug("found component") + if component.FirmwareInstalled == "" { return errors.Wrap(ErrInstalledVersionUnknown, "set TaskParameters.Force=true to skip this check") } @@ -210,13 +224,13 @@ func (h *actionHandler) checkCurrentFirmware(a sw.StateSwitch, c sw.TransitionAr if equal { tctx.Logger.WithFields( logrus.Fields{ - "component": action.Firmware.Component, - "vendor": action.Firmware.Vendor, - "models": action.Firmware.Models, - "expectedVersion": action.Firmware.Version, - "installedVersion": component.FirmwareInstalled, - }).Error("Installed firmware version equals expected - set TaskParameters.Force=true to disable this check") - + "action id": action.ID, + "condition id": action.TaskID, + "component": action.Firmware.Component, + "vendor": action.Firmware.Vendor, + "models": action.Firmware.Models, + "expectedVersion": action.Firmware.Version, + }).Warn("Installed firmware version equals expected") return ErrInstalledFirmwareEqual } diff --git a/internal/outofband/action_handlers_test.go b/internal/outofband/action_handlers_test.go index efd1f211..43d4201f 100644 --- a/internal/outofband/action_handlers_test.go +++ b/internal/outofband/action_handlers_test.go @@ -1,14 +1,254 @@ package outofband import ( + "context" "os" "testing" + "github.com/bmc-toolbox/common" + sw "github.com/filanov/stateswitch" + "github.com/golang/mock/gomock" "github.com/metal-toolbox/flasher/internal/fixtures" "github.com/metal-toolbox/flasher/internal/model" + modeltest "github.com/metal-toolbox/flasher/internal/model/test" + sm "github.com/metal-toolbox/flasher/internal/statemachine" + "github.com/pkg/errors" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +type fakeStateSwitch struct{} + +func (_ fakeStateSwitch) State() sw.State { + return sw.State("bogus") +} +func (_ fakeStateSwitch) SetState(sw.State) error { + return nil +} + +func Test_checkCurrentFirmware(t *testing.T) { + t.Parallel() + hPtr := &actionHandler{} + t.Run("type-check error", func(t *testing.T) { + t.Parallel() + ctx := &sm.HandlerContext{} + fss := &fakeStateSwitch{} + err := hPtr.checkCurrentFirmware(fss, ctx) + require.Error(t, err) + require.ErrorIs(t, err, sm.ErrActionTypeAssertion) + }) + t.Run("inventory error", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + dq := modeltest.NewMockDeviceQueryor(ctrl) + ctx := &sm.HandlerContext{ + Ctx: context.Background(), + Logger: logrus.NewEntry(&logrus.Logger{}), + DeviceQueryor: dq, + } + act := &model.Action{ + VerifyCurrentFirmware: true, + Firmware: model.Firmware{ + Component: "the-component", + }, + } + dq.EXPECT().Inventory(gomock.Any()).Times(1).Return(nil, errors.New("pound sand")) + err := hPtr.checkCurrentFirmware(act, ctx) + require.Error(t, err) + require.Equal(t, "pound sand", err.Error()) + }) + t.Run("nil inventory", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + dq := modeltest.NewMockDeviceQueryor(ctrl) + ctx := &sm.HandlerContext{ + Ctx: context.Background(), + Logger: logrus.NewEntry(&logrus.Logger{}), + DeviceQueryor: dq, + } + act := &model.Action{ + VerifyCurrentFirmware: true, + Firmware: model.Firmware{ + Component: "the-component", + }, + } + dq.EXPECT().Inventory(gomock.Any()).Times(1).Return(nil, nil) + err := hPtr.checkCurrentFirmware(act, ctx) + require.Error(t, err) + require.ErrorIs(t, err, model.ErrComponentConverter) + }) + t.Run("bad component slug", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + dq := modeltest.NewMockDeviceQueryor(ctrl) + ctx := &sm.HandlerContext{ + Ctx: context.Background(), + Logger: logrus.NewEntry(&logrus.Logger{}), + DeviceQueryor: dq, + } + act := &model.Action{ + VerifyCurrentFirmware: true, + Firmware: model.Firmware{ + Component: "cool-bios", + Vendor: "Dell-icious", + Models: []string{ + "PowerEdge R6515", + }, + }, + } + dev := common.NewDevice() + dev.Model = "PowerEdge R6515" + dev.Vendor = "Dell-icious" + dev.BIOS.Vendor = "Dell-icious" + dev.BIOS.Model = "Bios Model" + dq.EXPECT().Inventory(gomock.Any()).Times(1).Return(&dev, nil) + err := hPtr.checkCurrentFirmware(act, ctx) + require.Error(t, err) + require.ErrorIs(t, err, ErrComponentNotFound) + t.Logf("error: %s\n", err.Error()) + }) + t.Run("blank installed version", func(t *testing.T) { + t.Parallel() + conversionDebug := false + ctrl := gomock.NewController(t) + defer ctrl.Finish() + dq := modeltest.NewMockDeviceQueryor(ctrl) + ctx := &sm.HandlerContext{ + Ctx: context.Background(), + Logger: logrus.NewEntry(&logrus.Logger{}), + DeviceQueryor: dq, + } + act := &model.Action{ + VerifyCurrentFirmware: true, + Firmware: model.Firmware{ + Component: "bios", + Vendor: "dell", + Models: []string{ + "r6515", + }, + }, + } + dev := common.NewDevice() + dev.Model = "PowerEdge R6515" + dev.Vendor = "Dell-icious" // NB: This and the bogus BIOS vendor are ignored. + dev.BIOS.Vendor = "Dell-icious" + dev.BIOS.Model = "r6515" + dev.BIOS.Serial = "12345" + + if conversionDebug { + exp, cErr := model.NewComponentConverter().CommonDeviceToComponents(&dev) + require.NoError(t, cErr) + t.Logf("expected components length: %d\n", len(exp)) + for i, c := range exp { + t.Logf("component %d => %#v\n", i, c) + } + } + + dq.EXPECT().Inventory(gomock.Any()).Times(1).Return(&dev, nil) + err := hPtr.checkCurrentFirmware(act, ctx) + require.Error(t, err) + require.ErrorIs(t, err, ErrInstalledVersionUnknown) + }) + t.Run("equal installed version", func(t *testing.T) { + t.Parallel() + conversionDebug := false + ctrl := gomock.NewController(t) + defer ctrl.Finish() + dq := modeltest.NewMockDeviceQueryor(ctrl) + ctx := &sm.HandlerContext{ + Ctx: context.Background(), + Logger: logrus.NewEntry(&logrus.Logger{}), + DeviceQueryor: dq, + } + act := &model.Action{ + TaskID: "my-task-uuid", + VerifyCurrentFirmware: true, + Firmware: model.Firmware{ + Component: "bios", + Vendor: "dell", + Models: []string{ + "r6515", + }, + Version: "the version", + }, + } + dev := common.NewDevice() + dev.Model = "PowerEdge R6515" + dev.Vendor = "Dell-icious" // NB: This and the bogus BIOS vendor are ignored. + dev.BIOS.Vendor = "Dell-icious" + dev.BIOS.Model = "r6515" + dev.BIOS.Serial = "12345" + dev.BIOS.Firmware = &common.Firmware{ + Installed: "the version", + } + + if conversionDebug { + exp, cErr := model.NewComponentConverter().CommonDeviceToComponents(&dev) + require.NoError(t, cErr) + t.Logf("expected components length: %d\n", len(exp)) + for i, c := range exp { + t.Logf("component %d => %#v\n", i, c) + } + } + + dq.EXPECT().Inventory(gomock.Any()).Times(1).Return(&dev, nil) + err := hPtr.checkCurrentFirmware(act, ctx) + require.Error(t, err) + require.ErrorIs(t, err, ErrInstalledFirmwareEqual) + }) + t.Run("installed version does not match", func(t *testing.T) { + t.Parallel() + conversionDebug := false + ctrl := gomock.NewController(t) + defer ctrl.Finish() + dq := modeltest.NewMockDeviceQueryor(ctrl) + ctx := &sm.HandlerContext{ + Ctx: context.Background(), + Logger: logrus.NewEntry(&logrus.Logger{}), + DeviceQueryor: dq, + } + act := &model.Action{ + TaskID: "my-task-uuid", + VerifyCurrentFirmware: true, + Firmware: model.Firmware{ + Component: "bios", + Vendor: "dell", // this case has to match the vendor derived from the library + Models: []string{ + "r6515", + }, + Version: "the new version", + }, + } + dev := common.NewDevice() + dev.Model = "PowerEdge R6515" + dev.Vendor = "Dell-icious" // NB: This and the bogus BIOS vendor are ignored. + dev.BIOS.Vendor = "Dell-icious" + dev.BIOS.Model = "r6515" + dev.BIOS.Serial = "12345" + dev.BIOS.Firmware = &common.Firmware{ + Installed: "the version", + } + + if conversionDebug { + exp, cErr := model.NewComponentConverter().CommonDeviceToComponents(&dev) + require.NoError(t, cErr) + t.Logf("expected components length: %d\n", len(exp)) + for i, c := range exp { + t.Logf("component %d => %#v\n", i, c) + } + } + + dq.EXPECT().Inventory(gomock.Any()).Times(1).Return(&dev, nil) + err := hPtr.checkCurrentFirmware(act, ctx) + require.NoError(t, err) + }) + +} + func Test_pollFirmwareInstallStatus(t *testing.T) { testcases := []struct { name string diff --git a/internal/statemachine/actions.go b/internal/statemachine/actions.go index e73e6062..28e5b89c 100644 --- a/internal/statemachine/actions.go +++ b/internal/statemachine/actions.go @@ -11,6 +11,7 @@ import ( "github.com/metal-toolbox/flasher/internal/model" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" + "github.com/sirupsen/logrus" ) const ( @@ -166,6 +167,13 @@ func (a *ActionStateMachine) Run(ctx context.Context, action *model.Action, tctx startTS := time.Now() // publish task action running + tctx.Logger.WithFields(logrus.Fields{ + "action": action.ID, + "condition": action.TaskID, + "component": action.Firmware.Component, + "version": action.Firmware.Version, + }).Info("running action") + tctx.Task.Status = fmt.Sprintf( "component: %s, running action: %s ", action.Firmware.Component, @@ -183,6 +191,12 @@ func (a *ActionStateMachine) Run(ctx context.Context, action *model.Action, tctx err := a.sm.Run(transitionType, action, tctx) if err != nil { + tctx.Logger.WithError(err).WithFields(logrus.Fields{ + "action": action.ID, + "condition": action.TaskID, + "component": action.Firmware.Component, + "version": action.Firmware.Version, + }).Info("action error") a.registerTransitionMetrics(startTS, action, string(transitionType), "failed") // When the condition returns false, run the next transition @@ -218,12 +232,24 @@ func (a *ActionStateMachine) Run(ctx context.Context, action *model.Action, tctx // publish task action completion if action.Final { + tctx.Logger.WithFields(logrus.Fields{ + "action": action.ID, + "condition": action.TaskID, + "component": action.Firmware.Component, + "version": action.Firmware.Version, + }).Info("final action complete") tctx.Task.Status = fmt.Sprintf( "component: %s, completed firmware install, version: %s", action.Firmware.Component, action.Firmware.Version, ) } else { + tctx.Logger.WithFields(logrus.Fields{ + "action": action.ID, + "condition": action.TaskID, + "component": action.Firmware.Component, + "version": action.Firmware.Version, + }).Info("action complete") tctx.Task.Status = fmt.Sprintf( "component: %s, completed action: %s ", action.Firmware.Component,