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

nvml: add support for datadog agent >= 7.56 #2535

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

maxgio92
Copy link

@maxgio92 maxgio92 commented Nov 12, 2024

What does this PR do?

This PR introduces the NVML check protobuf code regenerated with Python implementation of protobuf version 5.27.2. It should be compatible with Agent >= 7.56.0.

$ docker run --rm -it --entrypoint pip datadog/agent:7.56.0 freeze | grep protobuf
protobuf @ https://agent-int-packages.datadoghq.com/external/protobuf/protobuf-5.27.2-cp38-abi3-manylinux2014_x86_64.whl#sha256=b848dbe1d57ed7c191dfc4ea64b8b004a3f9ece4bf4d0d80a367b76df20bf36e

Breaking changes: it is going to be incompatible with Datadog Agent < 7.56.0, because the runtime Python protobuf version should be greater or equal to the one used to regenerate the code.

Motivation

The current generated protobuf code for the NVML check of the integration with NVML is no longer compatible with the version of runtime protobuf implementation bundled with the Datadog Agent (since version 7.51).

See #2382.

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Git history is clean

Additional Notes

Fixes #2382

@maxgio92 maxgio92 changed the title chore(nvml/datadog_checks/nvml): regenerate protobuf code with protobuf 5.27.2 chore(nvml): regenerate check protobuf code with protobuf 5.27.2 Nov 12, 2024
@maxgio92 maxgio92 changed the title chore(nvml): regenerate check protobuf code with protobuf 5.27.2 fix(nvml): regenerate check protobuf code with protobuf 5.27.2 Nov 12, 2024
@maxgio92
Copy link
Author

Hi @Kyle-Neale, may I ask you a feedback around this?
Thank you

@maxgio92 maxgio92 changed the title fix(nvml): regenerate check protobuf code with protobuf 5.27.2 nvml check protobuf update: regenerate with protobuf 5.27.2 Nov 18, 2024
@arapulido
Copy link
Contributor

Hello @cep21! I think you are the maintainer for this particular integration. Would you mind having a look and see if this is something you would like to accept? Thanks!

Copy link
Contributor

@cep21 cep21 left a comment

Choose a reason for hiding this comment

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

The PR is fine, but I'm unable to maintain this module anymore. Can I be removed from the maintainers?

The current generated protobuf code is no longer compatible
with the version of runtime protobuf implementation bundled
with the Datadog Agent.

This api protobuf code is regenerated using Python protobuf
 5.27.2. It should be compatible with Agent >= 7.56.0.

Signed-off-by: Massimiliano Giovagnoli <[email protected]>
@maxgio92 maxgio92 force-pushed the nvml-protobuf-update branch from b721d01 to d07247f Compare November 21, 2024 21:37
@maxgio92 maxgio92 requested a review from cep21 November 21, 2024 21:37
@maxgio92
Copy link
Author

maxgio92 commented Nov 21, 2024

Thank you @cep21, I've noticed that there was a conflict, so I've solved it now.
Would you mind taking a look and if ok approving again?
Thank you very much

@maxgio92 maxgio92 force-pushed the nvml-protobuf-update branch from 8f6c4fa to c042150 Compare November 21, 2024 21:57
Signed-off-by: Massimiliano Giovagnoli <[email protected]>
@maxgio92 maxgio92 force-pushed the nvml-protobuf-update branch from c042150 to b8f31b3 Compare November 21, 2024 22:01
Bump pynvml patch version to 11.5.3 and grpcio
minor to 1.68.0.

Signed-off-by: Massimiliano Giovagnoli <[email protected]>
@maxgio92
Copy link
Author

maxgio92 commented Nov 22, 2024

I fixed the integration tests by bumping grpcio and pynvml to the latest available. They now pass.

Now the CI is complaining for non optimal code coverage 🫠

In the meantime I'm working to reach 75% target.

@maxgio92 maxgio92 changed the title nvml check protobuf update: regenerate with protobuf 5.27.2 nvml: add support for datadog agent 7.5x Nov 22, 2024
@maxgio92 maxgio92 changed the title nvml: add support for datadog agent 7.5x nvml: add support for datadog agent >= 7.56 Nov 22, 2024
Signed-off-by: Massimiliano Giovagnoli <[email protected]>
@maxgio92 maxgio92 force-pushed the nvml-protobuf-update branch from 6d9e6c8 to 2d46e2c Compare November 22, 2024 11:04
@maxgio92
Copy link
Author

maxgio92 commented Nov 22, 2024

codecov reports now 76.52% with the two unit tests I've just added 🥳 All checks are fine now.

@cep21 may I ask you to re-validate and possibly approve this PR? :)

@maxgio92
Copy link
Author

Hi @CEP2 , I noticed from the CODEOWNERS you're the only maintainer of this integration but 2 reviews are required. Do you know if anyone can approve in order to merge it?
cc @arapulido

@maxgio92
Copy link
Author

maxgio92 commented Dec 4, 2024

I think there are three approving reviews, which is more than 1. Am I missing something? 🤔

@arapulido
Copy link
Contributor

@maxgio92 one of the reviews need to come from someone with writing access to the repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to start NVML integration
5 participants