From bfb9e29c250bcfb34a6b1813ca46953503ca05e6 Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curii.com>
Date: Thu, 16 Feb 2023 00:24:46 -0500
Subject: [PATCH] 19973: Fix tests.

Stub pool AtQuota() was miscounting instances.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>
---
 lib/dispatchcloud/scheduler/run_queue.go      |  2 +
 lib/dispatchcloud/scheduler/run_queue_test.go | 55 ++++++++++++-------
 2 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/lib/dispatchcloud/scheduler/run_queue.go b/lib/dispatchcloud/scheduler/run_queue.go
index d0074ae28e..1e5ac2e046 100644
--- a/lib/dispatchcloud/scheduler/run_queue.go
+++ b/lib/dispatchcloud/scheduler/run_queue.go
@@ -154,8 +154,10 @@ tryrun:
 			} else if sch.pool.KillContainer(ctr.UUID, "about to start") {
 				logger.Info("not restarting yet: crunch-run process from previous attempt has not exited")
 			} else if sch.pool.StartContainer(it, ctr) {
+				logger.Trace("StartContainer => true")
 				// Success.
 			} else {
+				logger.Trace("StartContainer => false")
 				containerAllocatedWorkerBootingCount += 1
 				dontstart[it] = true
 			}
diff --git a/lib/dispatchcloud/scheduler/run_queue_test.go b/lib/dispatchcloud/scheduler/run_queue_test.go
index c6ed2187e5..8553993375 100644
--- a/lib/dispatchcloud/scheduler/run_queue_test.go
+++ b/lib/dispatchcloud/scheduler/run_queue_test.go
@@ -52,7 +52,14 @@ type stubPool struct {
 func (p *stubPool) AtQuota() bool {
 	p.Lock()
 	defer p.Unlock()
-	return len(p.unalloc)+len(p.running)+len(p.unknown) >= p.quota
+	n := len(p.running)
+	for _, nn := range p.unalloc {
+		n += nn
+	}
+	for _, nn := range p.unknown {
+		n += nn
+	}
+	return n >= p.quota
 }
 func (p *stubPool) Subscribe() <-chan struct{}  { return p.notify }
 func (p *stubPool) Unsubscribe(<-chan struct{}) {}
@@ -201,12 +208,8 @@ func (*SchedulerSuite) TestUseIdleWorkers(c *check.C) {
 // call Create().
 func (*SchedulerSuite) TestShutdownAtQuota(c *check.C) {
 	ctx := ctxlog.Context(context.Background(), ctxlog.TestLogger(c))
-	for quota := 1; quota < 3; quota++ {
+	for quota := 1; quota <= 3; quota++ {
 		c.Logf("quota=%d", quota)
-		shouldCreate := []arvados.InstanceType{}
-		for i := 1; i < quota; i++ {
-			shouldCreate = append(shouldCreate, test.InstanceType(3))
-		}
 		queue := test.Queue{
 			ChooseType: chooseType,
 			Containers: []arvados.Container{
@@ -245,21 +248,33 @@ func (*SchedulerSuite) TestShutdownAtQuota(c *check.C) {
 			canCreate: 0,
 		}
 		sch := New(ctx, arvados.NewClientFromEnv(), &queue, &pool, nil, time.Millisecond, time.Millisecond)
-		sch.runQueue()
 		sch.sync()
 		sch.runQueue()
 		sch.sync()
-		c.Check(pool.creates, check.DeepEquals, shouldCreate)
-		if len(shouldCreate) == 0 {
-			c.Check(pool.starts, check.DeepEquals, []string{})
-		} else {
+		switch quota {
+		case 1, 2:
+			// Can't create a type3 node for ctr3, so we
+			// shutdown an unallocated node (type2), and
+			// unlock both containers.
+			c.Check(pool.starts, check.HasLen, 0)
+			c.Check(pool.shutdowns, check.Equals, 1)
+			c.Check(pool.creates, check.HasLen, 0)
+			c.Check(queue.StateChanges(), check.DeepEquals, []test.QueueStateChange{
+				{UUID: test.ContainerUUID(3), From: "Locked", To: "Queued"},
+				{UUID: test.ContainerUUID(2), From: "Locked", To: "Queued"},
+			})
+		case 3:
+			// Creating a type3 instance works, so we
+			// start ctr2 on a type2 instance, and leave
+			// ctr3 locked while we wait for the new
+			// instance to come up.
 			c.Check(pool.starts, check.DeepEquals, []string{test.ContainerUUID(2)})
+			c.Check(pool.shutdowns, check.Equals, 0)
+			c.Check(pool.creates, check.DeepEquals, []arvados.InstanceType{test.InstanceType(3)})
+			c.Check(queue.StateChanges(), check.HasLen, 0)
+		default:
+			panic("test not written for quota>3")
 		}
-		c.Check(pool.shutdowns, check.Equals, 3-quota)
-		c.Check(queue.StateChanges(), check.DeepEquals, []test.QueueStateChange{
-			{UUID: "zzzzz-dz642-000000000000003", From: "Locked", To: "Queued"},
-			{UUID: "zzzzz-dz642-000000000000002", From: "Locked", To: "Queued"},
-		})
 	}
 }
 
@@ -293,15 +308,15 @@ func (*SchedulerSuite) TestEqualPriorityContainers(c *check.C) {
 	pool := stubPool{
 		quota: 2,
 		unalloc: map[arvados.InstanceType]int{
-			test.InstanceType(3): 1,
+			test.InstanceType(3): 2,
 		},
 		idle: map[arvados.InstanceType]int{
-			test.InstanceType(3): 1,
+			test.InstanceType(3): 2,
 		},
 		running:   map[string]time.Time{},
 		creates:   []arvados.InstanceType{},
 		starts:    []string{},
-		canCreate: 1,
+		canCreate: 0,
 	}
 	sch := New(ctx, arvados.NewClientFromEnv(), &queue, &pool, nil, time.Millisecond, time.Millisecond)
 	for i := 0; i < 30; i++ {
@@ -310,7 +325,7 @@ func (*SchedulerSuite) TestEqualPriorityContainers(c *check.C) {
 		time.Sleep(time.Millisecond)
 	}
 	c.Check(pool.shutdowns, check.Equals, 0)
-	c.Check(pool.starts, check.HasLen, 1)
+	c.Check(pool.starts, check.HasLen, 2)
 	unlocked := map[string]int{}
 	for _, chg := range queue.StateChanges() {
 		if chg.To == arvados.ContainerStateQueued {
-- 
2.30.2