release-engineering / pubtools-pulplib

A Pulp library for publishing tools
GNU General Public License v3.0
2 stars 24 forks source link

Repo Lock [RHELDST-14968] #198

Closed amcmahon-rh closed 1 year ago

amcmahon-rh commented 1 year ago

Some pub tasks work on mutable pulp data, so if two such tasks are run at the same time one task may overwrite the results from the other. To address this, we can create a "lock" using the repository notes field. The task submits a note and waits for it to become valid, then starts executing the actual task if it's the oldest valid lock in the repository.

First pass attempt at implementing what Rohan suggested. Looking for general comments on the overall approach. There's things like changing print statements to logs and adding tests that still need to be addressed.

I imagined this being used in a with statement. So the maintenance tasks become:

with self.pulp_client.get_repo_lock("redhat-maintenance"):
   report = self.get_maintenance_report().result()
   report = self.adjust_maintenance_report(report)
   self.set_maintenance(report).result()
amcmahon-rh commented 1 year ago

Last Friday I tested this out with a small script that added/ removed records from the QA "redhat-maintenance" repository. This worked as expected and checking repos.json showed the modifications from all the scripts.

Here's a Screenshot of the scripts in progress. I had all the commands written out and started each script within a second of each other. One thing that might stand out is on the bottom right it jumps from 3rd to 1st in the queue. That's because the sleep between querying the notes is based on how many locks there are in the queue. While the lock was in 3rd place, the sleep delay was long enough to skip over a query while it was 2nd place.

amcmahon-rh commented 1 year ago

@rohanpm

I've gone through and addressed your comments :)

I'm not entirely sure how I feel about having the AWAIT_ACTIVATION_SLEEP and LOCK_EXPIRATION_OFFSET values in the repo base file. I played with passing a kwarg dict instead but it ""looked yucky"". Though it's probably also a case of fixating too much on specific implementation details.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (8bca412) 100.00% compared to head (33e0d6b) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #198 +/- ## ========================================== Coverage 100.00% 100.00% ========================================== Files 46 47 +1 Lines 3008 3153 +145 ========================================== + Hits 3008 3153 +145 ``` | [Impacted Files](https://codecov.io/gh/release-engineering/pubtools-pulplib/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=release-engineering) | Coverage Δ | | |---|---|---| | [pubtools/pulplib/\_impl/client/client.py](https://codecov.io/gh/release-engineering/pubtools-pulplib/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=release-engineering#diff-cHVidG9vbHMvcHVscGxpYi9faW1wbC9jbGllbnQvY2xpZW50LnB5) | `100.00% <100.00%> (ø)` | | | [pubtools/pulplib/\_impl/fake/client.py](https://codecov.io/gh/release-engineering/pubtools-pulplib/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=release-engineering#diff-cHVidG9vbHMvcHVscGxpYi9faW1wbC9mYWtlL2NsaWVudC5weQ==) | `100.00% <100.00%> (ø)` | | | [pubtools/pulplib/\_impl/fake/controller.py](https://codecov.io/gh/release-engineering/pubtools-pulplib/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=release-engineering#diff-cHVidG9vbHMvcHVscGxpYi9faW1wbC9mYWtlL2NvbnRyb2xsZXIucHk=) | `100.00% <100.00%> (ø)` | | | [pubtools/pulplib/\_impl/fake/state.py](https://codecov.io/gh/release-engineering/pubtools-pulplib/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=release-engineering#diff-cHVidG9vbHMvcHVscGxpYi9faW1wbC9mYWtlL3N0YXRlLnB5) | `100.00% <100.00%> (ø)` | | | [pubtools/pulplib/\_impl/model/repository/base.py](https://codecov.io/gh/release-engineering/pubtools-pulplib/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=release-engineering#diff-cHVidG9vbHMvcHVscGxpYi9faW1wbC9tb2RlbC9yZXBvc2l0b3J5L2Jhc2UucHk=) | `100.00% <100.00%> (ø)` | | | [...btools/pulplib/\_impl/model/repository/repo\_lock.py](https://codecov.io/gh/release-engineering/pubtools-pulplib/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=release-engineering#diff-cHVidG9vbHMvcHVscGxpYi9faW1wbC9tb2RlbC9yZXBvc2l0b3J5L3JlcG9fbG9jay5weQ==) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=release-engineering). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=release-engineering)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

amcmahon-rh commented 1 year ago

I've gone through and refactored/ extended tests to meet the 100% coverage requirement, as well as addressed the issues causing the static analysis to fail.

The bandit test is failing with request_without_timeout for every detected issue. We discussed this earlier in the week since it was affecting rcm-pub, and there's already a bug lodged with bandit for false positives. https://github.com/PyCQA/bandit/issues/996

rohanpm commented 1 year ago

The bandit test is failing with request_without_timeout for every detected issue. We discussed this earlier in the week since it was affecting rcm-pub, and there's already a bug lodged with bandit for false positives. PyCQA/bandit#996

There is also RHELDST-16791 which says that we should be pinning the bandit version here, like we do with other packages used in CI, so the CI will not flip from passing to failing when nobody made changes breaking anything.