Pulp

I can't think of any reason why the proposed solution would cause trouble in Katello. We'll just want to make sure there is an obvious path for keeping Pulp data consistent.

In the case orphan_cleanup wins, plugin writers will need to be told they can no longer guarantee that just because content was there a moment ago, it is there now. I expect the exception can just bubble up to the user, and they user can restart the job at which point code like sync will get it right the second time.

In this case, my preference would be to extend Stages API to handle this exception behind the scene, re-download the content, rather than asking users to re-run the sync.

However this does not solve the upload usecase - How do we solve the case when content was initially brought into pulp not by sync operation but upload? I don't think telling we no longer guarantee that just because content was there a moment ago, it is there now will bring a lot of confidence to our users. Our value proposition is to reliably manage content and this statement contradicts to it and it worries me, especially when there is not provided good recovery path.

ipanova@redhat.com wrote:

In this case, my preference would be to extend Stages API to handle this exception behind the scene, re-download the content, rather than asking users to re-run the sync. I agree with you on this in the long term, but the complexity it brings I don't think is worth it right now. Consider these reasons and let me know if you still think it is.

1. I think this will make this a plugin-api breaking change, requiring any plugin with a custom pipeline (which is several) to take additional action to port onto it. This would be breaking because I believe to send content from later stages back to earlier stages would require the stages to reference each other.

2. The level of effort goes up significantly with this requirement when we could deliver this improvement later and keep the changes smaller.

However this does not solve the upload usecase - How do we solve the case when content was initially brought into pulp not by sync operation but upload? I don't think telling we no longer guarantee that just because content was there a moment ago, it is there now will bring a lot of confidence to our users. Our value proposition is to reliably manage content and this statement contradicts to it and it worries me, especially when there is not provided good recovery path.

I acknowledge that this is a step backwards in reliability, but I want to make two points about this.

1. If we leave the situation as-is, any pulp system will block all new tasks for a really long time every time. Really long like hours because it has to wait for every task on every worker to stop. Some sync's themselves can take hours, not to mention the many operations running on all workers. This is a huge cost for users to pay and I don't think we can justify that user experience.

2. The upload error case is a race condition, one that would occur I believe rarely and is easily recoverable.

My perspective is that, when I consider the balance between the user experience pain of deleting content (hours, unscalable, and almost unworkable) against the downsides to users (brief and recoverable) this bring overall huge benefits.

bmbouter wrote:

ipanova@redhat.com wrote:

In this case, my preference would be to extend Stages API to handle this exception behind the scene, re-download the content, rather than asking users to re-run the sync. I agree with you on this in the long term, but the complexity it brings I don't think is worth it right now. Consider these reasons and let me know if you still think it is.

1. I think this will make this a plugin-api breaking change, requiring any plugin with a custom pipeline (which is several) to take additional action to port onto it. This would be breaking because I believe to send content from later stages back to earlier stages would require the stages to reference each other.

2. The level of effort goes up significantly with this requirement when we could deliver this improvement later and keep the changes smaller.

I agree that current situation with the orphan clean-up is barely acceptable and we need to do something about it. However regardless how much effort will be invested now or later this change will remain a breaking one.

However this does not solve the upload usecase - How do we solve the case when content was initially brought into pulp not by sync operation but upload? I don't think telling we no longer guarantee that just because content was there a moment ago, it is there now will bring a lot of confidence to our users. Our value proposition is to reliably manage content and this statement contradicts to it and it worries me, especially when there is not provided good recovery path.

I acknowledge that this is a step backwards in reliability, but I want to make two points about this.

1. If we leave the situation as-is, any pulp system will block all new tasks for a really long time every time. Really long like hours because it has to wait for every task on every worker to stop. Some sync's themselves can take hours, not to mention the many operations running on all workers. This is a huge cost for users to pay and I don't think we can justify that user experience.

2. The upload error case is a race condition, one that would occur I believe rarely and is easily recoverable.

My perspective is that, when I consider the balance between the user experience pain of deleting content (hours, unscalable, and almost unworkable) against the downsides to users (brief and recoverable) this bring overall huge benefits.

It is hard to argue against the above mentioned points, but I still feel uneasy on taking a step back on reliability. I would love to hear what other people think.

if one shot upload - handle in 1 transaction artifact and content creation.

This is impossible (at least not without some major refactoring). Currently, artifact creation is handled in the viewset while content create is handled by a task.

That said I don't think it's a big deal. Worst case scenario the content creation fails and the user just has to adjust the preservation setting and retry it.

