Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[profiles] Reduce patch frequency #1317

Merged
merged 6 commits into from
Jul 26, 2024
Merged

Conversation

khewonc
Copy link
Contributor

@khewonc khewonc commented Jul 25, 2024

What does this PR do?

Nodes were being patched every reconcile when there was no change in labels. This only patches nodes when there is a label change

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Minimum Agent Versions

Are there minimum versions of the Datadog Agent and/or Cluster Agent required?

  • Agent: vX.Y.Z
  • Cluster Agent: vX.Y.Z

Describe your test plan

Test in staging:

  • Check the workqueue graphs to see if metrics stay consistent
  • Check that there aren't an enormous number of patch requests in the k8s requests by method/target object graphs

Checklist

  • PR has at least one valid label: bug, enhancement, refactoring, documentation, tooling, and/or dependencies
  • PR has a milestone or the qa/skip-qa label

@khewonc khewonc added the bug Something isn't working label Jul 25, 2024
@khewonc khewonc added this to the v1.8.0 milestone Jul 25, 2024
@khewonc khewonc requested a review from a team as a code owner July 25, 2024 16:13
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.51%. Comparing base (6151d9c) to head (12a9859).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1317      +/-   ##
==========================================
+ Coverage   47.36%   47.51%   +0.15%     
==========================================
  Files         230      230              
  Lines       22027    22040      +13     
==========================================
+ Hits        10432    10472      +40     
+ Misses      11005    10976      -29     
- Partials      590      592       +2     
Flag Coverage Δ
unittests 47.51% <100.00%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rollers/datadogagent/controller_reconcile_agent.go 53.44% <100.00%> (+9.44%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6151d9c...12a9859. Read the comment docs.

@@ -1534,3 +1536,432 @@ func Test_removeStaleStatus(t *testing.T) {
})
}
}

func Test_labelNodesWithProfiles(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding comprehensive test!

// the node, it should be added.
if !isDefaultProfile && !profileLabelExists {
newLabels[agentprofile.ProfileLabelKey] = profileNamespacedName.Name
if len(labelsToRemove) > 0 || len(labelsToAddOrChange) > 0 {
Copy link
Contributor

@celenechang celenechang Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a benefit to having this || instead of two separate conditionals? i think it might be more intuitive if it were separated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first for loop for labelsToRemove goes through all the node labels but the second one for labelsToAddOrChange doesn't. If we separated them, we'd need to loop through node.Labels twice

Copy link
Contributor

@celenechang celenechang Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah i see, thanks! so we only populate newLabels (with all wanted labels) if there's anything to change

Copy link
Contributor

@celenechang celenechang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small comment, otherwise looks good!

@khewonc khewonc merged commit f1f6bb2 into main Jul 26, 2024
19 checks passed
@khewonc khewonc deleted the khewonc/dap-label-removal-patch branch July 26, 2024 15:07
levan-m pushed a commit that referenced this pull request Jul 26, 2024
* Reduce patch requests

* Loop through all profilesByNode

* Refactor

* go lint
levan-m added a commit that referenced this pull request Jul 26, 2024
* Reduce patch requests

* Loop through all profilesByNode

* Refactor

* go lint

Co-authored-by: khewonc <39867936+khewonc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants