Skip to content

Commit

Permalink
Review suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@khewonc
khewonc committed May 29, 2024
1 parent e8429c0 commit 940df86
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 72 deletions.
31 changes: 15 additions & 16 deletions controllers/datadogagent/controller_reconcile_v2.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,11 @@ func (r *Reconciler) reconcileInstanceV2(ctx context.Context, logger logr.Logger
// -----------------------------

var err error
now := metav1.NewTime(time.Now())

result, err = r.reconcileV2ClusterAgent(logger, requiredComponents, features, instance, resourceManagers, newStatus)
if utils.ShouldReturn(result, err) {
return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err)
return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err, now)
}

// Start with an "empty" profile and provider
Expand All @@ -161,12 +162,11 @@ func (r *Reconciler) reconcileInstanceV2(ctx context.Context, logger logr.Logger
}

if r.options.DatadogAgentProfileEnabled {
now := metav1.NewTime(time.Now())
profileList, profilesByNode, e := r.profilesToApply(ctx, logger, nodeList, now)
var profilesByNode map[string]types.NamespacedName
profiles, profilesByNode, e = r.profilesToApply(ctx, logger, nodeList, now)
if err != nil {
return reconcile.Result{}, e
}
profiles = profileList

if err = r.handleProfiles(ctx, profilesByNode, instance.Namespace); err != nil {
return reconcile.Result{}, err
Expand All @@ -178,7 +178,7 @@ func (r *Reconciler) reconcileInstanceV2(ctx context.Context, logger logr.Logger
for provider := range providerList {
result, err = r.reconcileV2Agent(logger, requiredComponents, features, instance, resourceManagers, newStatus, provider, providerList, &profile)
if utils.ShouldReturn(result, err) {
return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err)
return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err, now)
}
}
}
Expand All @@ -189,7 +189,7 @@ func (r *Reconciler) reconcileInstanceV2(ctx context.Context, logger logr.Logger

result, err = r.reconcileV2ClusterChecksRunner(logger, requiredComponents, features, instance, resourceManagers, newStatus)
if utils.ShouldReturn(result, err) {
return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err)
return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err, now)
}

// ------------------------------
Expand All @@ -213,11 +213,10 @@ func (r *Reconciler) reconcileInstanceV2(ctx context.Context, logger logr.Logger
if !result.Requeue && result.RequeueAfter == 0 {
result.RequeueAfter = defaultRequeuePeriod
}
return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err)
return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err, now)
}