daviddavis wrote:

if one shot upload - handle in 1 transaction artifact and content creation.

This is impossible (at least not without some major refactoring). Currently, artifact creation is handled in the viewset while content create is handled by a task.

That said I don't think it's a big deal. Worst case scenario the content creation fails and the user just has to adjust the preservation setting and retry it.

In looking at those links, I agree with your conclusion. I also think it's not a very big deal either.

daviddavis wrote:

if one shot upload - handle in 1 transaction artifact and content creation.

This is impossible (at least not without some major refactoring). Currently, artifact creation is handled in the viewset while content create is handled by a task.

That said I don't think it's a big deal. Worst case scenario the content creation fails and the user just has to adjust the preservation setting and retry it.

+1 .

We use the general_create task to create content for one-shot uploads which basically sets up a subclass of SingleArtifactContentUploadSerializer and calls save() on it.

I'm not sure exactly where is best to update the timestamp if there's an existing content. I'm imagining I could update SingleArtifactContentUploadSerializer. But I am not sure how to do this in a general way to work for all plugins. For one thing, consider the pulp_rpm package case where it's extracting information from the artifact file and then raising a general ValidationError.

I see what you are saying. In the plugin code where we see the package with exists() we'd need to bump its time of interest.

What if instead of a validation error, the upload task would return the existing content unit (as if it were just uploaded; We might need to add ownership permissions at that point too.)?

Also how is the situation you are describing worse than what we already have? If an orphan cleanup is scheduled right after your upload task, you will never see the content you created even with today's code.

mdellweg wrote:

I see what you are saying. In the plugin code where we see the package with exists() we'd need to bump its time of interest.

What if instead of a validation error, the upload task would return the existing content unit (as if it were just uploaded; We might need to add ownership permissions at that point too.)?

Yeah..this is plugin specific, I am tempted to say that it should be plugins responsibility to add the content timestamp for the one-shot upload case. I also don't see a generic way to do it in core :/

Also how is the situation you are describing worse than what we already have? If an orphan cleanup is scheduled right after your upload task, you will never see the content you created even with today's code.

Right, it depends whether you have provided the repository you want the package to be added to https://github.com/pulp/pulpcore/blob/aed355fbc380a13a4a767e2627cbd54c7ff8f107/pulpcore/plugin/viewsets/content.py#L85

We probably will fall into the same same trap with /modify for the plugins that define their own endpoints https://github.com/pulp/pulp_container/blob/master/pulp_container/app/viewsets.py#L592

What if instead of a validation error, the upload task would return the existing content unit (as if it were just uploaded; We might need to add ownership permissions at that point too.)?

That sounds like a possible idea. We could notify plugins that they need to start returning the content unit instead of raising an exception and I can have core always bump the timestamp.

Also how is the situation you are describing worse than what we already have? If an orphan cleanup is scheduled right after your upload task, you will never see the content you created even with today's code.

This is true.

I am tempted to say that it should be plugins responsibility to add the content timestamp for the one-shot upload case.

That sounds like a good idea to me too.

We probably will fall into the same same trap with /modify for the plugins that define their own endpoints

Yes, good point.

daviddavis wrote:

What if instead of a validation error, the upload task would return the existing content unit (as if it were just uploaded; We might need to add ownership permissions at that point too.)?

That sounds like a possible idea. We could notify plugins that they need to start returning the content unit instead of raising an exception and I can have core always bump the timestamp.

Here is a ticket related to this https://pulp.plan.io/issues/7114#note-1

Also how is the situation you are describing worse than what we already have? If an orphan cleanup is scheduled right after your upload task, you will never see the content you created even with today's code.

This is true.

I am tempted to say that it should be plugins responsibility to add the content timestamp for the one-shot upload case.

That sounds like a good idea to me too.

How do we proceed? It does not feel 100% correct to roll out the non-blocking orphan clean-up without the upload part and seems like (ideally) first we need to adjust upload workflow in plugins so it can facilitate our needs in core. Or do we roll-out this change the way it is and basically tell the plugins to take care of upload and other modify-like endpoints, otherwise they risk to lose orphaned artifacts/content without the timespamp the moment orphan clean up is triggered?

We probably will fall into the same same trap with /modify for the plugins that define their own endpoints

Yes, good point.

Also I have noticed that this ticket does not capture the configurable option for X seconds/minutes to keep the orphans. I recall this has been discussed and contra-proposed to the hard-coded value. Should we add it?