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

Add replicaset and job metaGenerator based on watcher #44

Merged
merged 11 commits into from
May 26, 2023

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented May 16, 2023

Part of #31 to solve increased memory usage of add_resource_metadata.deployment and add_resource_metadata.cronjob based on what was described at #31.

The library "interface" that changes is the

func GetPodMetaGen(
cfg *config.C,
podWatcher kubernetes.Watcher,
nodeWatcher kubernetes.Watcher,
namespaceWatcher kubernetes.Watcher,
metaConf *AddResourceMetadataConfig) MetaGen {
with https://github.com/elastic/elastic-agent-autodiscover/pull/44/files#diff-b153a7b8a2ce2b522412808eec2fa56bfa612345ca05dba323a7ef5ad5d3511cR91-R98. With this change the GetPodMetaGen requires the 2 additional watchers.

We need to adopt accordingly Beats and Agent parts so as to use the new version of the library properly:

Note: I'm planning to add new tests for the 2 additional MetaGenerators in this PR however review can already start in the main codebase's changes.

Screenshots

The following screenshots indicate that the memory leak does not occur with this implementation as well as that the fields are populated properly.

mettricbeat-working-watchers-oom-3h
metricbeat-fixed-discover

@ChrsMark ChrsMark self-assigned this May 16, 2023
@ChrsMark ChrsMark requested review from a team, MichaelKatsoulis, constanca-m and gizas and removed request for a team May 16, 2023 13:24
@elasticmachine
Copy link
Collaborator

elasticmachine commented May 16, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-05-18T11:04:58.948+0000

  • Duration: 11 min 21 sec

Test stats 🧪

Test Results
Failed 0
Passed 252
Skipped 0
Total 252

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@ChrsMark ChrsMark changed the title Add replicaset metaGenerator based on watcher Add replicaset and job metaGenerator based on watcher May 16, 2023
ChrsMark added 3 commits May 16, 2023 17:08
Signed-off-by: ChrsMark <[email protected]>
@ChrsMark ChrsMark marked this pull request as ready for review May 17, 2023 14:15
@ChrsMark ChrsMark requested a review from a team as a code owner May 17, 2023 14:15
@ChrsMark ChrsMark requested review from rdner and leehinman and removed request for a team May 17, 2023 14:15
Signed-off-by: ChrsMark <[email protected]>
@ChrsMark ChrsMark removed request for rdner and leehinman May 17, 2023 15:13
kubernetes/metadata/job.go Outdated Show resolved Hide resolved
kubernetes/metadata/job.go Outdated Show resolved Hide resolved
Signed-off-by: ChrsMark <[email protected]>
ChrsMark added 4 commits May 18, 2023 12:20
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
@ChrsMark
Copy link
Member Author

@MichaelKatsoulis @gizas this one is ready for review when you have the time :)

@ChrsMark ChrsMark requested a review from gizas May 26, 2023 07:27
@MichaelKatsoulis
Copy link
Contributor

Very nice work!

meta := p.job.GenerateFromName(po.Namespace + "/" + jobName)
cronjobName, _ := meta.GetValue("cronjob.name")
if cronjobName != "" {
_, _ = out.Put("cronjob.name", cronjobName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we can put a debug message here on every put action.
And then for cases that we want to troubleshoot OOM issues we can enable debug and check what put actions might take place.

@ChrsMark would it be valuable?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a valid point on high level, however I don't think a debug message at this place would actually help us. Here we just retrieve an already stored value and also this message would occur many times for big number of Pods so this would be an overkill. Sth helpful would be to check the size of the local caches (Informers' cache) but this is quite out of scope I think on this PR. In any case have created elastic/beats#35591 to evaluate further improvements.

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.

4 participants