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

Change locks to use flock, instead of pid checking #6858

Merged
merged 1 commit into from Jul 3, 2017

Conversation

Projects
None yet
7 participants
@keeperofdakeys
Contributor

keeperofdakeys commented Jun 17, 2017

Change locks to use flock, as pid checking is not sufficient when PID Namespaces are involved.

On my system I use pid namespaces for cron jobs. So when discovery.php -h new runs, it always has the pid of 3 - which prevents the pid based locks from working. Using flock instead gets around this issue.

An alternative to this would be to use mysql advisory locks (the GET_LOCK() function).

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jun 17, 2017

Thank you for submitting a PR @keeperofdakeys! We have found the following @murrant, @laf and @zarya based on the history of these files to review this PR.

Thank you for submitting a PR @keeperofdakeys! We have found the following @murrant, @laf and @zarya based on the history of these files to review this PR.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Jun 17, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Jun 17, 2017

CLA assistant check
All committers have signed the CLA.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 19, 2017

Member

Thanks for submitting this. Will run it on my dev install to test it out.

Member

laf commented Jun 19, 2017

Thanks for submitting this. Will run it on my dev install to test it out.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 19, 2017

Member

If you want this PR merged sooner, please rebase to include the fix for Travis breaking on PHP 5.6 tests

Member

laf commented Jun 19, 2017

If you want this PR merged sooner, please rebase to include the fix for Travis breaking on PHP 5.6 tests

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jun 19, 2017

Member

Not a fan of the global $lock_files variable, but if the is no good way to avoid it, it's ok as-is.

Member

murrant commented Jun 19, 2017

Not a fan of the global $lock_files variable, but if the is no good way to avoid it, it's ok as-is.

@keeperofdakeys

This comment has been minimized.

Show comment
Hide comment
@keeperofdakeys

keeperofdakeys Jun 19, 2017

Contributor

Personally I don't like global variables either, but I did that to preserve the api. If you don't mind it changing, we can return an array("filename" => "/path/to/blah", "handle" => $handle), or make it a Class (first seems simpler). Due to the nature of flock, the open handle needs to be kept somewhere.

Contributor

keeperofdakeys commented Jun 19, 2017

Personally I don't like global variables either, but I did that to preserve the api. If you don't mind it changing, we can return an array("filename" => "/path/to/blah", "handle" => $handle), or make it a Class (first seems simpler). Due to the nature of flock, the open handle needs to be kept somewhere.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 20, 2017

Member

Feel free to change how it works :)

Member

laf commented Jun 20, 2017

Feel free to change how it works :)

@keeperofdakeys

This comment has been minimized.

Show comment
Hide comment
@keeperofdakeys

keeperofdakeys Jun 20, 2017

Contributor

Is there any good reason why discovery.php tries to update the sql schema? It's hard to check "is a file flocked" without obtaining the lock, which is the logic used in discovery.php to determine whether to run the schema updater.

So we can either rip this out of discovery.php, or replace it with a file_exists check.

Contributor

keeperofdakeys commented Jun 20, 2017

Is there any good reason why discovery.php tries to update the sql schema? It's hard to check "is a file flocked" without obtaining the lock, which is the logic used in discovery.php to determine whether to run the schema updater.

So we can either rip this out of discovery.php, or replace it with a file_exists check.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 20, 2017

Member

discovery.php runs the schema updates so that anyone forgetting to run them if they've updated manually it will catch that. We shouldn't run in poller as poller is time sensitive.

What do you want to do with file_exists?

Member

laf commented Jun 20, 2017

discovery.php runs the schema updates so that anyone forgetting to run them if they've updated manually it will catch that. We shouldn't run in poller as poller is time sensitive.

What do you want to do with file_exists?

@keeperofdakeys

This comment has been minimized.

Show comment
Hide comment
@keeperofdakeys

keeperofdakeys Jun 20, 2017

Contributor

I was thinking we could just check if the lock file exists, if it doesn't then another schema update isn't occurring at that moment (or it had died).

After thinking about it though, we could get around this if update.php was refactored into a function that didn't die if it couldn't get a lock. Then discovery.php could just call it, and the other four invocations of update.php could themselves die if the update couldn't happen (ie. update_sql() returns false).

I'll start working on it and see how it goes.

Contributor

keeperofdakeys commented Jun 20, 2017

I was thinking we could just check if the lock file exists, if it doesn't then another schema update isn't occurring at that moment (or it had died).

After thinking about it though, we could get around this if update.php was refactored into a function that didn't die if it couldn't get a lock. Then discovery.php could just call it, and the other four invocations of update.php could themselves die if the update couldn't happen (ie. update_sql() returns false).

I'll start working on it and see how it goes.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jun 29, 2017

Member

@keeperofdakeys the class looks good :)

Unfortunately, I am working on some updates that conflict with your schema update changes. Would be happy to merge my changes in later, but I think your code needs some changes. If you want you can roll back your schema update changes and I will merge this and update my PR to use FileLock.

