minor changes

Signed-off-by: Ajay Mishra <ajmishra@nvidia.com>

Author: XRFXLP

commons/pkg/kubeclient/nodepatch_test.go

@@ -33,7 +33,7 @@ func node(labels, annotations map[string]string) *v1.Node {
 	}
 }
 
-func TestNodeMergePatch(t *testing.T) {
+func TestNodeMergePatch_MetadataChanges_ReturnsExpectedPatch(t *testing.T) {
 	tests := []struct {
 		name     string
 		original *v1.Node
@@ -110,7 +110,7 @@ func TestNodeMergePatch(t *testing.T) {
 // only one annotation and clears Spec entirely — and a patch derived from that
 // projection must not describe the fields the projection dropped, or it would erase
 // them on the real object.
-func TestNodeMergePatchLeavesProjectedFieldsAlone(t *testing.T) {
+func TestNodeMergePatch_ProjectedFields_LeavesThemAlone(t *testing.T) {
 	projected := &v1.Node{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        "node-1",
@@ -132,7 +132,7 @@ func TestNodeMergePatchLeavesProjectedFieldsAlone(t *testing.T) {
 		"a cleared Spec must never reach the patch, or real taints would be dropped")
 }
 
-func TestNodeMergePatchIgnoresSpecChanges(t *testing.T) {
+func TestNodeMergePatch_SpecChanges_ReturnsNoPatch(t *testing.T) {
 	original := node(nil, nil)
 	modified := original.DeepCopy()
 	modified.Spec.Unschedulable = true

labeler/pkg/labeler/labeler_test.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
 	listersv1 "k8s.io/client-go/listers/core/v1"
+	k8stesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
@@ -2440,12 +2441,11 @@ func TestUpdateNodeLabelsForPod_ManagedGate(t *testing.T) {
 	})
 }
 
-// TestLabelerWriteCosts pins the API-call budget of the label write path. Reading the
-// node from the informer cache instead of the API server is what makes a label change
-// cost a single PATCH, and it lets a reconcile with nothing to do cost nothing at all
-// — on a large cluster that difference is per node, on every reconcile.
-func TestLabelerWriteCosts(t *testing.T) {
-	nodeName := "gpu-node"
+// TestUpdateNodeLabels_CachedNode_UsesPatchOnly pins the API-call budget and payload
+// of the label write path.
+func TestUpdateNodeLabels_CachedNode_UsesPatchOnly(t *testing.T) {
+	const nodeName = "gpu-node"
+
 	clientset := fake.NewSimpleClientset(&corev1.Node{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:   nodeName,
@@ -2454,50 +2454,66 @@ func TestLabelerWriteCosts(t *testing.T) {
 	})
 
 	labeler, _ := startTransformTestLabeler(t, clientset, devicecounts.Config{})
-
-	// Let startup reconciliation settle, so every write counted below is one this
-	// test asked for.
 	requireCachedLabel(t, labeler, nodeName, KataEnabledLabel, LabelValueFalse)
 
-	t.Run("a label change costs one patch and nothing else", func(t *testing.T) {
-		// KataEnabledLabel is the labeler's own output, not one of the inputs
-		// nodeRequiresReconciliation watches, so dropping it cannot kick off a
-		// reconcile behind this test's back.
-		node, err := clientset.CoreV1().Nodes().Get(context.Background(), nodeName, metav1.GetOptions{})
-		require.NoError(t, err)
-		delete(node.Labels, KataEnabledLabel)
-		_, err = clientset.CoreV1().Nodes().Update(context.Background(), node, metav1.UpdateOptions{})
-		require.NoError(t, err)
-
-		requireCachedLabelAbsent(t, labeler, nodeName, KataEnabledLabel)
-		clientset.ClearActions()
-
-		require.NoError(t, labeler.updateNodeLabels(nodeName))
+	tests := []struct {
+		name          string
+		setup         func(*testing.T)
+		expectedPatch string
+	}{
+		{
+			name: "label change costs one merge patch",
+			setup: func(t *testing.T) {
+				node, getErr := clientset.CoreV1().Nodes().Get(t.Context(), nodeName, metav1.GetOptions{})
+				require.NoError(t, getErr)
+				delete(node.Labels, KataEnabledLabel)
+				_, updateErr := clientset.CoreV1().Nodes().Update(t.Context(), node, metav1.UpdateOptions{})
+				require.NoError(t, updateErr)
+				requireCachedLabelAbsent(t, labeler, nodeName, KataEnabledLabel)
+			},
+			expectedPatch: fmt.Sprintf(
+				`{"metadata":{"labels":{%q:%q}}}`,
+				KataEnabledLabel,
+				LabelValueFalse,
+			),
+		},
+		{
+			name: "no-op reconcile costs no API call",
+			setup: func(t *testing.T) {
+				requireCachedLabel(t, labeler, nodeName, KataEnabledLabel, LabelValueFalse)
+			},
+		},
+	}
 
-		assert.Equal(t, 1, countNodeActions(clientset, "patch"), "the write itself")
-		assert.Equal(t, 0, countNodeActions(clientset, "get"), "the node came from the cache")
-		assert.Equal(t, 0, countNodeActions(clientset, "update"), "PUT is no longer used")
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.setup(t)
+			clientset.ClearActions()
 
-		updated, err := clientset.CoreV1().Nodes().Get(context.Background(), nodeName, metav1.GetOptions{})
-		require.NoError(t, err)
-		assert.Equal(t, LabelValueFalse, updated.Labels[KataEnabledLabel], "the patch landed")
-		assert.Equal(t, LabelValueTrue, updated.Labels[gpuPresentLabel],
-			"a merge patch must leave labels it does not mention alone")
-	})
+			require.NoError(t, labeler.updateNodeLabels(nodeName))
 
-	t.Run("a reconcile with nothing to do costs no api call", func(t *testing.T) {
-		requireCachedLabel(t, labeler, nodeName, KataEnabledLabel, LabelValueFalse)
-		clientset.ClearActions()
+			expectedPatchCount := 0
+			if tt.expectedPatch != "" {
+				expectedPatchCount = 1
+				action := nodePatchAction(t, clientset.Actions())
+				assert.Equal(t, types.MergePatchType, action.GetPatchType())
+				assert.JSONEq(t, tt.expectedPatch, string(action.GetPatch()))
+			}
 
-		require.NoError(t, labeler.updateNodeLabels(nodeName))
+			assert.Equal(t, expectedPatchCount, countNodeActions(clientset, "patch"))
+			assert.Equal(t, 0, countNodeActions(clientset, "get"))
+			assert.Equal(t, 0, countNodeActions(clientset, "update"))
 
-		assert.Equal(t, 0, countNodeActions(clientset, "patch"))
-		assert.Equal(t, 0, countNodeActions(clientset, "get"))
-		assert.Equal(t, 0, countNodeActions(clientset, "update"))
-	})
+			updated, getErr := clientset.CoreV1().Nodes().Get(t.Context(), nodeName, metav1.GetOptions{})
+			require.NoError(t, getErr)
+			assert.Equal(t, LabelValueFalse, updated.Labels[KataEnabledLabel], "the desired label is present")
+			assert.Equal(t, LabelValueTrue, updated.Labels[gpuPresentLabel],
+				"a merge patch must leave labels it does not mention alone")
+		})
+	}
 }
 
-// TestLabelsConvergeUnderNodeChurn attacks the assumption the PATCH path rests on:
+// TestLabelsConvergeUnderNodeChurn_FinalInputs_Converge attacks the assumption the PATCH path rests on:
 // that reading the node from the informer cache can go stale without ever losing a
 // write.
 //
@@ -2513,7 +2529,7 @@ func TestLabelerWriteCosts(t *testing.T) {
 // The churn below rewrites the input far faster than the cache can follow. Whatever
 // the labeler does in the middle, every node has to end up on the label its final
 // input implies.
-func TestLabelsConvergeUnderNodeChurn(t *testing.T) {
+func TestLabelsConvergeUnderNodeChurn_FinalInputs_Converge(t *testing.T) {
 	const (
 		nodeCount = 20
 		rounds    = 8
@@ -2613,6 +2629,22 @@ func countNodeActions(clientset *fake.Clientset, verb string) int {
 	return count
 }
 
+func nodePatchAction(t *testing.T, actions []k8stesting.Action) k8stesting.PatchAction {
+	t.Helper()
+
+	for _, action := range actions {
+		if action.GetVerb() == "patch" && action.GetResource().Resource == "nodes" {
+			patchAction, ok := action.(k8stesting.PatchAction)
+			require.True(t, ok)
+
+			return patchAction
+		}
+	}
+
+	require.FailNow(t, "node PATCH action not found")
+	return nil
+}
+
 // requireCachedLabel waits for the informer cache to show the given label, which is
 // how a test tells that the labeler has finished reacting to earlier events.
 func requireCachedLabel(t *testing.T, l *Labeler, nodeName, key, value string) {