Update remote artifact urls on sync if the url of the artifact has changed - #1623
Conversation
|
Discussion needed: https://pulp.plan.io/issues/9395#note-6 |
dralley
changed the title
c165188 to
36c6d6e
Compare
|
Attached issue: https://pulp.plan.io/issues/9395 |
|
|
||
| if d_artifact.remote.pk == remote_artifact.remote_id: | ||
| key = f"{str(content_artifact.pk)}-{str(d_artifact.remote.pk)}" | ||
| remote_artifact.url = d_artifact.url |
There was a problem hiding this comment.
TL;DR:
If the relative path of the artifact within the repo has changed, or if the base path of the repo has changed, we update the RemoteArtifact to match the new URL
There was a problem hiding this comment.
Here are some specific examples on how this problem can affect plugins:
- RPM plugin: part of the remote_artifact.url is composed of
location_hrefwhich is specified in the repodata. It might happen so that it will change in the remote repo , and with the next sync no new RemoteArtifacts will be created because of uniqueness constraint and old RemoteArtifacts would point to an invalid url and as a result user will get 404. - File plugin: create and sync a
local_repoAwith on_demand policy fromhttps://remote.repos.com/repoA/PULP_MANIFEST. Then updateremote.urlto point tohttps://remote.repos.com/exact_copy_of_repoA/PULP_MANIFEST. Imagine thatremote.repos.com/repoA. has been removed/broken and onlyremote.repos.com/exact_copy_of_repoAis left. Re-synclocal_repoA. Users will get 404 afterwards becauseremote_artifact.urlwill still point to old unavailable url and not new remote_artifacts would be created. - Container plugin: similar to file plugin workflow just the url to the registry would change
I'd prefer to change uniqueness constraint of the RemoteArtifact so it also contains the url in addition to content_artifact and remote however due to outlined by @dralley reasons that this would not be backportable taken by him approach makes sense.
| break | ||
|
|
||
| if d_artifact.remote.pk == remote_artifact.remote_id: | ||
| key = f"{str(content_artifact.pk)}-{str(d_artifact.remote.pk)}" |
There was a problem hiding this comment.
Do we need that key? The only use of it i see is as a key for the dictionary. So it imposes some uniqueness.
There was a problem hiding this comment.
I don't know. I feel like it shouldn't be necessary if everything is working correctly and there are no duplicates, but at this stage I'm biased towards caution and I don't know if there are any scenarios where duplicates might be expected.
36c6d6e to
718d12d
Compare
| @@ -0,0 +1 @@ | |||
| Fixed an issue where on_demand content might not be downloaded properly if the remote URL was changed (even if re-synced). | |||
There was a problem hiding this comment.
How can we write an automated test for this?
There was a problem hiding this comment.
I think if we had a pulp_file fixture that had a different layout but the same content we could test this by:
- sync one of the fixtures with on_demand
- change the remote.url to the other fixture
- observe the 404
- sync
- observe the 404's go away
There was a problem hiding this comment.
I think we cannot, because a change to the layout would yield a different set of content, because relative_path is part of it.
718d12d to
28f4ad3
Compare
28f4ad3 to
ab74c2a
Compare
mdellweg
left a comment
mdellweg
left a comment
There was a problem hiding this comment.
My concerns have been addressed.
ipanova
left a comment
ipanova
left a comment
There was a problem hiding this comment.
Thank you for adding the test!
6 participants
closes: #9395
https://pulp.plan.io/issues/9395