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

CA-404013: do not relock the mutex when backing up rrds #6215

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

psafont
Copy link
Member

@psafont psafont commented Jan 8, 2025

The point of using try_lock is to not get lock while trying to hold the mutex.
Releasing it and blocking to lock it defeats the purpose of using try_lock.
Reorganise the sequence to read and copy all the rrds first while under the
locked mutex, release it, and then archive the copies.

This doesn't fix CA-404013 directly, so this can wait until the release is cut. I'm opening otherwise I'll forget to open it when things are unlocked.

I've run the smoke and validation tests, as well as checking that the rrd files in the bugtools are well formed for hosts and vms (they are created suing this function)

The point of using try_lock is to not get the thread suspended while trying to
hold the mutex. Releasing it and calling `lock` may suspend the thread and
defeats the purpose of using try_lock in the first place.

Reorganise the sequence to read and copy all the rrds first while under the
locked mutex, release it, and then archive the copies.

Signed-off-by: Pau Ruiz Safont <[email protected]>
It causes logspam and hasn't been useful in the years it's been present.
Lowering it to a debug statement would still cause it to be logged, so drop the
message completely instead.

Signed-off-by: Pau Ruiz Safont <[email protected]>
Copy link
Member

@robhoes robhoes left a comment

Choose a reason for hiding this comment

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

Much better.

@snwoods snwoods self-requested a review January 8, 2025 11:11
@lindig
Copy link
Contributor

lindig commented Jan 10, 2025

I suggest to merge this now

@robhoes robhoes added this pull request to the merge queue Jan 10, 2025
Merged via the queue into xapi-project:master with commit f9b5e52 Jan 10, 2025
15 checks passed
@psafont psafont deleted the private/paus/one-unlock branch January 10, 2025 14:58
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