Enhance replicate delete policy #6247
Conversation
gerrod3
force-pushed
the
repli-delete-policy
branch
from
7f1ca13 to
7ad9a18
Compare
|
Please take the following as a pure observation, something to be aware of: |
pulpcore/app/models/replica.py
Outdated
| NONE = "none" | ||
| DELETE_CHOICES = ( | ||
| (ALL, "Delete all local objects not present on upstream."), | ||
| (LABELED, "Delete local labeled objects not present on upstream."), |
There was a problem hiding this comment.
What should happen if you have a local repository without a label. Should replicate replace that, leave it alone or simply fail?
I guess this question extends the policy beyond just "deleting".
There was a problem hiding this comment.
Note that my description isn't that great. The 'local labeled objects' refers to objects that have been labeled by replicate with UpstreamPulp:{id}. So any repository without this label means it wasn't created(tracked) by replicate and thus will not be removed under this policy. Say there is a unlabeled repository with the same name as an upstream one (a case our users will be in on their first replicate if this is merged), then that repository will get the new label and then be tracked for all future replicates.
There was a problem hiding this comment.
Will we set the labels anyway? Then users could run one more "old style" replication to label all the affected ones and change the flag then.
Now we could choose to "protect" manually created repositories from the replicators.
gerrod3
force-pushed
the
repli-delete-policy
branch
6 times, most recently
from
6875981 to
2d7ed9b
Compare
|
Ok I updated the PR to change the field to just |
| upstream_labels = getattr(upstream_object, "pulp_labels", {}) | ||
| upstream_labels["UpstreamPulp"] = str(self.server.pk) | ||
| return upstream_labels |
There was a problem hiding this comment.
So if we replicate from another replica, we overwrite the "UpstreamPulp" label.
I think this is exactly what we should do. ;)
| if self.server.policy == UpstreamPulp.LABELED: | ||
| if object.pulp_labels.get("UpstreamPulp") != str(self.server.pk): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
This means in the "NODELETE" policy, we still allow local repositories to be overwritten, right?
There was a problem hiding this comment.
Yes, would you like it to also be protected?
There was a problem hiding this comment.
I'm wondering if that would make a forth policy. We can also just document that and maybe add the forth policy later.
pulpcore/app/serializers/replica.py
Outdated
| @@ -91,6 +91,13 @@ class UpstreamPulpSerializer(ModelSerializer, HiddenFieldsMixin): | |||
| read_only=True, | |||
| ) | |||
|
|
|||
| policy = serializers.ChoiceField( | |||
| choices=UpstreamPulp.POLICY_CHOICES, | |||
| default="all", | |||
There was a problem hiding this comment.
Not sure if this would be more consistent...
| default="all", | |
| default=UpstreamPulp.ALL, |
But actually defaults on serializers interfere badly with partial_update.
We should really do:
| default="all", | |
| required=False, |
and let the model handle the default.
gerrod3
force-pushed
the
repli-delete-policy
branch
from
2d7ed9b to
49dc219
Compare
|
There's a migration conflict needing this to rebase. |
gerrod3
force-pushed
the
repli-delete-policy
branch
from
49dc219 to
c1a0785
Compare
Added a new field
delete_policythat will determine how replicate will clean up the extra/leftover objects present in the downstream Pulp. The field is set to the defaultallwhich is the current behavior of deleting everything. The other two options arenoneandlabeled.noneis self-explanatory. Replicate now also labels each object created/updated on a replicate with the labelUpstreamPulp:{upstream.pk}. This new label is what is used by thelabeledpolicy to determine which objects should be deleted. Only objects with this label and are not present in the Upstream will be deleted under this policy, everything else will be left.Also, added copying the Upstream objects' labels onto the downstream. This change isn't strictly necessary for the new delete feature, so I can remove it, add a new field for customization, or move it into another PR if wanted.
fixes: #5214