Test step monitor run function and fix found bug (#103)

This PR fixes an issue where the step monitor calculates the number of
reorderings based on the `hosts` list provided by Nova, which may not
necessarily be sorted by the hosts' weights. Now, we compare the input
and output weights correctly.

Author: PhilippMatthes

internal/scheduler/monitor.go

@@ -217,14 +217,19 @@ func (s *StepMonitor) Run(request api.Request) (map[string]float64, error) {
 		defer timer.ObserveDuration()
 	}
 
-	weights, err := s.Step.Run(request)
+	inWeights := request.GetWeights()
+	outWeights, err := s.Step.Run(request)
 	if err != nil {
 		return nil, err
 	}
+	slog.Info(
+		"scheduler: finished step", "name", stepName,
+		"inWeights", inWeights, "outWeights", outWeights,
+	)
 
 	// Observe how much the step modifies the weights of the hosts.
 	if s.stepHostWeight != nil {
-		for host, weight := range weights {
+		for host, weight := range outWeights {
 			s.stepHostWeight.WithLabelValues(host, stepName).Add(weight)
 			if weight != 0.0 {
 				slog.Info("scheduler: modified host weight", "name", stepName, "weight", weight)
@@ -233,11 +238,9 @@ func (s *StepMonitor) Run(request api.Request) (map[string]float64, error) {
 	}
 
 	// Observe how many hosts are removed from the state.
-	hostsInScenario := make(map[string]struct{})
-	for _, host := range request.GetHosts() {
-		hostsInScenario[host] = struct{}{}
-	}
-	nHostsRemoved := len(hostsInScenario) - len(weights)
+	hostsIn := request.GetHosts()
+	hostsOut := slices.Collect(maps.Keys(outWeights))
+	nHostsRemoved := len(hostsIn) - len(hostsOut)
 	if nHostsRemoved < 0 {
 		slog.Info("scheduler: removed hosts", "name", stepName, "count", nHostsRemoved)
 	}
@@ -248,14 +251,19 @@ func (s *StepMonitor) Run(request api.Request) (map[string]float64, error) {
 	// Observe the number of reorderings conducted by the scheduler.
 	if s.reorderingsObserver != nil {
 		// Calculate the Levenshtein distance between the hosts going in and out.
-		hosts := slices.Collect(maps.Keys(weights))
-		sort.Slice(hosts, func(i, j int) bool {
-			return weights[hosts[i]] > weights[hosts[j]]
+		sort.Slice(hostsIn, func(i, j int) bool {
+			return inWeights[hostsIn[i]] > inWeights[hostsIn[j]]
+		})
+		sort.Slice(hostsOut, func(i, j int) bool {
+			return outWeights[hostsOut[i]] > outWeights[hostsOut[j]]
 		})
-		distance := levenshteinDistance(request.GetHosts(), hosts)
-		slog.Info("scheduler: reorderings", "name", stepName, "distance", distance, "hosts_in", request.GetHosts(), "hosts_out", hosts)
+		distance := levenshteinDistance(hostsIn, hostsOut)
+		slog.Info(
+			"scheduler: reorderings", "name", stepName, "distance", distance,
+			"hostsIn", hostsIn, "hostsOut", hostsOut,
+		)
 		s.reorderingsObserver.Observe(float64(distance))
 	}
 
-	return weights, nil
+	return outWeights, nil
 }

internal/scheduler/monitor_test.go

@@ -3,7 +3,57 @@
 
 package scheduler
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/cobaltcore-dev/cortex/internal/scheduler/api"
+	"github.com/cobaltcore-dev/cortex/testlib/monitoring"
+	testlibAPI "github.com/cobaltcore-dev/cortex/testlib/scheduler/api"
+	"github.com/cobaltcore-dev/cortex/testlib/scheduler/plugins"
+)
+
+func TestStepMonitorRun(t *testing.T) {
+	runTimer := &monitoring.MockObserver{}
+	removedHostsObserver := &monitoring.MockObserver{}
+	reorderingsObserver := &monitoring.MockObserver{}
+	monitor := &StepMonitor{
+		Step: &plugins.MockStep{
+			Name: "mock_step",
+			RunFunc: func(request api.Request) (map[string]float64, error) {
+				return map[string]float64{"host1": 0.0, "host2": 1.0, "host3": 0.0}, nil
+			},
+		},
+		runTimer:             runTimer,
+		stepHostWeight:       nil,
+		removedHostsObserver: removedHostsObserver,
+		reorderingsObserver:  reorderingsObserver,
+	}
+	request := &testlibAPI.MockRequest{
+		Hosts:   []string{"host1", "host2", "host3"},
+		Weights: map[string]float64{"host1": 0.0, "host2": 0.0, "host3": 0.0},
+	}
+	if _, err := monitor.Run(request); err != nil {
+		t.Fatalf("Run() error = %v, want nil", err)
+	}
+	if len(removedHostsObserver.Observations) != 1 {
+		t.Errorf("removedHostsObserver.Observations = %v, want 1", len(removedHostsObserver.Observations))
+	}
+	if removedHostsObserver.Observations[0] != 0 {
+		t.Errorf("removedHostsObserver.Observations[0] = %v, want 0", removedHostsObserver.Observations[0])
+	}
+	if len(reorderingsObserver.Observations) != 1 {
+		t.Errorf("reorderingsObserver.Observations = %v, want 1", len(reorderingsObserver.Observations))
+	}
+	if reorderingsObserver.Observations[0] != 2 {
+		t.Errorf("reorderingsObserver.Observations[0] = %v, want 2", reorderingsObserver.Observations[0])
+	}
+	if len(runTimer.Observations) != 1 {
+		t.Errorf("runTimer.Observations = %v, want 1", len(runTimer.Observations))
+	}
+	if runTimer.Observations[0] <= 0 {
+		t.Errorf("runTimer.Observations[0] = %v, want > 0", runTimer.Observations[0])
+	}
+}
 
 func TestLevenshteinDistance(t *testing.T) {
 	tests := []struct {

internal/scheduler/validation_test.go

@@ -11,29 +11,11 @@ import (
 	"github.com/cobaltcore-dev/cortex/internal/db"
 	"github.com/cobaltcore-dev/cortex/internal/scheduler/api"
 	testlibAPI "github.com/cobaltcore-dev/cortex/testlib/scheduler/api"
+	"github.com/cobaltcore-dev/cortex/testlib/scheduler/plugins"
 )
 
-// MockStep is a manual mock implementation of the plugins.Step interface.
-type MockStep struct {
-	Name     string
-	InitFunc func(db db.DB, opts conf.RawOpts) error
-	RunFunc  func(request api.Request) (map[string]float64, error)
-}
-
-func (m *MockStep) GetName() string {
-	return m.Name
-}
-
-func (m *MockStep) Init(db db.DB, opts conf.RawOpts) error {
-	return m.InitFunc(db, opts)
-}
-
-func (m *MockStep) Run(request api.Request) (map[string]float64, error) {
-	return m.RunFunc(request)
-}
-
 func TestStepValidator_GetName(t *testing.T) {
-	mockStep := &MockStep{
+	mockStep := &plugins.MockStep{
 		Name: "mock-step",
 	}
 
@@ -47,7 +29,7 @@ func TestStepValidator_GetName(t *testing.T) {
 }
 
 func TestStepValidator_Init(t *testing.T) {
-	mockStep := &MockStep{
+	mockStep := &plugins.MockStep{
 		InitFunc: func(db db.DB, opts conf.RawOpts) error {
 			return nil
 		},
@@ -66,7 +48,7 @@ func TestStepValidator_Init(t *testing.T) {
 }
 
 func TestStepValidator_Run_ValidHosts(t *testing.T) {
-	mockStep := &MockStep{
+	mockStep := &plugins.MockStep{
 		RunFunc: func(request api.Request) (map[string]float64, error) {
 			return map[string]float64{
 				"host1": 1.0,
@@ -102,7 +84,7 @@ func TestStepValidator_Run_ValidHosts(t *testing.T) {
 }
 
 func TestStepValidator_Run_HostNumberMismatch(t *testing.T) {
-	mockStep := &MockStep{
+	mockStep := &plugins.MockStep{
 		RunFunc: func(request api.Request) (map[string]float64, error) {
 			return map[string]float64{
 				"host1": 1.0,
@@ -137,7 +119,7 @@ func TestStepValidator_Run_HostNumberMismatch(t *testing.T) {
 }
 
 func TestStepValidator_Run_DisabledValidation(t *testing.T) {
-	mockStep := &MockStep{
+	mockStep := &plugins.MockStep{
 		RunFunc: func(request api.Request) (map[string]float64, error) {
 			return map[string]float64{
 				"host1": 1.0,
@@ -171,7 +153,7 @@ func TestStepValidator_Run_DisabledValidation(t *testing.T) {
 }
 
 func TestValidateStep(t *testing.T) {
-	mockStep := &MockStep{}
+	mockStep := &plugins.MockStep{}
 	disabledValidations := conf.SchedulerStepDisabledValidationsConfig{
 		SameHostNumberInOut: true,
 	}

testlib/monitoring/mock.go

@@ -0,0 +1,13 @@
+// Copyright 2025 SAP SE
+// SPDX-License-Identifier: Apache-2.0
+
+package monitoring
+
+type MockObserver struct {
+	// Observations recorded by the mock observer.
+	Observations []float64
+}
+
+func (m *MockObserver) Observe(value float64) {
+	m.Observations = append(m.Observations, value)
+}

testlib/scheduler/plugins/interface.go

@@ -0,0 +1,29 @@
+// Copyright 2025 SAP SE
+// SPDX-License-Identifier: Apache-2.0
+
+package plugins
+
+import (
+	"github.com/cobaltcore-dev/cortex/internal/conf"
+	"github.com/cobaltcore-dev/cortex/internal/db"
+	"github.com/cobaltcore-dev/cortex/internal/scheduler/api"
+)
+
+// MockStep is a manual mock implementation of the plugins.Step interface.
+type MockStep struct {
+	Name     string
+	InitFunc func(db db.DB, opts conf.RawOpts) error
+	RunFunc  func(request api.Request) (map[string]float64, error)
+}
+
+func (m *MockStep) GetName() string {
+	return m.Name
+}
+
+func (m *MockStep) Init(db db.DB, opts conf.RawOpts) error {
+	return m.InitFunc(db, opts)
+}
+
+func (m *MockStep) Run(request api.Request) (map[string]float64, error) {
+	return m.RunFunc(request)
+}