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

Fix cleanup leak in pipeline collectors #223

Merged
merged 3 commits into from
Dec 27, 2023

Conversation

zwpaper
Copy link
Contributor

@zwpaper zwpaper commented Dec 18, 2023

cleanup is shadowed by the later ones, especially the GPU Collector cleanup, which never has a chance to be executed.

this may be related to #215

@nvvfedorov
Copy link
Collaborator

Thank you for your contribution. Can you add a unit/integration test or add steps on how the change can be tested?

@zwpaper
Copy link
Contributor Author

zwpaper commented Dec 19, 2023

hi @nvvfedorov, as the current code base, it is not easy to add tests for this.

maybe we refactor the NewMetricsPipeline(c *Config) (*MetricsPipeline, func(), error) to

type DCGMCollectorConstructor func([]Counter, *Config, dcgm.Field_Entity_Group) (*DCGMCollector, func(), error)

NewMetricsPipeline(c *Config, NewDCGMCollector DCGMCollectorConstructor) (*MetricsPipeline, func(), error)

so that we can pass a mock DCGMCollectorConstructor into it and test the cleanups

@nvvfedorov
Copy link
Collaborator

hi @nvvfedorov, as the current code base, it is not easy to add tests for this.

maybe we refactor the NewMetricsPipeline(c *Config) (*MetricsPipeline, func(), error) to

type DCGMCollectorConstructor func([]Counter, *Config, dcgm.Field_Entity_Group) (*DCGMCollector, func(), error)

NewMetricsPipeline(c *Config, NewDCGMCollector DCGMCollectorConstructor) (*MetricsPipeline, func(), error)

so that we can pass a mock DCGMCollectorConstructor into it and test the cleanups

This makes sense. Let's make the code better.

@zwpaper zwpaper force-pushed the bug-shadowed-cleanup branch 2 times, most recently from c6877ea to 462072b Compare December 20, 2023 06:46
@zwpaper
Copy link
Contributor Author

zwpaper commented Dec 20, 2023

Hi @nvvfedorov, tests added, PTAL

Copy link
Collaborator

@nvvfedorov nvvfedorov left a comment

Choose a reason for hiding this comment

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

Please put temporary files into the temporary directory. See my comment.

pkg/dcgmexporter/pipeline_test.go Outdated Show resolved Hide resolved
@zwpaper zwpaper force-pushed the bug-shadowed-cleanup branch from 462072b to b643547 Compare December 21, 2023 04:50
@zwpaper zwpaper requested a review from nvvfedorov December 21, 2023 04:51
@zwpaper
Copy link
Contributor Author

zwpaper commented Dec 25, 2023

Hi @nvvfedorov, I have updated the code, PTAL

Copy link
Collaborator

@nvvfedorov nvvfedorov left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/dcgmexporter/pipeline_test.go Show resolved Hide resolved
@zwpaper zwpaper force-pushed the bug-shadowed-cleanup branch from b643547 to 7c1f90d Compare December 27, 2023 02:54
@zwpaper
Copy link
Contributor Author

zwpaper commented Dec 27, 2023

Hi @nvvfedorov, I have updated to sign the commits, can you help merge this PR, or do we need to assign another maintainer?

@nvvfedorov nvvfedorov merged commit 12aa37b into NVIDIA:main Dec 27, 2023
1 check passed
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.

2 participants