-
Notifications
You must be signed in to change notification settings - Fork 79
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
#682: Sign the metadata for all releases in a repo concurrently, greatly speeding up the publish task in environments where signing is slow. #683
Conversation
d1bb687
to
1c83cac
Compare
I don't think this failure of the lint job is related to this code change. |
Yea, something weird happened there I restarted the jobs and it looks like it's fine. |
@hstct ping, I believe this is ready for merging. We have been using this patch for the last two months and have not had any issues. |
…eeding up the publish task in environments where signing is slow. closes pulp#682
1c83cac
to
ffea67c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM.
Creating signed publications already has test coverage.
I also performed the following local test steps:
# Example Manual signing service test workflow for Pulp oci-env:
## Import the gpg keys from pulp-fixtures:
oci-env shell
su pulp
curl -L https://github.com/pulp/pulp-fixtures/raw/master/common/GPG-PRIVATE-KEY-pulp-qe | gpg --import
echo "6EDF301256480B9B801EBA3D05A5E6DA269D9D98:6:" | gpg --import-ownertrust
## Create the signing service (from outside of oci-env shell):
SIGNING_SERVICE_NAME='Pulp_QE'
oci-env pulpcore-manager add-signing-service --class 'deb:AptReleaseSigningService' \
"${SIGNING_SERVICE_NAME}" \
/src/pulp_deb/pulp_deb/tests/functional/sign_deb_release.sh \
'6EDF301256480B9B801EBA3D05A5E6DA269D9D98'
## Syncing and Publishing:
SIGNING_SERVICE_NAME='Pulp_QE'
ENTITIES_NAME='test'
REMOTE_OPTIONS='--url=https://fixtures.pulpproject.org/debian/ --distribution=ragnarok --component=asgard --policy=on_demand'
oci-env pulp deb remote create --name=${ENTITIES_NAME} ${REMOTE_OPTIONS}
oci-env pulp deb repository create --name=${ENTITIES_NAME} --remote=${ENTITIES_NAME}
oci-env pulp deb repository sync --name=${ENTITIES_NAME}
oci-env pulp deb publication create --simple --structured --repository=${ENTITIES_NAME} --signing-service=${SIGNING_SERVICE_NAME}
APT_PUBLICATION_HREF=<copy pulp_href from previous command>
oci-env pulp deb distribution create --name=${ENTITIES_NAME} --base-path=${ENTITIES_NAME} --publication=${APT_PUBLICATION_HREF}
@hstct Do you agree? Do you want to hit merge? |
Thanks! I was playing around with the new test infrastructure to see if I needed to add more, but I agree that there's really no "functional change" and the basic functionality was already covered by tests. I'll take a look at my other PRs. |
This PR breaks
_ReleaseHelper.finish()
up into three parts,save_unsigned_metadata
,sign_metadata
, andsave_signed_metdata
, and then callssign_metadata
concurrently for all releases at once.There are potentially many other ways to accomplish the same thing, but this seemed the simplest to me. We don't have to worry about thread management or locking. The database driver doesn't have to be async compatible. An the overall semantics of the Task remain the same, it will still block the worker until it's done, it just happens much faster in environments with slow signing.
Even in
asyncio
there are other ways this could be accomplished. We could for example useasyncio.Tasks
andasyncio.wait
. The benefit ofasyncio.gather
is simplicity, and it also offers by default behavior of halting immediately upon an Exception and passing it up the synchronous stack, which is behavior you have to program in explicitly if you're usingTasks
/wait
.