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

Ensure kubernetes caches don't expire if they are being read #10946

Merged
merged 6 commits into from Mar 4, 2019

Conversation

Projects
None yet
5 participants
@jsoriano
Copy link
Member

commented Feb 26, 2019

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix #10658

@jsoriano jsoriano self-assigned this Feb 26, 2019

@jsoriano jsoriano force-pushed the jsoriano:kubernetes-metrics-cache-cleanup branch 2 times, most recently from 1902871 to c36d589 Feb 26, 2019

@jsoriano jsoriano force-pushed the jsoriano:kubernetes-metrics-cache-cleanup branch from c36d589 to 1f26a33 Feb 26, 2019

@sayden
Copy link
Contributor

left a comment

I know it's a WIP. Just left some comments if they help.

Show resolved Hide resolved metricbeat/module/kubernetes/util/metrics_cache.go Outdated
Show resolved Hide resolved metricbeat/module/kubernetes/util/metrics_cache.go Outdated

@jsoriano jsoriano marked this pull request as ready for review Feb 27, 2019

@jsoriano jsoriano requested a review from elastic/infrastructure as a code owner Feb 27, 2019

@jsoriano jsoriano added needs_backport and removed backport labels Feb 27, 2019

@jsoriano

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

I have been testing this with master and 6.6.1 and it seems to work.
I have also fixed some issues on tests with the initial version to avoid flakyness and to pass the race detector.

As caches are now, #10658 can still happen if period of container metricset is greater than 120 seconds, fixing this would require to further refactor the global cache. I leave this for future improvements (#10965) as I don't think many people will have these periods. A quick "fix" would be to increase the default timeout.

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

It only quickly peaked at the cache, but it looks similar to libbeat/common/cache.go (godoc) which expires elements based on their last access time, has a goroutine to periodically do cleanup, and allows injecting a clock for testing (but injecting the clock isn't exported so you'd need to expose this).

@jsoriano

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@andrewkroh oh, this actually looks like what we'd need here, I will give a try to this implementation, thanks!

@jsoriano jsoriano force-pushed the jsoriano:kubernetes-metrics-cache-cleanup branch from 8481def to 304bec5 Feb 28, 2019


// Check it expired
assert.Equal(t, 0.0, test.Get("foo"))
}

This comment has been minimized.

Copy link
@jsoriano

jsoriano Feb 28, 2019

Author Member

Test removed because libbeat cache implementation is used now and it has its own tests for this.

@sayden

sayden approved these changes Mar 4, 2019

@jsoriano jsoriano merged commit 106df3d into elastic:master Mar 4, 2019

6 of 7 checks passed

beats-ci Build finished.
Details
CLA All commits in pull request signed
Details
Hound No violations found. Woof!
codecov/patch 74.07% of diff hit (target 57.33%)
Details
codecov/project 57.37% (+0.03%) compared to eaf0889
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jsoriano jsoriano deleted the jsoriano:kubernetes-metrics-cache-cleanup branch Mar 4, 2019

jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 4, 2019

Ensure kubernetes caches don't expire if they are being read (elastic…
…#10946)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix elastic#10658

(cherry picked from commit 106df3d)

@jsoriano jsoriano removed the needs_backport label Mar 4, 2019

jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 4, 2019

Ensure kubernetes caches don't expire if they are being read (elastic…
…#10946)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix elastic#10658

(cherry picked from commit 106df3d)

jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 4, 2019

Ensure kubernetes caches don't expire if they are being read (elastic…
…#10946)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix elastic#10658

(cherry picked from commit 106df3d)

jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 4, 2019

Ensure kubernetes caches don't expire if they are being read (elastic…
…#10946)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix elastic#10658

(cherry picked from commit 106df3d)

jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 4, 2019

Ensure kubernetes caches don't expire if they are being read (elastic…
…#10946)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix elastic#10658

(cherry picked from commit 106df3d)

jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 4, 2019

Ensure kubernetes caches don't expire if they are being read (elastic…
…#10946)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix elastic#10658

(cherry picked from commit 106df3d)

jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 4, 2019

Ensure kubernetes caches don't expire if they are being read (elastic…
…#10946)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix elastic#10658

(cherry picked from commit 106df3d)

jsoriano added a commit that referenced this pull request Mar 5, 2019

Ensure kubernetes caches don't expire if they are being read (#10946) (
…#11057)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix #10658

(cherry picked from commit 106df3d)

jsoriano added a commit that referenced this pull request Mar 5, 2019

Ensure kubernetes caches don't expire if they are being read (#10946) (
…#11058)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix #10658

(cherry picked from commit 106df3d)

jsoriano added a commit that referenced this pull request Mar 5, 2019

Ensure kubernetes caches don't expire if they are being read (#10946) (
…#11059)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix #10658

(cherry picked from commit 106df3d)

jsoriano added a commit that referenced this pull request Mar 14, 2019

Ensure kubernetes caches don't expire if they are being read (#10946) (
…#11060)

Some metrics in metricbeat kubernetes module are cached during a time,
if they are not updated they are removed. But it is usual to have pods or
containers that are not updated during more time that the expiration cache.
Current implementation was not renovating expiration times for cache
entries so all were eventually removed if updates for them are not received.
Replace it with the cache implementation available in libbeat, but keeping
the existing interface.

Also, use slashes instead of dashes to generate unique container uids.
Dashes can be used by kubernetes names, what could lead to ambiguous
keys for the caches.

Fix #10658

(cherry picked from commit 106df3d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.