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

Use PodResources mount as a path instead of file #1674

Merged
merged 6 commits into from
Feb 13, 2025

Conversation

gjulianm
Copy link
Collaborator

@gjulianm gjulianm commented Feb 10, 2025

What does this PR do?

This PR fixes the PodResources socket mount introduced in #1650. By using a file mount directly, if kubelet restarts and recreates the socket, the container will not see the new socket and will keep the reference to the old one, thus losing the connection.

Motivation

Fix a bug with kubelet restarts.

Additional Notes

Minimum Agent Versions

No changes w.r.t #1650

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

Describe your test plan

Unit tests should reflect the new change in mounts.

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

@gjulianm gjulianm self-assigned this Feb 10, 2025
@gjulianm gjulianm force-pushed the guillermo.julian/pod-resources-socket-dir branch from 12751ee to bdc2433 Compare February 10, 2025 11:34
@gjulianm gjulianm added the bug Something isn't working label Feb 10, 2025
@gjulianm gjulianm added this to the v1.12.0 milestone Feb 10, 2025
@gjulianm gjulianm force-pushed the guillermo.julian/pod-resources-socket-dir branch from df9c058 to 0327b63 Compare February 10, 2025 11:58
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.49%. Comparing base (91095b8) to head (c277a79).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1674   +/-   ##
=======================================
  Coverage   49.49%   49.49%           
=======================================
  Files         218      218           
  Lines       21244    21244           
=======================================
  Hits        10515    10515           
  Misses      10182    10182           
  Partials      547      547           
Flag Coverage Δ
unittests 49.49% <100.00%> (ø)

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

Files with missing lines Coverage Δ
...ller/datadogagent/defaults/datadogagent_default.go 91.45% <100.00%> (ø)
...nternal/controller/datadogagent/override/global.go 56.52% <100.00%> (ø)
pkg/testutils/builder.go 91.67% <100.00%> (ø)

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 91095b8...c277a79. Read the comment docs.

@gjulianm gjulianm marked this pull request as ready for review February 10, 2025 12:39
@gjulianm gjulianm requested review from a team as code owners February 10, 2025 12:39
@tbavelier tbavelier modified the milestones: v1.12.0, v1.13.0 Feb 10, 2025
Copy link
Member

@tbavelier tbavelier left a comment

Choose a reason for hiding this comment

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

Not a blocker: could we also envision a test to check a custom provided path (e.g. spec.global.podResourcesSocketPath= /foo/bar) ?

})

podResourcesVol, podResourcesMount := volume.GetVolumes(v2alpha1.KubeletPodResourcesVolumeName, config.Kubelet.PodResourcesSocket, config.Kubelet.PodResourcesSocket, false)
podResourcesVol, podResourcesMount := volume.GetVolumes(v2alpha1.KubeletPodResourcesVolumeName, config.Kubelet.PodResourcesSocketDir, config.Kubelet.PodResourcesSocketDir, false)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
podResourcesVol, podResourcesMount := volume.GetVolumes(v2alpha1.KubeletPodResourcesVolumeName, config.Kubelet.PodResourcesSocketDir, config.Kubelet.PodResourcesSocketDir, false)
podResourcesVol, podResourcesMount := volume.GetVolumes(v2alpha1.KubeletPodResourcesVolumeName, config.Kubelet.PodResourcesSocketDir, config.Kubelet.PodResourcesSocketDir, true)

Could we use readOnly: true on this volume or that prevents from communicating with the socket ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my testing it prevented communication, yes.

api/datadoghq/v2alpha1/datadogagent_types.go Outdated Show resolved Hide resolved
api/datadoghq/v2alpha1/datadogagent_types.go Outdated Show resolved Hide resolved
@janine-c janine-c self-assigned this Feb 10, 2025
@janine-c janine-c removed their assignment Feb 10, 2025
@tbavelier tbavelier merged commit ba5e92f into main Feb 13, 2025
19 checks passed
@tbavelier tbavelier deleted the guillermo.julian/pod-resources-socket-dir branch February 13, 2025 11:49
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