Sync with metadata mirroring #502

dralley · 2021-05-01T00:07:56Z

[noissue]

pulp_file/app/tasks/synchronizing.py

pulpbot · 2021-05-01T12:26:21Z

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

dralley · 2021-05-03T14:07:56Z

I guess the question is, do we want to merge this as-is (with some tests), and then fix it later, or wait until we can add support to DeclarativeVersion, which might take a few cycles especially if the changes are invasive.

I kind of think we need to do it now, especially since the RPM plugin has no choice.

mdellweg · 2021-05-03T16:34:00Z

pulp_file/app/tasks/synchronizing.py

@@ -42,7 +47,24 @@ def synchronize(remote_pk, repository_pk, mirror):

    first_stage = FileFirstStage(remote)
    dv = DeclarativeVersion(first_stage, repository, mirror=mirror)
-    return dv.create()
+    rv = dv.create()
+    if mirror:


Wasn't the mirror variable meant to be a different thing? i.e. distiguishing additive sync from the other one?

mirror signals that we want an exact copy of the content in the remote repository - and therefore implies that any content that isn't in the remote repository should be removed.

This fits into that same conceptual model. The user gets the same sync behavior they wanted, and also an automatic publication with the exact metadata that was downloaded - an exact copy of the remote repository. They're free to ignore it though, and it wouldn't prevent any future publishes if that is how their workflow is set up.

This is not especially useful for the file plugin, since publishing is so fast now, and since the metadata is trivial to reproduce exactly. For the RPM and perhaps Debian plugins though, it is impossible for Pulp to reproduce the metadata exactly, due to timestamps and signatures.

mdellweg · 2021-05-04T09:15:11Z

pulp_file/app/tasks/synchronizing.py

+        # accomodate this use case
+        global metadata_files
+        with FilePublication.create(rv, pass_through=True) as publication:
+            (mdfile_path, relative_path) = metadata_files.pop()


This looks strange to me: metadata_files is a list, and we only pop a single entry from it. Why don't we just put that tuple in the variable? Or if we want to make it more portable we should loop over that list.

I was just trying to keep it consistent with the RPM plugin version I'm working on - there's no reason we couldn't do that. You're right about looping over the list though.

Since then I've made changes that mess up the "consistency", anyway.

ipanova · 2021-05-05T15:30:38Z

I guess the question is, do we want to merge this as-is (with some tests), and then fix it later, or wait until we can add support to DeclarativeVersion, which might take a few cycles especially if the changes are invasive.

I kind of think we need to do it now, especially since the RPM plugin has no choice.

The main driver of this feature is RPM plugin which also has tight deadlines with regards to the delivery dates.
Based on this ticket there is not seem to be an easy solution to adapt these changes an incorporate into DV https://pulp.plan.io/issues/8687.

My suggestions would be to take the approach we have as of today and work towards a better one later.

dralley · 2021-05-27T20:19:11Z

The RPM version of this PR has now merged

mdellweg · 2021-05-31T04:36:35Z

pulp_file/tests/functional/api/test_sync.py

+
+    def test_01_sync(self):
+        """Assert that syncing the repository w/ mirror=True creates a publication."""
+        self.assertEqual(self.publications_api.list().count, 0)


This assumption prevents to run this test in a box you are doing other experiments.
We should instead look if the number increased by one.

Are we not making that assumption in a great many places? Any test that cleans up orphan content afterwards for example.

I don't think Pytest does any parallelization (edit: by default)

publications are not cleaned by orphan cleanup. So that doesn't apply here.
This would really require a pclean.

The problem is that the list of created resources isn't separated by type (like e.g. the repository version summary), so trying to verify this from the created resources is kind of hacky (but doable if it's strongly desired).

Let me know if this looks better.

Sorry, i should have explained better: I was thinking about assigning the number of publications to a variable without asserting before and assert on length==N+1 after the autopublish.
But the way it is now looks even better.
BTW, I have seen codepaths to rely on the order of created_resources. As in version = c_r[0]; publication = c_r[1].

When synced with "mirror=True", we take the manifest we downloaded and immediately create a publication with it, rather than regenerating the whole manifest. closes: #8851 https://pulp.plan.io/issues/8851

dralley · 2021-06-02T14:07:19Z

I've created an issue for this feature

dralley force-pushed the metadata_mirroring branch 2 times, most recently from b3149be to 7a85a26 Compare May 1, 2021 00:09

dralley commented May 1, 2021

View reviewed changes

pulp_file/app/tasks/synchronizing.py Show resolved Hide resolved

mdellweg reviewed May 3, 2021

View reviewed changes

dralley changed the title ~~[PoC] Sync with metadata mirroring~~ Sync with metadata mirroring May 3, 2021

dralley marked this pull request as ready for review May 3, 2021 19:20

dralley force-pushed the metadata_mirroring branch from 7a85a26 to 098e5f6 Compare May 4, 2021 03:50

mdellweg reviewed May 4, 2021

View reviewed changes

dralley force-pushed the metadata_mirroring branch from 098e5f6 to 4641bc0 Compare May 21, 2021 02:01

mdellweg reviewed May 31, 2021

View reviewed changes

dralley force-pushed the metadata_mirroring branch 3 times, most recently from ad13158 to d592450 Compare June 1, 2021 20:59

mdellweg approved these changes Jun 2, 2021

View reviewed changes

Sync with metadata mirroring

1c75aad

When synced with "mirror=True", we take the manifest we downloaded and immediately create a publication with it, rather than regenerating the whole manifest. closes: #8851 https://pulp.plan.io/issues/8851

dralley force-pushed the metadata_mirroring branch from d592450 to 1c75aad Compare June 2, 2021 14:06

dralley merged commit 8723071 into pulp:master Jun 2, 2021

dralley deleted the metadata_mirroring branch June 2, 2021 14:33

pulpbot mentioned this pull request Jan 17, 2022

DeclarativeVersion API doesn't accomodate metadata mirroring use cases pulp/pulpcore#1989

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync with metadata mirroring #502

Sync with metadata mirroring #502

dralley commented May 1, 2021

pulpbot commented May 1, 2021

dralley commented May 3, 2021 •

edited

Loading

mdellweg May 3, 2021

dralley May 3, 2021 •

edited

Loading

mdellweg May 4, 2021

dralley May 4, 2021

dralley May 4, 2021

ipanova commented May 5, 2021

dralley commented May 27, 2021

mdellweg May 31, 2021

dralley Jun 1, 2021 •

edited

Loading

mdellweg Jun 1, 2021

dralley Jun 1, 2021

dralley Jun 1, 2021

mdellweg Jun 2, 2021

dralley commented Jun 2, 2021

Sync with metadata mirroring #502

Sync with metadata mirroring #502

Conversation

dralley commented May 1, 2021

pulpbot commented May 1, 2021

dralley commented May 3, 2021 • edited Loading

Choose a reason for hiding this comment

dralley May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipanova commented May 5, 2021

dralley commented May 27, 2021

Choose a reason for hiding this comment

dralley Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley commented Jun 2, 2021

dralley commented May 3, 2021 •

edited

Loading

dralley May 3, 2021 •

edited

Loading

dralley Jun 1, 2021 •

edited

Loading