[FS-1253] make 'no active condition' handling uniform (#185)

metal-toolbox · Feb 26, 2024 · c38f31b · c38f31b
1 parent 159cd77
commit c38f31b
Show file tree

Hide file tree

Showing 5 changed files with 20 additions and 14 deletions.
diff --git 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
@@ -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
@@ -108,14 +108,15 @@ 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
 	err = store.Create(context.TODO(), serverID, condition)
 	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
@@ -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
@@ -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(),
 		}