Functions and code should not be mixed in the same file.
Some of your locks are acquired after the DB update process is run.

Member

murrant commented Jun 29, 2017

@keeperofdakeys the class looks good :)

Unfortunately, I am working on some updates that conflict with your schema update changes. Would be happy to merge my changes in later, but I think your code needs some changes. If you want you can roll back your schema update changes and I will merge this and update my PR to use FileLock.

Functions and code should not be mixed in the same file.
Some of your locks are acquired after the DB update process is run.

@keeperofdakeys

This comment has been minimized.

Show comment
Hide comment
@keeperofdakeys

keeperofdakeys Jun 29, 2017

Contributor

I accidentally pushed this a bit early. In light of what you're doing, I'll undo all my update schema changes, and just introduce the new FileLock. I'll need to leave the old locks for now so that the old schema update code will still work however.

Contributor

keeperofdakeys commented Jun 29, 2017

I accidentally pushed this a bit early. In light of what you're doing, I'll undo all my update schema changes, and just introduce the new FileLock. I'll need to leave the old locks for now so that the old schema update code will still work however.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@keeperofdakeys

This comment has been minimized.

Show comment
Hide comment
@keeperofdakeys

keeperofdakeys Jun 29, 2017

Contributor

Unfortunately I'm not well versed in modern PHP, and scrutinizer is giving me this error:
It seems like fopen($this->file, 'w+') of type resource is incompatible with the declared type boolean of property $handle.

Should this be declared as NULL instead, or is there a documentation/attribute that I need?

Contributor

keeperofdakeys commented Jun 29, 2017

Unfortunately I'm not well versed in modern PHP, and scrutinizer is giving me this error:
It seems like fopen($this->file, 'w+') of type resource is incompatible with the declared type boolean of property $handle.

Should this be declared as NULL instead, or is there a documentation/attribute that I need?

@keeperofdakeys

This comment has been minimized.

Show comment
Hide comment
@keeperofdakeys

keeperofdakeys Jun 29, 2017

Contributor
  • Fixing some linting issues on PHP7.
Contributor

keeperofdakeys commented Jun 29, 2017

  • Fixing some linting issues on PHP7.
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jun 29, 2017

Member

You can add a phpdoc comment for the property to declare the type.
/** $handle resource|false */

Or something like that, on mobile right now.

You can also ignore scrutinizer. It is there to warn you about potential issues.

Member

murrant commented Jun 29, 2017

You can add a phpdoc comment for the property to declare the type.
/** $handle resource|false */

Or something like that, on mobile right now.

You can also ignore scrutinizer. It is there to warn you about potential issues.

Add a new locking framework that uses flock.
Change locks to use flock, as pid checking is not
sufficient when PID Namespaces are involved.
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Jun 30, 2017

The inspection completed: 6 updated code elements

The inspection completed: 6 updated code elements

@laf

laf approved these changes Jul 1, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 1, 2017

Member

Thanks @keeperofdakeys, tested and looks good to me.

Member

laf commented Jul 1, 2017

Thanks @keeperofdakeys, tested and looks good to me.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 3, 2017

Member

LGTM :)

LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
SNMP: Get[0/0.00s] Walk [0/0.00s]
MySQL: Cell[1/0.00s] Row[0/-0.00s] Rows[4/0.03s] Column[0/0.00s] Update[0/0.00s] Insert[0/0.00s] Delete[0/0.00s]
LibreNMS Discovery
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
SNMP: Get[0/0.00s] Walk [0/0.00s]
MySQL: Cell[1/0.00s] Row[0/-0.00s] Rows[4/0.03s] Column[0/0.00s] Update[0/0.00s] Insert[0/0.00s] Delete[0/0.00s]
LibreNMS Discovery
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
Member

murrant commented Jul 3, 2017

LGTM :)

LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
SNMP: Get[0/0.00s] Walk [0/0.00s]
MySQL: Cell[1/0.00s] Row[0/-0.00s] Rows[4/0.03s] Column[0/0.00s] Update[0/0.00s] Insert[0/0.00s] Delete[0/0.00s]
LibreNMS Discovery
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
SNMP: Get[0/0.00s] Walk [0/0.00s]
MySQL: Cell[1/0.00s] Row[0/-0.00s] Rows[4/0.03s] Column[0/0.00s] Update[0/0.00s] Insert[0/0.00s] Delete[0/0.00s]
LibreNMS Discovery
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting
LibreNMS Discovery
LibreNMS Discovery
Failed to acquire lock new-discovery, exiting

@murrant murrant merged commit 701fbbc into librenms:master Jul 3, 2017

3 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 3, 2017

Member

Thanks for contributing @keeperofdakeys :D

image

Member

laf commented Jul 3, 2017

Thanks for contributing @keeperofdakeys :D

image

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.