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

Add advisory_lock utility from AWX #661

Open
wants to merge 9 commits into
base: devel
Choose a base branch
from

Conversation

AlanCoding
Copy link
Member

@AlanCoding AlanCoding commented Dec 2, 2024

This migrates advisory_lock from AWX to DAB. This is similar in spirit to #660

Importantly, I went the route of not adding a new dependency, because there was only a single method from the library we were using, and I'm pretty confident that the MIT license lets you do whatever you want. In doing this, I deleted the use of six for python2 compatibility which further reduced the complexity of the code I ported.

Additionally requested reviewers @Alex-Izquierdo @TheRealHaoLiu

@AlanCoding AlanCoding changed the title Run linters and add docs Add advisory_lock utility from AWX Dec 2, 2024
@AlanCoding AlanCoding marked this pull request as ready for review December 2, 2024 19:37
# uses miliseconds units
with advisory_lock('test_lock_session_timeout_milliseconds', lock_session_timeout_milliseconds=2):
time.sleep(3)
assert 'the connection is lost' in str(exc)
Copy link
Member Author

Choose a reason for hiding this comment

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

@TheRealHaoLiu I was really uncomfortable with this lock_session_timeout_milliseconds when it went into AWX. Historically, we have accumulated untestable code, and then gotten issues reported due to some esoteric circumstance, and gotten into a pickle about how or if we will address it.

Here, this is offering true functional testing. It sets the timeout for the current session and actually watches it time out.

I would like to convert AWX to run checks with postgres, but now is not the time for that. For this overused / abused utility in AWX, holding it to the higher code quality standard in DAB is a good idea, particularly since I'm basically saying we should reuse it in eda-server.

Copy link
Member

@john-westcott-iv john-westcott-iv left a comment

Choose a reason for hiding this comment

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

It might be nice to have vendor specific files in ansible_base/lib/utils/db i.e. something for postgres/mssql/sqlite and then the db.py would have "Generic methods" which would check the vendor and then call items from the DB specific files.



@contextmanager
def advisory_lock(*args, lock_session_timeout_milliseconds=0, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some comments here to explain the logic?

Copy link

sonarcloud bot commented Dec 3, 2024

@AlanCoding
Copy link
Member Author

It might be nice to have vendor specific files in ansible_base/lib/utils/db i.e. something for postgres/mssql/sqlite

Maybe sometime, I just don't feel like we're really there yet. AWX has probably the most postgres-specific code, and it's just lying around scatter-shot.

It might be nice to attempt sqlite solution, probably like this:

https://stackoverflow.com/questions/6931342/system-wide-mutex-in-python-on-linux

class Locker:
    def __enter__ (self):
        self.fp = open("./lockfile.lck")
        fcntl.flock(self.fp.fileno(), fcntl.LOCK_EX)

    def __exit__ (self, _type, value, tb):
        fcntl.flock(self.fp.fileno(), fcntl.LOCK_UN)
        self.fp.close()

Then that would share a lot of feature and not share a lot of features. So then you have a lot of param handling logic to error if it's 1 vendor and not another. Definitely going in the opposite direction of django-pglocks where this comes from. And I do think that would be good for when we introduce a sqlite version, but that just seems too arbitrary and premature until we're at that point.

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.

3 participants