Refactor session health check management and update related tests

Author: srahul3

agent/consul/state/session.go

@@ -220,7 +220,7 @@ func (s *Store) SessionCreate(idx uint64, sess *structs.Session) error {
 	// future.
 
 	// Call the session creation
-	if err := sessionCreateTxn(tx, idx, sess); err != nil {
+	if err := s.sessionCreateTxn(tx, idx, sess); err != nil {
 		return err
 	}
 
@@ -230,7 +230,7 @@ func (s *Store) SessionCreate(idx uint64, sess *structs.Session) error {
 // sessionCreateTxn is the inner method used for creating session entries in
 // an open transaction. Any health checks registered with the session will be
 // checked for failing status. Returns any error encountered.
-func sessionCreateTxn(tx WriteTxn, idx uint64, sess *structs.Session) error {
+func (s *Store) sessionCreateTxn(tx WriteTxn, idx uint64, sess *structs.Session) error {
 	// Check that we have a session ID
 	if sess.ID == "" {
 		return ErrMissingSessionID
@@ -271,7 +271,7 @@ func sessionCreateTxn(tx WriteTxn, idx uint64, sess *structs.Session) error {
 		return fmt.Errorf("failed inserting session: %s", err)
 	}
 
-	return nil
+	return s.updateSessionCheck(tx, idx, sess, api.HealthPassing)
 }
 
 // SessionGet is used to retrieve an active session from the state store.
@@ -450,11 +450,11 @@ func (s *Store) deleteSessionTxn(tx WriteTxn, idx uint64, sessionID string, entM
 	}
 
 	// session invalidating the health-checks
-	return s.markSessionCheckCritical(tx, idx, session)
+	return s.updateSessionCheck(tx, idx, session, api.HealthCritical)
 }
 
-// markSessionCheckCritical The method marks the health-checks associated with the session as critical
-func (s *Store) markSessionCheckCritical(tx WriteTxn, idx uint64, session *structs.Session) error {
+// updateSessionCheck The method updates the health-checks associated with the session
+func (s *Store) updateSessionCheck(tx WriteTxn, idx uint64, session *structs.Session, checkState string) error {
 	// Find all checks for the given Node
 	iter, err := tx.Get(tableChecks, indexNode, Query{Value: session.Node, EnterpriseMeta: session.EnterpriseMeta})
 	if err != nil {
@@ -464,8 +464,13 @@ func (s *Store) markSessionCheckCritical(tx WriteTxn, idx uint64, session *struc
 	for check := iter.Next(); check != nil; check = iter.Next() {
 		if hc := check.(*structs.HealthCheck); hc.Type == "session" && hc.Definition.SessionName == session.Name {
 			updatedCheck := hc.Clone()
-			updatedCheck.Status = api.HealthCritical
-			updatedCheck.Output = fmt.Sprintf("Session '%s' is invalid", session.Name)
+			updatedCheck.Status = checkState
+			switch {
+			case checkState == api.HealthPassing:
+				updatedCheck.Output = fmt.Sprintf("Session '%s' in force", session.ID)
+			default:
+				updatedCheck.Output = fmt.Sprintf("Session '%s' is invalid", session.ID)
+			}
 
 			if err := s.ensureCheckTxn(tx, idx, true, updatedCheck); err != nil {
 				return err

agent/consul/state/session_ce.go

@@ -151,9 +151,9 @@ func validateSessionChecksTxn(tx ReadTxn, session *structs.Session) error {
 		}
 
 		// Verify that the check is not in critical state
-		status := check.(*structs.HealthCheck).Status
-		if status == api.HealthCritical {
-			return fmt.Errorf("Check '%s' is in %s state", checkID, status)
+		healthCheck := check.(*structs.HealthCheck)
+		if healthCheck.Status == api.HealthCritical && healthCheck.Type != "session" {
+			return fmt.Errorf("Check '%s' is in %s state", checkID, healthCheck.Status)
 		}
 	}
 	return nil

agent/consul/state/session_test.go

@@ -962,7 +962,48 @@ func TestStateStore_Session_Invalidate_PreparedQuery_Delete(t *testing.T) {
 	}
 }
 
-func TestHealthCheck_SessionDestroy(t *testing.T) {
+func TestHealthCheck_SessionRegistrationFail(t *testing.T) {
+	s := testStateStore(t)
+
+	var check *structs.HealthCheck
+	// setup node
+	testRegisterNode(t, s, 1, "foo-node")
+	testRegisterCheckCustom(t, s, 1, "foo", func(chk *structs.HealthCheck) {
+		chk.Node = "foo-node"
+		chk.Type = "tll"
+		chk.Status = api.HealthCritical
+		chk.Definition = structs.HealthCheckDefinition{
+			SessionName: "test-session",
+		}
+		check = chk
+	})
+
+	// Ensure the index was not updated if nothing was destroyed.
+	if idx := s.maxIndex("sessions"); idx != 0 {
+		t.Fatalf("bad index: %d", idx)
+	}
+
+	// Register a new session
+	sess := &structs.Session{
+		ID:     testUUID(),
+		Node:   "foo-node",
+		Name:   "test-session",
+		Checks: make([]types.CheckID, 0),
+	}
+
+	sess.Checks = append(sess.Checks, check.CheckID)
+	// assert the check is critical initially
+	assertHealthCheckStatus(t, s, sess, check.CheckID, api.HealthCritical)
+
+	if err := s.SessionCreate(2, sess); err == nil {
+		// expecting error: Check 'foo' is in critical state
+		t.Fatalf("expected error, got nil")
+	}
+}
+
+// Allow the session to be created even if the check is critical.
+// This is mainly to discount the health check of type `session`
+func TestHealthCheck_SessionRegistrationAllow(t *testing.T) {
 	s := testStateStore(t)
 
 	var check *structs.HealthCheck
@@ -971,7 +1012,7 @@ func TestHealthCheck_SessionDestroy(t *testing.T) {
 	testRegisterCheckCustom(t, s, 1, "foo", func(chk *structs.HealthCheck) {
 		chk.Node = "foo-node"
 		chk.Type = "session"
-		chk.Status = api.HealthPassing
+		chk.Status = api.HealthCritical
 		chk.Definition = structs.HealthCheckDefinition{
 			SessionName: "test-session",
 		}
@@ -985,50 +1026,80 @@ func TestHealthCheck_SessionDestroy(t *testing.T) {
 
 	// Register a new session
 	sess := &structs.Session{
-		ID:   testUUID(),
-		Node: "foo-node",
-		Name: "test-session",
+		ID:     testUUID(),
+		Node:   "foo-node",
+		Name:   "test-session",
+		Checks: make([]types.CheckID, 0),
 	}
+
+	sess.Checks = append(sess.Checks, check.CheckID)
+	// assert the check is critical initially
+	assertHealthCheckStatus(t, s, sess, check.CheckID, api.HealthCritical)
+
 	if err := s.SessionCreate(2, sess); err != nil {
-		t.Fatalf("err: %s", err)
+		t.Fatalf("The system shall allow session to be created ignoring the session check is critical. err: %s", err)
 	}
+}
 
-	_, hc, err := s.ChecksInState(nil, api.HealthAny, structs.DefaultEnterpriseMetaInPartition(""), structs.DefaultPeerKeyword)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	// iterate over checks and find the right one
-	found := false
-	for _, c := range hc {
-		if c.CheckID == check.CheckID && c.Status == api.HealthPassing {
-			found = true
-			break
+func TestHealthCheck_Session(t *testing.T) {
+	s := testStateStore(t)
+
+	var check *structs.HealthCheck
+	// setup node
+	testRegisterNode(t, s, 1, "foo-node")
+	testRegisterCheckCustom(t, s, 1, "foo", func(chk *structs.HealthCheck) {
+		chk.Node = "foo-node"
+		chk.Type = "session"
+		chk.Status = api.HealthCritical
+		chk.Definition = structs.HealthCheckDefinition{
+			SessionName: "test-session",
 		}
+		check = chk
+	})
+
+	// Ensure the index was not updated if nothing was destroyed.
+	if idx := s.maxIndex("sessions"); idx != 0 {
+		t.Fatalf("bad index: %d", idx)
+	}
+
+	// Register a new session
+	sess := &structs.Session{
+		ID:   testUUID(),
+		Node: "foo-node",
+		Name: "test-session",
 	}
+	// assert the check is critical initially
+	assertHealthCheckStatus(t, s, sess, check.CheckID, api.HealthCritical)
 
-	if !found {
-		t.Fatalf("check is expected to be passing")
+	if err := s.SessionCreate(2, sess); err != nil {
+		t.Fatalf("The system shall allow session to be created ignoring the session check is critical. err: %s", err)
 	}
+	// assert the check is critical after session creation
+	assertHealthCheckStatus(t, s, sess, check.CheckID, api.HealthPassing)
 
 	// Destroy the session.
 	if err := s.SessionDestroy(3, sess.ID, nil); err != nil {
 		t.Fatalf("err: %s", err)
 	}
+	// assert the check is critical after session destroy
+	assertHealthCheckStatus(t, s, sess, check.CheckID, api.HealthCritical)
+}
 
-	_, hc, err = s.ChecksInState(nil, api.HealthAny, structs.DefaultEnterpriseMetaInPartition(""), structs.DefaultPeerKeyword)
+func assertHealthCheckStatus(t *testing.T, s *Store, session *structs.Session, checkID types.CheckID, expectedStatus string) {
+	_, hc, err := s.NodeChecks(nil, session.Node, structs.DefaultEnterpriseMetaInPartition(""), structs.DefaultPeerKeyword)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
-	// iterate over checks and find the right one
-	found = false
+	// assert the check is healthy
 	for _, c := range hc {
-		if c.CheckID == check.CheckID && c.Status == api.HealthCritical {
-			found = true
-			break
+		if c.CheckID == checkID {
+			if c.Status != expectedStatus {
+				t.Fatalf("check is expected to be %s but actually it is %s", expectedStatus, c.Status)
+			} else {
+				return
+			}
 		}
 	}
 
-	if !found {
-		t.Fatalf("check is expected to be critical")
-	}
+	t.Fatalf("check %s, is not found", string(checkID))
 }