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

Compare label hashes instead of recalculating spec hash for daemonsets #1193

Merged
merged 1 commit into from
May 23, 2024

Conversation

khewonc
Copy link
Contributor

@khewonc khewonc commented May 20, 2024

What does this PR do?

#1173 was merged to fix label overrides for pod templates in daemonsets. It caused unnecessary update requests each reconcile since it recalculated the pod template spec from the existing daemonset to compare to the operator-generated daemonset. Kubernetes would fill in values in the existing daemonset, which meant the hashes would always be different and the operator would attempt to update the daemonset even when there were no functional changes.

An example of one of the differences in the existing daemonset where kubernetes added a default value and the operator-generated daemonset:

Existing daemonset:

{Name:DD_KUBERNETES_KUBELET_HOST Value: ValueFrom:&EnvVarSource{FieldRef:&ObjectFieldSelector{APIVersion: ,FieldPath:status.hostIP,}

Operator-generated daemonset:

{Name:DD_KUBERNETES_KUBELET_HOST Value: ValueFrom:&EnvVarSource{FieldRef:&ObjectFieldSelector{APIVersion:v1,FieldPath:status.hostIP,}

This PR takes the pod template labels for the existing and new daemonsets and creates hashes to compare them instead of recalculating the agentspechash.

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: n/a
  • Cluster Agent: n/a

Describe your test plan

  1. Testing label overrides with 1.7.0+:
  • Create a DDA with or without node agent label overrides
  • Add a label to modify the value of one of the label selectors on the daemonset
  override:
    nodeAgent:
      labels:
        "agent.datadoghq.com/name": "bar"
  • Check that the label on the agent pod remains the same as the label selector (agent.datadoghq.com/name=datadog instead of agent.datadoghq.com/name=bar) and that this log is in the operator logs:
{"level":"INFO","ts":"2024-05-08T18:00:52Z","logger":"controllers.DatadogAgent","msg":"Selector value does not match template labels, modifying template labels","datadogagent":"default/datadog","component":"nodeAgent","daemonset.Namespace":"default","daemonset.Name":"datadog-agent","selector label":"agent.datadoghq.com/name: datadog","template label":"agent.datadoghq.com/name: bar"}
  • Add (or change) the node agent label override
  override:
    nodeAgent:
      labels:
        "foo": "bar"
  • Check that the new label was added to the agent pod
  • Remove the override. The label should not be present in the new agent pod
  1. Testing daemonset restarts from previous operator version:
  • Create a DDA with operator <1.7.0 with a node label override
  override:
    nodeAgent:
      labels:
        "foo": "bar"
  • The daemonset label should be the same as the pod template label. Also note the agent.datadoghq.com/agentspechash annotation on the daemonset to use for comparison in later steps
  • Change the label override
  override:
    nodeAgent:
      labels:
        "foo": "bar123"
  • The agent.datadoghq.com/agentspechash annotation value should change, the daemonset label should change to foo:bar123, but the pod template spec should still have the old foo:bar label and the node agent pod shouldn't have restarted
  • Update the operator to 1.7.0+
  • The node agent pod should have restarted. The agent.datadoghq.com/agentspechash annotation value should be the same and both the daemonset label and pod template spec label should say foo:bar123
  • After a few minutes, check the operator logs to make sure that the Updating Daemonset and Creating Daemonset logs don't constantly appear

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 May 20, 2024
@khewonc khewonc added this to the v1.7.0 milestone May 20, 2024
@khewonc khewonc requested review from a team as code owners May 20, 2024 16:55
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 59.53%. Comparing base (0b47ddc) to head (83ab686).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1193      +/-   ##
==========================================
- Coverage   59.54%   59.53%   -0.02%     
==========================================
  Files         174      174              
  Lines       21598    21604       +6     
==========================================
  Hits        12861    12861              
- Misses       7958     7964       +6     
  Partials      779      779              
Flag Coverage Δ
unittests 59.53% <0.00%> (-0.02%) ⬇️

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

Files Coverage Δ
...ers/datadogagent/controller_reconcile_v2_common.go 27.75% <0.00%> (-0.61%) ⬇️

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 0b47ddc...83ab686. Read the comment docs.

@khewonc khewonc merged commit 356b42d into main May 23, 2024
21 checks passed
@khewonc khewonc deleted the khewonc/ds-label-override-fix branch May 23, 2024 19:23
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.

3 participants