From c38f31bd801e3a48e6ff757996a154a5a6ca6a06 Mon Sep 17 00:00:00 2001 From: Doctor Vince Date: Mon, 26 Feb 2024 10:38:23 -0500 Subject: [PATCH] [FS-1253] make 'no active condition' handling uniform (#185) --- internal/orchestrator/updates.go | 10 +++++++++- internal/store/nats.go | 6 +----- internal/store/nats_test.go | 9 +++++---- pkg/api/v1/events/handlers.go | 5 +++-- pkg/api/v1/routes/handlers.go | 4 ++-- 5 files changed, 20 insertions(+), 14 deletions(-) diff --git a/internal/orchestrator/updates.go b/internal/orchestrator/updates.go index 3e5eec8a..865db08a 100644 --- a/internal/orchestrator/updates.go +++ b/internal/orchestrator/updates.go @@ -11,6 +11,7 @@ import ( "github.com/google/uuid" "github.com/metal-toolbox/conditionorc/internal/metrics" "github.com/metal-toolbox/conditionorc/internal/status" + "github.com/metal-toolbox/conditionorc/internal/store" "github.com/nats-io/nats.go" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" @@ -324,6 +325,11 @@ func (o *Orchestrator) eventUpdate(ctx context.Context, evt *v1types.ConditionUp // queue any follow-on work as required active, err := o.repository.GetActiveCondition(ctx, evt.ConditionUpdate.ServerID) + if err != nil && errors.Is(err, store.ErrConditionNotFound) { + // nothing more to do + return nil + } + if err != nil { o.logger.WithError(err).WithFields(logrus.Fields{ "condition.id": evt.ConditionUpdate.ConditionID, @@ -332,7 +338,9 @@ func (o *Orchestrator) eventUpdate(ctx context.Context, evt *v1types.ConditionUp metrics.DependencyError("nats", "retrieve active condition") return errors.Wrap(errCompleteEvent, err.Error()) } - // seeing as we only *just* completed this event it's hard to believe we'd + + // Publish the next event iff that event is in the pending state. + // Seeing as we only *just* completed this event it's hard to believe we'd // lose the race to publish the next one, but it's possible I suppose. if active != nil && active.State == rctypes.Pending { byt := active.MustBytes() diff --git a/internal/store/nats.go b/internal/store/nats.go index 2f0917f8..c8b8a4fe 100644 --- a/internal/store/nats.go +++ b/internal/store/nats.go @@ -115,16 +115,12 @@ func (n *natsStore) GetActiveCondition(ctx context.Context, serverID uuid.UUID) }) cr, err := n.getCurrentConditionRecord(otelCtx, serverID) - if errors.Is(err, ErrConditionNotFound) { - return nil, nil - } - if err != nil { return nil, err } if rctypes.StateIsComplete(cr.State) { - return nil, nil + return nil, ErrConditionNotFound } var active *rctypes.Condition diff --git a/internal/store/nats_test.go b/internal/store/nats_test.go index 75346045..80cbcfe1 100644 --- a/internal/store/nats_test.go +++ b/internal/store/nats_test.go @@ -108,7 +108,7 @@ func TestCreateReadUpdate(t *testing.T) { require.ErrorIs(t, err, ErrConditionNotFound) active, err := store.GetActiveCondition(context.TODO(), serverID) - require.NoError(t, err) + require.ErrorIs(t, err, ErrConditionNotFound) require.Nil(t, active) // add a condition @@ -116,6 +116,7 @@ func TestCreateReadUpdate(t *testing.T) { require.NoError(t, err) active, err = store.GetActiveCondition(context.TODO(), serverID) + require.NoError(t, err) require.NotNil(t, active) // get the new condition @@ -138,7 +139,7 @@ func TestCreateReadUpdate(t *testing.T) { require.NoError(t, err) active, err = store.GetActiveCondition(context.TODO(), serverID) - require.NoError(t, err) + require.ErrorIs(t, err, ErrConditionNotFound) require.Nil(t, active) cr, err = store.Get(context.TODO(), serverID) @@ -220,7 +221,7 @@ func TestMultipleConditionUpdate(t *testing.T) { require.NoError(t, err, "second update - succeeded") active, err = store.GetActiveCondition(context.TODO(), serverID) - require.NoError(t, err, "GetActiveCondition IV") + require.ErrorIs(t, err, ErrConditionNotFound, "GetActiveCondition IV") require.Nil(t, active) cr, err := store.Get(context.TODO(), serverID) @@ -262,7 +263,7 @@ func TestMultipleConditionUpdate(t *testing.T) { require.NoError(t, err, "first update - failed") active, err = store.GetActiveCondition(context.TODO(), serverID) - require.NoError(t, err, "GetActiveCondition III") + require.ErrorIs(t, err, ErrConditionNotFound, "GetActiveCondition III") require.Nil(t, active) cr, err := store.Get(context.TODO(), serverID) diff --git a/pkg/api/v1/events/handlers.go b/pkg/api/v1/events/handlers.go index bc394965..da59463b 100644 --- a/pkg/api/v1/events/handlers.go +++ b/pkg/api/v1/events/handlers.go @@ -86,8 +86,9 @@ func (h *Handler) UpdateCondition(ctx context.Context, updEvt *v1types.Condition if err != nil { if errors.Is(err, store.ErrConditionNotFound) { h.logger.WithFields(logrus.Fields{ - "serverID": updEvt.ConditionUpdate.ServerID, - "conditionKind": updEvt.Kind, + "server_id": updEvt.ConditionUpdate.ServerID, + "condition_id": updEvt.ConditionUpdate.ConditionID, + "condition_kind": updEvt.Kind, }).Error("no existing condition found for update") return err } diff --git a/pkg/api/v1/routes/handlers.go b/pkg/api/v1/routes/handlers.go index 263aa3e0..d0b3b5af 100644 --- a/pkg/api/v1/routes/handlers.go +++ b/pkg/api/v1/routes/handlers.go @@ -105,7 +105,7 @@ func (r *Routes) serverConditionCreate(c *gin.Context) (int, *v1types.ServerResp } active, err := r.repository.GetActiveCondition(otelCtx, serverID) - if err != nil { + if err != nil && !errors.Is(err, store.ErrConditionNotFound) { return http.StatusServiceUnavailable, &v1types.ServerResponse{ Message: "error checking server state: " + err.Error(), } @@ -154,7 +154,7 @@ func (r *Routes) serverDelete(c *gin.Context) (int, *v1types.ServerResponse) { } active, err := r.repository.GetActiveCondition(otelCtx, serverID) - if err != nil { + if err != nil && !errors.Is(err, store.ErrConditionNotFound) { return http.StatusServiceUnavailable, &v1types.ServerResponse{ Message: "error checking server state: " + err.Error(), }