CP-49918: Moved pool_update.apply from scripts/extensions to python3/extensions directory

[ashwin9390]

Main ticket CP-49100 :Move python3 scripts from scripts directory to python3 directory.

Sub task CP-49918:

• Modified python3/Makefile to include pool_update.apply
• Removed pool_update.apply from scripts/Makefile
• Fixed import order

Signed-off-by: Ashwinh ashwin.h@cloud.com

[bernhardkaindl]

Suggested change

Don't add yet another (3rd) empty line: Two empty line are more than enough here.

[bernhardkaindl]

Sorry for the huge amount of words, the easiest summary is: Revert this change in the PR.

Link to the code in the file view: https://github.com/xapi-project/xen-api/pull/5779/files#diff-81fb66aea8de2a10f60ee92de3aeeea4d180b1bb131d70f8cf86c38cab38fe52R153

Before this PR, the code was:

        if lock_acquired:
            lock.release()
            if os.path.isfile(lock_file):
                os.remove(lock_file)

This is correct - the lock file must only be removed when the lock was acquired.

This PR changes the context to always remove the lock file, even when the lock was not acquired:

        if lock_acquired:
            lock.release()
        # BUG: The indentation of the next 4 lines needs to be moved 4 spaces right:
        try:
            os.remove(lock_file)
        except Exception as e:
            pass

This calls os.remove() always, because the try: is no longer within "if lock_acquired:". This defeats file-based locking, see my later review comment in the conversation of the PR.

The try around os.remove() is a good idea, and it can be written shorter (with import contextlib moved to the top of the file of course):

Suggested change

	try:
	os.remove(lock_file)
	except Exception as e:
	pass
	import contextlib
	with contextlib.suppress(OSError):
	os.remove(lock_file)

[bernhardkaindl]

My 3rd review comment is about a location in this PR that would introduce a regression on locking:

The change in this PR deletes the lock_file even when the lock was not acquired. This means that if one process holds the lock, a 2nd tries to get it, fails to acquire the lock, but with this change (the one which changed the indentation level on which os.remove(lock_file) is called), the lock_file is removed even in this case.

That means, on the next attempt to acquire the lock, this next attempt will always get into the critical section of the lock (even if the 1st process is still in it), because with the removal of the locked file, the lock itself is no longer accessible and that defeats the locking implementation.

For demonstration, here is a working PoC that shows the issue introduced by this change currently:

import fasteners, os, sys, time

lock = fasteners.InterProcessLock("lock")
lock_acquired = lock.acquire(blocking=False)
if lock_acquired:
    print("sleep 1")
    time.sleep(3)
    print("Process 1 is still in the critical phase")
    lock.release()
    # The lock file must only be deleted when the lock was actually acquired (before this PR)
    # os.remove("lock")  # This is the previous, correct context for deleting the lock file
    sys.exit()

time.sleep(1)
# This is the new context of deleting the lockfile (always, which is incorrect)
os.remove("lock")  # BUG: DO NOT DELETE THE LOCKFILE IF YOU DID NOT ACQUIRE THE LOCK!

lock = fasteners.InterProcessLock("lock")
lock_acquired = lock.acquire(blocking=False)
print("Lock acquired after deleting the lock file:", lock_acquired)
os.remove("lock")

Reproducing the locking issue:

python locking.py & python locking.py 
[1] 378970
sleep 1
Lock acquired after deleting the lock file: True
Process 1 is still in the critical phase

Testing shows: Keeping os.remove("lock") inside if lock_acquired: (and not always), locking works again, but not when the lockfile is always removed.

[bernhardkaindl]

Checked: The updated PR now applies purely the formatting changes by isort and black and it contains no change that would modify the "compiled" code.

Before we can merge it, we've to fix CI again. It looks like we need to do another merge from master into feature/py3.