From 5f3b7a8b042044208d81bae1e628ea6056e4e677 Mon Sep 17 00:00:00 2001 From: khewonc <39867936+khewonc@users.noreply.github.com> Date: Fri, 26 Jul 2024 11:07:47 -0400 Subject: [PATCH] [profiles] Reduce patch frequency (#1317) * Reduce patch requests * Loop through all profilesByNode * Refactor * go lint --- .../controller_reconcile_agent.go | 45 +- .../controller_reconcile_agent_test.go | 431 ++++++++++++++++++ 2 files changed, 461 insertions(+), 15 deletions(-) diff --git a/controllers/datadogagent/controller_reconcile_agent.go b/controllers/datadogagent/controller_reconcile_agent.go index e2e66f02a..9d1ff1851 100644 --- a/controllers/datadogagent/controller_reconcile_agent.go +++ b/controllers/datadogagent/controller_reconcile_agent.go @@ -283,30 +283,45 @@ func (r *Reconciler) labelNodesWithProfiles(ctx context.Context, profilesByNode return err } - _, profileLabelExists := node.Labels[agentprofile.ProfileLabelKey] - newLabels := map[string]string{} - for k, v := range node.Labels { - // If the profile uses the old profile label key, it should be removed - if k != agentprofile.OldProfileLabelKey { - newLabels[k] = v - } - } + labelsToRemove := map[string]bool{} + labelsToAddOrChange := map[string]string{} // If the profile is the default one and the label exists in the node, // it should be removed. - if isDefaultProfile && profileLabelExists { - delete(newLabels, agentprofile.ProfileLabelKey) + if isDefaultProfile { + if _, profileLabelExists := node.Labels[agentprofile.ProfileLabelKey]; profileLabelExists { + labelsToRemove[agentprofile.ProfileLabelKey] = true + } + } else { + // If the profile is not the default one and the label does not exist in + // the node, it should be added. If the label value is outdated, it + // should be updated. + if profileLabelValue := node.Labels[agentprofile.ProfileLabelKey]; profileLabelValue != profileNamespacedName.Name { + labelsToAddOrChange[agentprofile.ProfileLabelKey] = profileNamespacedName.Name + } + } + + // Remove old profile label key if it is present + if _, oldProfileLabelExists := node.Labels[agentprofile.OldProfileLabelKey]; oldProfileLabelExists { + labelsToRemove[agentprofile.OldProfileLabelKey] = true } - // If the profile is not the default one and the label does not exist in - // the node, it should be added. - if !isDefaultProfile && !profileLabelExists { - newLabels[agentprofile.ProfileLabelKey] = profileNamespacedName.Name + if len(labelsToRemove) > 0 || len(labelsToAddOrChange) > 0 { + for k, v := range node.Labels { + if _, ok := labelsToRemove[k]; ok { + continue + } + newLabels[k] = v + } + + for k, v := range labelsToAddOrChange { + newLabels[k] = v + } } if len(newLabels) == 0 { - return nil + continue } modifiedNode := node.DeepCopy() diff --git a/controllers/datadogagent/controller_reconcile_agent_test.go b/controllers/datadogagent/controller_reconcile_agent_test.go index ea9f54492..b935feea2 100644 --- a/controllers/datadogagent/controller_reconcile_agent_test.go +++ b/controllers/datadogagent/controller_reconcile_agent_test.go @@ -10,6 +10,7 @@ import ( datadoghqv2alpha1 "github.com/DataDog/datadog-operator/apis/datadoghq/v2alpha1" apiutils "github.com/DataDog/datadog-operator/apis/utils" "github.com/DataDog/datadog-operator/controllers/datadogagent/component/agent" + "github.com/DataDog/datadog-operator/pkg/agentprofile" "github.com/DataDog/datadog-operator/pkg/kubernetes" edsdatadoghqv1alpha1 "github.com/DataDog/extendeddaemonset/api/v1alpha1" @@ -18,6 +19,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" @@ -1534,3 +1536,432 @@ func Test_removeStaleStatus(t *testing.T) { }) } } + +func Test_labelNodesWithProfiles(t *testing.T) { + sch := runtime.NewScheme() + _ = scheme.AddToScheme(sch) + ctx := context.Background() + + testCases := []struct { + name string + description string + profilesByNode map[string]types.NamespacedName + nodes []client.Object + wantNodeLabels map[string]map[string]string + }{ + { + name: "label multiple profile nodes and default node", + description: "node-1 and node-2 should be labeled with profile name, node-default should stay nil", + profilesByNode: map[string]types.NamespacedName{ + "node-1": { + Namespace: "foo", + Name: "profile-1", + }, + "node-2": { + Namespace: "foo", + Name: "profile-2", + }, + "node-default": { + Namespace: "", + Name: "default", + }, + }, + nodes: []client.Object{ + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Labels: map[string]string{ + "1": "1", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-2", + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-default", + }, + }, + }, + wantNodeLabels: map[string]map[string]string{ + "node-1": { + agentprofile.ProfileLabelKey: "profile-1", + "1": "1", + }, + "node-2": { + agentprofile.ProfileLabelKey: "profile-2", + }, + "node-default": nil, + }, + }, + { + name: "label multiple profile nodes, default node has profile label", + description: "node-1 and node-2 should be labeled with profile name, profile label should be removed from node-default", + profilesByNode: map[string]types.NamespacedName{ + "node-1": { + Namespace: "foo", + Name: "profile-1", + }, + "node-2": { + Namespace: "foo", + Name: "profile-2", + }, + "node-default": { + Namespace: "", + Name: "default", + }, + }, + nodes: []client.Object{ + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Labels: map[string]string{ + "1": "1", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-2", + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-default", + Labels: map[string]string{ + agentprofile.ProfileLabelKey: "profile-1", + "foo": "bar", + }, + }, + }, + }, + wantNodeLabels: map[string]map[string]string{ + "node-1": { + agentprofile.ProfileLabelKey: "profile-1", + "1": "1", + }, + "node-2": { + agentprofile.ProfileLabelKey: "profile-2", + }, + "node-default": { + "foo": "bar", + }, + }, + }, + { + name: "remove old profile label", + description: "old profile label should be removed from node-2 and node-default", + profilesByNode: map[string]types.NamespacedName{ + "node-1": { + Namespace: "foo", + Name: "profile-1", + }, + "node-2": { + Namespace: "foo", + Name: "profile-2", + }, + "node-default": { + Namespace: "", + Name: "default", + }, + }, + nodes: []client.Object{ + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Labels: map[string]string{ + "1": "1", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-2", + Labels: map[string]string{ + agentprofile.OldProfileLabelKey: "profile-2", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-default", + Labels: map[string]string{ + agentprofile.ProfileLabelKey: "profile-1", + agentprofile.OldProfileLabelKey: "profile-2", + "foo": "bar", + }, + }, + }, + }, + wantNodeLabels: map[string]map[string]string{ + "node-1": { + agentprofile.ProfileLabelKey: "profile-1", + "1": "1", + }, + "node-2": { + agentprofile.ProfileLabelKey: "profile-2", + }, + "node-default": { + "foo": "bar", + }, + }, + }, + { + name: "outdated label value", + description: "profile label value should be replaced with the profile-1", + profilesByNode: map[string]types.NamespacedName{ + "node-1": { + Namespace: "foo", + Name: "profile-1", + }, + "node-2": { + Namespace: "foo", + Name: "profile-2", + }, + "node-default": { + Namespace: "", + Name: "default", + }, + }, + nodes: []client.Object{ + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Labels: map[string]string{ + "1": "1", + agentprofile.ProfileLabelKey: "profile-2", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-2", + Labels: map[string]string{ + agentprofile.OldProfileLabelKey: "profile-2", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-default", + Labels: map[string]string{ + agentprofile.ProfileLabelKey: "profile-1", + agentprofile.OldProfileLabelKey: "profile-2", + "foo": "bar", + }, + }, + }, + }, + wantNodeLabels: map[string]map[string]string{ + "node-1": { + agentprofile.ProfileLabelKey: "profile-1", + "1": "1", + }, + "node-2": { + agentprofile.ProfileLabelKey: "profile-2", + }, + "node-default": { + "foo": "bar", + }, + }, + }, + { + name: "no changes", + description: "no changes needed", + profilesByNode: map[string]types.NamespacedName{ + "node-1": { + Namespace: "foo", + Name: "profile-1", + }, + "node-2": { + Namespace: "foo", + Name: "profile-2", + }, + "node-default": { + Namespace: "", + Name: "default", + }, + }, + nodes: []client.Object{ + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Labels: map[string]string{ + "1": "1", + agentprofile.ProfileLabelKey: "profile-1", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-2", + Labels: map[string]string{ + agentprofile.ProfileLabelKey: "profile-2", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-default", + Labels: map[string]string{ + "foo": "bar", + }, + }, + }, + }, + wantNodeLabels: map[string]map[string]string{ + "node-1": { + agentprofile.ProfileLabelKey: "profile-1", + "1": "1", + }, + "node-2": { + agentprofile.ProfileLabelKey: "profile-2", + }, + "node-default": { + "foo": "bar", + }, + }, + }, + { + name: "labels to remove only", + description: "node-1 old profile label key and node-default profile label key should be removed", + profilesByNode: map[string]types.NamespacedName{ + "node-1": { + Namespace: "foo", + Name: "profile-1", + }, + "node-2": { + Namespace: "foo", + Name: "profile-2", + }, + "node-default": { + Namespace: "", + Name: "default", + }, + }, + nodes: []client.Object{ + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Labels: map[string]string{ + "1": "1", + agentprofile.ProfileLabelKey: "profile-1", + agentprofile.OldProfileLabelKey: "profile-2", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-2", + Labels: map[string]string{ + agentprofile.ProfileLabelKey: "profile-2", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-default", + Labels: map[string]string{ + "foo": "bar", + agentprofile.ProfileLabelKey: "profile-2", + }, + }, + }, + }, + wantNodeLabels: map[string]map[string]string{ + "node-1": { + agentprofile.ProfileLabelKey: "profile-1", + "1": "1", + }, + "node-2": { + agentprofile.ProfileLabelKey: "profile-2", + }, + "node-default": { + "foo": "bar", + }, + }, + }, + { + name: "labels to add/change only", + description: "node-1 profile label key should be changed and node-2 profile label key should be added", + profilesByNode: map[string]types.NamespacedName{ + "node-1": { + Namespace: "foo", + Name: "profile-1", + }, + "node-2": { + Namespace: "foo", + Name: "profile-2", + }, + "node-default": { + Namespace: "", + Name: "default", + }, + }, + nodes: []client.Object{ + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Labels: map[string]string{ + "1": "1", + agentprofile.ProfileLabelKey: "profile-2", + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-2", + Labels: map[string]string{}, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-default", + Labels: map[string]string{ + "foo": "bar", + }, + }, + }, + }, + wantNodeLabels: map[string]map[string]string{ + "node-1": { + agentprofile.ProfileLabelKey: "profile-1", + "1": "1", + }, + "node-2": { + agentprofile.ProfileLabelKey: "profile-2", + }, + "node-default": { + "foo": "bar", + }, + }, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + fakeClient := fake.NewClientBuilder().WithScheme(sch).WithObjects(tt.nodes...).Build() + + r := &Reconciler{ + client: fakeClient, + } + + err := r.labelNodesWithProfiles(ctx, tt.profilesByNode) + assert.NoError(t, err) + + nodeList := &corev1.NodeList{} + err = fakeClient.List(ctx, nodeList) + assert.NoError(t, err) + assert.Len(t, nodeList.Items, len(tt.wantNodeLabels)) + + for _, node := range nodeList.Items { + expectedNodeLabels, ok := tt.wantNodeLabels[node.Name] + assert.True(t, ok) + assert.Equal(t, expectedNodeLabels, node.Labels) + } + }) + } +}