Add a package retention policy feature #1752

dralley · 2020-06-18T02:31:42Z

closes: #5367
https://pulp.plan.io/issues/5367

pep8speaks · 2020-06-18T02:31:46Z

Hello @dralley! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-02 17:45:55 UTC

pulpbot · 2020-06-18T02:32:30Z

Attached issue: https://pulp.plan.io/issues/5950

dralley · 2020-06-18T02:32:52Z

Need to sort out how to get the extension installed on user systems.

dralley · 2020-06-18T02:37:30Z

pulp_rpm/app/models/package.py

@@ -219,6 +223,9 @@ class Meta:
            'name', 'epoch', 'version', 'release', 'arch', 'checksum_type', 'pkgId'
        )

+    class ReadonlyMeta:
+        readonly = ["evr"]


This is necessary because otherwise Django will attempt to insert an empty string into the field by default.

pulp_rpm/app/migrations/0012_rpm_evr_extension.py

dralley · 2020-06-24T23:03:41Z

@goosemania @mdellweg This is feature-complete, I just need to write a functional test (already tested manually) and decide if we want to go forwards with dumping the evr_t extension into the DB separate from Katello or accomplish that in a different way.

Feel free to drop comments.

dralley · 2020-06-24T23:23:46Z

pulp_rpm/app/serializers.py

