From 00e304b5d4d2a8d473cd8a337d7d0654ca1f87d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kuba=20Tu=C5=BCnik?= Date: Mon, 18 Nov 2024 21:31:56 +0100 Subject: [PATCH] CA: remove Clear from ClusterSnapshot It's now redundant - SetClusterState with empty arguments does the same thing. --- .../simulator/clustersnapshot/basic.go | 8 +++---- .../clustersnapshot/clustersnapshot.go | 2 -- .../clustersnapshot_benchmark_test.go | 2 +- .../clustersnapshot/clustersnapshot_test.go | 21 ++++++++++++++----- .../simulator/clustersnapshot/delta.go | 6 +++--- .../simulator/clustersnapshot/test_utils.go | 2 +- 6 files changed, 25 insertions(+), 16 deletions(-) diff --git a/cluster-autoscaler/simulator/clustersnapshot/basic.go b/cluster-autoscaler/simulator/clustersnapshot/basic.go index 38b29de9e306..be8388c5ce8b 100644 --- a/cluster-autoscaler/simulator/clustersnapshot/basic.go +++ b/cluster-autoscaler/simulator/clustersnapshot/basic.go @@ -196,7 +196,7 @@ func (data *internalBasicSnapshotData) removePod(namespace, podName, nodeName st // NewBasicClusterSnapshot creates instances of BasicClusterSnapshot. func NewBasicClusterSnapshot() *BasicClusterSnapshot { snapshot := &BasicClusterSnapshot{} - snapshot.Clear() + snapshot.clear() return snapshot } @@ -234,7 +234,7 @@ func (snapshot *BasicClusterSnapshot) AddNodeInfo(nodeInfo *framework.NodeInfo) // SetClusterState sets the cluster state. func (snapshot *BasicClusterSnapshot) SetClusterState(nodes []*apiv1.Node, scheduledPods []*apiv1.Pod) error { - snapshot.Clear() + snapshot.clear() knownNodes := make(map[string]bool) for _, node := range nodes { @@ -297,8 +297,8 @@ func (snapshot *BasicClusterSnapshot) Commit() error { return nil } -// Clear reset cluster snapshot to empty, unforked state -func (snapshot *BasicClusterSnapshot) Clear() { +// clear reset cluster snapshot to empty, unforked state +func (snapshot *BasicClusterSnapshot) clear() { baseData := newInternalBasicSnapshotData() snapshot.data = []*internalBasicSnapshotData{baseData} } diff --git a/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot.go b/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot.go index 969986ee00a9..1c60fcc0b730 100644 --- a/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot.go +++ b/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot.go @@ -59,8 +59,6 @@ type ClusterSnapshot interface { Revert() // Commit commits changes done after forking. Commit() error - // Clear reset cluster snapshot to empty, unforked state. - Clear() } // ErrNodeNotFound means that a node wasn't found in the snapshot. diff --git a/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot_benchmark_test.go b/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot_benchmark_test.go index fc1c519c911d..fb6468adad6f 100644 --- a/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot_benchmark_test.go +++ b/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot_benchmark_test.go @@ -80,7 +80,7 @@ func BenchmarkAddNodeInfo(b *testing.B) { b.Run(fmt.Sprintf("%s: AddNodeInfo() %d", snapshotName, tc), func(b *testing.B) { for i := 0; i < b.N; i++ { b.StopTimer() - clusterSnapshot.Clear() + assert.NoError(b, clusterSnapshot.SetClusterState(nil, nil)) b.StartTimer() for _, node := range nodes { err := clusterSnapshot.AddNodeInfo(framework.NewTestNodeInfo(node)) diff --git a/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot_test.go b/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot_test.go index 5b564f64ecdd..4eeb67253558 100644 --- a/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot_test.go +++ b/cluster-autoscaler/simulator/clustersnapshot/clustersnapshot_test.go @@ -275,7 +275,7 @@ func TestForking(t *testing.T) { } } -func TestClear(t *testing.T) { +func TestSetClusterState(t *testing.T) { // Run with -count=1 to avoid caching. localRand := rand.New(rand.NewSource(time.Now().Unix())) @@ -309,10 +309,21 @@ func TestClear(t *testing.T) { snapshot := startSnapshot(t, snapshotFactory, state) compareStates(t, state, getSnapshotState(t, snapshot)) - snapshot.Clear() + assert.NoError(t, snapshot.SetClusterState(nil, nil)) compareStates(t, snapshotState{}, getSnapshotState(t, snapshot)) }) + t.Run(fmt.Sprintf("%s: clear base %d nodes %d pods and set a new state", name, nodeCount, podCount), + func(t *testing.T) { + snapshot := startSnapshot(t, snapshotFactory, state) + compareStates(t, state, getSnapshotState(t, snapshot)) + + newNodes, newPods := createTestNodes(13), createTestPods(37) + assignPodsToNodes(newPods, newNodes) + assert.NoError(t, snapshot.SetClusterState(newNodes, newPods)) + + compareStates(t, snapshotState{nodes: newNodes, pods: newPods}, getSnapshotState(t, snapshot)) + }) t.Run(fmt.Sprintf("%s: clear fork %d nodes %d pods %d extra nodes %d extra pods", name, nodeCount, podCount, extraNodeCount, extraPodCount), func(t *testing.T) { snapshot := startSnapshot(t, snapshotFactory, state) @@ -332,11 +343,11 @@ func TestClear(t *testing.T) { compareStates(t, snapshotState{allNodes, allPods}, getSnapshotState(t, snapshot)) - snapshot.Clear() + assert.NoError(t, snapshot.SetClusterState(nil, nil)) compareStates(t, snapshotState{}, getSnapshotState(t, snapshot)) - // Clear() should break out of forked state. + // SetClusterState() should break out of forked state. snapshot.Fork() }) } @@ -718,7 +729,7 @@ func TestPVCClearAndFork(t *testing.T) { volumeExists := snapshot.StorageInfos().IsPVCUsedByPods(schedulerframework.GetNamespacedName("default", "claim1")) assert.Equal(t, true, volumeExists) - snapshot.Clear() + assert.NoError(t, snapshot.SetClusterState(nil, nil)) volumeExists = snapshot.StorageInfos().IsPVCUsedByPods(schedulerframework.GetNamespacedName("default", "claim1")) assert.Equal(t, false, volumeExists) diff --git a/cluster-autoscaler/simulator/clustersnapshot/delta.go b/cluster-autoscaler/simulator/clustersnapshot/delta.go index 3f7322b4684d..869e494e0226 100644 --- a/cluster-autoscaler/simulator/clustersnapshot/delta.go +++ b/cluster-autoscaler/simulator/clustersnapshot/delta.go @@ -389,7 +389,7 @@ func (snapshot *DeltaClusterSnapshot) StorageInfos() schedulerframework.StorageI // NewDeltaClusterSnapshot creates instances of DeltaClusterSnapshot. func NewDeltaClusterSnapshot() *DeltaClusterSnapshot { snapshot := &DeltaClusterSnapshot{} - snapshot.Clear() + snapshot.clear() return snapshot } @@ -423,7 +423,7 @@ func (snapshot *DeltaClusterSnapshot) AddNodeInfo(nodeInfo *framework.NodeInfo) // SetClusterState sets the cluster state. func (snapshot *DeltaClusterSnapshot) SetClusterState(nodes []*apiv1.Node, scheduledPods []*apiv1.Pod) error { - snapshot.Clear() + snapshot.clear() knownNodes := make(map[string]bool) for _, node := range nodes { @@ -489,6 +489,6 @@ func (snapshot *DeltaClusterSnapshot) Commit() error { // Clear reset cluster snapshot to empty, unforked state // Time: O(1) -func (snapshot *DeltaClusterSnapshot) Clear() { +func (snapshot *DeltaClusterSnapshot) clear() { snapshot.data = newInternalDeltaSnapshotData() } diff --git a/cluster-autoscaler/simulator/clustersnapshot/test_utils.go b/cluster-autoscaler/simulator/clustersnapshot/test_utils.go index c457d6bf187d..f0cd8c67546e 100644 --- a/cluster-autoscaler/simulator/clustersnapshot/test_utils.go +++ b/cluster-autoscaler/simulator/clustersnapshot/test_utils.go @@ -34,7 +34,7 @@ func InitializeClusterSnapshotOrDie( pods []*apiv1.Pod) { var err error - snapshot.Clear() + assert.NoError(t, snapshot.SetClusterState(nil, nil)) for _, node := range nodes { err = snapshot.AddNodeInfo(framework.NewTestNodeInfo(node))