Automatically detect deleted resources

[rafaelweingartner]

This patch is created on top of #1385, #1381, #1387, and #1388. Therefore, we need to merge them first. Then, the extra commits here will disappear.

While executing some Gnocchi optimizations (#1307), we noticed that some deleted/removed resources do not have the "ended_at" field with a datetime. This can cause slowness with time, as more and more "zombie" resources are left there, and this has a direct impact in the MySQL queries executed with the aggregates API.

This patch introduces a new parameter called metric_inactive_after, which defines for how long a metric can go without receiving new data points until we consider it as inactive. Then, when all metrics of a resource are in inactive state, we can mark/consider the resource as removed.

[rafaelweingartner]

Thanks @pedro-martins !

[rafaelweingartner]

@pedro-martins and @chungg I have rebased this PR and it is ready for your reviews

[pedro-martins]

Hi Rafael, nice feature, I just leave some comments here, but the code overall is good to me. Thanks for this patch.

[rafaelweingartner]

Thanks @pedro-martins for your review!

[rafaelweingartner]

@chungg thanks for your review! I have added the code changes you suggested, and there are some remarks open, which I would like to hear your response before closing them.

[rafaelweingartner]

Hello @chungg is there something else that needs to be addressed here?

[rafaelweingartner]

Hello guys,
Is there something missing here to move on and merge it?

This patch is the base for some further optimizations that we are doing and that we would like to propose upstream.

[chungg]

i'm ok with this although i think it'd be better if the update logic was in sql rather than python. something like following but in orm. could also be done later.

update resource r set ended_at = %s where not exists (select 1 from metric m where m.resource_id=a.id AND m.last_measure_timestamp >= %s) and r.ended_at is null;

Pull request has been modified.

[rafaelweingartner]

@chungg thanks for your review again. I guess there is nothing else to update here. Can we merge the patch? We have other on top of this, that we would like to start working on.

[rafaelweingartner]

Hello guys,
the failing tests do not seem to be related to the patch itself. Do you have an idea on how to fix it? @chungg and @tobias-urdin ?

When I run here in my machine, it works 😄

[Callum027]

Hello guys, the failing tests do not seem to be related to the patch itself. Do you have an idea on how to fix it? @chungg and @tobias-urdin ?

When I run here in my machine, it works 😄

I haven't done any investigation into the cause, but it seems like there's some kind of race condition for some specific tests. It's not ideal, but I've just been force-pushing unchanged commits on my PRs until the tests pass.

[rafaelweingartner]

Hello guys, the failing tests do not seem to be related to the patch itself. Do you have an idea on how to fix it? @chungg and @tobias-urdin ?
When I run here in my machine, it works 😄

I haven't done any investigation into the cause, but it seems like there's some kind of race condition for some specific tests. It's not ideal, but I've just been force-pushing unchanged commits on my PRs until the tests pass.

So it is not just me :)

[Callum027]

Hello guys, the failing tests do not seem to be related to the patch itself. Do you have an idea on how to fix it? @chungg and @tobias-urdin ?
When I run here in my machine, it works 😄

I haven't done any investigation into the cause, but it seems like there's some kind of race condition for some specific tests. It's not ideal, but I've just been force-pushing unchanged commits on my PRs until the tests pass.

So it is not just me :)

Yep, I've got retries set up in our internal CI pipelines for our Gnocchi deployment for the same reason.

There also seems to be a recurring error where the uWSGI build fails due to some weird system-related issues such as /bin/sh failing to run. In this case it seems like flakiness specific to the GitHub Actions runners.

[Callum027]

Looking at the code, it looks like there's a separate metricd thread that processes incoming measures in an infinite loop with a 0.1 second delay betwen the iterations.

https://github.com/gnocchixyz/gnocchi/blob/master/gnocchi/tests/functional/fixtures.py#L264-L279

100ms is actually a decently long amount of time due to the speed at which these tests are run. The particular tests that fail are probably running their checks before the metricd threads has processed them, and just fail instead of rechecking.

[rafaelweingartner]

Looking at the code, it looks like there's a separate metricd thread that processes incoming measures in an infinite loop with a 0.1 second delay betwen the iterations.

https://github.com/gnocchixyz/gnocchi/blob/master/gnocchi/tests/functional/fixtures.py#L264-L279

100ms is actually a decently long amount of time due to the speed at which these tests are run. The particular tests that fail are probably running their checks before the metricd threads has processed them, and just fail instead of rechecking.

Cool, that gave me an idea. What do you think of a config like this one then in the tests?
8fc168f

This would repeat the test if it fails, and wait 1 seconds. Therefore, we could guarantee that the MetricD will already have been executed in the next try.

[Callum027]

Looking at the code, it looks like there's a separate metricd thread that processes incoming measures in an infinite loop with a 0.1 second delay betwen the iterations.
https://github.com/gnocchixyz/gnocchi/blob/master/gnocchi/tests/functional/fixtures.py#L264-L279
100ms is actually a decently long amount of time due to the speed at which these tests are run. The particular tests that fail are probably running their checks before the metricd threads has processed them, and just fail instead of rechecking.

Cool, that gave me an idea. What do you think of a config like this one then in the tests? 8fc168f

This would repeat the test if it fails, and wait 1 seconds. Therefore, we could guarantee that the MetricD will already have been executed in the next try.

That's a great idea. There's normally two tests that are affected by that race condition so if we could retry them when they fail, that should solve it nicely without changing the current behaviour much.

[rafaelweingartner]

Thanks for the help @Callum027. @chungg and @tobias-urdin what do you think about this patch now?

[Callum027]

Hi @chungg and @tobias-urdin , any chance of getting this and the other PRs I also have proposed looked at sometime soon?

[Callum027]

Hi @rafaelweingartner, I have a question about this proposal.

Can a resource be "un-ended" if we need to start supplying metrics for that resource again (e.g. from Ceilometer)?

The particular use case I'm thinking about is Swift containers. The resource ID for a Swift container in Gnocchi is composed of its project ID and container name, which of course can be reused if you create a new container with the same name. Using this change to clean up old resources is quite beneficial I think, but if you're storing metrics for Swift container in your Gnocchi deployment I'm not sure this would be usable as-is.

[rafaelweingartner]

Hi @rafaelweingartner, I have a question about this proposal.

Can a resource be "un-ended" if we need to start supplying metrics for that resource again (e.g. from Ceilometer)?

The particular use case I'm thinking about is Swift containers. The resource ID for a Swift container in Gnocchi is composed of its project ID and container name, which of course can be reused if you create a new container with the same name. Using this change to clean up old resources is quite beneficial I think, but if you're storing metrics for Swift container in your Gnocchi deployment I'm not sure this would be usable as-is.

Yes, this usecase is considered. See: https://github.com/gnocchixyz/gnocchi/pull/1386/files#diff-9c436196247ad6c3c12d0b085d6e2ae7b57c54684a298457fdcf74c9ff3ac63eR700.

We also added this process due to other situations we have seen in the past, such as bad Ceilometer configurations, which led the system to loose monitoring data for almost a month, and so on. Therefore, with an implementation like this one, we would mark the resources as "finished", but then when the collection resumes, we would mark it as "alive" again.

[tobias-urdin]

I will merge this next week unless there is any objections from previous reviewers @chungg @Callum027 @pedro-martins