From c7a061a220796c8538b3329a14d3306c4799ec21 Mon Sep 17 00:00:00 2001
From: Jordan Barrett <jordan.barrett@canonical.com>
Date: Wed, 26 Jul 2023 12:18:01 +0800
Subject: [PATCH] add TestPlanChange - fix bug removing old buffers

---
 internals/overlord/logstate/gatherer_test.go |  7 --
 internals/overlord/logstate/manager.go       |  2 +-
 internals/overlord/logstate/manager_test.go  | 89 +++++++++++++++++---
 3 files changed, 80 insertions(+), 18 deletions(-)

diff --git a/internals/overlord/logstate/gatherer_test.go b/internals/overlord/logstate/gatherer_test.go
index fc45b57f2..e149c37d2 100644
--- a/internals/overlord/logstate/gatherer_test.go
+++ b/internals/overlord/logstate/gatherer_test.go
@@ -11,13 +11,6 @@ import (
 	. "gopkg.in/check.v1"
 )
 
-// need to test
-//  - move over existing puller
-//  - create new puller for existing service
-//    - before svc -/-> tgt, now svc ---> tgt
-//  - remove old pullers
-//    - before svc ---> tgt, now svc -/-> tgt
-
 type gathererSuite struct{}
 
 var _ = Suite(&gathererSuite{})
diff --git a/internals/overlord/logstate/manager.go b/internals/overlord/logstate/manager.go
index d202d4732..ad8ebf75f 100644
--- a/internals/overlord/logstate/manager.go
+++ b/internals/overlord/logstate/manager.go
@@ -75,7 +75,7 @@ func (m *LogManager) PlanChanged(pl *plan.Plan) {
 
 	// Remove old buffers
 	for svc := range m.buffers {
-		if _, ok := pl.Services[svc]; ok {
+		if _, ok := pl.Services[svc]; !ok {
 			// Service has been removed
 			delete(m.buffers, svc)
 		}
diff --git a/internals/overlord/logstate/manager_test.go b/internals/overlord/logstate/manager_test.go
index 4748697f4..0355a8455 100644
--- a/internals/overlord/logstate/manager_test.go
+++ b/internals/overlord/logstate/manager_test.go
@@ -15,6 +15,85 @@ type managerSuite struct{}
 
 var _ = Suite(&managerSuite{})
 
+func (s *managerSuite) TestPlanChange(c *C) {
+	m := NewLogManager()
+
+	svc1 := newTestService("svc1")
+	svc2 := newTestService("svc2")
+	svc3 := newTestService("svc3")
+
+	m.PlanChanged(&plan.Plan{
+		Services: map[string]*plan.Service{
+			svc1.name: svc1.config,
+			svc2.name: svc2.config,
+			svc3.name: svc3.config,
+		},
+		LogTargets: map[string]*plan.LogTarget{
+			"tgt1": {Name: "tgt1", Services: []string{"all", "-svc3"}},
+			"tgt2": {Name: "tgt2", Services: []string{}},
+			"tgt3": {Name: "tgt3", Services: []string{"all"}},
+		},
+	})
+	m.ServiceStarted(svc1.config, svc1.ringBuffer)
+	m.ServiceStarted(svc2.config, svc2.ringBuffer)
+	m.ServiceStarted(svc3.config, svc3.ringBuffer)
+
+	checkGatherers(c, m.gatherers, map[string][]string{
+		"tgt1": {"svc1", "svc2"},
+		"tgt2": {},
+		"tgt3": {"svc1", "svc2", "svc3"},
+	})
+	checkBuffers(c, m.buffers, []string{"svc1", "svc2", "svc3"})
+
+	svc4 := newTestService("svc4")
+
+	m.PlanChanged(&plan.Plan{
+		Services: map[string]*plan.Service{
+			svc1.name: svc1.config,
+			svc2.name: svc2.config,
+			svc4.name: svc4.config,
+		},
+		LogTargets: map[string]*plan.LogTarget{
+			"tgt1": {Name: "tgt1", Services: []string{"svc1"}},
+			"tgt2": {Name: "tgt2", Services: []string{"svc1", "svc4"}},
+			"tgt4": {Name: "tgt4", Services: []string{"all", "-svc2"}},
+		},
+	})
+	m.ServiceStarted(svc4.config, svc4.ringBuffer)
+	// simulate service restart for svc2
+	m.ServiceStarted(svc2.config, svc2.ringBuffer)
+
+	checkGatherers(c, m.gatherers, map[string][]string{
+		"tgt1": {"svc1"},
+		"tgt2": {"svc1", "svc4"},
+		"tgt4": {"svc1", "svc4"},
+	})
+	// svc3 no longer exists so we should have dropped the reference to its buffer
+	checkBuffers(c, m.buffers, []string{"svc1", "svc2", "svc4"})
+}
+
+func checkGatherers(c *C, gatherers map[string]*logGatherer, expected map[string][]string) {
+	c.Assert(gatherers, HasLen, len(expected))
+	for tgtName, svcs := range expected {
+		g, ok := gatherers[tgtName]
+		c.Assert(ok, Equals, true)
+
+		c.Assert(g.pullers, HasLen, len(svcs))
+		for _, svc := range svcs {
+			_, ok := g.pullers[svc]
+			c.Check(ok, Equals, true)
+		}
+	}
+}
+
+func checkBuffers(c *C, buffers map[string]*servicelog.RingBuffer, expected []string) {
+	c.Assert(buffers, HasLen, len(expected))
+	for _, svcName := range expected {
+		_, ok := buffers[svcName]
+		c.Check(ok, Equals, true)
+	}
+}
+
 // TODO: why is this test intermittently failing?
 func (s *managerSuite) TestTimelyShutdown(c *C) {
 	teardownTimeout = 2 * time.Millisecond
@@ -60,16 +139,6 @@ func (s *managerSuite) TestTimelyShutdown(c *C) {
 	}
 }
 
-// need to test
-// - copy over existing gatherer
-// - removing old gatherers/buffers
-// - service restart
-//
-
-// services added/removed
-// targets  added/removed
-// svc->tgt added/removed
-
 type slowFlushingClient struct {
 	flushTime time.Duration
 }