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

Use the new locks for schema updates #6931

Merged
merged 7 commits into from Jul 17, 2017

Conversation

Projects
None yet
6 participants
@murrant
Member

murrant commented Jul 3, 2017

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

No code changes inside the block (try ?w=1 on the github url)

Thanks @keeperofdakeys

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jul 3, 2017

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

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

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 3, 2017

Member

@keeperofdakeys @murrant I've removed the discovery one as it's not needed as we already do the check in sql-schema/update.php

Member

laf commented Jul 3, 2017

@keeperofdakeys @murrant I've removed the discovery one as it's not needed as we already do the check in sql-schema/update.php

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 3, 2017

Member

@laf I don't think that is what the discovery lock was for. I remember someone having long running new device discovery.

On this PR, do you think we should make it wait before continuing or just continue straight away like it is now?

Also, will this work for distributed pollers?

Member

murrant commented Jul 3, 2017

@laf I don't think that is what the discovery lock was for. I remember someone having long running new device discovery.

On this PR, do you think we should make it wait before continuing or just continue straight away like it is now?

Also, will this work for distributed pollers?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 3, 2017

Member

@murrant The long running issue was multiple stacked new discoveries which https://github.com/murrant/librenms/blob/c8c6bd0f1f410b1a5c75d81f34071f24080f2da8/discovery.php#L36 fixes.

I don't think waiting is the best option really

Member

laf commented Jul 3, 2017

@murrant The long running issue was multiple stacked new discoveries which https://github.com/murrant/librenms/blob/c8c6bd0f1f410b1a5c75d81f34071f24080f2da8/discovery.php#L36 fixes.

I don't think waiting is the best option really

@keeperofdakeys

This comment has been minimized.

Show comment
Hide comment
@keeperofdakeys

keeperofdakeys Jul 3, 2017

Contributor

If you're concerned about distributed pollers, you could use mysql named locks (GET_LOCK()) for schema updates.

Contributor

keeperofdakeys commented Jul 3, 2017

If you're concerned about distributed pollers, you could use mysql named locks (GET_LOCK()) for schema updates.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 3, 2017

Member

@laf Oh, I didn't even know that was there. Read your comment on mobile and didn't notice the change. 👍

@keeperofdakeys That's what we would have to do, but the question is are we concerned about distributed pollers?

Member

murrant commented Jul 3, 2017

@laf Oh, I didn't even know that was there. Read your comment on mobile and didn't notice the change. 👍

@keeperofdakeys That's what we would have to do, but the question is are we concerned about distributed pollers?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 3, 2017

Member

As for the waiting, I don't think it is the best thing either. But do realize I mean wait with a timeout and periodically while waiting check the lock.

Member

murrant commented Jul 3, 2017

As for the waiting, I don't think it is the best thing either. But do realize I mean wait with a timeout and periodically while waiting check the lock.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 3, 2017

Member

I guess we have 4:59m to play with before a new discovery process is kicked off so go for it. 30 seconds maybe?

Member

laf commented Jul 3, 2017

I guess we have 4:59m to play with before a new discovery process is kicked off so go for it. 30 seconds maybe?

Add MysqlLock
Use that for the schema updates
Wait up to 30s for other schema updates to complete.
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 3, 2017

Member

@laf we could use this to do per-device discovery/polling locking like the python poller service does. Probably not in this PR though :)

Member

murrant commented Jul 3, 2017

@laf we could use this to do per-device discovery/polling locking like the python poller service does. Probably not in this PR though :)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 3, 2017

Member

@murrant The mysql locking here, how does that work for replicated mysql or mysql clusters?

Member

laf commented Jul 3, 2017

@murrant The mysql locking here, how does that work for replicated mysql or mysql clusters?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 3, 2017

Member

@laf From the MySQL docs:

This function is unsafe for statement-based replication. A warning is logged if you use this function when binlog_format is set to STATEMENT.

So, assuming that is not the case, things should work. I don't have access to either of those setups at this time.

For Galera:

Unsupported explicit locking include LOCK TABLES, FLUSH TABLES {explicit table list} WITH READ LOCK, (GET_LOCK(), RELEASE_LOCK(),…). Using transactions properly should be able to overcome these limitations. Global locking operators like FLUSH TABLES WITH READ LOCK are supported.

So, sounds like a a no. Perhaps we need a fallback to file based locking?

Member

murrant commented Jul 3, 2017

@laf From the MySQL docs:

This function is unsafe for statement-based replication. A warning is logged if you use this function when binlog_format is set to STATEMENT.

So, assuming that is not the case, things should work. I don't have access to either of those setups at this time.

For Galera:

Unsupported explicit locking include LOCK TABLES, FLUSH TABLES {explicit table list} WITH READ LOCK, (GET_LOCK(), RELEASE_LOCK(),…). Using transactions properly should be able to overcome these limitations. Global locking operators like FLUSH TABLES WITH READ LOCK are supported.

So, sounds like a a no. Perhaps we need a fallback to file based locking?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 3, 2017

Member

@f0o Any thoughts?

Member

laf commented Jul 3, 2017

@f0o Any thoughts?

@keeperofdakeys

This comment has been minimized.

Show comment
Hide comment
@keeperofdakeys

keeperofdakeys Jul 3, 2017

Contributor

For flock, I'd remove the LOCK_NB bitmask if you wanted to wait forever.

Contributor

keeperofdakeys commented Jul 3, 2017

For flock, I'd remove the LOCK_NB bitmask if you wanted to wait forever.

@murrant murrant added the Blocker 🚫 label Jul 5, 2017

Switch the schema lock back to a file lock for now.
Make FileLock support indefinite locking without polling.
Add a warning to MysqlLock for scenarios where it won't work.
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 8, 2017

Member

@murrant Ready for another test?

Member

laf commented Jul 8, 2017

@murrant Ready for another test?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 8, 2017

Member

Yeah, passed my tests. With the 30s wait they all run, but not concurrently. I set it to 0 to check that again.

Member

murrant commented Jul 8, 2017

Yeah, passed my tests. With the 30s wait they all run, but not concurrently. I set it to 0 to check that again.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 13, 2017

Member

@f0o Any comments on this?

@murrant Is the MySQL lock actually used here?

Member

laf commented Jul 13, 2017

@f0o Any comments on this?

@murrant Is the MySQL lock actually used here?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 14, 2017

Member

@laf not anymore

Member

murrant commented Jul 14, 2017

@laf not anymore

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 14, 2017

Member

What's the need for LibreNMS/MysqlLock.php then?

Member

laf commented Jul 14, 2017

What's the need for LibreNMS/MysqlLock.php then?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 15, 2017

Member

None, I can dump it but it works fine as long as there is not statement based replication or galera

Member

murrant commented Jul 15, 2017

None, I can dump it but it works fine as long as there is not statement based replication or galera

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 16, 2017

Member

Testing overnight.

Just needs the mysqllock stuff removing from test unit. I can do that tomorrow if you don't get round to it.

Member

laf commented Jul 16, 2017

Testing overnight.

Just needs the mysqllock stuff removing from test unit. I can do that tomorrow if you don't get round to it.

@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 Jul 17, 2017

The inspection completed: 682 Issues, 21 Patches

The inspection completed: 682 Issues, 21 Patches

@laf laf merged commit 181b0fb into librenms:master Jul 17, 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
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 17, 2017

Member

Thanks @laf

Member

murrant commented Jul 17, 2017

Thanks @laf

@murrant murrant deleted the murrant:schema-lock branch Jul 17, 2017

@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.