Bug 1879126

Summary: no-op repo sync leads to real-op repo publish in certain situations
Product: Red Hat Satellite Reporter: Pavel Moravec <pmoravec>
Component: PulpAssignee: satellite6-bugs <satellite6-bugs>
Status: CLOSED ERRATA QA Contact: Lai <ltran>
Severity: high Docs Contact:
Priority: medium    
Version: 6.7.0CC: bmbouter, dkliban, egolov, ggainey, ipanova, jjansky, ktordeur, mmccune, rchan, ttereshc, wpinheir
Target Milestone: 6.9.0Keywords: Triaged
Target Release: Unused   
Hardware: x86_64   
OS: Linux   
Whiteboard:
Fixed In Version: pulp-2.21.4 Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
: 1899310 (view as bug list) Environment:
Last Closed: 2021-04-21 13:17:46 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
pulp-server-2.21.0.4-2.HOTFIXRHBZ1879126.el7sat.noarch.rpm none

Description Pavel Moravec 2020-09-15 13:47:05 UTC 
Description of problem:
There are few scenarios leading to a redundant "real-op" publish(*) of a repo in pulp. 

(recall: each repo sync automatically triggers publish of the repo; if nothing has changed, the publish is no-op - BUT the decision "nothing has changed" wrongly detects "a change" even if the sync didnt update the repo at all)

Particular reproducer:
- republish repository metadata of a repo
  - this will forcefully publish the repo
- then invoke optimised repo sync
  - this wont sync anything new, BUT the repo will be re-published, redundantly

(alternate reproducer: do a full repo sync, followed by optimised/normal repo sync - again, the 2nd (no-op) sync triggers "real-op" publish)

The cause is here: https://github.com/pulp/pulp/blob/2-master/server/pulp/server/controllers/repository.py#L1201

Here, we compare previous and current publish override options. So if a new repo sync does not change anything but it was called with different (even empty) override config, the repo *will be re-published - since same_override=False and https://github.com/pulp/pulp/blob/2-master/server/pulp/server/controllers/repository.py#L1215 evals to False .


As a consequence, redundant repo is re-published (which can also trigger redundant sync of the repo to Capsule, after all).


Version-Release number of selected component (if applicable):
Sat 6.7
pulp-server-2.21.0.1-1.el7sat.noarch


How reproducible:
100%


Steps to Reproduce:
1. Have a fully synced repo in Sat
2. Do either action:
  - invoke Complete Sync
  - or Republish Repository metadata
3. Check the repo was re-published
4. Invoke a regular sync
5. Check the repo was republished

Standalone pulp minimalistic reproducer: call repo publish once with override_config\":{\"force_full\":true}, then without it (or with force_full:false) - even the 2nd publish will be a "real op".


Actual results:
5. Repo was republished


Expected results:
5. Repo isnt republished


Additional info:
I understand there can be legit situations where diff between previous and current override_config *must* result in re-publish. I just dont know them and the current logic is simply too strict..
Comment 1 Pavel Moravec 2020-09-17 14:44:25 UTC 
Some minimalistic patch to ignore _changes_ in force_full override config:


--- a/pulp/server/controllers/repository.py
+++ b/pulp/server/controllers/repository.py
@@ -1160,13 +1160,17 @@ def check_publish(repo_obj, dist_id, dis
         skip_for_predistributor = published_after_predistributor or not \
             predistributor_last_published
 
-    same_override = dist.last_override_config == config_override
-    if not same_override:
+    if not (dist.last_override_config == config_override):
         # Use raw pymongo not to fire the signal hander
         model.Distributor.objects(
             repo_id=repo_obj.repo_id,
             distributor_id=dist_id).update(set__last_override_config=config_override)
 
+    # do not compare force_full from config override - this particular difference itself does not
+    # determine the need of repo publish
+    config_override = {k: v for k, v in config_override.items() if k != 'force_full'}
+    dist.last_override_config = {k: v for k, v in dist.last_override_config.items() if k != 'force_full'}
+    same_override = dist.last_override_config == config_override
     # Check if content has not changed since last publish and a predistributor is not defined.
     unchanged_content_and_no_predistributor = last_published and not last_updated and \
         not units_removed and not predistributor_id


(a review from pulp devels is welcomed, though..)
Comment 2 Tanya Tereshchenko 2020-09-18 06:43:21 UTC 
Pavel, thanks for the patch. It looks reasonable to me.
Feel free to open the upstream PR. I'll file the upstream bug.
Comment 3 pulp-infra@redhat.com 2020-09-18 07:05:41 UTC 
The Pulp upstream bug status is at NEW. Updating the external tracker on this bug.
Comment 4 pulp-infra@redhat.com 2020-09-18 07:05:42 UTC 
The Pulp upstream bug priority is at Normal. Updating the external tracker on this bug.
Comment 5 Pavel Moravec 2020-09-29 20:16:26 UTC 
(In reply to Tanya Tereshchenko from comment #2)
> Pavel, thanks for the patch. It looks reasonable to me.
> Feel free to open the upstream PR. I'll file the upstream bug.

Here you are :)

https://github.com/pulp/pulp/pull/3999


(majority of redundant repo publishes were in my case really due to config_override altering from {} to {force_full=False})
Comment 6 pulp-infra@redhat.com 2020-10-22 19:06:18 UTC 
The Pulp upstream bug status is at POST. Updating the external tracker on this bug.
Comment 7 pulp-infra@redhat.com 2020-10-26 13:06:45 UTC 
The Pulp upstream bug status is at MODIFIED. Updating the external tracker on this bug.
Comment 8 pulp-infra@redhat.com 2020-10-26 14:07:41 UTC 
All upstream Pulp bugs are at MODIFIED+. Moving this bug to POST.
Comment 9 pulp-infra@redhat.com 2020-11-02 16:08:24 UTC 
The Pulp upstream bug status is at CLOSED - CURRENTRELEASE. Updating the external tracker on this bug.
Comment 10 Mike McCune 2020-11-04 22:19:28 UTC 
*** Satellite 6.7.4 and 6.7.5 Hotfix Available ***

1) Download pulp-server-2.21.0.4-2.HOTFIXRHBZ1879126.el7sat.noarch.rpm from this bugzilla to your Satellite

2) stop services:

satellite-maintain service stop

3) Install:

rpm -Uvh pulp-server-2.21.0.4-2.HOTFIXRHBZ1879126.el7sat.noarch.rpm

4) restart:

satellite-maintain service start

5) resume operations
Comment 11 Mike McCune 2020-11-04 22:20:26 UTC 
Created attachment 1726745 [details]
pulp-server-2.21.0.4-2.HOTFIXRHBZ1879126.el7sat.noarch.rpm
Comment 14 Lai 2021-01-05 17:02:28 UTC 
Steps to retest

1. Upload manifest
2. Enabled rhel7 repo (content -> repositories)
3. Sync repo
4. Once repo is sync, resync it
5. Content -> product -> rhel7 product -> rhel7 repo -> select action -> advanced sync.  choose Complete sync and sync
6. Once sync is completed, do a regular sync

Expected:
3. Should take a long time
4. Should take seconds
5. Should take a long time
6. Should take seconds

Actual
3. Regular sync took 46 minutes
4. Took 26 seconds
5. Complete sync took 22 minutes
6. Took 26 seconds

Originally, after the complete sync took place and resyncing happens, it took a long time.  After the fix, the syncing took seconds to complete which is what step 6 indicates.

Verified in 6.9.0_07
Comment 17 errata-xmlrpc 2021-04-21 13:17:46 UTC 
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (Moderate: Satellite 6.9 Release), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2021:1313