Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backend, frontend: implement project and build deletion in Pulp #3330

Merged
merged 4 commits into from
Sep 11, 2024

Conversation

FrostyX
Copy link
Member

@FrostyX FrostyX commented Jul 14, 2024

Fix #3318
Fix #3319

backend/copr_backend/storage.py Fixed Show fixed Hide fixed
backend/copr_backend/storage.py Fixed Show fixed Hide fixed
backend/copr_backend/storage.py Fixed Show fixed Hide fixed
@FrostyX
Copy link
Member Author

FrostyX commented Jul 14, 2024

Not yet ready, I am submitting a draft, so you can see the progress.
So far the code is refactored, so that the deletion logic is in the storage.py instead of actions.py. The BackendStorage is more or less finished, and I am currently working on the PulpStorage implementation (which is strangely complicated).

@FrostyX FrostyX force-pushed the pulp-deleting branch 5 times, most recently from 13d22df to c4fefed Compare July 22, 2024 09:27
@FrostyX FrostyX force-pushed the pulp-deleting branch 2 times, most recently from c66881f to 4cad51e Compare July 22, 2024 10:06
backend/copr_backend/storage.py Fixed Show fixed Hide fixed
backend/copr_backend/storage.py Fixed Show fixed Hide fixed
backend/copr_backend/storage.py Fixed Show fixed Hide fixed
@FrostyX FrostyX marked this pull request as ready for review July 29, 2024 21:08
@nikromen nikromen self-assigned this Aug 7, 2024
@FrostyX FrostyX added the pulp label Aug 28, 2024
@FrostyX
Copy link
Member Author

FrostyX commented Sep 5, 2024

@nikromen I rebased the PR from main and fixed merge conflicts. And I also fixed the bug with apparently successfully finished builds which were marked as failed

Copy link
Member

@nikromen nikromen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think the PR is nearly finished. Just some things still don't work as expected so I listed them below.

Overall I think that newly added methods to pulp client should work.

response = requests.get(url, **self.request_params)
return response.json()["results"][0]["pulp_href"]

def wait_for_finished_task(self, task):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this worked, thanks! What if the task gets stuck on pulp side? Do they have some hard timeout? Or do we get stuck here forever?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea to be honest, if a task can get stuck on the Pulp side. I think we will get stuck in this loop "forever". But "forever" should be only until Pulp gets unstuck.

Should I add some timeout on our side?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say rather yes, since some rare errors may happen and then we could end up here again #3343. Let's just put there some unlikely high timeout - e.g. 5 hours.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

backend/copr_backend/storage.py Show resolved Hide resolved
backend/copr_backend/storage.py Outdated Show resolved Hide resolved
# pylint: disable=too-many-locals
result = True
for chroot, subdirs in chroot_builddirs.items():
if chroot == "srpm-builds":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is because repodata aren't created for srpm build thus we don't need to delete anything for srpm builds?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering what will happen with logs? We delete them in the backend storage but not in pulp - is that part of delete artifacts? Then the logs will still remain

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is because repodata aren't created for srpm build thus we don't need to delete anything for srpm builds?

(So far) we don't upload results of source builds to Pulp, so there is nothing to remove. I added a code comment so it's clear.

However, in that case, we probably need to remove them from the backend storage, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got srpm stored inside the pulp instance:

lftp localhost:/pulp/content/nikromen/test-pulp-3/fedora-39-x86_64/Packages/p> ls
drwxr-xr-x  --  ..
-rw-r--r--  --  python-dside-2.3.3-1.fc39.src.rpm
-rw-r--r--  --  python3-dside-2.3.3-1.fc39.noarch.rpm
lftp localhost:/pulp/content/nikromen/test-pulp-3/fedora-39-x86_64/Packages/p>

but it is somehow deleted with the build deletion - probably the resources maps also the srpms.

However, in that case, we probably need to remove them from the backend storage, right?

I only checked the existence of the project inside pulp - after meeting I'll review backend storage if there are some remnants

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after meeting I'll review backend storage if there are some remnants

yes, there are remnants from the deleted builds and deleted projects - so we need to temporarily call the delete methods from backend storage inside pulp storage to remove what's on backend.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

backend/copr_backend/pulp.py Outdated Show resolved Hide resolved
backend/copr_backend/pulp.py Outdated Show resolved Hide resolved
backend/copr_backend/storage.py Outdated Show resolved Hide resolved
@FrostyX
Copy link
Member Author

FrostyX commented Sep 6, 2024

Thank you very much for the review @nikromen

@nikromen
Copy link
Member

nikromen commented Sep 9, 2024

Thank you for the fixes! Now I can confirm deleting of project/package works! Packages/projects inside pulp instance are created and deleted as they should.

I just found one more caveat (unrelated to this PR) - I can't install the packages via copr-cli :/ Could you please confirm that you were also unable to install https://copr.stg.fedoraproject.org/coprs/nikromen/test-pulp-3/ ? If so, I'd create a separate issue since this is unrelated to deletion.

@FrostyX
Copy link
Member Author

FrostyX commented Sep 9, 2024

I just found one more caveat (unrelated to this PR) - I can't install the packages via copr-cli :/

You mean dnf copr? That is being implemented in PR #3383 (still missing support for DNF5)

@nikromen
Copy link
Member

nikromen commented Sep 9, 2024

ahaaa :D nice nice

@praiskup praiskup merged commit 286a913 into fedora-copr:main Sep 11, 2024
8 checks passed
FrostyX added a commit to FrostyX/copr that referenced this pull request Nov 20, 2024
Fix fedora-copr#3507

The issue was introduced in PR fedora-copr#3330. We previously did this on
the backend:

    devel = uses_devel_repo(self.front_url, ownername, projectname)

It looks like an unnecessary request, so I am sending the attribute
as a part of the action data.
FrostyX added a commit to FrostyX/copr that referenced this pull request Nov 20, 2024
Fix fedora-copr#3507

The issue was introduced in PR fedora-copr#3330. We previously did this on
the backend:

    devel = uses_devel_repo(self.front_url, ownername, projectname)

It looks like an unnecessary request, so I am sending the attribute
as a part of the action data.
praiskup pushed a commit that referenced this pull request Nov 25, 2024
Fix #3507

The issue was introduced in PR #3330. We previously did this on
the backend:

    devel = uses_devel_repo(self.front_url, ownername, projectname)

It looks like an unnecessary request, so I am sending the attribute
as a part of the action data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pulp - delete single build Pulp - delete projects
3 participants