func (r *Reconciler) updateStatusIfNeededV2(logger logr.Logger, agentdeployment *datadoghqv2alpha1.DatadogAgent, newStatus *datadoghqv2alpha1.DatadogAgentStatus, result reconcile.Result, currentError error) (reconcile.Result, error) {
now := metav1.NewTime(time.Now())
func (r *Reconciler) updateStatusIfNeededV2(logger logr.Logger, agentdeployment *datadoghqv2alpha1.DatadogAgent, newStatus *datadoghqv2alpha1.DatadogAgentStatus, result reconcile.Result, currentError error, now metav1.Time) (reconcile.Result, error) {
if currentError == nil {
datadoghqv2alpha1.UpdateDatadogAgentStatusConditions(newStatus, now, datadoghqv2alpha1.DatadogAgentReconcileErrorConditionType, metav1.ConditionFalse, "DatadogAgent_reconcile_ok", "DatadogAgent reconcile ok", false)
} else {
Expand Down Expand Up @@ -303,7 +302,7 @@ func (r *Reconciler) updateMetricsForwardersFeatures(dda *datadoghqv2alpha1.Data
// }
}

// ProfilesToApply gets a list of profiles and returns the ones that should be
// profilesToApply gets a list of profiles and returns the ones that should be
// applied in the cluster.
// - If there are no profiles, it returns the default profile.
// - If there are no conflicting profiles, it returns all the profiles plus the default one.
Expand All @@ -321,26 +320,26 @@ func (r *Reconciler) profilesToApply(ctx context.Context, logger logr.Logger, no
return nil, nil, err
}

var profilesToApply []datadoghqv1alpha1.DatadogAgentProfile
profileAppliedPerNode := make(map[string]types.NamespacedName, len(nodeList))
var profileListToApply []datadoghqv1alpha1.DatadogAgentProfile
profileAppliedByNode := make(map[string]types.NamespacedName, len(nodeList))

sortedProfiles := agentprofile.SortProfiles(profilesList.Items)
for _, profile := range sortedProfiles {

profileAppliedPerNode, err = agentprofile.ProfileToApply(logger, &profile, nodeList, profileAppliedPerNode, now)
profileAppliedByNode, err = agentprofile.ProfileToApply(logger, &profile, nodeList, profileAppliedByNode, now)
r.updateDAPStatus(logger, &profile)
if err != nil {
// profile is invalid or conflicts
logger.Error(err, "profile cannot be applied", "name", profile.Name, "namespace", profile.Namespace)
continue
}
profilesToApply = append(profilesToApply, profile)
profileListToApply = append(profileListToApply, profile)
}

// add default profile
profilesToApply = agentprofile.ApplyDefaultProfile(profilesToApply, profileAppliedPerNode, nodeList)
profileListToApply = agentprofile.ApplyDefaultProfile(profileListToApply, profileAppliedByNode, nodeList)

return profilesToApply, profileAppliedPerNode, nil
return profileListToApply, profileAppliedByNode, nil
}

func (r *Reconciler) getNodeList(ctx context.Context) ([]corev1.Node, error) {
Expand Down
34 changes: 17 additions & 17 deletions controllers/datadogagent/controller_reconcile_v2_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ func Test_profilesToApply(t *testing.T) {
ctx := context.Background()

testCases := []struct {
name string
nodeList []corev1.Node
profileList []client.Object
wantProfilesToApply func() []v1alpha1.DatadogAgentProfile
wantProfileAppliedPerNode map[string]types.NamespacedName
wantError error
name string
nodeList []corev1.Node
profileList []client.Object
wantProfilesToApply func() []v1alpha1.DatadogAgentProfile
wantProfileAppliedByNode map[string]types.NamespacedName
wantError error
}{
{
name: "no user-created profiles to apply",
Expand All @@ -69,7 +69,7 @@ func Test_profilesToApply(t *testing.T) {
wantProfilesToApply: func() []v1alpha1.DatadogAgentProfile {
return []v1alpha1.DatadogAgentProfile{defaultProfile()}
},
wantProfileAppliedPerNode: map[string]types.NamespacedName{
wantProfileAppliedByNode: map[string]types.NamespacedName{
"node1": {
Namespace: "",
Name: "default",
Expand All @@ -86,7 +86,7 @@ func Test_profilesToApply(t *testing.T) {
profileList: []client.Object{},
wantProfilesToApply: func() []v1alpha1.DatadogAgentProfile {
return []v1alpha1.DatadogAgentProfile{defaultProfile()}
}, wantProfileAppliedPerNode: map[string]types.NamespacedName{},
}, wantProfileAppliedByNode: map[string]types.NamespacedName{},
},
{
name: "no nodes",
Expand All @@ -112,7 +112,7 @@ func Test_profilesToApply(t *testing.T) {
profileList[0].ResourceVersion = "1000"
return profileList
},
wantProfileAppliedPerNode: map[string]types.NamespacedName{},
wantProfileAppliedByNode: map[string]types.NamespacedName{},
},
{
name: "one profile",
Expand Down Expand Up @@ -162,7 +162,7 @@ func Test_profilesToApply(t *testing.T) {
profileList[0].ResourceVersion = "1000"
return profileList
},
wantProfileAppliedPerNode: map[string]types.NamespacedName{
wantProfileAppliedByNode: map[string]types.NamespacedName{
"node1": {
Namespace: testNamespace,
Name: "1",
Expand Down Expand Up @@ -244,7 +244,7 @@ func Test_profilesToApply(t *testing.T) {
profileList[1].ResourceVersion = "1000"
return profileList
},
wantProfileAppliedPerNode: map[string]types.NamespacedName{
wantProfileAppliedByNode: map[string]types.NamespacedName{
"node1": {
Namespace: testNamespace,
Name: "1",
Expand Down Expand Up @@ -348,7 +348,7 @@ func Test_profilesToApply(t *testing.T) {
return profileList
},

wantProfileAppliedPerNode: map[string]types.NamespacedName{
wantProfileAppliedByNode: map[string]types.NamespacedName{
"node1": {
Namespace: testNamespace,
Name: "3",
Expand Down Expand Up @@ -411,7 +411,7 @@ func Test_profilesToApply(t *testing.T) {
profileList[0].ResourceVersion = "1000"
return profileList
},
wantProfileAppliedPerNode: map[string]types.NamespacedName{
wantProfileAppliedByNode: map[string]types.NamespacedName{
"node1": {
Namespace: testNamespace,
Name: "1",
Expand Down Expand Up @@ -442,7 +442,7 @@ func Test_profilesToApply(t *testing.T) {
wantProfilesToApply: func() []v1alpha1.DatadogAgentProfile {
return generateProfileList([]string{}, []time.Time{})
},
wantProfileAppliedPerNode: map[string]types.NamespacedName{
wantProfileAppliedByNode: map[string]types.NamespacedName{
"node1": {
Namespace: "",
Name: "default",
Expand Down Expand Up @@ -549,7 +549,7 @@ func Test_profilesToApply(t *testing.T) {
profileList[1].ResourceVersion = "1000"
return profileList
},
wantProfileAppliedPerNode: map[string]types.NamespacedName{
wantProfileAppliedByNode: map[string]types.NamespacedName{
"node1": {
Namespace: testNamespace,
Name: "1",
Expand All @@ -574,13 +574,13 @@ func Test_profilesToApply(t *testing.T) {
},
}

profilesToApply, profileAppliedPerNode, err := r.profilesToApply(ctx, logger, tt.nodeList, metav1.NewTime(t1))
profilesToApply, profileAppliedByNode, err := r.profilesToApply(ctx, logger, tt.nodeList, metav1.NewTime(t1))
require.NoError(t, err)

wantProfilesToApply := tt.wantProfilesToApply()
assert.Equal(t, wantProfilesToApply, profilesToApply)
// assert.ElementsMatch(t, wantProfilesToApply, profilesToApply)
assert.Equal(t, tt.wantProfileAppliedPerNode, profileAppliedPerNode)
assert.Equal(t, tt.wantProfileAppliedByNode, profileAppliedByNode)
})
}
}
Expand Down
38 changes: 16 additions & 22 deletions pkg/agentprofile/agent_profile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,8 @@ const (

// ProfileToApply validates a profile spec and returns a map that maps each
// node name to the profile that should be applied to it.
func ProfileToApply(logger logr.Logger, profile *datadoghqv1alpha1.DatadogAgentProfile, nodes []v1.Node, profileAppliedPerNode map[string]types.NamespacedName,
func ProfileToApply(logger logr.Logger, profile *datadoghqv1alpha1.DatadogAgentProfile, nodes []v1.Node, profileAppliedByNode map[string]types.NamespacedName,
now metav1.Time) (map[string]types.NamespacedName, error) {
conflicts := false
nodesThatMatchProfile := map[string]bool{}
profileStatus := datadoghqv1alpha1.DatadogAgentProfileStatus{}

Expand All @@ -48,24 +47,29 @@ func ProfileToApply(logger logr.Logger, profile *datadoghqv1alpha1.DatadogAgentP
profileStatus.Conditions = SetDatadogAgentProfileCondition(profileStatus.Conditions, NewDatadogAgentProfileCondition(ValidConditionType, metav1.ConditionFalse, now, InvalidConditionReason, err.Error()))
profileStatus.Valid = metav1.ConditionFalse
UpdateProfileStatus(profile, profileStatus, now)
return profileAppliedPerNode, err
return profileAppliedByNode, err
}

for _, node := range nodes {
matchesNode, err := profileMatchesNode(profile, node.Labels)
if err != nil {
logger.Error(err, "profile selector is invalid, skipping", "name", profile.Name, "namespace", profile.Namespace)
profileStatus.Conditions = SetDatadogAgentProfileCondition(profileStatus.Conditions, NewDatadogAgentProfileCondition(ValidConditionType, metav1.ConditionFalse, now, InvalidConditionReason, err.Error()))
profileStatus.Valid = metav1.ConditionFalse
UpdateProfileStatus(profile, profileStatus, now)
return profileAppliedPerNode, err
return profileAppliedByNode, err
}
profileStatus.Valid = metav1.ConditionTrue
profileStatus.Conditions = SetDatadogAgentProfileCondition(profileStatus.Conditions, NewDatadogAgentProfileCondition(ValidConditionType, metav1.ConditionTrue, now, ValidConditionReason, "Valid manifest"))

if matchesNode {
if existingProfile, found := profileAppliedPerNode[node.Name]; found {
if existingProfile, found := profileAppliedByNode[node.Name]; found {
// Conflict. This profile should not be applied.
conflicts = true
logger.Info("conflict with existing profile, skipping", "conflicting profile", profile.Namespace+"/"+profile.Name, "existing profile", existingProfile.String())
break
profileStatus.Conditions = SetDatadogAgentProfileCondition(profileStatus.Conditions, NewDatadogAgentProfileCondition(AppliedConditionType, metav1.ConditionFalse, now, ConflictConditionReason, "Conflict with existing profile"))
profileStatus.Applied = metav1.ConditionFalse
UpdateProfileStatus(profile, profileStatus, now)
return profileAppliedByNode, fmt.Errorf("conflict with existing profile")
} else {
nodesThatMatchProfile[node.Name] = true
profileStatus.Conditions = SetDatadogAgentProfileCondition(profileStatus.Conditions, NewDatadogAgentProfileCondition(AppliedConditionType, metav1.ConditionTrue, now, AppliedConditionReason, "Profile applied"))
Expand All @@ -74,35 +78,25 @@ func ProfileToApply(logger logr.Logger, profile *datadoghqv1alpha1.DatadogAgentP
}
}

profileStatus.Valid = metav1.ConditionTrue
profileStatus.Conditions = SetDatadogAgentProfileCondition(profileStatus.Conditions, NewDatadogAgentProfileCondition(ValidConditionType, metav1.ConditionTrue, now, ValidConditionReason, "Valid manifest"))

if conflicts {
profileStatus.Conditions = SetDatadogAgentProfileCondition(profileStatus.Conditions, NewDatadogAgentProfileCondition(AppliedConditionType, metav1.ConditionFalse, now, ConflictConditionReason, "Conflict with existing profile"))
profileStatus.Applied = metav1.ConditionFalse
UpdateProfileStatus(profile, profileStatus, now)
return profileAppliedPerNode, fmt.Errorf("conflict with existing profile")
}

for node := range nodesThatMatchProfile {
profileAppliedPerNode[node] = types.NamespacedName{
profileAppliedByNode[node] = types.NamespacedName{
Namespace: profile.Namespace,
Name: profile.Name,
}
}

UpdateProfileStatus(profile, profileStatus, now)

return profileAppliedPerNode, nil
return profileAppliedByNode, nil
}

func ApplyDefaultProfile(profilesToApply []datadoghqv1alpha1.DatadogAgentProfile, profileAppliedPerNode map[string]types.NamespacedName, nodes []v1.Node) []datadoghqv1alpha1.DatadogAgentProfile {
func ApplyDefaultProfile(profilesToApply []datadoghqv1alpha1.DatadogAgentProfile, profileAppliedByNode map[string]types.NamespacedName, nodes []v1.Node) []datadoghqv1alpha1.DatadogAgentProfile {
profilesToApply = append(profilesToApply, defaultProfile())

// Apply the default profile to all nodes that don't have a profile applied
for _, node := range nodes {
if _, found := profileAppliedPerNode[node.Name]; !found {
profileAppliedPerNode[node.Name] = types.NamespacedName{
if _, found := profileAppliedByNode[node.Name]; !found {
profileAppliedByNode[node.Name] = types.NamespacedName{
Name: defaultProfileName,
}
}
Expand Down
Loading

0 comments on commit 940df86

Please sign in to comment.