@@ -306,16 +306,25 @@ class RpmRepositorySerializer(RepositorySerializer):
    """

    metadata_signing_service = serializers.HyperlinkedRelatedField(
-        help_text="A reference to an associated signing service.",
+        help_text=_("A reference to an associated signing service."),


A few of these were missing some i11n

dralley · 2020-06-24T23:24:31Z

pulp_rpm/app/serializers.py

        view_name='signing-services-detail',
        queryset=AsciiArmoredDetachedSigningService.objects.all(),
        many=False,
        required=False,
        allow_null=True
    )
+    package_retention_policy = serializers.IntegerField(


Completely open to renaming this

Yeah, I'm +1 to rename but I don't have good suggestions :D

For the sake of brainstorming ideas:

latest_package_count
package_recency
recent_package_count
keep_recent_packages
retain_package_count

+1 to keep_recent_packages or (Ina's suggestion) retain_package_versions

@ipanova @ggainey @dkliban @goosemania @daviddavis Any strong preferences between those two (or a different one)?

How about retained_package_versions? keep_ and retain_ are verbs, but what you're really specifying is a noun, "how many package-versions should I keep in this repo". Does that make sense?

I think it could be considered either a noun or a verb from the user perspective. It's a fine suggestion, though. I don't really want to bike shed this forever, maybe we should just set up a poll and whichever one wins is the one we go with.

Good idea - maybe a quick 24-hr poll, with my, ipanova's, and your suggestions? Note that I am not strongly tied to my suggestion - any of those three would be fine I think, if you picked one and just went with it I'd be fine.

goosemania

Great job! 🐱

goosemania · 2020-06-25T10:54:11Z

pulp_rpm/app/migrations/0012_RAW_rpm_evr_extension.py

+CREATE TRIGGER evr_update_trigger
+  BEFORE UPDATE OF epoch, version, release
+  ON rpm_package
+  FOR EACH ROW
+  WHEN (
+    OLD.epoch IS DISTINCT FROM NEW.epoch OR
+    OLD.version IS DISTINCT FROM NEW.version OR
+    OLD.release IS DISTINCT FROM NEW.release
+  )
+  EXECUTE PROCEDURE evr_trigger();


Will it be ever used?
I'm +1 to keep just because it's a safe thing to do. I'm just wondering if there is any workflow or use case for it.

I don't think so.

goosemania · 2020-06-25T11:01:15Z

pulp_rpm/app/models/package.py

 from django.db import models
+from django.db.models import Window, F


Nit-pick. Would it make sense to do one way or the other and not both?

from django.db import models from django.db.models import ...

Leaving it this way is my personal preference, since we've got 3 levels of imports and not just 2.

goosemania · 2020-06-25T11:09:01Z

pulp_rpm/app/models/package.py

+        return self.annotate(
+            age=Window(
+                expression=RowNumber(),
+                partition_by=[F('name'), F('arch')],
+                order_by=F('evr').desc()
+            )
+        )


Sweeeet 🥮

goosemania · 2020-06-25T11:25:44Z

pulp_rpm/app/models/repository.py

+                if package.age > self.package_retention_policy:
+                    old_packages.append(package.pk)
+
+            new_version.remove_content(Content.objects.filter(pk__in=old_packages))


I meant to ask if it's ok to remove the ones which are not a part of any repo version apart from this incomplete one. And it seem like yes, because remove_content does this https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/repository.py#L602.

Not related to this PR but related to the topic of removal.
I guess this improvement ^^ to the removal was added later than the advisory changes. I added clarifications as you asked but now it seems like the code can be simplified, please check #1757

goosemania · 2020-06-25T11:46:18Z

pulp_rpm/app/serializers.py

+        help_text=_("The number of verisons of each package to keep in the repository; "
+                    "older versions will be purged. A value of '0' will disable this"
+                    "feature and keep all versions of the package."),
+        min_value=0,


Alternatively, it can be PositiveIntegerField.

I think for the serializer there is no such thing as PositiveIntegerField

https://www.django-rest-framework.org/api-guide/fields/#integerfield

goosemania · 2020-06-25T12:04:21Z

pulp_rpm/app/models/package.py

+        Partitioning by architecture is important because there i686 and x86_64 packages
+        with same name and same or close verison numbers are not interchangeable.


It just crossed my mind that in Pulp3, SRPMs are also stored as Package, the only difference is in arch.
Since you are already taking care of it, maybe it make sense to throw in a comment about SRPMs specifically, it might not be obvious that they are covered.

goosemania · 2020-06-25T12:09:07Z

pulp_rpm/app/serializers.py

        view_name='signing-services-detail',
        queryset=AsciiArmoredDetachedSigningService.objects.all(),
        many=False,
        required=False,
        allow_null=True
    )
+    package_retention_policy = serializers.IntegerField(


Yeah, I'm +1 to rename but I don't have good suggestions :D

For the sake of brainstorming ideas:

latest_package_count
package_recency
recent_package_count
keep_recent_packages
retain_package_count

closes: #5402 https://pulp.plan.io/issues/5402 [nocoverage]

dralley · 2020-07-01T03:12:25Z

pulp_rpm/tests/functional/api/test_retention_policy.py

+        cls.remote_api = RemotesRpmApi(cls.client)
+        delete_orphans(cls.cfg)
+
+    def test_sync_with_retention(self):


@goosemania This functional test only tests the retention policy feature with sync, do you think that is sufficient for now?

Works for me. I saw you've already put a note in the coverage doc, thanks!

dralley · 2020-07-01T03:33:15Z

pulp_rpm/tests/functional/api/test_retention_policy.py

+            {'duck': ['0.7', '0.6'], 'kangaroo': ['0.2'], 'walrus': ['0.71']},
+            versions_for_packages
+        )
+        # TODO: Test that modular RPMs unaffected?


@goosemania Do you think this needs a test up-front or just document it for later?

I'm fine with having it later.

goosemania · 2020-07-01T11:14:14Z

coverage.md

@@ -41,3 +41,6 @@ Manual Coverage
 | As a user, when a module is removed, its packages are removed as well ( not referenced by other modules) | NO |  |
 | **Consumer cases** |  |  |
 | As a user, I can use dnf to install all the content served by Pulp | PART | only covers rpm installation |
+| **Retention** |  |  |
+| As a user, I can have a repository option that retains the latest N packages of the same name | PART | No coverage of packages with differing arch in same repo (src, i686, x86_64), no coverage of non-sync repo modifications. |


dralley · 2020-07-01T14:40:59Z

pulp_rpm/tests/functional/api/test_retention_policy.py

+)
+
+
+class RetentionPolicyTestCase(PulpTestCase):


I can rename this too but the name still seems kind of fitting when used as a noun.

dralley · 2020-07-01T14:42:42Z

CHANGES/5367.feature

@@ -0,0 +1 @@
+Add a retention policy feature - when specified, the latest N versions of each package will be kept and older versions will be purged.


I mentioned elsewhere but, "retention policy" still seems like an apt description of the feature in noun form, so I'm not sure if we want to adjust this wording as well?

This prompted me to think about how users will learn about new configuration.
What about adding to the docs what is the parameter and how to use it?

Depsolving for PackageGroups, PackageEnvirnment and PackageCategory. Required PR: pulp#1752 closes: #6316 https://pulp.plan.io/issues/6316

Depsolving for PackageGroups, PackageEnvirnment and PackageCategory. Required PR: pulp#1752 closes: #6316 https://pulp.plan.io/issues/6316 [nocoverage]

dralley · 2020-07-01T20:07:24Z

pulp_rpm/tests/functional/api/test_retention_policy.py

+        self.addCleanup(self.remote_api.delete, remote.pulp_href)
+
+        with self.assertRaises(ApiException):
+            self.sync(repository=repo, remote=remote, optimize=False, mirror=True)


Not sure if there's a way to assert the specific code (400).

There is:
https://github.com/pulp/pulp_container/blob/c91bed1868a1c3c536f90637d43b0be74d50c573/pulp_container/tests/functional/api/test_crud_distributions.py#L163

goosemania

Thanks! Looks good!

goosemania · 2020-07-02T16:51:19Z

pulp_rpm/app/serializers.py

        view_name='signing-services-detail',
        queryset=AsciiArmoredDetachedSigningService.objects.all(),
        many=False,
        required=False,
        allow_null=True
    )
+    retain_package_versions = serializers.IntegerField(
+        help_text=_("The number of verisons of each package to keep in the repository; "


typo in versions

goosemania · 2020-07-02T16:52:07Z

pulp_rpm/app/serializers.py

        view_name='signing-services-detail',
        queryset=AsciiArmoredDetachedSigningService.objects.all(),
        many=False,
        required=False,
        allow_null=True
    )
+    retain_package_versions = serializers.IntegerField(
+        help_text=_("The number of verisons of each package to keep in the repository; "
+                    "older versions will be purged. A value of '0' will disable this"


up to you if it's helpful to note that '0 is a default.

goosemania · 2020-07-02T16:52:58Z

pulp_rpm/app/viewsets.py

+        if repository.retain_package_versions > 0 and mirror:
+            raise DRFValidationError("Cannot use 'retain_package_versions' with mirror-mode sync")


closes: #5367 https://pulp.plan.io/issues/5367

dralley added the Work In Progress label Jun 18, 2020

dralley force-pushed the rpm-evr branch from 3111509 to 4e7882f Compare June 18, 2020 02:32

dralley commented Jun 18, 2020

View reviewed changes

mdellweg reviewed Jun 18, 2020

View reviewed changes

pulp_rpm/app/migrations/0012_rpm_evr_extension.py Outdated Show resolved Hide resolved

dralley force-pushed the rpm-evr branch 10 times, most recently from 99fdb67 to 67b62a8 Compare June 22, 2020 17:52

dralley requested a review from a team June 24, 2020 18:39

dralley force-pushed the rpm-evr branch from 4c20031 to 2230a3d Compare June 24, 2020 18:39

dralley changed the title ~~Add database support for comparing EVR~~ Add a retention policy feature Jun 24, 2020

dralley force-pushed the rpm-evr branch 2 times, most recently from fa57930 to 49c737c Compare June 24, 2020 19:13

dralley changed the title ~~Add a retention policy feature~~ Add a package retention policy feature Jun 24, 2020

dralley force-pushed the rpm-evr branch 3 times, most recently from 598f682 to 662f635 Compare June 24, 2020 20:54

dralley commented Jun 24, 2020

View reviewed changes

goosemania reviewed Jun 25, 2020

View reviewed changes

dralley force-pushed the rpm-evr branch from 9313809 to c3dc97d Compare June 25, 2020 15:46

Add database support for comparing EVR

ce6c364

closes: #5402 https://pulp.plan.io/issues/5402 [nocoverage]

dralley force-pushed the rpm-evr branch 3 times, most recently from 1607553 to bf6e891 Compare July 1, 2020 03:09

dralley removed the Work In Progress label Jul 1, 2020

dralley commented Jul 1, 2020

View reviewed changes

dralley force-pushed the rpm-evr branch from bf6e891 to 25f2da9 Compare July 1, 2020 03:30

dralley commented Jul 1, 2020

View reviewed changes

goosemania reviewed Jul 1, 2020

View reviewed changes

dralley force-pushed the rpm-evr branch 3 times, most recently from 4ff2f30 to 72e3b8f Compare July 1, 2020 14:39

dralley commented Jul 1, 2020

View reviewed changes

dralley force-pushed the rpm-evr branch from 72e3b8f to e523dba Compare July 1, 2020 14:46

pavelpicka added a commit to pavelpicka/pulp_rpm that referenced this pull request Jul 1, 2020

Copy for comps types

212585d

Depsolving for PackageGroups, PackageEnvirnment and PackageCategory. Required PR: pulp#1752 closes: #6316 https://pulp.plan.io/issues/6316

pavelpicka mentioned this pull request Jul 1, 2020

Copy for comps types #1760

Merged

pavelpicka added a commit to pavelpicka/pulp_rpm that referenced this pull request Jul 1, 2020

Copy for comps types

f733cb0

Depsolving for PackageGroups, PackageEnvirnment and PackageCategory. Required PR: pulp#1752 closes: #6316 https://pulp.plan.io/issues/6316 [nocoverage]

pavelpicka added a commit to pavelpicka/pulp_rpm that referenced this pull request Jul 1, 2020

Copy for comps types

202b183

Depsolving for PackageGroups, PackageEnvirnment and PackageCategory. Required PR: pulp#1752 closes: #6316 https://pulp.plan.io/issues/6316 [nocoverage]

dralley force-pushed the rpm-evr branch from e523dba to c82975a Compare July 1, 2020 19:56

dralley commented Jul 1, 2020

View reviewed changes

dralley force-pushed the rpm-evr branch 2 times, most recently from 4cd02fe to 9e45c34 Compare July 2, 2020 14:16

goosemania approved these changes Jul 2, 2020

View reviewed changes

Add a retention policy feature for purging older packages

4d0247c

closes: #5367 https://pulp.plan.io/issues/5367

dralley force-pushed the rpm-evr branch from 9e45c34 to 4d0247c Compare July 2, 2020 17:45

dralley merged commit 14b350e into pulp:master Jul 2, 2020

dralley deleted the rpm-evr branch July 2, 2020 18:32

sjha4 mentioned this pull request Aug 3, 2020

Bump python-pulp-rpm to 3.5.0 theforeman/foreman-packaging#5611

Merged

		from django.db import models
		from django.db.models import Window, F

		Partitioning by architecture is important because there i686 and x86_64 packages
		with same name and same or close verison numbers are not interchangeable.

		@@ -0,0 +1 @@
		Add a retention policy feature - when specified, the latest N versions of each package will be kept and older versions will be purged.

		if repository.retain_package_versions > 0 and mirror:
		raise DRFValidationError("Cannot use 'retain_package_versions' with mirror-mode sync")

Add a package retention policy feature #1752

Add a package retention policy feature #1752

Conversation

dralley commented Jun 18, 2020 • edited

pep8speaks commented Jun 18, 2020 • edited

Comment last updated at 2020-07-02 17:45:55 UTC

pulpbot commented Jun 18, 2020

dralley commented Jun 18, 2020

dralley Jun 18, 2020 • edited

Choose a reason for hiding this comment

dralley commented Jun 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goosemania Jun 25, 2020 • edited

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

Choose a reason for hiding this comment

goosemania left a 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

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

Choose a reason for hiding this comment

goosemania Jun 25, 2020 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goosemania left a 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 18, 2020 •

edited

pep8speaks commented Jun 18, 2020 •

edited

dralley Jun 18, 2020 •

edited

dralley commented Jun 24, 2020 •

edited

goosemania Jun 25, 2020 •

edited

goosemania Jun 25, 2020 •

edited