In my system(centos 7.4), pagure's worker forget to delete the entry in table project_locks. My SQLAlchemy version is 1.2.0b3, and version 1.1 has the same problem.
I got this fixed by the modification below.
# pagure/lib/model.py class ProjectLocker(object): """ This is used as a context manager to lock a project. This is used as a context manager to make it very explicit when we unlock the project, and so that we unlock even if an exception occurs. """ def __exit__(self, *exargs): _log.info('Releasing lock for %d', self.project_id) self.session.delete(self.lock) self.session.commit() self.session.remove() _log.info('Released lock for %d', self.project_id)
Is the code supposed to run correctly with some other versions of SQLAlchemy?
This change looks good to me.
@puiterwijk since you added this code, do you want to chime in?
I'm still using F25 on my laptop for dev and this has python-sqlalchemy-1.0.19-1.fc25.x86_64 and prod has: python-sqlalchemy-0.9.8-2.el7.x86_64 so that could explain the difference.
On the other side I think this change makes sense.
Would you like to open a pull-request for this?
No, please don't do this. The whole intention is for these records to never disappear. They get created when the projects gets created, and the locking happens at the DBMS level: the ProjectLock system grabs a database-level lock on one of the entries, and then later release them. The existence of these entries does not indicate anything, the only thing that's important is the database-level lock held on them.
https://pagure.io/pagure/blob/master/f/pagure/lib/init.py#_1395 is where they are created, together with the project.
The reason the locks work with DBMS-level locking, is that if we were to use the existence of the record as lock indicator, and as such the creation of the record would be the grabbing of the lock, that means that if worker A has the lock, and worker B tries to grab the lock, worker B performs an operation that will fail (since the uniqueness constraint gets broken). Right now, that is a risk, but that should only happen if someone manually deletes the records from the database.
With the DBMS-level locking, worker A and worker B just both ask the database whether they can please lock the record, and if worker A has the lock already, worker B will be made to wait patiently.
Metadata Update from @puiterwijk: - Issue close_status updated to: Invalid - Issue status updated to: Closed (was: Open)
Ok, I get it. The problem I encountered is when I delete a project, I can't create a new project with the same project_id again. Pagure complains I'm trying to insert the same record into project_locks. I don't know why my database won't delete the entry in project_locks properly when I remove a project.
Metadata Update from @crab2313: - Issue status updated to: Open (was: Closed)
This is the schema about project_locks in my database. sqlite> .schema project_locks CREATE TABLE project_locks ( project_id INTEGER NOT NULL, lock_type VARCHAR(6) NOT NULL, CONSTRAINT project_locks_pkey PRIMARY KEY (project_id, lock_type), CONSTRAINT project_locks_project_id_fkey FOREIGN KEY(project_id) REFERENCES projects (id) ON DELETE CASCADE ON UPDATE CASCADE, CONSTRAINT lock_type_enum CHECK (lock_type IN ('WORKER')) ); sqlite>
postgresql has the same problem.
That part is supposed to be fixed by your DBMS: CONSTRAINT project_locks_project_id_fkey FOREIGN KEY(project_id) REFERENCES projects (id) ON DELETE CASCADE ON UPDATE CASCADE,. This most likely just does not work right in sqlite though.
CONSTRAINT project_locks_project_id_fkey FOREIGN KEY(project_id) REFERENCES projects (id) ON DELETE CASCADE ON UPDATE CASCADE,
Yeah, SQLite has its foreign key support disabled by default: https://sqlite.org/foreignkeys.html (look at "Enabling Foreign Key Support"). This means that the cascade constraint isn't used.
@puiterwijk thank you for the explanation.