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.

Sign up for GitHub

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

Concurrency issues when multiple patches happen for the same resources #1277

Merged
merged 1 commit into from
Jan 2, 2023
Merged

Concurrency issues when multiple patches happen for the same resources #1277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions gnocchi/indexer/sqlalchemy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,12 +897,29 @@ def create_resource(self, resource_type, id,
return r

@retry_on_deadlock
def update_resource(self, resource_type,
resource_id, ended_at=_marker, metrics=_marker,
append_metrics=False,
create_revision=True,
def update_resource(self, resource_type, resource_id,
ended_at=_marker, metrics=_marker,
append_metrics=False, create_revision=True,
**kwargs):
with self.facade.writer() as session:
data_to_update = kwargs.copy()

data_to_update['ended_at'] = ended_at
data_to_update['metrics'] = metrics
if create_revision:
resource = self.get_resource(
resource_type, resource_id, with_metrics=True)
if not utils.is_resource_revision_needed(
resource, data_to_update):
LOG.info("We thought that a revision for resource "
"[%s] was needed. However, after locking the "
"table and checking it again, we found that it "
"is not needed anymore. This is due to a "
"concurrency issue that might happen. Therefore, "
"no revision is going to be generated at this "
"time.", data_to_update)
create_revision = False
Copy link
Member

Choose a reason for hiding this comment

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

i wonder if it makes sense to just move the check revision logic to https://github.com/gnocchixyz/gnocchi/blob/master/gnocchi/indexer/sqlalchemy.py#L922-L923. that way it would be behind the lock created by for_update.

that said, now it's always locking and i've no idea the impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, it's probably very hard to test the race condition there, but do we have a test for the normal case of a new revision to verify this doesn't break normal behavior? (either unit or a gabbit)

Also please squash (cleanup) the commits into one (or more, whatever is preferred) but provide good commit messages.

Thanks for the review. I squashed the commits. Also, regarding tests, there are already gabbi tests that cover resource revisions, and all of them are passing just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chungg I agree with you. Ideally, I Would have done that. However, I implemented this way to be less intrusive as possible in the code base.

I can move the decision to generate revision to the update_resource method only, if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this, I'll let @chungg respond and approve if he's OK with it.


mappers = self._resource_type_to_mappers(session, resource_type)
resource_cls = mappers["resource"]
resource_history_cls = mappers["history"]
Expand Down
15 changes: 5 additions & 10 deletions gnocchi/rest/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,16 +1073,11 @@ def patch(self):
etag_set_headers(resource)
return resource

for k, v in six.iteritems(body):
if k != 'metrics' and getattr(resource, k) != v:
create_revision = True
break
else:
if 'metrics' not in body:
# No need to go further, we assume the db resource
# doesn't change between the get and update
return resource
create_revision = False
create_revision = utils.is_resource_revision_needed(resource, body)
if not create_revision and 'metrics' not in body:
# No need to go further, we assume the db resource
# doesn't change between the get and update
return resource

try:
resource = pecan.request.indexer.update_resource(
Expand Down
15 changes: 15 additions & 0 deletions gnocchi/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,3 +351,18 @@ def __call__(self, retry_state):
def retry_on_exception_and_log(msg):
return tenacity.Retrying(
wait=wait_exponential, retry=_retry_on_exception_and_log(msg)).wraps


def is_resource_revision_needed(resource, request_attributes):
for k, v in six.iteritems(request_attributes):
if not hasattr(resource, k):
continue

database_attribute = getattr(resource, k)
if k != 'metrics' and database_attribute != v:
LOG.debug("Generating a new revision for resource [%s] "
"because the attribute key [%s] with its value [%s] "
"is different from the one found in the database [%s]"
".", resource, k, v, database_attribute)
return True
return False