Collector does not retry sample create transaction in case of deadlock and gives up, losing data

[Rohit Jaiswal]

When there are multiple mySQL db processes executing like ceilometer-expirer trying to expire samples and remove orphaned metering data, it acquire locks on tables. An incoming sample create from collector also tries to acquire locks on same table but due to locks already been acquired, either times out or gets rolled back (leading to an error in collector).

The sample insert transaction does not seem to get re-tried and the sample data gets lost.

The sqlalchemy storage driver should implement transaction retries, especially in case of deadlocks.

[ZhiQiang Fan (aji-zqfan)]

for avoid data loss, you can try requeue_event_on_dispatcher_error
https://github.com/openstack/ceilometer/blob/3f9e48155a6dd474a7843dc9aaaef378c7f4ca53/ceilometer/collector.py#L45

there are two problems when ceilometer-expirer runs:
1) potential dead lock, see bug: https://bugs.launchpad.net/bugs/1431986
2) too long time cost which may cause AMQP holds too many messages if requeue_event_on_dispatcher_error is enabled in large scale env

[Rohit Jaiswal (rohit-jaiswal-3)]

Is it possible to try to break the atomic commit of sample create into multiple sub-commits (commit after creating a resource, followed by a commit after inserting meter and final commit after inserting sample)? I think the reason for that deadlock is also because the expirer and the collector (inserter) processes try to acquire locks on same tables at same time, so if we break down the commit of the inserter into multiple smaller commits, it might alleviate the problem.

Similar proposed for expirer, since both processes try to acquire locks on all tables, so by breaking down the commits we might help the deadlock issue.

The drawback is we lose atomicity of the inserts, but the order is still intact i.e we create meters and resources before creating sample so if sample create fails we still have the resource and not the other way around.

[gordon chung (chungg)]

i'd highly advise against multiple commits. it will destroy write performance.

i didn't work on this but it'd be a lot better to batch inserts but it requires requeuing data which isn't necessary for other backends (only sql)

[gordon chung (chungg)]

@Rohit, just curious but are you guys actually testing sql in a large (not small) environment? i stopped SQL work because i didn't want to keep investing time in a backend no one used/could use.

[Rohit Jaiswal (rohit-jaiswal-3)]

@gordc: i was referring to breaking the expirer commit into smaller sub-commits. i agree that sample create should be a single transaction for performance reasons. We found that by doing so, we were able to reduce the occurrence of deadlocks to a great extent. We dont see any deadlocks now. Not to say that its completely eliminated but definitely reduced the possibility. We tested with mysql in a large environment.

[gordon chung (chungg)]

ah cool cool. yeah, that sounds good.

i think one thing we can also look at is implementing the deadlock decorator that Rossella implemented in oslo.db: https://review.openstack.org/#/c/109549/

did you want this bug to cover that?

[Rohit Jaiswal (rohit-jaiswal-3)]

Yes, i think the sample create transaction can use this decorator. An alternative is to requeue the sample in case of a generic dispatcher error using requeue_sample_on_dispatcher_error = True; however this puts extra load on the queue in a large environment and also doesnt seem to work. i created another bug to track that - https://bugs.launchpad.net/ceilometer/+bug/1433881

[Rohit Jaiswal (rohit-jaiswal-3)]

According to the review - retry_on_deadlock decorator can be used on any db.api function. I think the ceilometer storage driver implementation does not use oslo.db.api. If that is correct, this would be a bigger refactoring involved and increase the scope of the change if we were to use this decorator. I also see that there is a decorator in oslo.db.api to retry database connections, whereas we have our own retry decorator for that purpose.

[Rohit Jaiswal (rohit-jaiswal-3)]

@gordc: Let me what you think on this. AFAIK, the oslo.db.api decorator implemented by https://review.openstack.org/#/c/109549/ can be used when oslo.db.api is used/implemented by the storage driver. I think ceilometer storage layer doesnt yet use this, so not sure if we can use the oslo.db decorator. In this case, it makes sense to create a retry decorator in ceilometer/storage/sqlalchemy/utils.py.

[gordon chung (chungg)]

hmm.. so i think if you avoid api stuff and if you use wrap_db_retry decorator it would work (https://github.com/openstack/oslo.db/blob/master/oslo_db/api.py#L83-L152).

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/167034

[Rohit Jaiswal (rohit-jaiswal-3)]

Thanks for the pointer, Gordon. I uploaded a patch. I wasnt able to create a good test case for this. i wanted to mock the time.sleep (from the oslo.db.api.wrap_db_retry class) so as to assert the call_count of the mocked sleep function. But it seems like time is not an attribute of the wrap_db_retry class, so i cant do this. Any pointers would help. Thanks!

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/167034
Committed: https://git.openstack.org/cgit/openstack/ceilometer/commit/?id=9793fe7fa00afd8688df0d2b37cd0a772eb774fb
Submitter: Jenkins
Branch: master

commit 9793fe7fa00afd8688df0d2b37cd0a772eb774fb
Author: Rohit Jaiswal <email address hidden>
Date: Mon Mar 23 15:51:51 2015 -0700

    Using oslo.db retry decorator for sample create

    Decorated record_metering_data method of
    impl_sqlalchemy with oslo.db.api.wrap_db_retry
    decorator to retry on deadlock errors. This will
    allow for configurable number of retry attempts
    when a deadlock occurs while creating a sample.

    Change-Id: I528ea4576f1daccae8fb53393b3c48ae45c509f5
    Closes-Bug: 1432914