Skip to content

Commit

Permalink
v1/routes: CreateCondition to revert on condition publish failure
Browse files Browse the repository at this point in the history
  • Loading branch information
@joelrebel
joelrebel committed Aug 11, 2023
1 parent 3a3e1bd commit 9cb40ce
Show file tree
Hide file tree
Showing 2 changed files with 173 additions and 13 deletions.
20 changes: 16 additions & 4 deletions pkg/api/v1/routes/handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,7 @@ func (r *Routes) serverConditionCreate(c *gin.Context) (int, *v1types.ServerResp
}
}

// XXX: this operation can't ignore errors
// - if this method fails, should the condition be deleted?
//
// publish the condition
// publish the condition and in case of publish failure - revert.
err = r.publishCondition(otelCtx, serverID, facilityCode, condition)
if err != nil {
r.logger.WithFields(logrus.Fields{
Expand All @@ -290,6 +287,21 @@ func (r *Routes) serverConditionCreate(c *gin.Context) (int, *v1types.ServerResp
metrics.PublishErrors.With(
prometheus.Labels{"conditionKind": string(condition.Kind)},
).Inc()

deleteErr := r.repository.Delete(otelCtx, serverID, condition.Kind)
if deleteErr != nil {
r.logger.WithFields(logrus.Fields{
"error": deleteErr,
}).Info("condition deletion failed")

return http.StatusInternalServerError, &v1types.ServerResponse{
Message: deleteErr.Error(),
}
}

return http.StatusInternalServerError, &v1types.ServerResponse{
Message: "condition create failed: " + err.Error(),
}
}

metrics.ConditionQueued.With(
Expand Down
166 changes: 157 additions & 9 deletions pkg/api/v1/routes/handlers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,14 @@ import (
"github.com/metal-toolbox/conditionorc/internal/store"
v1types "github.com/metal-toolbox/conditionorc/pkg/api/v1/types"
ptypes "github.com/metal-toolbox/conditionorc/pkg/types"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"go.hollow.sh/toolbox/events"
mockevents "go.hollow.sh/toolbox/events/mock"
)

func mockserver(t *testing.T, logger *logrus.Logger, repository store.Repository) (*gin.Engine, error) {
func mockserver(t *testing.T, logger *logrus.Logger, repository store.Repository, stream events.Stream) (*gin.Engine, error) {
t.Helper()

gin.SetMode(gin.ReleaseMode)
Expand All @@ -39,6 +42,10 @@ func mockserver(t *testing.T, logger *logrus.Logger, repository store.Repository
),
}

if stream != nil {
options = append(options, WithStreamBroker(stream))
}

v1Router, err := NewRoutes(options...)
if err != nil {
return nil, err
Expand Down Expand Up @@ -117,7 +124,7 @@ func TestServerConditionUpdate(t *testing.T) {

repository := store.NewMockRepository(ctrl)

server, err := mockserver(t, logrus.New(), repository)
server, err := mockserver(t, logrus.New(), repository, nil)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -307,27 +314,35 @@ func TestServerConditionUpdate(t *testing.T) {
// nolint:gocyclo // cyclomatic tests are cyclomatic
func TestServerConditionCreate(t *testing.T) {
serverID := uuid.New()
facilityCode := "foo-42"

// mock repository
ctrl := gomock.NewController(t)
defer ctrl.Finish()

repository := store.NewMockRepository(ctrl)

server, err := mockserver(t, logrus.New(), repository)
streamCtrl := gomock.NewController(t)
defer streamCtrl.Finish()

stream := mockevents.NewMockStream(streamCtrl)

server, err := mockserver(t, logrus.New(), repository, stream)
if err != nil {
t.Fatal(err)
}

testcases := []struct {
name string
mockStore func(r *store.MockRepository)
mockStream func(r *mockevents.MockStream)
request func(t *testing.T) *http.Request
assertResponse func(t *testing.T, r *httptest.ResponseRecorder)
}{
{
"invalid server ID error",
nil,
nil,
func(t *testing.T) *http.Request {
url := fmt.Sprintf("/api/v1/servers/%s/condition/%s", "123", "invalid")
request, err := http.NewRequestWithContext(context.TODO(), http.MethodPost, url, http.NoBody)
Expand All @@ -345,6 +360,7 @@ func TestServerConditionCreate(t *testing.T) {
{
"invalid server condition state",
nil,
nil,
func(t *testing.T) *http.Request {
url := fmt.Sprintf("/api/v1/servers/%s/condition/%s", uuid.New().String(), "asdasd")
request, err := http.NewRequestWithContext(context.TODO(), http.MethodPost, url, http.NoBody)
Expand All @@ -362,6 +378,7 @@ func TestServerConditionCreate(t *testing.T) {
{
"invalid server condition payload returns error",
nil,
nil,
func(t *testing.T) *http.Request {
url := fmt.Sprintf("/api/v1/servers/%s/condition/%s", serverID.String(), ptypes.FirmwareInstall)
request, err := http.NewRequestWithContext(context.TODO(), http.MethodPost, url, bytes.NewReader([]byte(``)))
Expand Down Expand Up @@ -411,6 +428,7 @@ func TestServerConditionCreate(t *testing.T) {
).
Times(1)
},
nil,
func(t *testing.T) *http.Request {
payload, err := json.Marshal(&v1types.ConditionCreate{Parameters: []byte(`{"some param": "1"}`)})
if err != nil {
Expand Down Expand Up @@ -462,7 +480,7 @@ func TestServerConditionCreate(t *testing.T) {
gomock.Eq(serverID),
).
Return(
&model.Server{ID: serverID, FacilityCode: "foo-42"},
&model.Server{ID: serverID, FacilityCode: facilityCode},
nil,
).
Times(1)
Expand All @@ -483,6 +501,16 @@ func TestServerConditionCreate(t *testing.T) {
}).
Times(1)
},
func(r *mockevents.MockStream) {
r.EXPECT().
Publish(
gomock.Any(),
gomock.Eq(fmt.Sprintf("%s.servers.%s", facilityCode, ptypes.FirmwareInstall)),
gomock.Any(),
).
Return(nil).
Times(1)
},
func(t *testing.T) *http.Request {
payload, err := json.Marshal(&v1types.ConditionCreate{Parameters: []byte(`{"some param": "1"}`)})
if err != nil {
Expand Down Expand Up @@ -536,7 +564,7 @@ func TestServerConditionCreate(t *testing.T) {
gomock.Eq(serverID),
).
Return(
&model.Server{ID: serverID, FacilityCode: "foo-42"},
&model.Server{ID: serverID, FacilityCode: facilityCode},
nil,
).
Times(1)
Expand All @@ -555,6 +583,16 @@ func TestServerConditionCreate(t *testing.T) {
}).
Times(1)
},
func(r *mockevents.MockStream) {
r.EXPECT().
Publish(
gomock.Any(),
gomock.Eq(fmt.Sprintf("%s.servers.%s", facilityCode, ptypes.FirmwareInstall)),
gomock.Any(),
).
Return(nil).
Times(1)
},
func(t *testing.T) *http.Request {
fault := ptypes.Fault{Panic: true, DelayDuration: "10s", FailAt: "foobar"}
payload, err := json.Marshal(&v1types.ConditionCreate{Parameters: []byte(`{"some param": "1"}`), Fault: &fault})
Expand Down Expand Up @@ -626,7 +664,7 @@ func TestServerConditionCreate(t *testing.T) {
gomock.Eq(serverID),
).
Return(
&model.Server{ID: serverID, FacilityCode: "foo-42"},
&model.Server{ID: serverID, FacilityCode: facilityCode},
nil,
).
Times(1)
Expand All @@ -647,6 +685,16 @@ func TestServerConditionCreate(t *testing.T) {
}).
Times(1)
},
func(r *mockevents.MockStream) {
r.EXPECT().
Publish(
gomock.Any(),
gomock.Eq(fmt.Sprintf("%s.servers.%s", facilityCode, ptypes.FirmwareInstall)),
gomock.Any(),
).
Return(nil).
Times(1)
},
func(t *testing.T) *http.Request {
payload, err := json.Marshal(&v1types.ConditionCreate{Parameters: []byte(`{"some param": "1"}`)})
if err != nil {
Expand Down Expand Up @@ -689,6 +737,8 @@ func TestServerConditionCreate(t *testing.T) {
}, nil).
Times(1)
},
func(r *mockevents.MockStream) {
},
func(t *testing.T) *http.Request {
url := fmt.Sprintf("/api/v1/servers/%s/condition/%s", serverID.String(), ptypes.FirmwareInstall)
request, err := http.NewRequestWithContext(context.TODO(), http.MethodPost, url, bytes.NewReader([]byte(`{"hello":"world"}`)))
Expand Down Expand Up @@ -732,6 +782,8 @@ func TestServerConditionCreate(t *testing.T) {
}, nil).
Times(1)
},
func(r *mockevents.MockStream) {
},
func(t *testing.T) *http.Request {
url := fmt.Sprintf("/api/v1/servers/%s/condition/%s", serverID.String(), ptypes.FirmwareInstall)
request, err := http.NewRequestWithContext(context.TODO(), http.MethodPost, url, bytes.NewReader([]byte(`{"hello":"world"}`)))
Expand All @@ -746,6 +798,98 @@ func TestServerConditionCreate(t *testing.T) {
assert.Contains(t, string(asBytes(t, r.Body)), "exclusive condition present")
},
},
{
"server condition publish failure results in created condition deletion",
// mock repository
func(r *store.MockRepository) {
parametersJSON, _ := json.Marshal(json.RawMessage(`{"some param": "1"}`))

// lookup for an existing condition
r.EXPECT().
Get(
gomock.Any(),
gomock.Eq(serverID),
gomock.Eq(ptypes.FirmwareInstall),
).
Return(nil, nil). // no condition exists
Times(1)

r.EXPECT().
List(
gomock.Any(),
gomock.Any(),
gomock.Any(),
).
Return(nil, nil).
Times(2)

// facility code lookup
r.EXPECT().
GetServer(
gomock.Any(),
gomock.Eq(serverID),
).
Return(
&model.Server{ID: serverID, FacilityCode: facilityCode},
nil,
).
Times(1)

// create condition query
r.EXPECT().
Create(
gomock.Any(),
gomock.Eq(serverID),
gomock.Any(),
).
DoAndReturn(func(_ context.Context, _ uuid.UUID, c *ptypes.Condition) error {
assert.Equal(t, ptypes.ConditionStructVersion, c.Version, "condition version mismatch")
assert.Equal(t, ptypes.FirmwareInstall, c.Kind, "condition kind mismatch")
assert.Equal(t, json.RawMessage(parametersJSON), c.Parameters, "condition parameters mismatch")
assert.Equal(t, ptypes.Pending, c.State, "condition state mismatch")
return nil
}).
Times(1)

// condition deletion due to publish failure
r.EXPECT().
Delete(gomock.Any(), gomock.Eq(serverID), gomock.Eq(ptypes.FirmwareInstall)).
Times(1).
Return(nil)

},
func(r *mockevents.MockStream) {
r.EXPECT().
Publish(
gomock.Any(),
gomock.Eq(fmt.Sprintf("%s.servers.%s", facilityCode, ptypes.FirmwareInstall)),
gomock.Any(),
).
Return(errors.New("gremlins in the pipes")).
Times(1)
},
func(t *testing.T) *http.Request {
payload, err := json.Marshal(&v1types.ConditionCreate{Parameters: []byte(`{"some param": "1"}`)})
if err != nil {
t.Error()
}

url := fmt.Sprintf("/api/v1/servers/%s/condition/%s", serverID.String(), ptypes.FirmwareInstall)
request, err := http.NewRequestWithContext(context.TODO(), http.MethodPost, url, bytes.NewReader(payload))
if err != nil {
t.Fatal(err)
}

return request
},
func(t *testing.T, r *httptest.ResponseRecorder) {
assert.Equal(t, http.StatusInternalServerError, r.Code)
var resp v1types.ServerResponse
err = json.Unmarshal(r.Body.Bytes(), &resp)
assert.Nil(t, err)
assert.Contains(t, resp.Message, "gremlins")
},
},
}

for _, tc := range testcases {
Expand All @@ -754,6 +898,10 @@ func TestServerConditionCreate(t *testing.T) {
tc.mockStore(repository)
}

if tc.mockStream != nil {
tc.mockStream(stream)
}

recorder := httptest.NewRecorder()
server.ServeHTTP(recorder, tc.request(t))

Expand Down Expand Up @@ -788,7 +936,7 @@ func TestServerConditionList(t *testing.T) {

repository := store.NewMockRepository(ctrl)

server, err := mockserver(t, logrus.New(), repository)
server, err := mockserver(t, logrus.New(), repository, nil)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -897,7 +1045,7 @@ func TestServerConditionGet(t *testing.T) {

repository := store.NewMockRepository(ctrl)

server, err := mockserver(t, logrus.New(), repository)
server, err := mockserver(t, logrus.New(), repository, nil)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -1004,7 +1152,7 @@ func TestServerConditionDelete(t *testing.T) {

repository := store.NewMockRepository(ctrl)

server, err := mockserver(t, logrus.New(), repository)
server, err := mockserver(t, logrus.New(), repository, nil)
if err != nil {
t.Fatal(err)
}
Expand Down

0 comments on commit 9cb40ce

Please sign in to comment.