Fix rsync distributor skipping units #3039
Conversation
|
@dralley, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dkliban, @ipanova and @midnightercz to be potential reviewers. |
| @@ -258,6 +258,36 @@ def process_main(self): | |||
| raise PulpCodedException(message=output) | |||
|
|
|||
|
|
|||
| class UpdateLastPredistDateStep(PublishStep): | |||
There was a problem hiding this comment.
so i was thinking whether it is the correct place for the Step. Since this step is used just by rpm plugin, i thought maybe it would be better to keep in plugins/pulp_rpm/plugins/distributors/?
i looked into docker plugin and i saw some docker specific steps located and subclassed in plugins/pulp_docker/plugins/distributors/publish_steps.py
What do you think of this? @dralley
There was a problem hiding this comment.
So, it's currently only used in the RPM plugin, but should I go add this to docker as well? I haven't tested to see whether the docker rsync distributor is vulnerable to the same unit skipping issue.
There was a problem hiding this comment.
docker does not have the concept of a predistributor, it has postdistributor. That said the docker has different logic that should not be impacted by this issue.
There was a problem hiding this comment.
So is it likely that the only plugin that will need this code is the RPM plugin?
There was a problem hiding this comment.
I do think it should be left where it is. Considering that it's not used just by the rsync distributor but also the ISO rsync distributor, there's not an obvious place to put it at a lower level.
server/pulp/plugins/rsync/publish.py
Outdated
| """ | ||
| Save last_predist_last_published. | ||
| """ | ||
| if not self.distributor["scratchpad"]: |
There was a problem hiding this comment.
you will get the keyerror here case of scratchpad missing
server/pulp/plugins/rsync/publish.py
Outdated
|
|
||
| self.distributor["scratchpad"]["last_predist_last_published"] = self.date | ||
|
|
||
| try: |
There was a problem hiding this comment.
would not it be better to use already existing set_scratchpad()
Corrects the rsync distributor fast-forward logic so that it uses the proper date range to determine which units to include. This fixes an issue where units were not being published if the association, predistributor publish and rsync publish occurred in a certain order. closes #2532 https://pulp.plan.io/issues/2532
| not last_deleted) and not force_full and\ | ||
| not delete | ||
| return not force_full and not delete and not self.last_predist_last_published and \ | ||
| ((last_deleted and last_published and last_published > last_deleted) or not last_deleted) |
There was a problem hiding this comment.
last_published is twice in here
Corrects the rsync distributor fast-forward logic so that it
uses the proper date range to determine which units to include.
This fixes an issue where units were not being published if the
association, predistributor publish and rsync publish occurred
in a certain order.
closes #2532
https://pulp.plan.io/issues/2532