Issue #2992: remove select from add_rpm_sign() - koji

koji

#2992 remove select from add_rpm_sign()

Closed: Fixed 3 years ago by tkopecek. Opened 3 years ago by ktdreyer.

Reading the code change in PR #2909, I think we can optimize this code now.

Prior to PR #2909, the hub used to "SELECT" the rpmsigs table for existing signatures, and if it found no rows, it would "INSERT" the signatures.
PR #2909 added a catch statement for IntegrityError to return a friendlier error message, but it did not remove the SELECT sighash FROM rpmsigs ... query.

The "look before you leap" pattern is still racy and slow. Can we remove it and simply rely on catching the SQL error all the time?

The thing that makes me wonder about this is that there's an old comment in the code here:

# TODO[?] - if sighash is the same, handle more gracefully

I think we can drop this whole bit of code.

mikem commented 3 years ago

TODO[?] - if sighash is the same, handle more gracefully

I think I meant to make the call idempotent. I.e. if given the exact same signature that we already have, then avoid the error and just make it a no-op.

However, not sure how valuable this is in practice, and it's a little unclear what to do about the callbacks in that case (I don't know if we had the callbacks when I wrote that comment).

Edited 3 years ago by mikem

Metadata Update from @mikem:
- Custom field Size adjusted to None

3 years ago

mikem commented 3 years ago

Sounds reasonable to me

Metadata Update from @tkopecek:
- Issue set to the milestone: 1.27

3 years ago

ktdreyer commented 3 years ago

@jcupova mentioned to me that this would change the callback behavior slightly.

With the current hub code:

When no other clients call this addRPMSig method (there is no race), then the hub does not run the preRPMSign callback if the signature already exists.
When another client calls this addRPMSig method (and races), then the hub will fall through to the IntegrityError case. It will run the preRPMSign callback.

When we remove the SELECT query, then Koji's behavior will change to:

The hub will always run the preRPMSign callback, regardless of whether the signature exists in the DB or not.

IMO this is fine because it makes more sense to simplify the behavior and eliminate the race condition here. Moreover, we already have a separate postRPMSign callback that would not change, and that callback is arguably important anyway. The hub protonmsg plugin ignores preRPMSign and only fires on postRPMSign.

To summarize the definition of each callback:

preRPMSign - runs when a client asks to import a new signature for a build (regardless of whether that signature exists or not)
postRPMSign - runs when Koji successfully imports a new signature for a build that it did not have before.

Edited 3 years ago by ktdreyer